Update HPACK implementation to draft-09

* Remove the reference set.
* Exchange header and static table positions.

BUG=400336
This effort is continued on a different code review site, therefore this CL is closed without merging.

[Johnny, 2014-08-06 15:04:50]

Nice! A bunch of inline comments, but all pretty minor stuff.

This ought to land internally first, so once you're happy with it I'd patch this
into the internal tree and start a review there.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.cc
File net/spdy/hpack_decoder.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.c...
net/spdy/hpack_decoder.cc:7: #include <utility>
I don't see a use of this below?

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.c...
net/spdy/hpack_decoder.cc:155: header_table_.TryAddEntry(name, value);
ignore_result

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
File net/spdy/hpack_decoder_test.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:122: // fields should not show up in the next
block unless explicitly encoded.
Can probably just strike all mentions of "reference set" from the record.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:215: // the reference set, making a copy of
static table entries if
Comment update.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:506: 
nit: examples have inconsistent newlines here.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.cc
File net/spdy/hpack_encoder.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.c...
net/spdy/hpack_encoder.cc:41: }
:pseudo-headers must appear first. I think this Just Works due to the magic of
ASCII codepoint ordering, but would be good to document the requirement (and add
tests for it).

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.h
File net/spdy/hpack_encoder.h (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.h...
net/spdy/hpack_encoder.h:10: #include <utility>
Not used.

[Bence, 2014-08-06 20:20:23]

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.cc
File net/spdy/hpack_decoder.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.c...
net/spdy/hpack_decoder.cc:7: #include <utility>
On 2014/08/06 15:04:49, Johnny wrote:
> I don't see a use of this below?

It's for std::pair.  git cl lint complains if this include is missing.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.c...
net/spdy/hpack_decoder.cc:155: header_table_.TryAddEntry(name, value);
On 2014/08/06 15:04:49, Johnny wrote:
> ignore_result

Done.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
File net/spdy/hpack_decoder_test.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:122: // fields should not show up in the next
block unless explicitly encoded.
On 2014/08/06 15:04:49, Johnny wrote:
> Can probably just strike all mentions of "reference set" from the record.

Should I also remove the next line?
EXPECT_TRUE(DecodeHeaderBlock("\x82\x86"));

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:215: // the reference set, making a copy of
static table entries if
On 2014/08/06 15:04:50, Johnny wrote:
> Comment update.

Done.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:506: 
On 2014/08/06 15:04:50, Johnny wrote:
> nit: examples have inconsistent newlines here.

Done.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.h
File net/spdy/hpack_encoder.h (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.h...
net/spdy/hpack_encoder.h:10: #include <utility>
On 2014/08/06 15:04:50, Johnny wrote:
> Not used.

git cl lint complaining about it when using std::pair.

[Johnny, 2014-08-07 17:02:23]

Still looks good. IMO this is ready to land, just should happen on the internal
tree first.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.cc
File net/spdy/hpack_decoder.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.c...
net/spdy/hpack_decoder.cc:7: #include <utility>
On 2014/08/06 20:20:22, Bence wrote:
> On 2014/08/06 15:04:49, Johnny wrote:
> > I don't see a use of this below?
> 
> It's for std::pair.  git cl lint complains if this include is missing.

Oh, great. SGTM then.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
File net/spdy/hpack_decoder_test.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:122: // fields should not show up in the next
block unless explicitly encoded.
I'd probably move "\x82\x86" (corrected for the right indices) into L128's
HandleControlFrameHeadersData, just so that :method and :path expectations don't
need to be removed.

I'm also fine with just dropping it.

[Bence, 2014-08-08 13:57:56]

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.cc
File net/spdy/hpack_decoder.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder.c...
net/spdy/hpack_decoder.cc:7: #include <utility>
On 2014/08/07 17:02:22, Johnny wrote:
> On 2014/08/06 20:20:22, Bence wrote:
> > On 2014/08/06 15:04:49, Johnny wrote:
> > > I don't see a use of this below?
> > 
> > It's for std::pair.  git cl lint complains if this include is missing.
> 
> Oh, great. SGTM then.

Acknowledged.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
File net/spdy/hpack_decoder_test.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_decoder_t...
net/spdy/hpack_decoder_test.cc:122: // fields should not show up in the next
block unless explicitly encoded.
On 2014/08/07 17:02:22, Johnny wrote:
> I'd probably move "\x82\x86" (corrected for the right indices) into L128's
> HandleControlFrameHeadersData, just so that :method and :path expectations
don't
> need to be removed.
> 
> I'm also fine with just dropping it.

Done.

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.cc
File net/spdy/hpack_encoder.cc (right):

https://codereview.chromium.org/448433002/diff/20001/net/spdy/hpack_encoder.c...
net/spdy/hpack_encoder.cc:41: }
On 2014/08/06 15:04:50, Johnny wrote:
> :pseudo-headers must appear first. I think this Just Works due to the magic of
> ASCII codepoint ordering, but would be good to document the requirement (and
add
> tests for it).

I agree.  I'll do this on a separate CL.