1312881 - Measure the amount of max scroll Y per page

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Comment 1

[Chris Peterson [:cpeterson]]

Milan says Ryan will be adding probes for Quantum's scrolling metrics.

Comment 2

[Ryan Hunt [:rhunt]]

Just to clarify, what is the exact metric we want to be measuring here?

The maximum distance in CSS pixels scrolled down the page? Should this include other scrollable frames at all? I'm not sure if it's a problem, but I think the root element on pages like gmail don't scroll at all. The scrolling all happens in a sub frame.

And also, what do we want to do in the case of the user hitting the 'End' key and jumping to the end of the page. Should that count? This might be a shared discussing with bug 1297867.

Comment 3

[:Harald Kirschner :digitarald]

Thanks Ryan for diving into this!

> The maximum distance in CSS pixels scrolled down the page?

Yes

> Should this include other scrollable frames at all?

No, just the root element for now.

> What do we want to do in the case of the user hitting the 'End' key and jumping to the end of the page. Should that count?

Yes, this probe should both include keyboard and mouse/touch scroll input.

Comment 4

[Benjamin Smedberg]

Harald do you mean root *frame* or root *element*? I suspect most complex websites no longer scroll the root frame; they position a subframe as the main scrollport.

Comment 5

[:Harald Kirschner :digitarald]

> Harald do you mean root *frame* or root *element*?

Only body (root element) for the root frame (no iframes).

> I suspect most complex websites no longer scroll the root frame; they position a subframe as the main scrollport.

I reviewed top sites and have not seen too many cases of that. Not including them will reduce risk as the complex nature of these apps would skew this metric; like Google Spreadsheets' content is 2d canvas rendered with scrolling synthesized using events.

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: track-scroll-y.patch

This patch creates a new telemetry histogram and adds code to the browser-content.js frame script to track the maximum scrollY of a page.

Comment 7

[Ryan Hunt [:rhunt]]

Comment on attachment 8815935 [details] [diff] [review]
track-scroll-y.patch

Cancelling the review as we're going to need to make changes to not do this when private browsing is enabled.

Comment 8

[Benjamin Smedberg]

Per the other bug, I don't think we need to block this on private browsing because this doesn't actually record that you were using private browsing. I apologize for the confusion.

Comment 9

[Ryan Hunt [:rhunt]]

Attachment: track-scrolling2.patch

This patch adds to the browser-content.js frame script to track the max scrollY of a page. There's been a bit of back and forth on what that exactly is in the comments, you can read the exact definition in the histogram definition.

This telemetry probe is closely related to the probe in bug 1297867, and both can be implemented in basically the same way. So I made this patch to be the second part of a two part patch, the first part is in bug 1297867.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Pretty sure we're talking about the Y-axis here.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8818038 [details] [diff] [review]
track-scrolling2.patch

Review of attachment 8818038 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/browser-content.js
@@ +1785,4 @@
>    }, { passive: true });
> +
> +  content.addEventListener("unload", function() {
> +    if (content.location.protocol === "about:") {

Out of curiosity, why the exclusion of about: pages for this one probe? Should we be ignoring about: pages for the other probe as well?

Comment 12

[:Harald Kirschner :digitarald]

> Out of curiosity, why the exclusion of about: pages for this one probe? Should we be ignoring about: pages for the other probe as well?

I didn't think of this specifically in the initial spec, but it makes absolute sense to focus this measurement on actual web content. As we own control about: pages we can instrument them on a much deeper level if needed.

Comment 13

[Ryan Hunt [:rhunt]]

(In reply to Mike Conley (:mconley) from comment #11)
> Comment on attachment 8818038 [details] [diff] [review]
> track-scrolling2.patch
> 
> Review of attachment 8818038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/browser-content.js
> @@ +1785,4 @@
> >    }, { passive: true });
> > +
> > +  content.addEventListener("unload", function() {
> > +    if (content.location.protocol === "about:") {
> 
> Out of curiosity, why the exclusion of about: pages for this one probe?
> Should we be ignoring about: pages for the other probe as well?

Basically what Harald said, I was trying to keep this focused on actual web content. I missed that I was applying this to only one of the probes when I was combining the two approaches. I think that should be changed.

Comment 14

[Ryan Hunt [:rhunt]]

Attachment: scroll-tracking2.patch

Same as before, but with excluding 'about:' pages moved to apply to both probes.

Comment 15

[Ryan Hunt [:rhunt]]

Comment on attachment 8818627 [details] [diff] [review]
scroll-tracking2.patch

feedback? for data review

Comment 16

[Benjamin Smedberg]

Comment on attachment 8818627 [details] [diff] [review]
scroll-tracking2.patch

data-r=me

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9560724b684c
Add a telemetry probe to track the max scrollY of a page. r=mconley data-review=bsmedberg

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9560724b684c

Comment 19

[Ryan Hunt [:rhunt]]

I backed out the original patch for causing bug 1325645, and for while I investigate the mass test failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bbaff1fd9d2

Comment 20

[Ryan Hunt [:rhunt]]

Attachment: track-max-scroll-y.patch

This patch corrects the issues of the previous patch, as discussed here. [1]

It's the second part of the patch attached to the bug 1297867, like last time.

I'll add a reviewer when it's not a holiday anymore.

Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdea25f90e16090a3a49e64b93aebfc1fb827618

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1297867#c65

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8821789 [details] [diff] [review]
track-max-scroll-y.patch

Review of attachment 8821789 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/browser-content.js
@@ +1810,5 @@
>          this._prevScrollY = content.scrollY;
> +
> +        if (content.scrollY > this._maxScrollY) {
> +          this._maxScrollY = content.scrollY;
> +        }

Please replace the above if-statement with:
this._maxScrollY = Math.max(content.scrollY, this._maxScrollY);

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce32b66735c
Add a telemetry probe to track the max scrollY of a page. r=jaws data-review=bsmedberg

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fce32b66735c

Comment 24

[:Harald Kirschner :digitarald]

Comment on attachment 8821789 [details] [diff] [review]
track-max-scroll-y.patch

Approval Request Comment
[User impact if declined]:
None for user. We might miss an opportunity to baseline this probe during e10s rollout.

[Has the fix been verified in Nightly?]:
Telemetry data verified on Nightly & Aurora.

[Is the change risky?]:
Minimal risk.

[Why is the change risky/not risky?]:
Only tracks max scrolling position per page.

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8821789 [details] [diff] [review]
track-max-scroll-y.patch

another telemetry probe for scrolling, beta52+

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7ae89f085815