[page_load_metrics] Add a NavigationThrottle for richer abort metrics

[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

TBR=jam@chromium.org

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

Committed: https://crrev.com/4bd8a19469434b79ff524323f44eca517e799321
Cr-Commit-Position: refs/heads/master@{#409237}

[Charlie Harrison, 2016-07-12 16:10:57]

Description was changed from

==========
Add a NavigationThrottle to help track provisional abort metrics

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

to

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

BUG=627501
==========

[Charlie Harrison, 2016-07-12 16:11:46]

Description was changed from

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

BUG=627501
==========

to

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

BUG=627501
==========

[Charlie Harrison, 2016-07-12 16:14:08]

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

[Charlie Harrison, 2016-07-12 16:14:09]

PTAL!

Note: this patch adds an unfortunate side effect of making our abort unit tests
fragile, as the unit testing framework of NavigateAndCommit() does not go
through all the NavigationThrottle callbacks.

It seems like this should be fixed in the content testing framework. clamy@, is
there a crbug for this already or should we make one?

[Bryan McQuade, 2016-07-12 16:45:29]

Nice change, thanks! I did a quick first pass and left a few questions /
comments. Will do another pass soon.

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_navigation_throttle.cc
(right):

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_navigation_throttle.cc:26: if
(observer)
just for my own knowledge, are there cases where we expect the observer to be
false? or should every webcontents have a MWCO?

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_navigation_throttle.cc:31:
MetricsNavigationThrottle::WillRedirectRequest() {
looks like the base class implementation is identical - can we omit this from
our impl?

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_navigation_throttle.cc:36:
MetricsNavigationThrottle::WillProcessResponse() {
same - can we omit this?

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:390: //
TODO(csharrison): We can get an approximation of whether the abort is user
is the intent to record whether it's user initiated once we've updated browser
navigations to report true for user initiated? or has that happened already?

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:193:
ui::PageTransition page_transition() { return page_transition_; }
nit: can we make this method const?

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:246:
ui::PageTransition page_transition_;
rather than defaulting to PAGE_TRANSITION_LAST_CORE, which feels a bit
unintuitive, why don't we use base::Optional<ui::PageTransition> here instead?
and page_transition() can return a const Optional<>&, or it can be documented to
only be valid to invoke after some certain time, and we can DCHECK() that the
optional is set when it gets invoked.

[Charlie Harrison, 2016-07-12 19:20:22]

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_navigation_throttle.cc
(right):

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_navigation_throttle.cc:26: if
(observer)
On 2016/07/12 16:45:29, Bryan McQuade wrote:
> just for my own knowledge, are there cases where we expect the observer to be
> false? or should every webcontents have a MWCO?

There are some times when a web contents does not have a MWCO. There are
features (like reader mode) that insert WC into the tab strip during navigation.

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_navigation_throttle.cc:31:
MetricsNavigationThrottle::WillRedirectRequest() {
On 2016/07/12 16:45:29, Bryan McQuade wrote:
> looks like the base class implementation is identical - can we omit this from
> our impl?

Done.

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_navigation_throttle.cc:36:
MetricsNavigationThrottle::WillProcessResponse() {
On 2016/07/12 16:45:29, Bryan McQuade wrote:
> same - can we omit this?

Done.

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:390: //
TODO(csharrison): We can get an approximation of whether the abort is user
On 2016/07/12 16:45:29, Bryan McQuade wrote:
> is the intent to record whether it's user initiated once we've updated browser
> navigations to report true for user initiated? or has that happened already?

It isn't necessary to do the browser navigation change (though it will be
convenient). I was just delaying this change to a follow up CL.

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:193:
ui::PageTransition page_transition() { return page_transition_; }
On 2016/07/12 16:45:29, Bryan McQuade wrote:
> nit: can we make this method const?

Done.

https://codereview.chromium.org/2132603002/diff/120001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:246:
ui::PageTransition page_transition_;
On 2016/07/12 16:45:29, Bryan McQuade wrote:
> rather than defaulting to PAGE_TRANSITION_LAST_CORE, which feels a bit
> unintuitive, why don't we use base::Optional<ui::PageTransition> here instead?
> and page_transition() can return a const Optional<>&, or it can be documented
to
> only be valid to invoke after some certain time, and we can DCHECK() that the
> optional is set when it gets invoked.

Done.

[Bryan McQuade, 2016-07-13 18:23:36]

Thanks! I'm looking forward to getting this extra data so we can improve abort
tracking. A couple more comments/questions.

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:710:
NotifyAbortedProvisionalLoadsNewNavigation(it.first->second.get());
should we not call this and just wait to invoke it in WillStartNavRequest? What
is the benefit to calling it here as well?

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:894: //
TODO(csharrison): This is an implementation flaw due to how the unit
which tests in particular break due to this? is it just the MWCO unit test? Or
do our observer tests also break? How much work would it take to fix the test
limitation?

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:906:
PageLoadTracker* new_navigation) {
nit: given that the type is now a PageLoadTracker let's rename to new_page_load.
Similarly, let's rename the method NotifyAbortedProvisionalLoadsNewPageLoad

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:179: bool
WasAbortedRecently(base::TimeTicks abort_cause_time);
let's make this a const method

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:193: const
base::Optional<ui::PageTransition> page_transition() const {
do you want to return a const reference here, to avoid the copy?

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:246: //
TODO(csharrison): Some transitions are only known at commit time. Consider
is the value that NavigationHandle::GetPageTransition() reports at commit ever
less accurate than what we would have received from the throttle? If not, seems
like we might as well just update the PageLoadTracker::Commit() function to use
the updated page transition as part of this change.

[Charlie Harrison, 2016-07-13 18:58:05]

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

[Charlie Harrison, 2016-07-13 18:58:19]

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:710:
NotifyAbortedProvisionalLoadsNewNavigation(it.first->second.get());
On 2016/07/13 18:23:36, Bryan McQuade wrote:
> should we not call this and just wait to invoke it in WillStartNavRequest?
What
> is the benefit to calling it here as well?

It is only really for current unit tests which do not call WillStartNavRequest
:(
I am not sure all the are cases where a load can be aborted between start and
WillStartNavRequest, but this code also guards against that.

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:894: //
TODO(csharrison): This is an implementation flaw due to how the unit
On 2016/07/13 18:23:36, Bryan McQuade wrote:
> which tests in particular break due to this? is it just the MWCO unit test? Or
> do our observer tests also break? How much work would it take to fix the test
> limitation?

The tests that fail if we include UNKNOWN_NAVIGATION here are:
AbortsPageLoadMetricsObserverTest.UnknownNavigationBeforeCommit
FromGWSPageLoadMetricsObserverTest.UnknownNavigationBeforeCommit
MetricsWebContentsObserverTest.LogAbortChainsNoCommit

Due to another abort reason occurring to a load that was aborted with
UNKNOWN_NAVIGATION.

I looked at the test infra and it seemed relatively daunting to fix this at the
fundamental level. clamy@, do you have an idea of how difficult it would be to
fully integrate NavigationThrottle/Handles in unit tests?

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:906:
PageLoadTracker* new_navigation) {
On 2016/07/13 18:23:36, Bryan McQuade wrote:
> nit: given that the type is now a PageLoadTracker let's rename to
new_page_load.
> Similarly, let's rename the method NotifyAbortedProvisionalLoadsNewPageLoad

Done.

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:179: bool
WasAbortedRecently(base::TimeTicks abort_cause_time);
On 2016/07/13 18:23:36, Bryan McQuade wrote:
> let's make this a const method

Done.

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:193: const
base::Optional<ui::PageTransition> page_transition() const {
Ah yes, this was a typo :)

https://codereview.chromium.org/2132603002/diff/140001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:246: //
TODO(csharrison): Some transitions are only known at commit time. Consider
On 2016/07/13 18:23:36, Bryan McQuade wrote:
> is the value that NavigationHandle::GetPageTransition() reports at commit ever
> less accurate than what we would have received from the throttle? If not,
seems
> like we might as well just update the PageLoadTracker::Commit() function to
use
> the updated page transition as part of this change.

It will only be more accurate. Done.

[commit-bot: I haz the power, 2016-07-13 18:58:50]

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-07-13 20:05:12]

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

[commit-bot: I haz the power, 2016-07-13 20:05:13]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-15 15:58:48]

Overall this seems great and I am really looking forward to having this
additional data to improve our abort metrics. I'm a little concerned with the
issue of not invoking the throttle callback in tests, which means tests aren't
really testing the same codepath as production.

Fixing the test infrastructure to do the right thing seems most desirable, but
if that's not possible, I do wonder if faking the call to
WillStartNavigationRequest on our MWCO, such that order of execution is more
consistent between testing and production, is better.

I also see NavigationHandle::RegisterThrottleForTesting() and NavigationHandle::
CallWillStartRequestForTesting. Might these be useful as ways to drive the
throttle codepath in our tests?

[Charlie Harrison, 2016-07-15 16:00:17]

On 2016/07/15 15:58:48, Bryan McQuade wrote:
> Overall this seems great and I am really looking forward to having this
> additional data to improve our abort metrics. I'm a little concerned with the
> issue of not invoking the throttle callback in tests, which means tests aren't
> really testing the same codepath as production.
> 
> Fixing the test infrastructure to do the right thing seems most desirable, but
> if that's not possible, I do wonder if faking the call to
> WillStartNavigationRequest on our MWCO, such that order of execution is more
> consistent between testing and production, is better.
> 
> I also see NavigationHandle::RegisterThrottleForTesting() and
NavigationHandle::
> CallWillStartRequestForTesting. Might these be useful as ways to drive the
> throttle codepath in our tests?

I'll look into it, and possibly set up a meeting with clamy@ for Monday. Agreed
that if it's not too much work this is desireable.

[Charlie Harrison, 2016-07-20 20:23:28]

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

[commit-bot: I haz the power, 2016-07-20 20:23:53]

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-07-20 20:26:10]

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

[commit-bot: I haz the power, 2016-07-20 20:26:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Charlie Harrison, 2016-07-20 20:31:30]

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

[commit-bot: I haz the power, 2016-07-20 20:32:17]

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

[Charlie Harrison, 2016-07-20 20:33:59]

This is ready for another pass. I've updated the MWCO in a significant way by
replacing DidStartNavigation wholesale by WillStartNavigationRequest.

A bunch of tests needed cleaning up, but I think it's sane.

[commit-bot: I haz the power, 2016-07-20 21:15:41]

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

[commit-bot: I haz the power, 2016-07-20 21:15:42]

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

[Bryan McQuade, 2016-07-20 21:51:11]

Thanks! I looked at the MWCO changes - those look great now. Thanks for
addressing the testing challenges. Just a couple small comments.

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:379:
page_transition_ = navigation_handle->GetPageTransition();
maybe add a comment here noting that the page transition can be more accurate at
commit time, so we update it here as well as setting it in the constructor? that
way someone doesn't remove this in a later change thinking it had already been
set correctly in the constructor.

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:876: 
nit: remove empty line

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:304: void
WillStartNavigationRequest(content::NavigationHandle* navigation_handle);
this is great - a really nice clean change. my only question/concern is whether
there are cases where we would have gotten a DidStartNavigation call that we
won't get a WillStartNavigationRequestCall. Is that a possibility? Could that
cause us to fail to track certain pages that we'd otherwise want to track? My
guess is that it's not possible, but I wanted to check to be sure.

[Bryan McQuade, 2016-07-20 21:51:12]

Thanks! I looked at the MWCO changes - those look great now. Thanks for
addressing the testing challenges. Just a couple small comments.

[Charlie Harrison, 2016-07-21 14:06:43]

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

[commit-bot: I haz the power, 2016-07-21 14:06:54]

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

[Charlie Harrison, 2016-07-21 14:16:16]

Thanks!

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:379:
page_transition_ = navigation_handle->GetPageTransition();
On 2016/07/20 21:51:11, Bryan McQuade wrote:
> maybe add a comment here noting that the page transition can be more accurate
at
> commit time, so we update it here as well as setting it in the constructor?
that
> way someone doesn't remove this in a later change thinking it had already been
> set correctly in the constructor.

Done.

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:876: 
On 2016/07/20 21:51:11, Bryan McQuade wrote:
> nit: remove empty line

Oops. Done.

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2132603002/diff/200001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:304: void
WillStartNavigationRequest(content::NavigationHandle* navigation_handle);
On 2016/07/20 21:51:11, Bryan McQuade wrote:
> this is great - a really nice clean change. my only question/concern is
whether
> there are cases where we would have gotten a DidStartNavigation call that we
> won't get a WillStartNavigationRequestCall. Is that a possibility? Could that
> cause us to fail to track certain pages that we'd otherwise want to track? My
> guess is that it's not possible, but I wanted to check to be sure.

I'm fairly sure this will always be called. It corresponds to
ResourceHandler::OnWillStart which is "Called before the net::URLRequest (whose
url is |url|) is to be started."

+clamy@ in case I'm wrong.

[commit-bot: I haz the power, 2016-07-21 14:44:18]

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

[commit-bot: I haz the power, 2016-07-21 14:44:19]

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

[Charlie Harrison, 2016-07-21 14:48:05]

Looks like tests are failing, investigating.

[Charlie Harrison, 2016-07-21 15:41:26]

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

[commit-bot: I haz the power, 2016-07-21 15:41:40]

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-07-21 17:08:46]

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

[commit-bot: I haz the power, 2016-07-21 17:08:47]

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

[Charlie Harrison, 2016-07-21 19:30:47]

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

[commit-bot: I haz the power, 2016-07-21 19:31:10]

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-07-21 19:50:37]

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

[commit-bot: I haz the power, 2016-07-21 19:50:38]

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

[Charlie Harrison, 2016-07-21 21:43:14]

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

[commit-bot: I haz the power, 2016-07-21 21:44:10]

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-07-21 22:55:17]

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

