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

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.

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.

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.

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

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

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.

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.

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

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?

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

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

Comment on attachment 8818627 [details] [diff] [review] scroll-tracking2.patch feedback? for data review

Comment on attachment 8818627 [details] [diff] [review] scroll-tracking2.patch data-r=me

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

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

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

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

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 on attachment 8821789 [details] [diff] [review] track-max-scroll-y.patch another telemetry probe for scrolling, beta52+