RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v9]

[Lance Andersen]

On Mon, 26 Jul 2021 03:28:44 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> 4890732: GZIPOutputStream doesn't support optional GZIP fields
>
> Lin Zang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits:
> 
>  - change since version to 18
>  - Merge branch 'master' into gzip-field
>  - Merge branch 'master' into gzip-field
>  - Add api in GZIPInputStream to get header data
>  - Merge remote-tracking branch 'upstream/master' into gzip-field
>  - remove trailing spaces
>  - Use record and Builder pattern
>  - add class GZIPHeaderData, refine testcases
>  - update copyright
>  - reuse arguments constructor for non-argument one.
>  - ... and 3 more: https://git.openjdk.java.net/jdk/compare/e627caec...b1868e8f

Thank you for reviving the discussion.

I have not gone through the latest update in detail but there are some changes that are needed. Before moving forward with the CSR, I would like to give time for additional feedback  on naming and design.

  I am not sure  the builder names withXXX are the preferred naming pattern.

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 33:

> 31:  * @author      Lin Zang
> 32:  * @since 18
> 33:  *

The overview section needs more detail and examples of the usage

Please remove the author tag as we have moved away from this tag (you can see who was involved via the file history

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 114:

> 112:      */
> 113:     public GZIPHeaderBuilder withFileComment(String fileComment) {
> 114:         if (fileComment == null || fileComment.length() == 0) {

What happens if the String contains characters outside of ISO_8859_1?

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 136:

> 134:         return this;
> 135:     }
> 136: 

Is this really needed as a public method ?

src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 74:

> 72:                             int size,
> 73:                             boolean syncFlush,
> 74:                             GZIPHeaderBuilder.GZIPHeaderData gzipHeaderData)

Given the Gzip header data is so infrequently modified,  one additional constructor is all that really need.

src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 91:

> 89:      * Creates a new output stream with the specified buffer size and
> 90:      * flush mode. And leave all other header fields set to default value.
> 91:      *

This needs rewording as we do not typically start a sentence with "And..."

src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 113:

> 111:     /**
> 112:      * Creates a new output stream with the specified buffer size.
> 113:      * And leave all other header fields set to default value.

Same comment regarding "And..."

src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 144:

> 142:     /**
> 143:      * Creates a new output stream with a default buffer size and
> 144:      * the specified flush mode. And leave all other header fields

Same comment regarding "And..."

-------------

Changes requested by lancea (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3072