Associate a main resource request with its PageLoadTracker.

Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSXYM/edit
for more details.

BUG=680983
Review-Url: https://codereview.chromium.org/2624283004
Cr-Commit-Position: refs/heads/master@{#444159}
Committed: https://chromium.googlesource.com/chromium/src/+/67906e1cd7937494e078325ead8bdb95093bbf29

[Bryan McQuade, 2017-01-12 17:38:12]

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

[commit-bot: I haz the power, 2017-01-12 17:38:46]

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

[Bryan McQuade, 2017-01-12 17:41:18]

Description was changed from

==========
Associate a main resource request with its PageLoadTracker.

BUG=
==========

to

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with attributes that
match the main resource request.

BUG=
==========

[Bryan McQuade, 2017-01-12 17:47:55]

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

[commit-bot: I haz the power, 2017-01-12 17:48:14]

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

[Bryan McQuade, 2017-01-12 17:50:54]

Description was changed from

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with attributes that
match the main resource request.

BUG=
==========

to

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with attributes that
match the main resource request.

This change also makes a small change to URL tracking in PageLoadTracker.
We now keep track of both the initial url (start_url_) as well as the
current URL, updated at redirect time and at commit time. This is more
generic and useful than the previous implementation, which tracked only
the start URL and the committed URL.

BUG=
==========

[Bryan McQuade, 2017-01-12 18:00:38]

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

[commit-bot: I haz the power, 2017-01-12 18:00:56]

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, 2017-01-12 18:46:06]

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

[commit-bot: I haz the power, 2017-01-12 18:46:09]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-01-12 20:14:31]

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

[Bryan McQuade, 2017-01-12 20:15:08]

Description was changed from

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with attributes that
match the main resource request.

This change also makes a small change to URL tracking in PageLoadTracker.
We now keep track of both the initial url (start_url_) as well as the
current URL, updated at redirect time and at commit time. This is more
generic and useful than the previous implementation, which tracked only
the start URL and the committed URL.

BUG=
==========

to

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

BUG=
==========

[commit-bot: I haz the power, 2017-01-12 20:15:46]

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, 2017-01-12 22:58:51]

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

[commit-bot: I haz the power, 2017-01-12 22:58:52]

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

[Bryan McQuade, 2017-01-13 01:03:10]

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

[commit-bot: I haz the power, 2017-01-13 01:04:59]

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

[Bryan McQuade, 2017-01-13 01:46:35]

Description was changed from

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

BUG=
==========

to

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
for more details.

BUG=
==========

[Bryan McQuade, 2017-01-13 01:48:49]

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

[commit-bot: I haz the power, 2017-01-13 01:49:40]

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

[Bryan McQuade, 2017-01-13 01:54:46]

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

[commit-bot: I haz the power, 2017-01-13 01:55:51]

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

[Bryan McQuade, 2017-01-13 02:27:01]

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

[commit-bot: I haz the power, 2017-01-13 02:27:21]

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

[Bryan McQuade, 2017-01-13 02:44:18]

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

[commit-bot: I haz the power, 2017-01-13 02:44:28]

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, 2017-01-13 05:52:05]

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

[commit-bot: I haz the power, 2017-01-13 05:52:05]

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

[Bryan McQuade, 2017-01-13 12:42:23]

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

[commit-bot: I haz the power, 2017-01-13 12:42:32]

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

[Bryan McQuade, 2017-01-13 13:23:34]

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

[commit-bot: I haz the power, 2017-01-13 13:23:49]

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, 2017-01-13 14:16:34]

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

[commit-bot: I haz the power, 2017-01-13 14:16:35]

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

[Bryan McQuade, 2017-01-13 14:29:38]

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

[commit-bot: I haz the power, 2017-01-13 14:29:51]

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

[Bryan McQuade, 2017-01-13 14:32:06]

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

[commit-bot: I haz the power, 2017-01-13 14:32:13]

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

[Bryan McQuade, 2017-01-13 14:58:04]

Description was changed from

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
for more details.

BUG=
==========

to

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
for more details.

BUG=680983
==========

[Bryan McQuade, 2017-01-13 15:22:33]

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

[commit-bot: I haz the power, 2017-01-13 15:22:52]

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

[Bryan McQuade, 2017-01-13 15:23:16]

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

[Bryan McQuade, 2017-01-13 15:23:35]

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

[Bryan McQuade, 2017-01-13 15:23:51]

PTAL, thanks!

[RyanSturm, 2017-01-13 15:39:49]

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

