Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=552397
Committed: https://crrev.com/90be5dd78a10259fbf4df3c7c181bbffebd98348
Cr-Commit-Position: refs/heads/master@{#430946}

[Bence, 2016-10-28 14:53:30]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 14:54:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 15:07:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 15:07:57]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Bence, 2016-10-28 15:40:09]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 15:40:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 16:54:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 16:54:53]

Dry run: This issue passed the CQ dry run.

[Bence, 2016-10-28 17:08:14]

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

[Bence, 2016-10-28 17:08:15]

Ryan: PTAL.  Thank you.

[Ryan Hamilton, 2016-10-28 19:02:02]

Looks good. two small questions.

https://codereview.chromium.org/2461683002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2461683002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:49:
quic_version_(session->GetQuicVersion()),
We're only speaking 1 version of QUIC at a time in Chrome, but if we ever did
version negotiation this might grab the first version not the negotiated
version. Can you also set in QuicHttpStream::DoRequestStreamComplete in case it
changed?

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
net/spdy/spdy_http_stream.cc:337:
HttpResponseInfo::ConnectionInfoToString(response_info_->connection_info);
Why is "alpn_negotiated_protocol" being set to a ConnectionInfo value?

[Bence, 2016-11-01 14:13:04]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 14:13:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 15:06:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 15:06:50]

Dry run: This issue passed the CQ dry run.

[Bence, 2016-11-01 15:11:23]

Ryan: PTAL.  Thank you.

https://codereview.chromium.org/2461683002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2461683002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:49:
quic_version_(session->GetQuicVersion()),
On 2016/10/28 19:02:02, Ryan Hamilton wrote:
> We're only speaking 1 version of QUIC at a time in Chrome, but if we ever did
> version negotiation this might grab the first version not the negotiated
> version. Can you also set in QuicHttpStream::DoRequestStreamComplete in case
it
> changed?

Hm, I see.  QuicFramer's version can change after QuicHttpStream is
instantiated, so QuicHttpStream::quic_version needs to be updated.

I don't like that QuicHttpStream needs to know about where in the state machine
can the version change and needs an update.  I would prefer to wire version
changes through the QuicChromiumClientSession::Observer interface, because it's
more explicit this way.  WDYT?

But really, I wish |session_| was not a WeakPtr, so that
session_->GetQuicVersion() could be called every time its value is needed,
instead of having to store and update |quic_version_|.

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
net/spdy/spdy_http_stream.cc:337:
HttpResponseInfo::ConnectionInfoToString(response_info_->connection_info);
On 2016/10/28 19:02:02, Ryan Hamilton wrote:
> Why is "alpn_negotiated_protocol" being set to a ConnectionInfo value?

This is for consistency with QuicHttpStream, where
response_info_->alpn_negotiated_protocol() is also set to a string generated
from ConnectionInfo.

The actual value of the alpn string is dropped in SSLClientSocketImpl by choice,
only a NextProto value generated from it is stored.  So at this point, it has to
be reconstructed somehow anyway.

Really this is just a symptom of how inaccurately |alpn_negotiated_protocol| is
named.

[Ryan Hamilton, 2016-11-03 14:33:05]

lgtm

https://codereview.chromium.org/2461683002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2461683002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:49:
quic_version_(session->GetQuicVersion()),
On 2016/11/01 15:11:23, Bence wrote:
> On 2016/10/28 19:02:02, Ryan Hamilton wrote:
> > We're only speaking 1 version of QUIC at a time in Chrome, but if we ever
did
> > version negotiation this might grab the first version not the negotiated
> > version. Can you also set in QuicHttpStream::DoRequestStreamComplete in case
> it
> > changed?
> 
> Hm, I see.  QuicFramer's version can change after QuicHttpStream is
> instantiated, so QuicHttpStream::quic_version needs to be updated.
> 
> I don't like that QuicHttpStream needs to know about where in the state
machine
> can the version change and needs an update.  I would prefer to wire version
> changes through the QuicChromiumClientSession::Observer interface, because
it's
> more explicit this way.  WDYT?

SGTM!

> But really, I wish |session_| was not a WeakPtr, so that
> session_->GetQuicVersion() could be called every time its value is needed,
> instead of having to store and update |quic_version_|.

