Process HTTP/2 ALTSVC frames.

Process HTTP/2 ALTSVC frames.

BUG=617177
Committed: https://crrev.com/1e6a36e2596fb48df03ec8a1d736b82f8e38e4f3
Cr-Commit-Position: refs/heads/master@{#397894}

[Bence, 2016-06-03 17:50:05]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2016-06-03 17:50:06]

Ryan: PTAL.  Thank you.

[Ryan Hamilton, 2016-06-03 19:05:18]

Dang! That was fast. NICE! Just a couple minor comments.

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/buffered_spdy_...
File net/spdy/buffered_spdy_framer_unittest.cc (right):

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/buffered_spdy_...
net/spdy/buffered_spdy_framer_unittest.cc:138: altsvc_vector) override {}
perhaps we could save the alt svc vector and write a test which makes sure it's
the same?

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

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:2498: return;
I'm surprised that this is, in the spec, to be ignored instead of closing the
connection with an error.

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:2524: AlternativeServiceInfoVector
alternative_service_info_vector;
I think there's a constructor you can call to set the size which you could use
to set it to the size of the input vector to (possibly) avoid a reallocation.

[Bence, 2016-06-03 20:57:52]

PTAL.  Thank you.

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/buffered_spdy_...
File net/spdy/buffered_spdy_framer_unittest.cc (right):

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/buffered_spdy_...
net/spdy/buffered_spdy_framer_unittest.cc:138: altsvc_vector) override {}
On 2016/06/03 19:05:18, Ryan Hamilton wrote:
> perhaps we could save the alt svc vector and write a test which makes sure
it's
> the same?

Done.

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

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:2498: return;
On 2016/06/03 19:05:18, Ryan Hamilton wrote:
> I'm surprised that this is, in the spec, to be ignored instead of closing the
> connection with an error.

Acknowledged.

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:2524: AlternativeServiceInfoVector
alternative_service_info_vector;
On 2016/06/03 19:05:18, Ryan Hamilton wrote:
> I think there's a constructor you can call to set the size which you could use
> to set it to the size of the input vector to (possibly) avoid a reallocation.

Didn't find a constructor call, how about std::vector::reserve()?

[Ryan Hamilton, 2016-06-03 21:26:19]

lgtm, but one suggestion

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

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:2524: AlternativeServiceInfoVector
alternative_service_info_vector;
On 2016/06/03 20:57:52, Bence wrote:
> On 2016/06/03 19:05:18, Ryan Hamilton wrote:
> > I think there's a constructor you can call to set the size which you could
use
> > to set it to the size of the input vector to (possibly) avoid a
reallocation.
> 
> Didn't find a constructor call, how about std::vector::reserve()?

Does this work?

http://en.cppreference.com/w/cpp/container/vector/vector
explicit vector( size_type count );
3) Constructs the container with count default-inserted instances of T. No
copies are made.

[Bence, 2016-06-04 02:41:32]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2016-06-04 02:41:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031373002/40001

[Bence, 2016-06-04 02:43:55]

Thank you.

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

https://codereview.chromium.org/2031373002/diff/20001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:2524: AlternativeServiceInfoVector
alternative_service_info_vector;
On 2016/06/03 21:26:18, Ryan Hamilton wrote:
> On 2016/06/03 20:57:52, Bence wrote:
> > On 2016/06/03 19:05:18, Ryan Hamilton wrote:
> > > I think there's a constructor you can call to set the size which you could
> use
> > > to set it to the size of the input vector to (possibly) avoid a
> reallocation.
> > 
> > Didn't find a constructor call, how about std::vector::reserve()?
> 
> Does this work?
> 
> http://en.cppreference.com/w/cpp/container/vector/vector
> explicit vector( size_type count );
> 3) Constructs the container with count default-inserted instances of T. No
> copies are made.

It could work, but note that the for loop can skip entries if |protocol_id|
string cannot be parsed into an AlternateProtocol, and if so, placeholder for
those omitted entries would manually have to be erased from the end.  So I would
need to have a running int i counter, do vector[i] = AltSvcInfo(...) instead of
push_back(), and in the end, delete size()-i or so entries from the end.  If you
see an easier way, please let me know, I'm happy to do a follow-up CL.

[commit-bot: I haz the power, 2016-06-04 02:45:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-04 02:48:04]

Description was changed from

==========
Process HTTP/2 ALTSVC frames.

BUG=617177
==========

to

==========
Process HTTP/2 ALTSVC frames.

BUG=617177

Committed: https://crrev.com/1e6a36e2596fb48df03ec8a1d736b82f8e38e4f3
Cr-Commit-Position: refs/heads/master@{#397894}
==========

[commit-bot: I haz the power, 2016-06-04 02:48:05]

Patchset 3 (id:??) landed as
https://crrev.com/1e6a36e2596fb48df03ec8a1d736b82f8e38e4f3
Cr-Commit-Position: refs/heads/master@{#397894}