Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]
On Tue, 28 Sep 2021 03:10:33 GMT, Lin Zang wrote: > Dear All， This PR has been pending there for quite a long time. I am > wondering maybe this PR is not so interesting? I would like to leave this PR > open for a while more, and if no new update, I would let it close > automatically by timeout. Thanks! > > Lin Hi Lin, Thank you for your continued work on this. I think we need to flush out the API more. Alan made a great point in his [comment regarding the mutability of GZIPHeaderData](https://github.com/openjdk/jdk/pull/3072#issuecomment-887629007 ): due to the inclusion of an array element within a record definition. This still needs to be addressed. We should also decide the best location for the header flag constants. I will try to spend a bit more time on this next week. - PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]
On Mon, 6 Sep 2021 07:20:11 GMT, Lin Zang 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 17 commits: > > - Merge branch 'master' into gzip-field > - delete trailing spaces > - refine code > - Merge branch 'master' into gzip-field > - 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 > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/c640fe42...196caedb Hi @liach， Thanks for your review! I updated the change based on your comments. BRs, Lin - PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]
On Mon, 6 Sep 2021 07:20:11 GMT, Lin Zang 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 17 commits:
>
> - Merge branch 'master' into gzip-field
> - delete trailing spaces
> - refine code
> - Merge branch 'master' into gzip-field
> - 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
> - ... and 7 more:
> https://git.openjdk.java.net/jdk/compare/c640fe42...196caedb
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 65:
> 63: * Add extra field in GZIP file header.
> 64: * This method verifies the extra fileds layout per RFC-1952.
> 65: * See comments of {@code verifyExtraFieldLayout}
You should specify the layout here than refer to a private method, as private
methods don't appear in generated online javadocs
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 186:
> 184: * @since 18
> 185: */
> 186: public byte[] generateBytes(byte cm,
Is there any reason to leave this public? If this is just an implementation
detail, this should be kept private as public apis are maintenance burdens that
are risky to change (hence csrs)
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 247:
> 245: *+=+
> 246: */
> 247: byte[] filenameBytes = filename.getBytes("ISO-8859-1");
Should use
[`StandardCharsets.ISO_8859_1`](https://github.com/openjdk/jdk/blob/8876eae42993d4425ba9886dde94b08f6101a257/src/java.base/share/classes/java/nio/charset/StandardCharsets.java#L55)
than string names. Using the charset object is significantly faster than
looking up charsets by string names, see
https://bugs.openjdk.java.net/browse/JDK-8272120
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 328:
> 326: String fileComment,
> 327: int headerCRC16,
> 328: byte[] headerBytes) {
Need to override the getter for byte array fields to return copies than
original arrays to prevent modifications. For the extraFieldBytes one, the call
to copy needs a null check too.
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 359:
> 357: private static final int FEXTRA = 4;// Extra field
> 358: private static final int FNAME = 8;// File name
> 359: private static final int FCOMMENT = 16; // File comment
Since the `flags` record component is public, These probably should be left
public as well to benefit users, especially given checking the flags can
replace null checks for `extraFieldBytes`, `filename`, and `fileContent`.
-
PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]
On Mon, 6 Sep 2021 07:20:11 GMT, Lin Zang 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 17 commits: > > - Merge branch 'master' into gzip-field > - delete trailing spaces > - refine code > - Merge branch 'master' into gzip-field > - 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 > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/c640fe42...196caedb Dear All， This PR has been pending there for quite a long time. I am wondering maybe this PR is not so interesting? I would like to leave this PR open for a while more, and if no new update, I would let it close automatically by timeout. Thanks! Lin - PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]
> 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 17 commits: - Merge branch 'master' into gzip-field - delete trailing spaces - refine code - Merge branch 'master' into gzip-field - 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 - ... and 7 more: https://git.openjdk.java.net/jdk/compare/c640fe42...196caedb - Changes: https://git.openjdk.java.net/jdk/pull/3072/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3072=11 Stats: 618 lines in 4 files changed: 568 ins; 26 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/3072.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3072/head:pull/3072 PR: https://git.openjdk.java.net/jdk/pull/3072