[commit-bot: I haz the power, 2016-07-21 22:55:19]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-07-22 14:16:57]

PTAL. I changed implementations to attach the metrics navigation throttle first.
This means that we'll always observe navigations even if another throttle wants
to defer them.

[Charlie Harrison, 2016-07-25 23:16:31]

bmcquade@, can you take another look?

[Bryan McQuade, 2016-07-25 23:45:24]

lgtm

[Bryan McQuade, 2016-07-25 23:45:39]

Thanks for this change! Really glad to have this for better abort metrics.

[Charlie Harrison, 2016-07-25 23:54:21]

csharrison@chromium.org changed reviewers:
+ isherman@chromium.org, jam@chromium.org

[Charlie Harrison, 2016-07-25 23:54:22]

Thanks!
clamy@ could you look at NavigationThrottle usage, and moving the
TestNavigationManager to the public API (there are some slight logic changes
that we discussed over chat).

+jam for components/page_load_metrics.gypi which will be going away soon
(bmcquade is migrating the code to //chrome).

+isherman for histogram deprecation.

[Ilya Sherman, 2016-07-26 02:11:44]

histograms.xml lgtm, thanks =)

[clamy, 2016-07-26 16:51:38]

clamy@chromium.org changed reviewers:
+ nasko@chromium.org

