On Mon, 26 Jul 2021 03:28:44 GMT, Lin Zang <lz...@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