[RyanSturm, 2017-01-13 15:39:50]

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
(const auto& kv : provisional_loads_) {
I'd actually suggest going a step further for the cases where the load hasn't
committed yet. If the request completes before the browser is ready to commit
due to some deferral in NavigationThrottle, you might want to store this request
info for when these provisional_loads_ commit or are deleted.

For instance, if you don't find a match in the committed loads, you could send a
copy of ExtraRequestInfo + request_id to each provisional load, when the load is
switched from provisional to committed, you could process the matching
ExtraRequestInfo (if there is one) in the observers at that point and throw the
rest out.

I bring this up because I think ReadyToCommitNavigation happens very shortly
before DidFinishNavigation (maybe even the same stack?, I'm not sure how
transfer navigations work here either), so you might not be buying anything for
requests that complete before commit.

[Bryan McQuade, 2017-01-13 15:47:14]

Thanks! Can you give a little more detail on the benefits to deferring the
callback for provisional loads until commit? I am not seeing what issues that
addresses & it does add complexity.

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
(const auto& kv : provisional_loads_) {
On 2017/01/13 at 15:39:50, Ryan Sturm wrote:
> I'd actually suggest going a step further for the cases where the load hasn't
committed yet. If the request completes before the browser is ready to commit
due to some deferral in NavigationThrottle, you might want to store this request
info for when these provisional_loads_ commit or are deleted.
> 
> For instance, if you don't find a match in the committed loads, you could send
a copy of ExtraRequestInfo + request_id to each provisional load, when the load
is switched from provisional to committed, you could process the matching
ExtraRequestInfo (if there is one) in the observers at that point and throw the
rest out.
> 
> I bring this up because I think ReadyToCommitNavigation happens very shortly
before DidFinishNavigation (maybe even the same stack?, I'm not sure how
transfer navigations work here either), so you might not be buying anything for
requests that complete before commit.

Ah, interesting, what problem would we solve by storing and deferring for non
committed loads? I don't see any significant differences in behavior & it adds
complexity to buffer until commit (it's not something we generally do with other
activity that can happen pre-commit, such as redirects or page visibility state
changes. My inclination is to keep this simple, since I don't think it affects
the observers in any significant way and just adds complexity here - they'll get
the OnLoadedResource callback before commit, and can increment any byte counters
etc, just as they would if we invoke the same callback later after commit.

[Bryan McQuade, 2017-01-13 15:53:17]

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
(const auto& kv : provisional_loads_) {
On 2017/01/13 at 15:47:14, Bryan McQuade wrote:
> On 2017/01/13 at 15:39:50, Ryan Sturm wrote:
> > I'd actually suggest going a step further for the cases where the load
hasn't committed yet. If the request completes before the browser is ready to
commit due to some deferral in NavigationThrottle, you might want to store this
request info for when these provisional_loads_ commit or are deleted.
> > 
> > For instance, if you don't find a match in the committed loads, you could
send a copy of ExtraRequestInfo + request_id to each provisional load, when the
load is switched from provisional to committed, you could process the matching
ExtraRequestInfo (if there is one) in the observers at that point and throw the
rest out.
> > 
> > I bring this up because I think ReadyToCommitNavigation happens very shortly
before DidFinishNavigation (maybe even the same stack?, I'm not sure how
transfer navigations work here either), so you might not be buying anything for
requests that complete before commit.
> 
> Ah, interesting, what problem would we solve by storing and deferring for non
committed loads? I don't see any significant differences in behavior & it adds
complexity to buffer until commit (it's not something we generally do with other
activity that can happen pre-commit, such as redirects or page visibility state
changes. My inclination is to keep this simple, since I don't think it affects
the observers in any significant way and just adds complexity here - they'll get
the OnLoadedResource callback before commit, and can increment any byte counters
etc, just as they would if we invoke the same callback later after commit.

Ah, maybe you are concerned that we won't have the GlobalRequestID at the point
the response completes? My analysis of call stack flows in the associated doc
suggests that's not the case, and my local testing confirms this. It's an
interesting idea to hold on to the ExtraRequestInfo just in case - that does
make the code more resilient. But I'm not sure it's worth the complexity
tradeoff given that the call ordering guarantees seem to match what we expect.

I'd propose that we proceed w/ this or something similar for now, and before we
add complexity, we have a bigger discussion w/ clamy to understand constraints
here & make changes as needed.

[Charlie Harrison, 2017-01-13 15:56:23]

Just nits.

I agree with bmcquade on the complexity tradeoff here.

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:187: //
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
optional: Can you shorten this with a https://goo.gl/ link?

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:209: // 5. the
URLRequests initiated by A complete
Shouldn't the request be cancelled? I guess it could be a detachable request
link link prefetch.

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:76: void
OnRequestComplete(const content::GlobalRequestID& request_id,
#include "content/public/browser/global_request_id.h"

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:431: // When browser side
navigation is enabled,
nit: Just replace this line with "PlzNavigate:", which is more common in
PlzNavigate code and is the common way of describing PlzNavigate-only paths.

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:440:
DCHECK(navigation_request_id_.value() != content::GlobalRequestID());
nit: DCHECK_NE

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:616: DCHECK(request_id !=
content::GlobalRequestID());
ditto

[Charlie Harrison, 2017-01-13 15:58:30]

Would you mind adding a test?

[RyanSturm, 2017-01-13 16:15:46]

Perhaps this should land and we can re-visit after new UMA comes in from canary.
This is much better than what is currently in, so seems fine to me.

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
(const auto& kv : provisional_loads_) {
On 2017/01/13 15:53:17, Bryan McQuade wrote:
> On 2017/01/13 at 15:47:14, Bryan McQuade wrote:
> > On 2017/01/13 at 15:39:50, Ryan Sturm wrote:
> > > I'd actually suggest going a step further for the cases where the load
> hasn't committed yet. If the request completes before the browser is ready to
> commit due to some deferral in NavigationThrottle, you might want to store
this
> request info for when these provisional_loads_ commit or are deleted.
> > > 
> > > For instance, if you don't find a match in the committed loads, you could
> send a copy of ExtraRequestInfo + request_id to each provisional load, when
the
> load is switched from provisional to committed, you could process the matching
> ExtraRequestInfo (if there is one) in the observers at that point and throw
the
> rest out.
> > > 
> > > I bring this up because I think ReadyToCommitNavigation happens very
shortly
> before DidFinishNavigation (maybe even the same stack?, I'm not sure how
> transfer navigations work here either), so you might not be buying anything
for
> requests that complete before commit.
> > 
> > Ah, interesting, what problem would we solve by storing and deferring for
non
> committed loads? I don't see any significant differences in behavior & it adds
> complexity to buffer until commit (it's not something we generally do with
other
> activity that can happen pre-commit, such as redirects or page visibility
state
> changes. My inclination is to keep this simple, since I don't think it affects
> the observers in any significant way and just adds complexity here - they'll
get
> the OnLoadedResource callback before commit, and can increment any byte
counters
> etc, just as they would if we invoke the same callback later after commit.
> 
> Ah, maybe you are concerned that we won't have the GlobalRequestID at the
point
> the response completes? My analysis of call stack flows in the associated doc
> suggests that's not the case, and my local testing confirms this. It's an
> interesting idea to hold on to the ExtraRequestInfo just in case - that does
> make the code more resilient. But I'm not sure it's worth the complexity
> tradeoff given that the call ordering guarantees seem to match what we expect.
> 
> I'd propose that we proceed w/ this or something similar for now, and before
we
> add complexity, we have a bigger discussion w/ clamy to understand constraints
> here & make changes as needed.

Like you said, clamy@ is the expert here, but I believe it is possible for a
response to be deferred by a navigation throttle in
NavigationHandleImpl::WillProcessResponse. 

For example:
ClearSiteDataThrottle::WillProcessResponse() can defer the response preventing
commit for some time. If the main page resource is small, I believe this can
cause the URLRequest completion event to get to the UI thread before the load is
committed. Admittedly, I have not verified it or dug through NavigationThrottles
to know exactly what they do.

It's not really evident to me how long a navigation throttle can defer
navigations.

I'm not opposed to this landing, and I would be interested to know how many 0
bytes cases this fixes. That said, knowing the complexity of chrome and that
there is a deferral mechanism that will defer committing while the IO thread
runs, I'd be surprised if this didn't run into problems later.

[Bryan McQuade, 2017-01-13 16:34:01]

On 2017/01/13 at 16:15:46, ryansturm wrote:
> Perhaps this should land and we can re-visit after new UMA comes in from
canary. This is much better than what is currently in, so seems fine to me.
> 
>
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
(const auto& kv : provisional_loads_) {
> On 2017/01/13 15:53:17, Bryan McQuade wrote:
> > On 2017/01/13 at 15:47:14, Bryan McQuade wrote:
> > > On 2017/01/13 at 15:39:50, Ryan Sturm wrote:
> > > > I'd actually suggest going a step further for the cases where the load
> > hasn't committed yet. If the request completes before the browser is ready
to
> > commit due to some deferral in NavigationThrottle, you might want to store
this
> > request info for when these provisional_loads_ commit or are deleted.
> > > > 
> > > > For instance, if you don't find a match in the committed loads, you
could
> > send a copy of ExtraRequestInfo + request_id to each provisional load, when
the
> > load is switched from provisional to committed, you could process the
matching
> > ExtraRequestInfo (if there is one) in the observers at that point and throw
the
> > rest out.
> > > > 
> > > > I bring this up because I think ReadyToCommitNavigation happens very
shortly
> > before DidFinishNavigation (maybe even the same stack?, I'm not sure how
> > transfer navigations work here either), so you might not be buying anything
for
> > requests that complete before commit.
> > > 
> > > Ah, interesting, what problem would we solve by storing and deferring for
non
> > committed loads? I don't see any significant differences in behavior & it
adds
> > complexity to buffer until commit (it's not something we generally do with
other
> > activity that can happen pre-commit, such as redirects or page visibility
state
> > changes. My inclination is to keep this simple, since I don't think it
affects
> > the observers in any significant way and just adds complexity here - they'll
get
> > the OnLoadedResource callback before commit, and can increment any byte
counters
> > etc, just as they would if we invoke the same callback later after commit.
> > 
> > Ah, maybe you are concerned that we won't have the GlobalRequestID at the
point
> > the response completes? My analysis of call stack flows in the associated
doc
> > suggests that's not the case, and my local testing confirms this. It's an
> > interesting idea to hold on to the ExtraRequestInfo just in case - that does
> > make the code more resilient. But I'm not sure it's worth the complexity
> > tradeoff given that the call ordering guarantees seem to match what we
expect.
> > 
> > I'd propose that we proceed w/ this or something similar for now, and before
we
> > add complexity, we have a bigger discussion w/ clamy to understand
constraints
> > here & make changes as needed.
> 
> Like you said, clamy@ is the expert here, but I believe it is possible for a
response to be deferred by a navigation throttle in
NavigationHandleImpl::WillProcessResponse. 
> 
> For example:
> ClearSiteDataThrottle::WillProcessResponse() can defer the response preventing
commit for some time. If the main page resource is small, I believe this can
cause the URLRequest completion event to get to the UI thread before the load is
committed. Admittedly, I have not verified it or dug through NavigationThrottles
to know exactly what they do.
> 
> It's not really evident to me how long a navigation throttle can defer
navigations.
> 
> I'm not opposed to this landing, and I would be interested to know how many 0
bytes cases this fixes. That said, knowing the complexity of chrome and that
there is a deferral mechanism that will defer committing while the IO thread
runs, I'd be surprised if this didn't run into problems later.

Yeah, that is totally right for code that's in the NavThrottle code path.
ReadyToCommitNav is not in this path, fortunately - it's in the same call stack
but diverges before the NavThrottle flow so as best I understand, calls to it
won't be delayed due to NavThrottles.

RE: 0 byte cases, because the histogram uses kB rather than bytes, we won't know
with certainty how many zero bytes vs rounded down from <1/2kb to 0kb cases we
are encountering. I will see if I can come up w/ a way to track the zero byte
cases, maybe with a counter or a separate temporary histogram.

[Bryan McQuade, 2017-01-13 16:40:21]

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

[commit-bot: I haz the power, 2017-01-13 16:40:56]

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

[Bryan McQuade, 2017-01-13 16:49:25]

On 2017/01/13 at 16:34:01, Bryan McQuade wrote:
> On 2017/01/13 at 16:15:46, ryansturm wrote:
> > Perhaps this should land and we can re-visit after new UMA comes in from
canary. This is much better than what is currently in, so seems fine to me.
> > 
> >
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> > 
> >
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
(const auto& kv : provisional_loads_) {
> > On 2017/01/13 15:53:17, Bryan McQuade wrote:
> > > On 2017/01/13 at 15:47:14, Bryan McQuade wrote:
> > > > On 2017/01/13 at 15:39:50, Ryan Sturm wrote:
> > > > > I'd actually suggest going a step further for the cases where the load
> > > hasn't committed yet. If the request completes before the browser is ready
to
> > > commit due to some deferral in NavigationThrottle, you might want to store
this
> > > request info for when these provisional_loads_ commit or are deleted.
> > > > > 
> > > > > For instance, if you don't find a match in the committed loads, you
could
> > > send a copy of ExtraRequestInfo + request_id to each provisional load,
when the
> > > load is switched from provisional to committed, you could process the
matching
> > > ExtraRequestInfo (if there is one) in the observers at that point and
throw the
> > > rest out.
> > > > > 
> > > > > I bring this up because I think ReadyToCommitNavigation happens very
shortly
> > > before DidFinishNavigation (maybe even the same stack?, I'm not sure how
> > > transfer navigations work here either), so you might not be buying
anything for
> > > requests that complete before commit.
> > > > 
> > > > Ah, interesting, what problem would we solve by storing and deferring
for non
> > > committed loads? I don't see any significant differences in behavior & it
adds
> > > complexity to buffer until commit (it's not something we generally do with
other
> > > activity that can happen pre-commit, such as redirects or page visibility
state
> > > changes. My inclination is to keep this simple, since I don't think it
affects
> > > the observers in any significant way and just adds complexity here -
they'll get
> > > the OnLoadedResource callback before commit, and can increment any byte
counters
> > > etc, just as they would if we invoke the same callback later after commit.
> > > 
> > > Ah, maybe you are concerned that we won't have the GlobalRequestID at the
point
> > > the response completes? My analysis of call stack flows in the associated
doc
> > > suggests that's not the case, and my local testing confirms this. It's an
> > > interesting idea to hold on to the ExtraRequestInfo just in case - that
does
> > > make the code more resilient. But I'm not sure it's worth the complexity
> > > tradeoff given that the call ordering guarantees seem to match what we
expect.
> > > 
> > > I'd propose that we proceed w/ this or something similar for now, and
before we
> > > add complexity, we have a bigger discussion w/ clamy to understand
constraints
> > > here & make changes as needed.
> > 
> > Like you said, clamy@ is the expert here, but I believe it is possible for a
response to be deferred by a navigation throttle in
NavigationHandleImpl::WillProcessResponse. 
> > 
> > For example:
> > ClearSiteDataThrottle::WillProcessResponse() can defer the response
preventing commit for some time. If the main page resource is small, I believe
this can cause the URLRequest completion event to get to the UI thread before
the load is committed. Admittedly, I have not verified it or dug through
NavigationThrottles to know exactly what they do.
> > 
> > It's not really evident to me how long a navigation throttle can defer
navigations.
> > 
> > I'm not opposed to this landing, and I would be interested to know how many
0 bytes cases this fixes. That said, knowing the complexity of chrome and that
there is a deferral mechanism that will defer committing while the IO thread
runs, I'd be surprised if this didn't run into problems later.
> 
> Yeah, that is totally right for code that's in the NavThrottle code path.
ReadyToCommitNav is not in this path, fortunately - it's in the same call stack
but diverges before the NavThrottle flow so as best I understand, calls to it
won't be delayed due to NavThrottles.
> 
> RE: 0 byte cases, because the histogram uses kB rather than bytes, we won't
know with certainty how many zero bytes vs rounded down from <1/2kb to 0kb cases
we are encountering. I will see if I can come up w/ a way to track the zero byte
cases, maybe with a counter or a separate temporary histogram.

Ah, I'm mistaken, the call to ReadyToCommit is deferred if any throttle defers.
:-(

Given this, I think it may be more sensible for us to hook into the throttle
flow. Charles suggests making sure we're early in the throttle ordering s.t. we
can reliably be called w/o deferral. A little fragile but the best overall
option. I'll take a look at this. Thanks for calling this out so I didn't miss
it!

[RyanSturm, 2017-01-13 16:56:00]

On 2017/01/13 16:34:01, Bryan McQuade wrote:
> On 2017/01/13 at 16:15:46, ryansturm wrote:
> > Perhaps this should land and we can re-visit after new UMA comes in from
> canary. This is much better than what is currently in, so seems fine to me.
> > 
> >
>
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
> (const auto& kv : provisional_loads_) {
> > On 2017/01/13 15:53:17, Bryan McQuade wrote:
> > > On 2017/01/13 at 15:47:14, Bryan McQuade wrote:
> > > > On 2017/01/13 at 15:39:50, Ryan Sturm wrote:
> > > > > I'd actually suggest going a step further for the cases where the load
> > > hasn't committed yet. If the request completes before the browser is ready
> to
> > > commit due to some deferral in NavigationThrottle, you might want to store
> this
> > > request info for when these provisional_loads_ commit or are deleted.
> > > > > 
> > > > > For instance, if you don't find a match in the committed loads, you
> could
> > > send a copy of ExtraRequestInfo + request_id to each provisional load,
when
> the
> > > load is switched from provisional to committed, you could process the
> matching
> > > ExtraRequestInfo (if there is one) in the observers at that point and
throw
> the
> > > rest out.
> > > > > 
> > > > > I bring this up because I think ReadyToCommitNavigation happens very
> shortly
> > > before DidFinishNavigation (maybe even the same stack?, I'm not sure how
> > > transfer navigations work here either), so you might not be buying
anything
> for
> > > requests that complete before commit.
> > > > 
> > > > Ah, interesting, what problem would we solve by storing and deferring
for
> non
> > > committed loads? I don't see any significant differences in behavior & it
> adds
> > > complexity to buffer until commit (it's not something we generally do with
> other
> > > activity that can happen pre-commit, such as redirects or page visibility
> state
> > > changes. My inclination is to keep this simple, since I don't think it
> affects
> > > the observers in any significant way and just adds complexity here -
they'll
> get
> > > the OnLoadedResource callback before commit, and can increment any byte
> counters
> > > etc, just as they would if we invoke the same callback later after commit.
> > > 
> > > Ah, maybe you are concerned that we won't have the GlobalRequestID at the
> point
> > > the response completes? My analysis of call stack flows in the associated
> doc
> > > suggests that's not the case, and my local testing confirms this. It's an
> > > interesting idea to hold on to the ExtraRequestInfo just in case - that
does
> > > make the code more resilient. But I'm not sure it's worth the complexity
> > > tradeoff given that the call ordering guarantees seem to match what we
> expect.
> > > 
> > > I'd propose that we proceed w/ this or something similar for now, and
before
> we
> > > add complexity, we have a bigger discussion w/ clamy to understand
> constraints
> > > here & make changes as needed.
> > 
> > Like you said, clamy@ is the expert here, but I believe it is possible for a
> response to be deferred by a navigation throttle in
> NavigationHandleImpl::WillProcessResponse. 
> > 
> > For example:
> > ClearSiteDataThrottle::WillProcessResponse() can defer the response
preventing
> commit for some time. If the main page resource is small, I believe this can
> cause the URLRequest completion event to get to the UI thread before the load
is
> committed. Admittedly, I have not verified it or dug through
NavigationThrottles
> to know exactly what they do.
> > 
> > It's not really evident to me how long a navigation throttle can defer
> navigations.
> > 
> > I'm not opposed to this landing, and I would be interested to know how many
0
> bytes cases this fixes. That said, knowing the complexity of chrome and that
> there is a deferral mechanism that will defer committing while the IO thread
> runs, I'd be surprised if this didn't run into problems later.
> 
> Yeah, that is totally right for code that's in the NavThrottle code path.
> ReadyToCommitNav is not in this path, fortunately - it's in the same call
stack
> but diverges before the NavThrottle flow so as best I understand, calls to it
> won't be delayed due to NavThrottles.
> 
> RE: 0 byte cases, because the histogram uses kB rather than bytes, we won't
know
> with certainty how many zero bytes vs rounded down from <1/2kb to 0kb cases we
> are encountering. I will see if I can come up w/ a way to track the zero byte
> cases, maybe with a counter or a separate temporary histogram.

Hmm. Seems fine to me. Like I said I'd be interested to see how much this
changes things.

I'd like a histogram that can track how many committed PageLoadTrackers *do not*
receive main frame request complete, but *do* receive other request complete
events as this would make it very clear how many of the main frame requests are
being dropped on pages with more than just a main frame request. WDYT?


The way I understood the throttle was for anything except DEFER, the completion
callback would be called, but in the DEFER case, the CompletionCallback (which
contains the actual commit call or delete call) is not called.

Under my assumption, ReadyToCommit and DidFinishNavigation happen on the same
stack and the motivation for having both is that in the former case you can push
an IPC to the renderer before commit actually happens, but DidFinishNavigation
is after the IPC for the commit happens.

This code is somewhat unclear to me, but I think it represents a Defer/Resume
pattern that happens before ReadyToCommit:
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_ha...
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_ha...

In both these cases MaybeTransferAndProceed (and thus ReadyToCommit) are only
called when the throttles all have returned PROCEED. If DEFER is returned,
nothing happens until Resume() is called.

[RyanSturm, 2017-01-13 17:04:13]

On 2017/01/13 16:56:00, Ryan Sturm wrote:
> On 2017/01/13 16:34:01, Bryan McQuade wrote:
> > On 2017/01/13 at 16:15:46, ryansturm wrote:
> > > Perhaps this should land and we can re-visit after new UMA comes in from
> > canary. This is much better than what is currently in, so seems fine to me.
> > > 
> > >
> >
>
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> > > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
> > > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:189: for
> > (const auto& kv : provisional_loads_) {
> > > On 2017/01/13 15:53:17, Bryan McQuade wrote:
> > > > On 2017/01/13 at 15:47:14, Bryan McQuade wrote:
> > > > > On 2017/01/13 at 15:39:50, Ryan Sturm wrote:
> > > > > > I'd actually suggest going a step further for the cases where the
load
> > > > hasn't committed yet. If the request completes before the browser is
ready
> > to
> > > > commit due to some deferral in NavigationThrottle, you might want to
store
> > this
> > > > request info for when these provisional_loads_ commit or are deleted.
> > > > > > 
> > > > > > For instance, if you don't find a match in the committed loads, you
> > could
> > > > send a copy of ExtraRequestInfo + request_id to each provisional load,
> when
> > the
> > > > load is switched from provisional to committed, you could process the
> > matching
> > > > ExtraRequestInfo (if there is one) in the observers at that point and
> throw
> > the
> > > > rest out.
> > > > > > 
> > > > > > I bring this up because I think ReadyToCommitNavigation happens very
> > shortly
> > > > before DidFinishNavigation (maybe even the same stack?, I'm not sure how
> > > > transfer navigations work here either), so you might not be buying
> anything
> > for
> > > > requests that complete before commit.
> > > > > 
> > > > > Ah, interesting, what problem would we solve by storing and deferring
> for
> > non
> > > > committed loads? I don't see any significant differences in behavior &
it
> > adds
> > > > complexity to buffer until commit (it's not something we generally do
with
> > other
> > > > activity that can happen pre-commit, such as redirects or page
visibility
> > state
> > > > changes. My inclination is to keep this simple, since I don't think it
> > affects
> > > > the observers in any significant way and just adds complexity here -
> they'll
> > get
> > > > the OnLoadedResource callback before commit, and can increment any byte
> > counters
> > > > etc, just as they would if we invoke the same callback later after
commit.
> > > > 
> > > > Ah, maybe you are concerned that we won't have the GlobalRequestID at
the
> > point
> > > > the response completes? My analysis of call stack flows in the
associated
> > doc
> > > > suggests that's not the case, and my local testing confirms this. It's
an
> > > > interesting idea to hold on to the ExtraRequestInfo just in case - that
> does
> > > > make the code more resilient. But I'm not sure it's worth the complexity
> > > > tradeoff given that the call ordering guarantees seem to match what we
> > expect.
> > > > 
> > > > I'd propose that we proceed w/ this or something similar for now, and
> before
> > we
> > > > add complexity, we have a bigger discussion w/ clamy to understand
> > constraints
> > > > here & make changes as needed.
> > > 
> > > Like you said, clamy@ is the expert here, but I believe it is possible for
a
> > response to be deferred by a navigation throttle in
> > NavigationHandleImpl::WillProcessResponse. 
> > > 
> > > For example:
> > > ClearSiteDataThrottle::WillProcessResponse() can defer the response
> preventing
> > commit for some time. If the main page resource is small, I believe this can
> > cause the URLRequest completion event to get to the UI thread before the
load
> is
> > committed. Admittedly, I have not verified it or dug through
> NavigationThrottles
> > to know exactly what they do.
> > > 
> > > It's not really evident to me how long a navigation throttle can defer
> > navigations.
> > > 
> > > I'm not opposed to this landing, and I would be interested to know how
many
> 0
> > bytes cases this fixes. That said, knowing the complexity of chrome and that
> > there is a deferral mechanism that will defer committing while the IO thread
> > runs, I'd be surprised if this didn't run into problems later.
> > 
> > Yeah, that is totally right for code that's in the NavThrottle code path.
> > ReadyToCommitNav is not in this path, fortunately - it's in the same call
> stack
> > but diverges before the NavThrottle flow so as best I understand, calls to
it
> > won't be delayed due to NavThrottles.
> > 
> > RE: 0 byte cases, because the histogram uses kB rather than bytes, we won't
> know
> > with certainty how many zero bytes vs rounded down from <1/2kb to 0kb cases
we
> > are encountering. I will see if I can come up w/ a way to track the zero
byte
> > cases, maybe with a counter or a separate temporary histogram.
> 
> Hmm. Seems fine to me. Like I said I'd be interested to see how much this
> changes things.
> 
> I'd like a histogram that can track how many committed PageLoadTrackers *do
not*
> receive main frame request complete, but *do* receive other request complete
> events as this would make it very clear how many of the main frame requests
are
> being dropped on pages with more than just a main frame request. WDYT?
> 
> 
> The way I understood the throttle was for anything except DEFER, the
completion
> callback would be called, but in the DEFER case, the CompletionCallback (which
> contains the actual commit call or delete call) is not called.
> 
> Under my assumption, ReadyToCommit and DidFinishNavigation happen on the same
> stack and the motivation for having both is that in the former case you can
push
> an IPC to the renderer before commit actually happens, but DidFinishNavigation
> is after the IPC for the commit happens.
> 
> This code is somewhat unclear to me, but I think it represents a Defer/Resume
> pattern that happens before ReadyToCommit:
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_ha...
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_ha...
> 
> In both these cases MaybeTransferAndProceed (and thus ReadyToCommit) are only
> called when the throttles all have returned PROCEED. If DEFER is returned,
> nothing happens until Resume() is called.

The throttle flow could work as well for this. If you are the *first* throttle
this should work. I don't know how to enforce being the first throttle, however.

I think the two options are enforce that you are the first throttle in the list,
or keep track of unmatched completed main frame requests on the provisional
loads.

[Charlie Harrison, 2017-01-13 17:19:14]

We should enforce throttle ordering with comments, which I neglected to do in my
first patch which made the metrics throttle first in the list :/

[commit-bot: I haz the power, 2017-01-13 17:25:35]

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

[commit-bot: I haz the power, 2017-01-13 17:25:36]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-01-13 18:02:59]

On 2017/01/13 17:19:14, Charlie Harrison wrote:
> We should enforce throttle ordering with comments, which I neglected to do in
my
> first patch which made the metrics throttle first in the list :/

Adding a non-throttling order-dependent throttle seems a little iffy. Especially
since not all devs read comments carefully enough and another throttle has
already moved to the front of the list.

It would be a lot of work, but adding a
OnBeforeThrottleResponse(NavigationHandle*) to WebContentsObserver directly
before throttling makes more sense to me.

If I were doing this, I probably would think adding a WCO method is too much
work as well, but it's a consideration you could think about.

[Bryan McQuade, 2017-01-13 18:41:59]

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

[commit-bot: I haz the power, 2017-01-13 18:42:41]

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

[Bryan McQuade, 2017-01-13 19:51:12]

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

[commit-bot: I haz the power, 2017-01-13 19:52:00]

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

[Bryan McQuade, 2017-01-13 19:55:25]

On 2017/01/13 at 18:02:59, ryansturm wrote:
> On 2017/01/13 17:19:14, Charlie Harrison wrote:
> > We should enforce throttle ordering with comments, which I neglected to do
in my
> > first patch which made the metrics throttle first in the list :/
> 
> Adding a non-throttling order-dependent throttle seems a little iffy.
Especially since not all devs read comments carefully enough and another
throttle has already moved to the front of the list.
> 
> It would be a lot of work, but adding a
OnBeforeThrottleResponse(NavigationHandle*) to WebContentsObserver directly
before throttling makes more sense to me.
> 
> If I were doing this, I probably would think adding a WCO method is too much
work as well, but it's a consideration you could think about.

adding comments, as well as adding an OnBeforeThrottleResponse, both seem like
potentially good options. given that it's unclear which we'll pursue and it may
take time to resolve (need to start a discussion w/ nasko and clamy to see if
they are open to OnBeforeThrottleResponse - i think it's a good idea to add, but
not my call), i'd like to proceed w/ this change as is, and start the discussion
about how to properly hook here in a separate thread and patch.

charles, i did add a basic browsertest which verifies that bytes for the main
frame are logged. in my testing, the response is sometimes received before
commit, sometimes after, which happens to give us coverage of both cases. it's
not as good as a deterministic test case for each, but i don't think we can
write such a test and have it be 100% deterministic (in particular i am not sure
how to force the before commit case). how do you feel about using this test to
give us coverage, given that it happens to cover both cases due to its own
nondeterminism about when the main url response finishes?

[Bryan McQuade, 2017-01-13 19:57:28]

On 2017/01/13 at 19:55:25, Bryan McQuade wrote:
> On 2017/01/13 at 18:02:59, ryansturm wrote:
> > On 2017/01/13 17:19:14, Charlie Harrison wrote:
> > > We should enforce throttle ordering with comments, which I neglected to do
in my
> > > first patch which made the metrics throttle first in the list :/
> > 
> > Adding a non-throttling order-dependent throttle seems a little iffy.
Especially since not all devs read comments carefully enough and another
throttle has already moved to the front of the list.
> > 
> > It would be a lot of work, but adding a
OnBeforeThrottleResponse(NavigationHandle*) to WebContentsObserver directly
before throttling makes more sense to me.
> > 
> > If I were doing this, I probably would think adding a WCO method is too much
work as well, but it's a consideration you could think about.
> 
> adding comments, as well as adding an OnBeforeThrottleResponse, both seem like
potentially good options. given that it's unclear which we'll pursue and it may
take time to resolve (need to start a discussion w/ nasko and clamy to see if
they are open to OnBeforeThrottleResponse - i think it's a good idea to add, but
not my call), i'd like to proceed w/ this change as is, and start the discussion
about how to properly hook here in a separate thread and patch.
> 
> charles, i did add a basic browsertest which verifies that bytes for the main
frame are logged. in my testing, the response is sometimes received before
commit, sometimes after, which happens to give us coverage of both cases. it's
not as good as a deterministic test case for each, but i don't think we can
write such a test and have it be 100% deterministic (in particular i am not sure
how to force the before commit case). how do you feel about using this test to
give us coverage, given that it happens to cover both cases due to its own
nondeterminism about when the main url response finishes?

(to clarify i'd like to avoid the overhead of adding comments and looping a
content reviewer in on this change, if we're going to introduce
OnBeforeThrottleResponse soon anyway. so what i propose is that we do neither
for this change, and get guidance from nasko/clamy on which of the 2 approaches
they are happiest with, then make that change in a follow up cl)

[Charlie Harrison, 2017-01-13 20:00:43]

I'm happy with either option. A single test is fine with me if you're having
trouble making the behavior deterministic.

[commit-bot: I haz the power, 2017-01-13 21:04:24]

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

[commit-bot: I haz the power, 2017-01-13 21:04:25]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-01-14 00:18:52]

On 2017/01/13 at 20:00:43, csharrison wrote:
> I'm happy with either option. A single test is fine with me if you're having
trouble making the behavior deterministic.

Great. I think the change is ready for final review then. Thanks!

Will start a thread w/ nasko and clamy re: options to make the code less fragile
next week.

[Charlie Harrison, 2017-01-17 14:47:48]

Would you address the nits I mentioned earlier? Otherwise LGTM

[Bryan McQuade, 2017-01-17 14:52:31]

On 2017/01/13 at 15:58:30, csharrison wrote:
> Would you mind adding a test?

Added a basic browsertest which does give us coverage here. It's not perfect but
I think it strikes the right complexity/coverage tradeoff.

[Bryan McQuade, 2017-01-17 15:06:26]

thanks! addressed comments

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:187: //
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
On 2017/01/13 at 15:56:23, Charlie Harrison wrote:
> optional: Can you shorten this with a https://goo.gl/ link?

done

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:209: // 5. the
URLRequests initiated by A complete
On 2017/01/13 at 15:56:23, Charlie Harrison wrote:
> Shouldn't the request be cancelled? I guess it could be a detachable request
link link prefetch.

Unfortunately that doesn't seem to be the case. I saw requests on twitter.com
get associated with the next committed load in multiple local tests.

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:431: // When browser side
navigation is enabled,
On 2017/01/13 at 15:56:23, Charlie Harrison wrote:
> nit: Just replace this line with "PlzNavigate:", which is more common in
PlzNavigate code and is the common way of describing PlzNavigate-only paths.

done

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:440:
DCHECK(navigation_request_id_.value() != content::GlobalRequestID());
On 2017/01/13 at 15:56:23, Charlie Harrison wrote:
> nit: DCHECK_NE

i tried this but it doesn't compile, as DCHECK_NE requires an operator<< which
isn't provided by GlobalRequestID yet. will ask clamy/nasko about this as well.
moved back to DCHECK for now per our chat.

https://codereview.chromium.org/2624283004/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:616: DCHECK(request_id !=
content::GlobalRequestID());
On 2017/01/13 at 15:56:23, Charlie Harrison wrote:
> ditto

same

[Bryan McQuade, 2017-01-17 15:06:54]

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

[mmenke, 2017-01-17 15:06:55]

Description was changed from

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
for more details.

BUG=680983
==========

to

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
for more details.

BUG=680983
==========

[commit-bot: I haz the power, 2017-01-17 15:07:06]

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, 2017-01-17 16:02:03]

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

[commit-bot: I haz the power, 2017-01-17 16:02:04]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-01-17 17:54:37]

a couple of nits, but lgtm.

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_navigation_throttle.h (right):

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_navigation_throttle.h:21: static
std::unique_ptr<content::NavigationThrottle> Create(
Can you move this throttle back to the beginning of the throttle list with a
comment about ordering the throttles that do nothing at the beginning?

FlashDownloadInterception::MaybeCreateThrottleFor(handle) is now called before
MetricsNavigationThrottle::Create in chrome_content_browser_client.cc

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:218: // the given
request_id. This method will return false before ReadyToCommit
nit: s/ReadyToCommit/WillProcessResponse/ twice in here.

[Bryan McQuade, 2017-01-17 19:18:12]

Thanks!

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_navigation_throttle.h (right):

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_navigation_throttle.h:21: static
std::unique_ptr<content::NavigationThrottle> Create(
On 2017/01/17 at 17:54:36, Ryan Sturm wrote:
> Can you move this throttle back to the beginning of the throttle list with a
comment about ordering the throttles that do nothing at the beginning?
> 
> FlashDownloadInterception::MaybeCreateThrottleFor(handle) is now called before
MetricsNavigationThrottle::Create in chrome_content_browser_client.cc

I'm going to start a thread about this w/ clamy and nasko - will either re-order
and add the comment or add a new OnBeforeWillProcessResponse callback depending
on their guidance. Will defer adding this comment for the time being.

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2624283004/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:218: // the given
request_id. This method will return false before ReadyToCommit
On 2017/01/17 at 17:54:37, Ryan Sturm wrote:
> nit: s/ReadyToCommit/WillProcessResponse/ twice in here.

Thanks! Fixed.

[Bryan McQuade, 2017-01-17 19:18:25]

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

[commit-bot: I haz the power, 2017-01-17 19:19: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, 2017-01-17 20:45:40]

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

[commit-bot: I haz the power, 2017-01-17 20:45:41]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-01-17 22:00:09]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-01-17 22:00:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2624283004/#ps380001
(title: "fix comment")

[commit-bot: I haz the power, 2017-01-17 22:00:31]

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, 2017-01-17 22:06:11]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1484690409150330,
"parent_rev": "0cf25b2f6bac451a6b6449398e30809607ec37bb", "commit_rev":
"67906e1cd7937494e078325ead8bdb95093bbf29"}

[commit-bot: I haz the power, 2017-01-17 22:06:46]

Description was changed from

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
for more details.

BUG=680983
==========

to

==========
Associate a main resource request with its PageLoadTracker.

A main resource request may complete either before or after commit.
For example, if the main resource response fits in a single TCP window,
it will likely complete before commit, whereas if the response is large,
it will likely complete after commit.

This change special cases main resource PageLoadTracker attribution, by
finding the provisional load or committed load with a GlobalRequestID that
matches the given URLRequest.

See
https://docs.google.com/document/d/1dZP0si0IKEMeVKGsohJ3fFWiKqh3nZBtuTFVkAPSX...
for more details.

BUG=680983

Review-Url: https://codereview.chromium.org/2624283004
Cr-Commit-Position: refs/heads/master@{#444159}
Committed:
https://chromium.googlesource.com/chromium/src/+/67906e1cd7937494e078325ead8b...
==========

[commit-bot: I haz the power, 2017-01-17 22:06:48]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/67906e1cd7937494e078325ead8b...