[clamy, 2016-07-26 17:09:23]

Thanks! A few comments.
@nasko: could you have a look at the public interface and check if the FTN id
usage is valid?

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.cc:368:
NavigationThrottle::ThrottleCheckResult WillStartRequest() override {
I'm not terribly fond of this. Attaching a throttle twice is really hard to
understand, and may break easily if we want to add the functionality of waiting
for redirects/ready to commit to the testNavigationManger (which we may want to
do in the future).

Instead, I would suggest adding a method to NavigationHandleImpl to attach the
throttle to the end.

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.cc:1539: if (did_finish_loop_runner_)
We may want to also quit the will_start_loop_runner_ here, and have
TestNavigationManager::WaitForWillStartRequest return a boolean. With the
throttle being in last position, people may want to wait for the start of the
request that is cancelled by another throttle (and ensure it doesn't start).

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.h:569: TestNavigationManager(int
frame_tree_node_id,
nasko: is it ok to expose the FTN id in this way? I think yes, but I want to
make sure.

[jam, 2016-07-26 17:33:48]

On 2016/07/25 23:54:22, csharrison wrote:
> Thanks!
> clamy@ could you look at NavigationThrottle usage, and moving the
> TestNavigationManager to the public API (there are some slight logic changes
> that we discussed over chat).
> 
> +jam for components/page_load_metrics.gypi which will be going away soon
> (bmcquade is migrating the code to //chrome).

you're already an owner for that per
https://cs.chromium.org/chromium/src/components/OWNERS?rcl=0&l=141 :)

> 
> +isherman for histogram deprecation.

[Charlie Harrison, 2016-07-26 19:53:32]

Description was changed from

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

BUG=627501
==========

to

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

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

[Charlie Harrison, 2016-07-26 19:56:07]

Thanks. There seems to be some problem that I traced back to
https://codereview.chromium.org/2140393002/

which hides the page transition of aborter page loads. I'll need to wait until
that is resolved before landing this.

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.cc:368:
NavigationThrottle::ThrottleCheckResult WillStartRequest() override {
On 2016/07/26 17:09:22, clamy wrote:
> I'm not terribly fond of this. Attaching a throttle twice is really hard to
> understand, and may break easily if we want to add the functionality of
waiting
> for redirects/ready to commit to the testNavigationManger (which we may want
to
> do in the future).
> 
> Instead, I would suggest adding a method to NavigationHandleImpl to attach the
> throttle to the end.

Hm okay. I've modified NavigationHandleImpl::RegisterNavigationThrottles so that
the normal throttles are added to the beginning. Does that seem ok to you?

https://codereview.chromium.org/2132603002/diff/260001/content/public/test/br...
content/public/test/browser_test_utils.cc:1539: if (did_finish_loop_runner_)
On 2016/07/26 17:09:22, clamy wrote:
> We may want to also quit the will_start_loop_runner_ here, and have
> TestNavigationManager::WaitForWillStartRequest return a boolean. With the
> throttle being in last position, people may want to wait for the start of the
> request that is cancelled by another throttle (and ensure it doesn't start).

Done.

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.cc:1567: // This will only be if
DidFinishNavigation is called before
added "true" here in next PS.

[Bryan McQuade, 2016-07-26 23:45:12]

FYI: I just landed the change to move page_load_metrics out of components, so
this change will need to be updated to match.

[clamy, 2016-07-27 17:02:24]

Thanks! It looks better now. I'm away for 2 weeks, so could you check with nasko
for the end of the review?

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.cc:359: // to the end of the list on
WillStartRequest.
Comment needs to be updated since we now always attach the throttle at the back
of the list.

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.h:601: bool WaitForWillStartRequest();
Could you add a WARN_UNUSED_RESULT at the end? We want users to explicitly check
that it succeeded before trying to do something else.

[Charlie Harrison, 2016-07-27 18:53:41]

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

[commit-bot: I haz the power, 2016-07-27 18:54:08]

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-07-27 19:29:25]

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

[commit-bot: I haz the power, 2016-07-27 19:29:26]

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

[Charlie Harrison, 2016-07-28 00:33:18]

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

[commit-bot: I haz the power, 2016-07-28 00:33:30]

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

[Charlie Harrison, 2016-07-28 01:29:59]

nasko, PTAL! Thanks!

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.cc:359: // to the end of the list on
WillStartRequest.
On 2016/07/27 17:02:24, clamy (ooo until Aug 16) wrote:
> Comment needs to be updated since we now always attach the throttle at the
back
> of the list.

Done.

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2132603002/diff/280001/content/public/test/br...
content/public/test/browser_test_utils.h:601: bool WaitForWillStartRequest();
On 2016/07/27 17:02:24, clamy (ooo until Aug 16) wrote:
> Could you add a WARN_UNUSED_RESULT at the end? We want users to explicitly
check
> that it succeeded before trying to do something else.

Done.

https://codereview.chromium.org/2132603002/diff/320001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2132603002/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:108:
class MetricsWebContentsObserverTest
Fixed this formatting in next CL.

https://codereview.chromium.org/2132603002/diff/320001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/320001/content/public/test/br...
content/public/test/browser_test_utils.cc:358: // WillStartRequest callback.
RegisterThrottleForTesting has this semantics.
s/has this semantics/has this behavior/g

[Charlie Harrison, 2016-07-28 04:22:08]

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

[commit-bot: I haz the power, 2016-07-28 04:22:25]

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-07-28 05:13:17]

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

[commit-bot: I haz the power, 2016-07-28 05:13:19]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-07-28 13:20:30]

csharrison@chromium.org changed reviewers:
- jam@chromium.org

[Charlie Harrison, 2016-07-28 13:20:31]

Removing jam, sorry for the bother!

[Charlie Harrison, 2016-08-01 13:45:13]

nasko@, ping. Would you please look at the //content changes?

[nasko, 2016-08-01 15:29:23]

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

https://codereview.chromium.org/2132603002/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_navigation_throttle.h:18: // is used to
obtain more reliable abort metrics (like page transition type).
Based on some poking I've done on separate CLs I've been working on, I'm not
sure that the page transition type is correct that early in renderer-initiated
navigations. Have you confirmed you are getting what you expect?

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:556:
RenderFrameDevToolsAgentHost::CreateThrottleForNavigation(this);
This throttle was added at the end of the list intentionally. Are we sure it is
ok to move before the rest?

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.cc:359: class
TestNavigationManagerThrottle : public NavigationThrottle {
Did this really need to move to the public files? It isn't exposed outside of
content/ (and it shouldn't be), so I wonder why the move.

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.h:584: // mutliple of these managers if
you want to pause multiple navigations.
nit: "multiple instances of this class if you ..."

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.h:587: // Monitors notifications within
the given frame tree node. Use the other
FrameTreeNode is mostly internal concept to content/. We have had to expose it
in a couple of places, but this one doesn't look necessary. Why not drop this
one, since the next constructor should be good enough?

I think an content/ internal version using the FTN will be nice to have.

[Charlie Harrison, 2016-08-01 17:42:19]

Thanks Nasko!

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

https://codereview.chromium.org/2132603002/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_navigation_throttle.h:18: // is used to
obtain more reliable abort metrics (like page transition type).
On 2016/08/01 15:29:22, nasko wrote:
> Based on some poking I've done on separate CLs I've been working on, I'm not
> sure that the page transition type is correct that early in renderer-initiated
> navigations. Have you confirmed you are getting what you expect?

It is generally correct. There are some gotchas around AUTO_SUBFRAME but we
aren't really concerned about that case. I tested:
link clicks
form submits
window.location = <new url>
window.location.reload
history.back

and they all have identical transition at WillStart and DidFinish navigation.
Let me know if you remember specific cases though. In general I would say we are
okay with being wrong for some subset of navigations, as long as our coverage
increases from the 0 it is at today :)

I'll add a thank you for making me do the work right now to verify, because I've
found that JS initiated history navs do not show up as RELOAD / FORWARD_BACK but
do show up as LINK | CLIENT_SIDE_REDIRECT (both before and after commit). I'll
include a histogram for that case in a later CL, as I suspect that will actually
get rid of a LOT of noise in the abort histograms!

I've added a TODO in MetricsWebContentsObserver.

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:556:
RenderFrameDevToolsAgentHost::CreateThrottleForNavigation(this);
On 2016/08/01 15:29:23, nasko wrote:
> This throttle was added at the end of the list intentionally. Are we sure it
is
> ok to move before the rest?

It is still at the end of the "real" throttles. The only throttles that can come
after it are ones from RegisterThrottleForTesting. If a time comes when a
testing throttles needs to come before a real throttle, I think we can add
support for it then.

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.cc:359: class
TestNavigationManagerThrottle : public NavigationThrottle {
On 2016/08/01 15:29:23, nasko wrote:
> Did this really need to move to the public files? It isn't exposed outside of
> content/ (and it shouldn't be), so I wonder why the move.

Hm well it is only needed by the TestNavigationManager so I assumed an anonymous
namespace in the TestNavigationManagers impl is the best approach. I could
alternatively keep this in the private file, but would have to expose it via
headers which is not really needed. Would you prefer that?

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.h:584: // mutliple of these managers if
you want to pause multiple navigations.
On 2016/08/01 15:29:23, nasko wrote:
> nit: "multiple instances of this class if you ..."

Done.

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.h:587: // Monitors notifications within
the given frame tree node. Use the other
On 2016/08/01 15:29:23, nasko wrote:
> FrameTreeNode is mostly internal concept to content/. We have had to expose it
> in a couple of places, but this one doesn't look necessary. Why not drop this
> one, since the next constructor should be good enough?
> 
> I think an content/ internal version using the FTN will be nice to have.

So, Camille wanted this functionality as a prereq for exposing class this to the
public API. What if we took a RenderFrameHost as argument, and internally used
its FTN? The exposed API could say something like:
"Intercepts navigations in the frame hosted by the given RFH". How does that
sound to you?

[nasko, 2016-08-01 23:38:11]

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:556:
RenderFrameDevToolsAgentHost::CreateThrottleForNavigation(this);
On 2016/08/01 17:42:19, csharrison wrote:
> On 2016/08/01 15:29:23, nasko wrote:
> > This throttle was added at the end of the list intentionally. Are we sure it
> is
> > ok to move before the rest?
> 
> It is still at the end of the "real" throttles. The only throttles that can
come
> after it are ones from RegisterThrottleForTesting. If a time comes when a
> testing throttles needs to come before a real throttle, I think we can add
> support for it then.

If it has no effect, then what necessitated the change?

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.cc:359: class
TestNavigationManagerThrottle : public NavigationThrottle {
On 2016/08/01 17:42:19, csharrison wrote:
> On 2016/08/01 15:29:23, nasko wrote:
> > Did this really need to move to the public files? It isn't exposed outside
of
> > content/ (and it shouldn't be), so I wonder why the move.
> 
> Hm well it is only needed by the TestNavigationManager so I assumed an
anonymous
> namespace in the TestNavigationManagers impl is the best approach. I could
> alternatively keep this in the private file, but would have to expose it via
> headers which is not really needed. Would you prefer that?

Ok, good point. Let's keep it here for now.

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.h:587: // Monitors notifications within
the given frame tree node. Use the other
On 2016/08/01 17:42:19, csharrison wrote:
> On 2016/08/01 15:29:23, nasko wrote:
> > FrameTreeNode is mostly internal concept to content/. We have had to expose
it
> > in a couple of places, but this one doesn't look necessary. Why not drop
this
> > one, since the next constructor should be good enough?
> > 
> > I think an content/ internal version using the FTN will be nice to have.
> 
> So, Camille wanted this functionality as a prereq for exposing class this to
the
> public API. What if we took a RenderFrameHost as argument, and internally used
> its FTN? The exposed API could say something like:
> "Intercepts navigations in the frame hosted by the given RFH". How does that
> sound to you?

We don't have anyone outside of content/ using this constructor, do we? If not,
we should not be exposing it.

[Charlie Harrison, 2016-08-02 02:36:01]

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

[commit-bot: I haz the power, 2016-08-02 02:36:19]

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-08-02 02:46:35]

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

[commit-bot: I haz the power, 2016-08-02 02:46:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Harrison, 2016-08-02 03:04:46]

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

[commit-bot: I haz the power, 2016-08-02 03:05:05]

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-08-02 04:10:22]

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

[commit-bot: I haz the power, 2016-08-02 04:10:23]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-02 13:09:15]

OK. I ended up adding a PerFrameTestNavigationManager private to content that
only applies frame_tree_node_id filtering. WDYT?

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2132603002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:556:
RenderFrameDevToolsAgentHost::CreateThrottleForNavigation(this);
On 2016/08/01 23:38:11, nasko wrote:
> On 2016/08/01 17:42:19, csharrison wrote:
> > On 2016/08/01 15:29:23, nasko wrote:
> > > This throttle was added at the end of the list intentionally. Are we sure
it
> > is
> > > ok to move before the rest?
> > 
> > It is still at the end of the "real" throttles. The only throttles that can
> come
> > after it are ones from RegisterThrottleForTesting. If a time comes when a
> > testing throttles needs to come before a real throttle, I think we can add
> > support for it then.
> 
> If it has no effect, then what necessitated the change?

The code moves the test navigation throttles to the end, so that
TestNavigationManager defers a navigation after the page_load_metrics system
observes it. Otherwise we can't use TestNavigationManager to test the new code
path.

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2132603002/diff/360001/content/public/test/br...
content/public/test/browser_test_utils.h:587: // Monitors notifications within
the given frame tree node. Use the other
On 2016/08/01 23:38:11, nasko wrote:
> On 2016/08/01 17:42:19, csharrison wrote:
> > On 2016/08/01 15:29:23, nasko wrote:
> > > FrameTreeNode is mostly internal concept to content/. We have had to
expose
> it
> > > in a couple of places, but this one doesn't look necessary. Why not drop
> this
> > > one, since the next constructor should be good enough?
> > > 
> > > I think an content/ internal version using the FTN will be nice to have.
> > 
> > So, Camille wanted this functionality as a prereq for exposing class this to
> the
> > public API. What if we took a RenderFrameHost as argument, and internally
used
> > its FTN? The exposed API could say something like:
> > "Intercepts navigations in the frame hosted by the given RFH". How does that
> > sound to you?
> 
> We don't have anyone outside of content/ using this constructor, do we? If
not,
> we should not be exposing it.

Fair point.  I've removed the method and added a subclass that does this
filtering private to content.

[nasko, 2016-08-02 16:22:16]

LGTM with a couple of nits.

https://codereview.chromium.org/2132603002/diff/420001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/420001/content/public/test/br...
content/public/test/browser_test_utils.cc:1628: will_start_loop_runner_->Quit();
Why would we have either of the two runners running? Would be good to document
in a comment.

https://codereview.chromium.org/2132603002/diff/420001/content/test/content_b...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/2132603002/diff/420001/content/test/content_b...
content/test/content_browser_test_utils_internal.h:161: class
PerFrameTestNavigationManager : public TestNavigationManager {
nit: Drop the "Per" prefix. We usually prefix something with "Frame" when it is
per frame, so it will be consistent with naming of other classes.

[Charlie Harrison, 2016-08-02 16:47:13]

Thanks, everyone!

https://codereview.chromium.org/2132603002/diff/420001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2132603002/diff/420001/content/public/test/br...
content/public/test/browser_test_utils.cc:1628: will_start_loop_runner_->Quit();
On 2016/08/02 16:22:16, nasko wrote:
> Why would we have either of the two runners running? Would be good to document
> in a comment.

For |did_finish_loop_runner_|, this is the normal place to quit the run loop to
notify the navigation is finished.

For |will_start_loop_runner_|, we could still be spinning here if the navigation
is cancelled while it is deferred.

I've clarified this in a comment.

https://codereview.chromium.org/2132603002/diff/420001/content/test/content_b...
File content/test/content_browser_test_utils_internal.h (right):

https://codereview.chromium.org/2132603002/diff/420001/content/test/content_b...
content/test/content_browser_test_utils_internal.h:161: class
PerFrameTestNavigationManager : public TestNavigationManager {
On 2016/08/02 16:22:16, nasko wrote:
> nit: Drop the "Per" prefix. We usually prefix something with "Frame" when it
is
> per frame, so it will be consistent with naming of other classes.

Done.

[Charlie Harrison, 2016-08-02 16:47:38]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-08-02 16:47:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, isherman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2132603002/#ps440001
(title: "nasko@ nits")

[commit-bot: I haz the power, 2016-08-02 16:48:14]

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-08-02 16:55:51]

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

[commit-bot: I haz the power, 2016-08-02 16:55:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Harrison, 2016-08-02 16:59:34]

Description was changed from

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

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

to

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

TBR=jam@chromium.org

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

[Charlie Harrison, 2016-08-02 17:00:28]

jam@ I am adding you to TBR for components/page_load_metrics.gypi as I am no
longer in OWNERS for that :)

[Charlie Harrison, 2016-08-02 17:01:44]

Actually ccing jam@, who I am adding to TBR for
components/page_load_metrics.gypi (I am no longer an owner).

[Charlie Harrison, 2016-08-02 17:02:04]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-08-02 17:02:41]

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-08-02 18:03:31]

Description was changed from

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

TBR=jam@chromium.org

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

to

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

TBR=jam@chromium.org

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

[commit-bot: I haz the power, 2016-08-02 18:03:32]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-08-02 18:05:35]

Description was changed from

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

TBR=jam@chromium.org

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

to

==========
[page_load_metrics] Add a NavigationThrottle for richer abort metrics

This patch adds a NavigationThrottle, which forwards calls to
WillStartRequest to the MetricsWebContentsObserver. At this
point in the navigation, the NavigationHandle has additional
data (like user gesture and transition type) that is extremely
useful to track abort types.

With this patch, we thread the page transition type at WillStartRequest
time into the PageLoadTracker, where we use it to attribute abort cause
to previously aborted provisional loads. This immediately yields a
dramatic improvement to the abort metrics, as suddenly the BeforeCommit
variants of all UnknownNavigation abort metrics will have a reliable
transition type that aborted them.

A followup patch will thread the user gesture bit to generate a more
accurate picture of "user initiated" aborts.

TBR=jam@chromium.org

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

Committed: https://crrev.com/4bd8a19469434b79ff524323f44eca517e799321
Cr-Commit-Position: refs/heads/master@{#409237}
==========

[commit-bot: I haz the power, 2016-08-02 18:05:37]

Patchset 23 (id:??) landed as
https://crrev.com/4bd8a19469434b79ff524323f44eca517e799321
Cr-Commit-Position: refs/heads/master@{#409237}