[PageLoadMetrics] Create page load timing metrics for H2/QUIC/H1 pages

Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bb150114cab368d5fcb45b9e3ee8c8f7d500921b
Cr-Commit-Position: refs/heads/master@{#429333}

[jkarlin, 2016-10-28 14:57:52]

Description was changed from

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
==========

to

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jkarlin, 2016-10-28 15:33:04]

Patchset #2 (id:20001) has been deleted

[jkarlin, 2016-10-28 15:33:09]

Patchset #1 (id:1) has been deleted

[jkarlin, 2016-10-28 15:34:01]

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

[commit-bot: I haz the power, 2016-10-28 15:34:18]

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

[jkarlin, 2016-10-28 15:36:24]

jkarlin@chromium.org changed reviewers:
+ clamy@chromium.org, csharrison@chromium.org, rkaplow@chromium.org

[jkarlin, 2016-10-28 15:36:25]

csharrison@chromium.org: PTAL at everything

clamy@chromium.org: Please review changes in content/

rkaplow@chromium.org: PTAL at histograms.xml

Thanks all!

[jkarlin, 2016-10-28 15:42:23]

Description was changed from

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-28 15:45:00]

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

[commit-bot: I haz the power, 2016-10-28 15:45:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2016-10-28 15:54:12]

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

[commit-bot: I haz the power, 2016-10-28 15:54:35]

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:56:57]

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

[commit-bot: I haz the power, 2016-10-28 16:56:58]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2016-10-28 19:24:25]

lgtm

[Charlie Harrison, 2016-10-31 15:27:17]

Generally LG.

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc:29:
case net::HttpResponseInfo::CONNECTION_INFO_HTTP1_1:
Did you consider merging 1.1 and 1.0 traffic?

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.h:35:
net::HttpResponseInfo::ConnectionInfo connection_info_;
#include "net/http/http_response_info.h"

https://codereview.chromium.org/2457583007/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2457583007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:231: return
connection_info_;
Hm it seems strange that we will get something unknown if this method is called
before the connection info is set in this class.

I wonder if we can DCHECK that accesses here are definitely after the value is
set?

[jkarlin, 2016-10-31 16:50:06]

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc:29:
case net::HttpResponseInfo::CONNECTION_INFO_HTTP1_1:
On 2016/10/31 15:27:17, Charlie Harrison wrote:
> Did you consider merging 1.1 and 1.0 traffic?

There are substantial differences between 1.1 and 1.0 (persistent connections,
caching improvements) that I'd just assume keep separate. Also, only .6% of
requests are 1.0 these days.

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.h:35:
net::HttpResponseInfo::ConnectionInfo connection_info_;
On 2016/10/31 15:27:17, Charlie Harrison wrote:
> #include "net/http/http_response_info.h"

Done.

https://codereview.chromium.org/2457583007/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2457583007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:231: return
connection_info_;
On 2016/10/31 15:27:17, Charlie Harrison wrote:
> Hm it seems strange that we will get something unknown if this method is
called
> before the connection info is set in this class.
> 
> I wonder if we can DCHECK that accesses here are definitely after the value is
> set?

Thanks. I've documented the behavior in the header file.  If we want to verify
that it was set here, it'd be better to use a unique_ptr and check that it's not
nullptr. The check will fail in some tests that mock this stuff, not sure how
hard that would be to fix. Let me know what you think.

[Charlie Harrison, 2016-10-31 16:54:58]

https://codereview.chromium.org/2457583007/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2457583007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:231: return
connection_info_;
On 2016/10/31 16:50:06, jkarlin wrote:
> On 2016/10/31 15:27:17, Charlie Harrison wrote:
> > Hm it seems strange that we will get something unknown if this method is
> called
> > before the connection info is set in this class.
> > 
> > I wonder if we can DCHECK that accesses here are definitely after the value
is
> > set?
> 
> Thanks. I've documented the behavior in the header file.  If we want to verify
> that it was set here, it'd be better to use a unique_ptr and check that it's
not
> nullptr. The check will fail in some tests that mock this stuff, not sure how
> hard that would be to fix. Let me know what you think.

It is unfortunate to add more things to NavigationHandle which require plumbing
through the navigation unit testing code if we want to do things "properly".
Still, this approach lgtm assuming clamy@ is fine with it.

[clamy, 2016-11-02 14:09:27]

Thanks! It looks mostly good, a few questions below.

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:148:
net::HttpResponseInfo::CONNECTION_INFO_QUIC, SSLStatus(),
Why do you use different values in WillRedirectRequest & WillProcessResponse?

https://codereview.chromium.org/2457583007/diff/120001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2457583007/diff/120001/content/public/browser...
content/public/browser/navigation_handle.h:172: // CONNECTION_INFO_UNKNOWN if
there hasn't been a response yet. The
nit: precise that a response may mean a redirect.

[jkarlin, 2016-11-02 15:06:55]

Patchset #6 (id:140001) has been deleted

[jkarlin, 2016-11-02 15:13:57]

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

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

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

[jkarlin, 2016-11-02 15:14:21]

Thanks clamy@! PTAL.

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:148:
net::HttpResponseInfo::CONNECTION_INFO_QUIC, SSLStatus(),
On 2016/11/02 14:09:27, clamy (slow) wrote:
> Why do you use different values in WillRedirectRequest & WillProcessResponse?

