Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481013007:
Improve tracking of user initiated page loads. (Closed)
|
Created:
4 years, 1 month ago by Bryan McQuade Modified:
4 years, 1 month ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove tracking of user initiated page loads.
* consider all browser initiated navs to be user initiated
* get rid of special case tracking of user initiated aborts
BUG=617904
Committed: https://crrev.com/386a541edc81f7e9302009e0122e07a467754767
Cr-Commit-Position: refs/heads/master@{#431558}
Patch Set 1 #Patch Set 2 : fix tests #
Total comments: 5
Patch Set 3 : revert broken logic #Patch Set 4 : revert broken logic #
Total comments: 4
Patch Set 5 : address comments #Patch Set 6 : fix comment #
Total comments: 4
Patch Set 7 : address comments #Messages
Total messages: 47 (31 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Improve tracking of user initiated page loads. BUG=617904 ========== to ========== Improve tracking of user initiated page loads. * consider all browser initiated navs to be user initiated * get rid of special case tracking of user initiated aborts * track user initiated stats for all types of aborts BUG=617904 ==========
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org, isherman@chromium.org
csharrison, PTAL for page_load_metrics isherman, PTAL for histograms.xml Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms.xml lgtm
I am confused why we are removing abort_user_initiated. Do you feel it has limited value?
On 2016/11/10 at 13:39:40, csharrison wrote: > I am confused why we are removing abort_user_initiated. Do you feel it has limited value? Ah, my understanding is that it's just set based on user_initiated && abort_type != CLIENT_REDIRECT. This can also be inferred in the observer implementation. Given that the only consumer of this field is the aborts observer, and that we also have a more general user_initiated field which that observer can use to compute the value of the old abort_user_initiated field, I felt it was better to reduce the number of fields in PageLoadExtraInfo and just infer this value directly in the aborts observer. If there is something I am missing though and my change does change the behavior in some way let me know & we can revert.
https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_tracker.cc:643: bool user_initiated, This is my main worry. We thread user_initiated through the abort pipeline to say if the abort itself was user initiated, but now we aren't using the value.
https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:81: bool IsAbortUserInitiated(const page_load_metrics::PageLoadExtraInfo& info) { thinking about this more, do we care about whether this nav was user initiated when deciding if the abort was user initiated? for instance, if a web page initiates a non-user-initiated navigation to some URL, and I then click the stop button for that nav, I think I'd consider that a user initiated abort, even though the nav being aborted wasn't user initiated. What do you think? If this makes sense to you then I think we should remove the info.user_gesture check from this method, and look only at the value of info.abort_user_initiated here. https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_tracker.cc:643: bool user_initiated, On 2016/11/10 at 14:02:01, Charlie Harrison wrote: > This is my main worry. We thread user_initiated through the abort pipeline to say if the abort itself was user initiated, but now we aren't using the value. Ah, I see now that this is whether the nav that aborted this page load was user initiated, rather than whether the current page load was user initiated. I overlooked that. I will revert my change and keep the abort_user_initiated_ bool that you added. Thanks for pointing this out!
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I reverted the abort_user_initiated change, thanks for catching that! https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:290: extra_info.abort_user_initiated, earlier, I noted that when deciding if the abort was user initiated, we should probably not also look at whether the nav being aborted was user initiated. thus the logic in IsAbortUserInitiated simplifies to checking this boolean, so I removed the function and inlined the logic. https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_tracker.h (left): https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_tracker.h:250: // This boolean is only an approximation. As the aborts pipeline is updated, I changed this comment as I feel this kind of comment is really more about the limitations of user initiated navigation tracking in general than it is specific to user-initiated abort tracking. The 'this is only set for navigations aborting navigations' also isn't totally accurate anymore now that a page backgrounding can cause an abort with an abort type of 'background' so I removed it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few more high level comments https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:81: bool IsAbortUserInitiated(const page_load_metrics::PageLoadExtraInfo& info) { On 2016/11/10 15:35:41, Bryan McQuade wrote: > thinking about this more, do we care about whether this nav was user initiated > when deciding if the abort was user initiated? for instance, if a web page > initiates a non-user-initiated navigation to some URL, and I then click the stop > button for that nav, I think I'd consider that a user initiated abort, even > though the nav being aborted wasn't user initiated. What do you think? If this > makes sense to you then I think we should remove the info.user_gesture check > from this method, and look only at the value of info.abort_user_initiated here. I'm on the fence about this. I feel like checking that the abortee was user initiated ensures that this metric is all about the user giving up on what they were trying to do. If the site is trying to navigate on its own I don't think the user is necessarily aware they are "giving up" on anything. https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_tracker.h (left): https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_tracker.h:250: // This boolean is only an approximation. As the aborts pipeline is updated, On 2016/11/10 15:51:22, Bryan McQuade wrote: > I changed this comment as I feel this kind of comment is really more about the > limitations of user initiated navigation tracking in general than it is specific > to user-initiated abort tracking. > > The 'this is only set for navigations aborting navigations' also isn't totally > accurate anymore now that a page backgrounding can cause an abort with an abort > type of 'background' so I removed it. I'd like the comment to remain general, as it is important that we eventually distinguish e.g. non-navigational aborts as user initiated or not (i.e. tab close vs process OOM).
Description was changed from ========== Improve tracking of user initiated page loads. * consider all browser initiated navs to be user initiated * get rid of special case tracking of user initiated aborts * track user initiated stats for all types of aborts BUG=617904 ========== to ========== Improve tracking of user initiated page loads. * consider all browser initiated navs to be user initiated * get rid of special case tracking of user initiated aborts BUG=617904 ==========
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! PTAL. https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2481013007/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:81: bool IsAbortUserInitiated(const page_load_metrics::PageLoadExtraInfo& info) { On 2016/11/10 at 20:33:04, Charlie Harrison wrote: > On 2016/11/10 15:35:41, Bryan McQuade wrote: > > thinking about this more, do we care about whether this nav was user initiated > > when deciding if the abort was user initiated? for instance, if a web page > > initiates a non-user-initiated navigation to some URL, and I then click the stop > > button for that nav, I think I'd consider that a user initiated abort, even > > though the nav being aborted wasn't user initiated. What do you think? If this > > makes sense to you then I think we should remove the info.user_gesture check > > from this method, and look only at the value of info.abort_user_initiated here. > > I'm on the fence about this. I feel like checking that the abortee was user initiated ensures that this metric is all about the user giving up on what they were trying to do. If the site is trying to navigate on its own I don't think the user is necessarily aware they are "giving up" on anything. That seems reasonable to me. I could see it being potentially misleading to consumers of the data, but OTOH once we have high quality user-initited navigation signals, maybe all of our metrics should only track user initiated navs by default, and we can break out the non-user-initiated navs into a separate suffix like we do with .Background. Something to consider as we work on improving the user gesture signals. https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/page_load_tracker.h (left): https://codereview.chromium.org/2481013007/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/page_load_tracker.h:250: // This boolean is only an approximation. As the aborts pipeline is updated, On 2016/11/10 at 20:33:04, Charlie Harrison wrote: > On 2016/11/10 15:51:22, Bryan McQuade wrote: > > I changed this comment as I feel this kind of comment is really more about the > > limitations of user initiated navigation tracking in general than it is specific > > to user-initiated abort tracking. > > > > The 'this is only set for navigations aborting navigations' also isn't totally > > accurate anymore now that a page backgrounding can cause an abort with an abort > > type of 'background' so I removed it. > > I'd like the comment to remain general, as it is important that we eventually distinguish e.g. non-navigational aborts as user initiated or not (i.e. tab close vs process OOM). Makes sense. I updated the comment to be a little more specific, and to reflect the change that we now track some non-nav-related user initiated aborts.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:111: // Whether the page load that aborted this page load was user initiated. nit: Can this comment be more general too? https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_tracker.cc:93: return handle->HasUserGesture() || !handle->IsRendererInitiated(); Can you reference the crbug that shows this property flip flopping on reload as a potential gotcha?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by bmcquade@chromium.org
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2481013007/#ps120001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:111: // Whether the page load that aborted this page load was user initiated. On 2016/11/10 at 22:26:05, Charlie Harrison wrote: > nit: Can this comment be more general too? Sure, I duped the other comment. https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2481013007/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_tracker.cc:93: return handle->HasUserGesture() || !handle->IsRendererInitiated(); On 2016/11/10 at 22:26:05, Charlie Harrison wrote: > Can you reference the crbug that shows this property flip flopping on reload as a potential gotcha? Done
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by bmcquade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Improve tracking of user initiated page loads. * consider all browser initiated navs to be user initiated * get rid of special case tracking of user initiated aborts BUG=617904 ========== to ========== Improve tracking of user initiated page loads. * consider all browser initiated navs to be user initiated * get rid of special case tracking of user initiated aborts BUG=617904 Committed: https://crrev.com/386a541edc81f7e9302009e0122e07a467754767 Cr-Commit-Position: refs/heads/master@{#431558} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/386a541edc81f7e9302009e0122e07a467754767 Cr-Commit-Position: refs/heads/master@{#431558} |
