Closed Bug 1312881 Opened 5 years ago Closed 5 years ago

Measure the amount of max scroll Y per page

Categories

(Core :: Graphics, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: Harald, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

User Story

Strawman: Measure the maximum amount of pixels scrolled down per page in a histogram.

Attachments

(1 file, 3 obsolete files)

No description provided.
User Story: (updated)
No longer blocks: 1305041
Flags: needinfo?(hkirschner)
Whiteboard: [measurement:client]
Flags: needinfo?(hkirschner)
Component: Telemetry → Graphics
Product: Toolkit → Core
No longer depends on: 1297867
Milan says Ryan will be adding probes for Quantum's scrolling metrics.
Assignee: nobody → 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.
Flags: needinfo?(hkirschner)
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.
Flags: needinfo?(hkirschner)
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.
Flags: needinfo?(hkirschner)
> 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.
Flags: needinfo?(hkirschner)
Attached patch track-scroll-y.patch (obsolete) — Splinter Review
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.
Attachment #8815935 - Flags: review?(mconley)
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.
Attachment #8815935 - Flags: review?(mconley)
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.
Attached patch track-scrolling2.patch (obsolete) — Splinter Review
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.
Attachment #8815935 - Attachment is obsolete: true
Attachment #8818038 - Flags: review?(mconley)
Pretty sure we're talking about the Y-axis here.
Summary: Measure the amount of max scroll X per page → Measure the amount of max scroll Y per page
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?
Attachment #8818038 - Flags: review?(mconley) → review+
> 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.
(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.
Attached patch scroll-tracking2.patch (obsolete) — Splinter Review
Same as before, but with excluding 'about:' pages moved to apply to both probes.
Attachment #8818038 - Attachment is obsolete: true
Comment on attachment 8818627 [details] [diff] [review]
scroll-tracking2.patch

feedback? for data review
Attachment #8818627 - Flags: feedback?(benjamin)
Comment on attachment 8818627 [details] [diff] [review]
scroll-tracking2.patch

data-r=me
Attachment #8818627 - Flags: feedback?(benjamin) → feedback+
Pushed by rhunt@eqrion.net:
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
https://hg.mozilla.org/mozilla-central/rev/9560724b684c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #8818627 - Attachment is obsolete: true
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);
Attachment #8821789 - Flags: review+
Pushed by rhunt@eqrion.net:
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
https://hg.mozilla.org/mozilla-central/rev/fce32b66735c
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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.
Attachment #8821789 - Flags: approval-mozilla-beta?
Comment on attachment 8821789 [details] [diff] [review]
track-max-scroll-y.patch

another telemetry probe for scrolling, beta52+
Attachment #8821789 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.