Pavel,
Overall this is very good. I have mostly minor comments.
> Though this is an implementation only package, it could use a few more
> comments
> to support maintainers. In the package.html, a couple of hints about what
> classes are
> the external interface would be useful and an example of use in Encode and/or
> Decode
> would help a lot.
+1 package-info.java is almost useless in its current form. Please a consider
adding
an example of encoding/decoding, as well as the quasi-public API ( the interface
HTTP2 is expected to use ).
A general comment; Quite a number of classes, especially Encoder and Decoder,
would benefit from a few relatively lightweight comments, e.g. IntegerReader
/**
* A reader that consumes bytes that conform to the HPACK, unsigned,
variable-length,
* Integer Representation, see section 5.1 of RFC 7541.
*/
public final class IntegerReader {
…
/**
* Attempts to read, or continues to read, the bytes from the given
* byte buffer until the Integer is reconstructed.
*
* @return true if, and only if, the Integer (@link #get() value} has
been fully
* reconstructed.
*/
public boolean read(ByteBuffer input) { … }
}
IntegerWriter.java
IntegerWriter.configure(int, int, int) could use Object.checkFromToIndex ??
StringReader.java
reset() can probably just reset its readers, regardless of huffman, since the
readers are always guaranteed to be present ( and reset is minimal ).
General comment; in many cases it would be useful to capture, at least, the
‘state’
when throwing InternalError.
IntegerReader.java
reset() assumes that there is no bugs in the code. Should is assign 0 to r,
value, and maxValue as well. Q: is it necessary to call configure after
reset?
ISO_8859_1.java
Sorry, I don’ get 'outstanding'. Are you catching RuntimeException somewhere,
or do you expect a user of the Decoder to do some special exception handling ?
HpackException.java
Do you need this ??? ( FIXME comment )
-Chris.
> On 4/6/2016 2:00 PM, Pavel Rappo wrote:
>> Hi,
>>
>> Could you please review my change for JDK-8153353?
>>
>> http://cr.openjdk.java.net/~prappo/8153353/webrev.00/
>>
>> This is an implementation of HPACK (Header Compression for HTTP/2) [1]
>> Internal API classes are (package sun.net.httpclient.hpack):
>>
>> Encoder, Decoder and DecodingCallback
>>
>> Example of their usage can be seen in the RFR for HTTP/2 implementation
>> published previously today [2, 3].
>>
>> Thanks,
>> -Pavel
>>
>> --------------------------------------------------------------------------------
>> [1] https://tools.ietf.org/html/rfc7541
>> [2]
>> http://cr.openjdk.java.net/~michaelm/8087124/webrev.1/src/java.httpclient/share/classes/java/net/http/Http2Connection.java.html
>> 304:306
>> [3]
>> http://cr.openjdk.java.net/~michaelm/8087124/webrev.1/src/java.httpclient/share/classes/java/net/http/Http2Connection.java.html
>> 682:695
>