To verify that the connection info is plumbed through for both redirects and
process response. See the SimpleDataChecks test below.

https://codereview.chromium.org/2457583007/diff/120001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2457583007/diff/120001/content/public/browser...
content/public/browser/navigation_handle.h:172: // CONNECTION_INFO_UNKNOWN if
there hasn't been a response yet. The
On 2016/11/02 14:09:27, clamy (slow) wrote:
> nit: precise that a response may mean a redirect.

Done.

[clamy, 2016-11-02 15:32:33]

Thanks! content/ lgtm with one comment.

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:148:
net::HttpResponseInfo::CONNECTION_INFO_QUIC, SSLStatus(),
On 2016/11/02 15:14:21, jkarlin wrote:
> On 2016/11/02 14:09:27, clamy (slow) wrote:
> > Why do you use different values in WillRedirectRequest &
WillProcessResponse?
> 
> To verify that the connection info is plumbed through for both redirects and
> process response. See the SimpleDataChecks test below.

Acknowledged. Could you add a comment explaining that?

[jkarlin, 2016-11-02 15:44:48]

Thanks!

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2457583007/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:148:
net::HttpResponseInfo::CONNECTION_INFO_QUIC, SSLStatus(),
On 2016/11/02 15:32:33, clamy (slow) wrote:
> On 2016/11/02 15:14:21, jkarlin wrote:
> > On 2016/11/02 14:09:27, clamy (slow) wrote:
> > > Why do you use different values in WillRedirectRequest &
> WillProcessResponse?
> > 
> > To verify that the connection info is plumbed through for both redirects and
> > process response. See the SimpleDataChecks test below.
> 
> Acknowledged. Could you add a comment explaining that?

Done.

[jkarlin, 2016-11-02 15:50:01]

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

[commit-bot: I haz the power, 2016-11-02 15:50:21]

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

[jkarlin, 2016-11-02 16:46:05]

The CQ bit was unchecked by jkarlin@chromium.org

[jkarlin, 2016-11-02 16:46:12]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2016-11-02 16:46:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, csharrison@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2457583007/#ps180001
(title: "Address comments from PS6")

[commit-bot: I haz the power, 2016-11-02 16:46:36]

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-02 17:55:34]

Description was changed from

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-02 17:55:36]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-11-02 18:23:24]

Description was changed from

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Create page load timing metrics for H2/QUIC/H1 pages

This CL pipes connection information through the NavigationHandle so that it
can be logged by a new page load metrics observer.

BUG=660378
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bb150114cab368d5fcb45b9e3ee8c8f7d500921b
Cr-Commit-Position: refs/heads/master@{#429333}
==========

[commit-bot: I haz the power, 2016-11-02 18:23:25]

Patchset 7 (id:??) landed as
https://crrev.com/bb150114cab368d5fcb45b9e3ee8c8f7d500921b
Cr-Commit-Position: refs/heads/master@{#429333}

[Bryan McQuade, 2016-11-05 00:24:01]

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

[Bryan McQuade, 2016-11-05 00:24:02]

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc:29:
case net::HttpResponseInfo::CONNECTION_INFO_HTTP1_1:
On 2016/10/31 at 16:50:06, jkarlin wrote:
> On 2016/10/31 15:27:17, Charlie Harrison wrote:
> > Did you consider merging 1.1 and 1.0 traffic?
> 
> There are substantial differences between 1.1 and 1.0 (persistent connections,
caching improvements) that I'd just assume keep separate. Also, only .6% of
requests are 1.0 these days.

If we're only going to log 1.1 I think we should rename the histogram to 'H11'
to be clear - H1 generally means HTTP/1.x (I definitely assumed it was 1.0 and
1.1 when I was looking at the histograms). Any concerns with this? I can send
out a patch for this change.

BTW where do you get the stat that H10 is only 0.6% of requests? Is there a net
histogram for that?

[jkarlin, 2016-11-07 13:07:36]

On 2016/11/05 00:24:02, Bryan McQuade wrote:
>
https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
> File
>
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc
> (right):
> 
>
https://codereview.chromium.org/2457583007/diff/80001/chrome/browser/page_loa...
>
chrome/browser/page_load_metrics/observers/protocol_page_load_metrics_observer.cc:29:
> case net::HttpResponseInfo::CONNECTION_INFO_HTTP1_1:
> On 2016/10/31 at 16:50:06, jkarlin wrote:
> > On 2016/10/31 15:27:17, Charlie Harrison wrote:
> > > Did you consider merging 1.1 and 1.0 traffic?
> > 
> > There are substantial differences between 1.1 and 1.0 (persistent
connections,
> caching improvements) that I'd just assume keep separate. Also, only .6% of
> requests are 1.0 these days.
> 
> If we're only going to log 1.1 I think we should rename the histogram to 'H11'
> to be clear - H1 generally means HTTP/1.x (I definitely assumed it was 1.0 and
> 1.1 when I was looking at the histograms). Any concerns with this? I can send
> out a patch for this change.
> 
> BTW where do you get the stat that H10 is only 0.6% of requests? Is there a
net
> histogram for that?

That's fine with me. Net.HttpResponseInfo.ConnectionInfo.MainFrame shows the
protocol data.