[+Brandon] For the test copyright line, use
* Copyright 2009 Google, Inc. All Rights Reserved. as in other test cases. Whether, in addition, to have a Sun copyright depends on whether you made "substantive modifications". Heuristic for that is 10 lines of code. Obviously, the Sun copyright got there via copy-n-paste from existing files. --- You still have @linke typos and lingering TABs to untabify. --- Otherwise, I'm happy enough with this - I think we have rough consensus. Martin On Wed, Sep 23, 2009 at 10:08, Xueming Shen <xueming.s...@sun.com> wrote: > > Martin, thanks for the comments. > > webrev has been updated accordingly, I will submit it for the CCC. > > What should the "google copyright" look like for InflateIn_DeflateOut.java? > > I picked it from > > http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Z_SYNC_FLUSH/test/java/util/zip/InflateIn_DeflateOut.java.html > > which uses the sun copyright, I guess it was copied from the your > DeflateIn_InflateOut.java test case. > > So if you can pass me your google version I will paste it in. I assume It > should be OK that I then put the > sun one on top of that. > > Glad to see the light at the end of the tunnel finally:-) > > sherman > > Martin Buchholz wrote: >> >> [+ Brandon] >> >> Sherman, >> Here are some review comments on "take1" >> >> Overall, looks good. >> This low-level API will be annoying to use, >> but that's mostly unavoidable. >> >> ---- >> typos: @linke sinze >> ---- >> source files contain TABs, no longer permitted >> ---- >> <tt> is denigrated in favor of @code >> ---- >> I don't think you really meant to include ./Flush.java in the webrev >> ---- >> {...@link Deflater.SYNC_FLUSH} >> => >> {...@link Deflater#SYNC_FLUSH} >> ---- >> Please use a Google copyright in InflateIn_DeflateOut >> ---- >> >> Martin >> >> On Tue, Sep 22, 2009 at 13:03, Xueming Shen <xueming.s...@sun.com> wrote: >> >>> >>> Thanks Alan! >>> >>> The webrev has been updated accordingly to address your comments (use >>> syncFlush instead of >>> doSyncFlush and those suggestions for Deflater.java). >>> >>> http://cr.openjdk.java.net/~sherman/zipflush/webrev >>> >>> Sherman >>> >>> PS. There was a "take2" for DOS that I think might be more consistent >>> with >>> the existing APIs, but it might be the >>> time to focus on one approach. If you're interested the "take2" is at >>> >>> >>> http://cr.openjdk.java.net/~sherman/zipflush/webrev.take2/src/share/classes/java/util/zip/DeflaterOutputStream.java.sdiff.html >>> >>> Alan Bateman wrote: >>> >>>> >>>> Welcome back (for some reason I thought you were gone for two weeks). >>>> >>>> It would be best to send the proposal to the mailing list so that others >>>> can comment. >>>> >>>> Personally, I don't like exposing the flush mode to subclasses but you >>>> are >>>> right that it is more consistent with the original design. If this >>>> approach >>>> is chosen then I would suggest that the flush implementation make a copy >>>> of >>>> the flush mode before testing and using it. Also, the new constructors >>>> will >>>> need to say that they initialize the flush mode based on the parameter >>>> (since it can change on the fly). BTW: What is the reason for renaming >>>> the >>>> parameter to flushDef. I prefer "syncFlush" over "doSyncFlush". >>>> >>>> I'm happy with Deflater. In the description of SYNC_FLUSH I would >>>> suggest >>>> changing the "," into a ";" or else make it into two sentences. In the >>>> new >>>> deflate method the description of NO_FLUSH needs a comma after >>>> "accumulate" >>>> for it to flow well. Similar issue in the SYNC_FLUSH description where >>>> you >>>> need a comma after "is flushed". There is also a typo [ comparssed :-) ] >>>> >>>> -Alan. >>>> >>>> >>>> >>> >>> > >