Yeah, that'd be nice. Alas, the object lifetimes don't permit that to work
easily :(

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
net/spdy/spdy_http_stream.cc:337:
HttpResponseInfo::ConnectionInfoToString(response_info_->connection_info);
On 2016/11/01 15:11:23, Bence wrote:
> On 2016/10/28 19:02:02, Ryan Hamilton wrote:
> > Why is "alpn_negotiated_protocol" being set to a ConnectionInfo value?
> 
> This is for consistency with QuicHttpStream, where
> response_info_->alpn_negotiated_protocol() is also set to a string generated
> from ConnectionInfo.
> 
> The actual value of the alpn string is dropped in SSLClientSocketImpl by
choice,
> only a NextProto value generated from it is stored.  So at this point, it has
to
> be reconstructed somehow anyway.
> 
> Really this is just a symptom of how inaccurately |alpn_negotiated_protocol|
is
> named.

Should we remove |alpn_negotiated_protocol| at some point?

[sunjian, 2016-11-03 18:55:02]

sunjian@chromium.org changed reviewers:
+ sunjian@chromium.org

[sunjian, 2016-11-03 18:55:03]

https://codereview.chromium.org/2461683002/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (left):

https://codereview.chromium.org/2461683002/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:795:
response_info_->alpn_negotiated_protocol = "quic/1+spdy/3";
Does that mean alpn_negotiated_protocol will always be the same as
connection_info? If this is the case, then i don't one of the fallbacks in my
cl:https://codereview.chromium.org/2429063002/patch/160001/170012.

[Bence, 2016-11-07 20:22:17]

bnc@chromium.org changed reviewers:
+ holte@chromium.org, juliatuttle@chromium.org

[Bence, 2016-11-07 20:22:18]

Julia: PTAL at components/domain_reliability/.
Steven: PTAL at tools/metrics/histograms/.

Thank you all.

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/2461683002/diff/20001/net/spdy/spdy_http_stre...
net/spdy/spdy_http_stream.cc:337:
HttpResponseInfo::ConnectionInfoToString(response_info_->connection_info);
On 2016/11/03 14:33:05, Ryan Hamilton wrote:
> On 2016/11/01 15:11:23, Bence wrote:
> > On 2016/10/28 19:02:02, Ryan Hamilton wrote:
> > > Why is "alpn_negotiated_protocol" being set to a ConnectionInfo value?
> > 
> > This is for consistency with QuicHttpStream, where
> > response_info_->alpn_negotiated_protocol() is also set to a string generated
> > from ConnectionInfo.
> > 
> > The actual value of the alpn string is dropped in SSLClientSocketImpl by
> choice,
> > only a NextProto value generated from it is stored.  So at this point, it
has
> to
> > be reconstructed somehow anyway.
> > 
> > Really this is just a symptom of how inaccurately |alpn_negotiated_protocol|
> is
> > named.
> 
> Should we remove |alpn_negotiated_protocol| at some point?

Yes please, I would love that!  It's just confusing to consumers of
HttpResponseInfo, and |connection_info| has more information anyway.

[Steven Holte, 2016-11-07 20:24:23]

histograms lgtm

[Julia Tuttle, 2016-11-07 20:47:43]

domain_reliability lgtm

[Bence, 2016-11-08 13:13:06]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2016-11-08 13:13:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 13:29:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 13:29:47]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bence, 2016-11-08 14:09:52]

Description was changed from

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
==========

to

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bence, 2016-11-08 14:10:00]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 14:10:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 15:29:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 15:29:02]

Dry run: This issue passed the CQ dry run.

[Bence, 2016-11-08 17:58:55]

bnc@chromium.org changed reviewers:
+ clamy@chromium.org, csharrison@chromium.org

[Bence, 2016-11-08 17:58:56]

clamy: PTAL at content/browser/frame_host/
csharrison: PTAL at chrome/browser/page_load_metrics/

Thank you all.

https://codereview.chromium.org/2461683002/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.cc (left):

https://codereview.chromium.org/2461683002/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:795:
response_info_->alpn_negotiated_protocol = "quic/1+spdy/3";
On 2016/11/03 18:55:03, sunjian wrote:
> Does that mean alpn_negotiated_protocol will always be the same as
> connection_info? If this is the case, then i don't one of the fallbacks in my
> cl:https://codereview.chromium.org/2429063002/patch/160001/170012.

Discussed offline.  We'll work together on assessing whether
alpn_negotiated_protocol can be removed, simplifying the API and making that
fallback unnecessary.

[Charlie Harrison, 2016-11-08 18:02:44]

page_load_metrics lgtm

[Bence, 2016-11-08 23:53:40]

Description was changed from

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=552397
==========

[clamy, 2016-11-09 14:20:12]

content/ lgtm.

[Bence, 2016-11-09 14:22:52]

The CQ bit was checked by bnc@chromium.org

[Bence, 2016-11-09 14:22:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
juliatuttle@chromium.org, rch@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2461683002/#ps100001
(title: "Update recently introduced CONNECTION_INFO_QUIC occurrences.")

[commit-bot: I haz the power, 2016-11-09 14:23:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 16:29:12]

Description was changed from

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=552397
==========

to

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=552397
==========

[commit-bot: I haz the power, 2016-11-09 16:29:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-09 16:31:51]

Description was changed from

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=552397
==========

to

==========
Slice enum HttpResponseInfo::ConnectionInfo by QUIC version.

This will surface QUIC version information in both
HttpResponseInfo::connection_info and
HttpResponseInfo::alpn_negotiated_protocol, which will then show up in
HttpResponseInfo.ConnectionInfo histograms, as well as on the Web platform in
chrome.loadtimes().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=552397

Committed: https://crrev.com/90be5dd78a10259fbf4df3c7c181bbffebd98348
Cr-Commit-Position: refs/heads/master@{#430946}
==========

[commit-bot: I haz the power, 2016-11-09 16:31:52]

Patchset 6 (id:??) landed as
https://crrev.com/90be5dd78a10259fbf4df3c7c181bbffebd98348
Cr-Commit-Position: refs/heads/master@{#430946}

[xunjieli, 2016-11-09 17:13:10]

Cronet tests are failing:
https://bugs.chromium.org/p/chromium/issues/detail?id=663780