On Mon, 6 Sep 2021 07:20:11 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 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
