Thread user gesture through page_load_metrics abort pipeline

Thread user gesture through page_load_metrics abort pipeline

BUG=633329
Committed: https://crrev.com/49ff2597f03279f06e31d9619606c94047206107
Cr-Commit-Position: refs/heads/master@{#412240}

[Charlie Harrison, 2016-08-05 15:06:15]

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

[commit-bot: I haz the power, 2016-08-05 15:06:45]

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-05 15:48:37]

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

[commit-bot: I haz the power, 2016-08-05 15:48:38]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-08 15:07:06]

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

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

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-08-08 15:20:35]

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

[Charlie Harrison, 2016-08-08 15:20:36]

Shivani, mind taking a quick look at this? I've updated a few histograms to
include the UserInitiated suffix, but it's just to see what kind of data we're
getting with the user_gesture bit.

[commit-bot: I haz the power, 2016-08-08 16:21:57]

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

[commit-bot: I haz the power, 2016-08-08 16:21:58]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2016-08-11 19:23:02]

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:246: //
TODO(crbug.com/617904): Browser initiated navigations should have
todo (csharrison) for consistency of style?

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:247: //
HasUserGesture() set to true.
do we want to add a dcheck here to assert this?

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:270:
!navigation_handle->IsRendererInitiated()),
call IsNavigationUserInitiated here?

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:258: bool
abort_user_initiated_;
May be briefly explain in the comment on why this is an approximation.

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:276: bool
user_gesture_;
typo: bit it

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:67:
bool AbortUserInitiated(const page_load_metrics::PageLoadExtraInfo& info) {
isAbortUserInitiated instead of AbortUserInitiated?

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:72:
bool user_inititaed,
typo: spelling of initiated

[Charlie Harrison, 2016-08-12 17:50:02]

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

[commit-bot: I haz the power, 2016-08-12 17:50:23]

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-08-12 17:51:30]

I've found a few problems with NavigationHandle::IsRendererInitiated
(crbug.com/637345). For now I am only using the user_gesture bit and filtering
out browser initiated navigations.

I wanted to do this anyways to see if we filter out any of the fast aborters if
we restrict a subset of valid navs.

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:246: //
TODO(crbug.com/617904): Browser initiated navigations should have
On 2016/08/11 19:23:00, shivanisha wrote:
> todo (csharrison) for consistency of style?

TODOs with crbugs are also ok style. I don't have strong sense of ownership of
that bug and might punt it.

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:247: //
HasUserGesture() set to true.
On 2016/08/11 19:23:00, shivanisha wrote:
> do we want to add a dcheck here to assert this?

No, because they don't currently have it set to true (see the linked bug).

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:270:
!navigation_handle->IsRendererInitiated()),
On 2016/08/11 19:23:01, shivanisha wrote:
> call IsNavigationUserInitiated here?

Done.

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:258: bool
abort_user_initiated_;
On 2016/08/11 19:23:01, shivanisha wrote:
> May be briefly explain in the comment on why this is an approximation.

Done.

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:276: bool
user_gesture_;
On 2016/08/11 19:23:01, shivanisha wrote:
> typo: bit it

Done.

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:67:
bool AbortUserInitiated(const page_load_metrics::PageLoadExtraInfo& info) {
On 2016/08/11 19:23:01, shivanisha wrote:
> isAbortUserInitiated instead of AbortUserInitiated?

Done.

https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:72:
bool user_inititaed,
On 2016/08/11 19:23:01, shivanisha wrote:
> typo: spelling of initiated

oops :P

[commit-bot: I haz the power, 2016-08-12 18:16:28]

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

[commit-bot: I haz the power, 2016-08-12 18:16:29]

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...)

[shivanisha, 2016-08-13 01:58:00]

On 2016/08/12 at 17:51:30, csharrison wrote:
> I've found a few problems with NavigationHandle::IsRendererInitiated
(crbug.com/637345). For now I am only using the user_gesture bit and filtering
out browser initiated navigations.
> 
> I wanted to do this anyways to see if we filter out any of the fast aborters
if we restrict a subset of valid navs.
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:246: //
TODO(crbug.com/617904): Browser initiated navigations should have
> On 2016/08/11 19:23:00, shivanisha wrote:
> > todo (csharrison) for consistency of style?
> 
> TODOs with crbugs are also ok style. I don't have strong sense of ownership of
that bug and might punt it.
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:247: //
HasUserGesture() set to true.
> On 2016/08/11 19:23:00, shivanisha wrote:
> > do we want to add a dcheck here to assert this?
> 
> No, because they don't currently have it set to true (see the linked bug).
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:270:
!navigation_handle->IsRendererInitiated()),
> On 2016/08/11 19:23:01, shivanisha wrote:
> > call IsNavigationUserInitiated here?
> 
> Done.
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.h:258: bool
abort_user_initiated_;
> On 2016/08/11 19:23:01, shivanisha wrote:
> > May be briefly explain in the comment on why this is an approximation.
> 
> Done.
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.h:276: bool
user_gesture_;
> On 2016/08/11 19:23:01, shivanisha wrote:
> > typo: bit it
> 
> Done.
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
> File
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc
(right):
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
>
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:67:
bool AbortUserInitiated(const page_load_metrics::PageLoadExtraInfo& info) {
> On 2016/08/11 19:23:01, shivanisha wrote:
> > isAbortUserInitiated instead of AbortUserInitiated?
> 
> Done.
> 
>
https://codereview.chromium.org/2223453003/diff/40001/chrome/browser/page_loa...
>
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:72:
bool user_inititaed,
> On 2016/08/11 19:23:01, shivanisha wrote:
> > typo: spelling of initiated
> 
> oops :P

LGTM

[Charlie Harrison, 2016-08-13 02:10:35]

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

[Charlie Harrison, 2016-08-13 02:10:35]

Thanks, Shivani. holte@ can you take a look at histograms.xml?

[Steven Holte, 2016-08-15 21:55:58]

lgtm

[Charlie Harrison, 2016-08-15 22:03:30]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-08-15 22:03:49]

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-15 22:06:19]

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

[commit-bot: I haz the power, 2016-08-15 22:06:20]

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-16 14:09:17]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-08-16 14:09:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
shivanisha@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2223453003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-16 14:09:36]

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

[commit-bot: I haz the power, 2016-08-16 15:18:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-16 15:20:35]

Description was changed from

==========
Thread user gesture through page_load_metrics abort pipeline

BUG=633329
==========

to

==========
Thread user gesture through page_load_metrics abort pipeline

BUG=633329

Committed: https://crrev.com/49ff2597f03279f06e31d9619606c94047206107
Cr-Commit-Position: refs/heads/master@{#412240}
==========

[commit-bot: I haz the power, 2016-08-16 15:20:36]

Patchset 5 (id:??) landed as
https://crrev.com/49ff2597f03279f06e31d9619606c94047206107
Cr-Commit-Position: refs/heads/master@{#412240}