New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 596956 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
(currently inactive on Chromium)
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

[scroll anchoring] add UMA metrics for scroll anchoring

Project Member Reported by skobes@chromium.org, Mar 22 2016

Issue description

To start with, we should add an UMA metric to track how often ScrollAnchor modifies the scroll position.

Caveats:

- This will give false positives when the anchoring scroll is immediately overridden by something else (e.g. smooth scroll,  bug 594456 ).

- This won't directly tell us if there was a "visible jump" due to a poorly chosen anchor node or some other bug.

Perhaps it's also useful to know the number of page views in which one or more anchoring scrolls occurred?  I'm not sure how hard this is with UMA.
 

Comment 1 by skobes@chromium.org, Mar 23 2016

Cc: -ymalik@chromium.org
Owner: ymalik@chromium.org
Status: Assigned (was: Available)

Comment 2 by ymalik@chromium.org, Mar 24 2016

I have a patch that gives us the following metrics:
- Number of times ScrollAnchor modifies scroll position
- Out of all the page visits, for how many of them did we have to adjust the scroll offset (limited by  Issue 513059 ).

I wanted to add an histogram that would tell us the number of times we do x adjustments on a page (where x = 1, 2, ..), but this is a little harder than I had anticipated. The problem is that we have a ScrollAnchor per FrameView/PaintLayerScrollableArea, but we need to update stats per page. Perhaps we will need something like UseCounter but for CustomCount histograms.

In any case, the above metrics should suffice for now? 

Comment 3 by ymalik@chromium.org, Mar 24 2016

Forgot to link the patch: https://codereview.chromium.org/1827793004/

Comment 4 by ojan@chromium.org, Mar 24 2016

What would we learn from "Number of times ScrollAnchor modifies scroll position"?

I would start with just doing the second one (# page visits where we adjusted the scroll offset).

Comment 5 by skobes@chromium.org, Mar 24 2016

In addition to # of page views I thought it would be useful to have a sense of how often anchoring scrolls occur during a session.  I think ymalik's patch will give us both of these, no?

Comment 6 by ojan@chromium.org, Mar 24 2016

If you think it's useful, LGTM. Histograms can be pretty expensive computationally (UseCounter only logs it's histograms as set points), but I think ScrollAnchor::restore should not be a hot codepath so it should be fine perf-wise, right?

Comment 7 by skobes@chromium.org, Mar 24 2016

Perhaps the UMA will tell us how "hot" the ScrollAnchor::restore codepath is.

Comment 8 by ymalik@chromium.org, Mar 29 2016

Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/947c497505bf933d448b5787568491a0203c35b6

commit 947c497505bf933d448b5787568491a0203c35b6
Author: ymalik <ymalik@chromium.org>
Date: Tue Mar 29 23:43:30 2016

Add initial UMA metrics for ScrollAnchoring

This CL adds the Layout.ScrollAnchor.AdjustedScrollOffset histogram which
simply counts the number of times an adjustment is made.

This CL also adds a UseCount to ScrollAnchor.

The first metric gives us an overall idea of the number of times we
adjust the scroll position. The UseCount tells us out of all the page
visits, for how many of them did we have to adjust the scroll offset.

BUG= 596956 

Review URL: https://codereview.chromium.org/1827793004

Cr-Commit-Position: refs/heads/master@{#383868}

[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/core/layout/ScrollAnchorTest.cpp
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/platform/testing/HistogramTester.cpp
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/platform/testing/HistogramTester.h
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/tools/metrics/histograms/histograms.xml

Project Member

Comment 10 by bugdroid1@chromium.org, Mar 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/947c497505bf933d448b5787568491a0203c35b6

commit 947c497505bf933d448b5787568491a0203c35b6
Author: ymalik <ymalik@chromium.org>
Date: Tue Mar 29 23:43:30 2016

Add initial UMA metrics for ScrollAnchoring

This CL adds the Layout.ScrollAnchor.AdjustedScrollOffset histogram which
simply counts the number of times an adjustment is made.

This CL also adds a UseCount to ScrollAnchor.

The first metric gives us an overall idea of the number of times we
adjust the scroll position. The UseCount tells us out of all the page
visits, for how many of them did we have to adjust the scroll offset.

BUG= 596956 

Review URL: https://codereview.chromium.org/1827793004

Cr-Commit-Position: refs/heads/master@{#383868}

[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/core/layout/ScrollAnchorTest.cpp
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/platform/testing/HistogramTester.cpp
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/third_party/WebKit/Source/platform/testing/HistogramTester.h
[modify] https://crrev.com/947c497505bf933d448b5787568491a0203c35b6/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)
Labels: Hotlist-Input-Dev

Sign in to add a comment