Closed Bug 1297867 Opened 8 years ago Closed 7 years ago

Measure the amount of scrolling in "session fragment"

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

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

People

(Reporter: Harald, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 8 obsolete files)

This is about implementing the tracking amount scrolled in a session fragment.

Still requires agreement of which unit to track.
:kats for input on unit and implementation
Flags: needinfo?(bugmail)
Blocks: 1247694
Priority: -- → P3
Whiteboard: [measurement:client]
What data is needed from this probe?
Should this e.g. record the scroll amount after each continuous scroll interaction ends into an exponential histogram?
Or is this just looking for the overall scroll amount per subsession?
> Should this e.g. record the scroll amount after each continuous scroll interaction ends into an exponential histogram?

Overall scroll amount. The per scroll amount will be too granular and might more reflect how users consume content (scrolling while reading or looking at products)

Waiting for :kats if the measure should be relative to window size or some pixel density independent unit.
I would lean towards an absolute measure in CSS pixels. Window sizes can vary wildly based on screen size and user preferences, whereas the CSS pixels scrolled on a page is relatively more stable since it's a function of the website. Over time as displays get larger we might see relative measures shrink even though engagement remains the same.

I guess actually the same could be said of webpages, so maybe we should measure the scroll amount as a percentage of page length.

Also we should clarify which scrolls are important here: just scrolling on the root scrollable of a page, or do we also care about scrolling iframes, scrollable divs, textareas, etc.? Do we want to cover all the different scroll methods (wheel, keyboard, scrollbar dragging, touch, etc) or restrict to specific input methods? We have a probe which measures the number of scrolls using different input methods (see SCROLL_INPUT_METHODS) but those are just a count of how many times the scroll was invoked using that method, and ignores how many pixels were scrolled each time.
Flags: needinfo?(bugmail)
> I guess actually the same could be said of webpages, so maybe we should measure the scroll amount as a percentage of page length.

What about vh unit, % of viewport height? Absolute page height is not known during loading while vh stays mostly constant apart from window resizing.

> Also we should clarify which scrolls are important here: just scrolling on the root scrollable of a page, or do we also care about scrolling iframes, scrollable divs, textareas, etc.?

For the sake of including all content that behaves "scrollable", I would include iframes and overflow-scroll elements but exclude inputs.

> Do we want to cover all the different scroll methods (wheel, keyboard, scrollbar dragging, touch, etc) or restrict to specific input methods?

I don't see a reason to restrict any methods. They might add different amount of scroll, but when a user wants to get somewhere on a page he will need to scroll the same distance.
Flags: needinfo?(bugmail)
(In reply to Harald Kirschner :digitarald from comment #6)
> > I guess actually the same could be said of webpages, so maybe we should measure the scroll amount as a percentage of page length.
> 
> What about vh unit, % of viewport height? Absolute page height is not known
> during loading while vh stays mostly constant apart from window resizing.

vh is effectively the same as window size, so what I said in comment 5 still applies to that. You're right though that we don't know the page height during loading. We could just record absolute CSS pixels scrolled then?

> For the sake of including all content that behaves "scrollable", I would
> include iframes and overflow-scroll elements but exclude inputs.

Sounds reasonable.

> I don't see a reason to restrict any methods. They might add different
> amount of scroll, but when a user wants to get somewhere on a page he will
> need to scroll the same distance.

Also sounds reasonable.
Flags: needinfo?(bugmail)
> We could just record absolute CSS pixels scrolled then?

For simplification this makes the most sense. We'll can later analyze longitudinal to test the idea of screen size skewing the data.
Flags: needinfo?(bugmail)
Ok.

Do you have somebody in mind to work on this already? Or should I try to find some cycles for it?
Flags: needinfo?(bugmail) → needinfo?(hkirschner)
> Do you have somebody in mind to work on this already? Or should I try to find some cycles for it?

You got recommended from a bunch of people. It would be invaluable to have you look into this.
Flags: needinfo?(hkirschner) → needinfo?(bugmail)
:kats, while it seems to be a rather straightforward metric from the outside, how do you estimate the complexity and effort involved?
There's two broad options for how to do this:
1) Instrument a codepath like ScrollFrameHelper::ScrollToWithOrigin which all scrolling goes through, and then exclude scrolls from JS and for form inputs and so on
2) Instrument the different places that receive user-driven scrolling, somewhat like we do for the SCROLL_INPUT_METHODS probe, and again ensure that we exclude form inputs and don't double-count anything or miss anything.

Both of these options have some inherent complexity in terms of making sure we include and exclude the right things. I would lean towards option (1) even though that also involves adding additional code in various places to distinguish between JS and non-JS scrolling.

I'd estimate that it would probably take about a week to get done.
Flags: needinfo?(bugmail)
:milan, could you prioritize this? We are planning this use this as a core KPI for performance work but it will require validation beforehand to baseline, which might take some iterations.
Flags: needinfo?(milan)
I'll take it into trello and get back to you.
Flags: needinfo?(milan)
Blocks: 1305041
No longer blocks: 1247694
Blocks: 1307244
r? for data collection process
Flags: needinfo?(rweiss)
Would you recommend to measure this in platform or frontend side?
Flags: needinfo?(bugmail)
I would lean towards platform.
Flags: needinfo?(bugmail)
First pass: Total number of CSS pixels scrolled in the session of Firefox.  Histogram, probably exponential, not normalized for the length of the session.
Assignee: nobody → rhunt
Ryan, let's see if we can do this in 52, but 53 for sure.
Priority: P3 → P2
Blocks: 1312881
Should this bug live in Core::Graphics?
Flags: needinfo?(milan)
Whiteboard: [measurement:client]
I'll move it, and if Harald needs it elsewhere, he can move it back.
Component: Telemetry → Graphics
Flags: needinfo?(milan)
Priority: P2 → P3
Product: Toolkit → Core
Whiteboard: [gfx-noted]
Taking it out of the engagement bucket for now as it tracks performance related interaction.
Flags: needinfo?(rweiss)
No longer blocks: 1305041, 1312881
(In reply to Harald Kirschner :digitarald from comment #22)
> Taking it out of the engagement bucket for now as it tracks performance
> related interaction.

Harald, what is the "engagement bucket"? Is scrolling distance still part of the Quantum release criteria?
Flags: needinfo?(hkirschner)
Engagement metrics were in the telemetry component and their data review process is currently being in reworked.
Flags: needinfo?(hkirschner)
I've been working on a prototype of this and I think it's ready. The general approach that I did was to instrument all the different input event handlers that we care about. Option 2, of the ones :kats listed.

I looked into option 1 and I think it is doable, but a lot of call sites would have to be updated and I'm not sure if I want to put a requirement on most of Gecko to know whether the scroll they're requesting is input related or not.

The basic types of input tracked and where they're tracked
1. Synchronous mouse scrolling (EventStateManager)
2. Asynchronous mouse/pan/fling scrolling (APZCallbackHelper)
3. Page Up, Page Down, Left, Right, Up, Down, Home, End Keys (nsPresShell)
4. Dragging the scrollbar (nsSliderFrame)
5. Clicking the scrollbar track (shift click and normal) (nsSliderFrame)
6. Clicking the scrollbar arrow (nsScrollbarButtonFrame)

I've done simple testing with and without APZ enabled and with and without scroll-behavior: smooth. I went to wikipedia and measured the page in CSS pixels, then did variations of all input methods to get to the bottom and then checked to see if the numbers added up. I also tested starting a scroll in JS to make sure that it doesn't count.
This is plumbing to allow us to track whether an APZ callback was originating from input or from something else like a javascript smooth scroll or a scroll snap.
Attachment #8811934 - Flags: review?(bugmail)
Just a helper function for converting units
Attachment #8811935 - Flags: review?(bugmail)
Data policy review.
Attachment #8811939 - Flags: feedback?(benjamin)
The code to do the actual scroll tracking. All the different locations for the probes and explanations are in the comment before the patches.
Attachment #8811940 - Flags: review?(bugmail)
Comment on attachment 8811939 [details] [diff] [review]
Part 3: Add a telemetry histogram for the total css pixels scrolled

Additionally: when should this metric expire in? I put in never only as a placeholder.
Attachment #8811939 - Flags: feedback?(hkirschner)
Attachment #8811934 - Flags: review?(bugmail) → review+
Comment on attachment 8811935 [details] [diff] [review]
Part 2: Add a function for converting scalar app units to CSS pixels

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

r=me with the change below

::: layout/base/Units.h
@@ +193,5 @@
>  struct CSSPixel {
>  
>    // Conversions from app units
>  
> +  static float FromAppUnits(const float aScalar) {

s/float aScalar/nscoord aScalar/

App units are by definition not floating point, so it doesn't make sense to have a floating point app unit. nscoord is the type used for uni-dimensional app units. It's also the argument type for NSAppUnitsToFloatPixels.

@@ +221,5 @@
>                       NSAppUnitsToFloatPixels(aMargin.bottom, float(AppUnitsPerCSSPixel())),
>                       NSAppUnitsToFloatPixels(aMargin.left, float(AppUnitsPerCSSPixel())));
>    }
>  
> +  static int32_t FromAppUnitsRounded(const float aScalar) {

Ditto here, aScalar should be an nscoord.
Attachment #8811935 - Flags: review?(bugmail) → review+
Comment on attachment 8811939 [details] [diff] [review]
Part 3: Add a telemetry histogram for the total css pixels scrolled

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

> Additionally: when should this metric expire in? I put in never only as a placeholder.

Never is expected as we want to use this as fine grained engagement metric.
Attachment #8811939 - Flags: feedback?(hkirschner) → feedback+
Comment on attachment 8811940 [details] [diff] [review]
Part 4: Track the amount of CSS pixels scrolled by user input in a subsession

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

So overall this looks ok. However given the amount of code duplication and having to handle early-exit conditions (particularly in nsSliderFrame), I'd suggest creating an RAII class (you can put it in gfx/layers/apz/util) to handle this. Initialize it with the nsIScrollableFrame pointer before the scroll happens. In the constructor, you can read the initial scroll position and in the destructor you can read the final scroll position and do the telemetry update. Then you don't have to worry about the early exit conditions as they will get handled automatically. You can add some extra braces as needed to limit the scope of the RAII instance and make sure it doesn't end up including unrelated scroll changes (although that seems unlikely anyway). The code added to APZCCallbackHelper doesn't neatly fit into the RAII pattern but all the rest should.

Also, did you verify that form input scrolling doesn't get counted by these?

::: dom/events/EventStateManager.cpp
@@ +2656,5 @@
>    nsIScrollableFrame::ScrollMomentum momentum =
>      aEvent->mIsMomentum ? nsIScrollableFrame::SYNTHESIZED_MOMENTUM_EVENT
>                          : nsIScrollableFrame::NOT_MOMENTUM;
>  
> +  float scrollBefore = aScrollableFrame->LastScrollDestination().y;

s/float/nscoord/, here and throughout this file, as well as the other files. If you extract to RAII then it just needs to be done there.

::: layout/xul/nsSliderFrame.cpp
@@ +565,5 @@
>        {
>          SetCurrentThumbPosition(scrollbar, mThumbStart, false, false);
> +      }
> +      else
> +      {

nit: cuddle the else with braces on a single line. This file seems to have all sorts of formatting so might as well stick to the standard Mozilla convention in new code.

But really with the RAII pattern I think you should use this problem goes away.
Attachment #8811940 - Flags: review?(bugmail) → review-
Also you could probably just inline the changes from part 2 into the RAII class, I'm not sure if it's worth adding new wrapper methods for NSAppUnitsToFloatPixels that are just called once.
Attached patch scroll-tracking.patch (obsolete) — Splinter Review
Updated with the review comments. This patch is all of the previous patches rolled into one.

As for form scrolling, it depends on what you mean. If you move the cursor or expand a text selection outside of view and it's scrolled into view, that won't count. If you scroll a scrollable textarea though, that does count. Tabbing around the page doesn't count either, whether it's form elements or buttons or anything else.

I hadn't thought about tabbing around much before because I usually use it with forms, but I guess you could use that for general navigation.
Attachment #8811934 - Attachment is obsolete: true
Attachment #8811935 - Attachment is obsolete: true
Attachment #8811939 - Attachment is obsolete: true
Attachment #8811940 - Attachment is obsolete: true
Attachment #8811939 - Flags: feedback?(benjamin)
Attachment #8812671 - Flags: review?(bugmail)
Comment on attachment 8812671 [details] [diff] [review]
scroll-tracking.patch

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

r=me with comments addressed. I'll let Harald decide if the scrolling of textareas is acceptable - per comment 6 it sounded like he didn't want that.

::: dom/events/EventStateManager.cpp
@@ +2657,5 @@
>      aEvent->mIsMomentum ? nsIScrollableFrame::SYNTHESIZED_MOMENTUM_EVENT
>                          : nsIScrollableFrame::NOT_MOMENTUM;
>  
>    nsIntPoint overflow;
> +  {

Add a comment like so:

{ // scope the TelemetryScrollTracker

::: gfx/layers/apz/util/TelemetryScrollTracker.h
@@ +18,5 @@
> +/**
> + * A RAII class used to collect the amount a nsIScrollableFrame scrolls
> + * and reports it to Telemetry.
> + */
> +class TelemetryScrollTracker

Add MOZ_RAII between "class" and "TelemetryScrollTracker". Also #include "mozilla/Attributes.h" to get that define.

::: gfx/layers/moz.build
@@ +7,5 @@
>  with Files('apz/**'):
>      BUG_COMPONENT = ('Core', 'Panning and Zooming')
>  
>  EXPORTS += [
> +    'apz/util/TelemetryScrollTracker.h',

put this in a new section called EXPORTS.mozilla, and include it using mozilla/TelemetryScrollTracker.h. (I try to stay away from top-level includes when possible. If it were up to me all these other files in EXPORTS would also be moved to EXPORTS.mozilla)

::: layout/xul/nsSliderFrame.cpp
@@ +566,2 @@
>        {
> +        TelemetryScrollTracker tracker(scrollableFrame);

I don't think you need the extra brace here, since the block ends after the second call to SetCurrentThumbPosition anyway. If you want to keep it for extra clarity that's fine but add a comment on the opening brace as I suggested above.

@@ +609,5 @@
>  
>      mozilla::Telemetry::Accumulate(mozilla::Telemetry::SCROLL_INPUT_METHODS,
>          (uint32_t) ScrollInputMethod::MainThreadScrollbarTrackClick);
>  
> +    {

Add comment

@@ +1317,5 @@
> +
> +  {
> +    TelemetryScrollTracker tracker(scrollableFrame);
> +    PageScroll(change);
> +  }

As before, don't need this block - but add a comment if you keep it.
Attachment #8812671 - Flags: review?(bugmail) → review+
Harald, do we want to include scrolling a textarea and/or tabbing between elements?
Flags: needinfo?(hkirschner)
> Harald, do we want to include scrolling a textarea and/or tabbing between elements?

No, let's limit this probe to scrolling to what we know as most intentional by users.
Flags: needinfo?(hkirschner)
Attached patch scroll-tracking.patch (obsolete) — Splinter Review
Updated for review comments. This patch also excludes scrolling in form inputs.

The ones I could find from research and testing are:
1. <textarea>'s
2. <select>'s
    a. with default popup list
    b. with 'multiple' attribute
3. autocomplete popup list
Attachment #8812671 - Attachment is obsolete: true
Attachment #8813458 - Flags: review?(bugmail)
Comment on attachment 8813458 [details] [diff] [review]
scroll-tracking.patch

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

LGTM, thanks!
Attachment #8813458 - Flags: review?(bugmail) → review+
You still need data collection review for the telemetry probe - see https://wiki.mozilla.org/Firefox/Data_Collection
Comment on attachment 8813458 [details] [diff] [review]
scroll-tracking.patch

Feedback for data collection review.
Attachment #8813458 - Flags: feedback?(benjamin)
Comment on attachment 8813458 [details] [diff] [review]
scroll-tracking.patch

The histogram description here needs to be much more precise. All of the details covered in this bug about scrolls on the toplevel page only and scrolls on the toplevel scrolling context only need to be included in the histogram description.

As implemented this is an opt-in metric, which is fine with me but eventually I expect that this will need to be opt-out to answer the questions.

I think that before we make this expires: never, we should prove it and make sure that it provides useful data. So I will suggest making this expires in 58 and after we've tested/correlated the data and proved that it's useful we can discuss making this permanent.

Does this metric include data from private windows? I believe that per our normal policies it should not.
Flags: needinfo?(rhunt)
Attachment #8813458 - Flags: feedback?(benjamin) → feedback-
This patch includes scrolling in any scrollable frame, but it doesn't include scrolling on input frames or scrolling initiated by JavaScript. So it does includes iframes and overflow:scroll frames. Not just the toplevel page only and toplevel scrolling context, if I understand you correctly.

Bug 1312881 tracks just the toplevel scroll frame. But the histogram description could definitely use this explanation, so I'll add that. I'll also update the expires to be 58.

And yes this does include data from private windows, so that sounds like it will need to change.
Flags: needinfo?(rhunt)
> This patch includes scrolling in any scrollable frame, but it doesn't include scrolling on input frames or scrolling initiated by JavaScript. So it does includes iframes and overflow:scroll frames. Not just the toplevel page only and toplevel scrolling context, if I understand you correctly.

Sorry for not catching this earlier, in my mind the previous discussion applied to both bugs. Could we fix this probe to behave like bug 1312881 and only track the top level frame, otherwise we can not compare the numbers? This would also simplify the code I assume.
Flags: needinfo?(rhunt)
Yes this will probably simplify the code greatly. But just to make sure that I absolutely have this down:

For both bugs, we want to only measure the scrolling that occurs on the root frame.

For this bug, we want to measure the absolute amount of scrolling, and over the duration of a session fragment. By absolute I mean if you scroll down 100px, then up 100px, we record 200px.

Where as with bug 1312881, we want to measure the maximum distance that a page is scrolled down to, on a per page basis. So scrolling down 100px, then up up 100px, we record 100px.
Flags: needinfo?(rhunt)
Thank you for summarizing it again, you got it down!
Flags: needinfo?(rhunt)
Ryan, after thinking through the private browsing issues last week I don't think we need to gate this on private browsing. We're not recording the fact that you are using private browsing, which is the bar we really care about. I hope that reduces the amount of work you have to do.
Talking with Harald on irc we decided that JS scrolls are probably not a very significant source of scrolling, and because excluding them complicates the implementation significantly we decided to not worry about it right now.
Flags: needinfo?(rhunt)
Attached patch track-scrolling1.patch (obsolete) — Splinter Review
This patch adds to the browser-content.js frame script to track the amount of scrolling in 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 1312881, and both can be implemented in basically the same way. So I made this patch to be the first part of a two part patch, the second part will be in bug 1312881 and add a few lines onto this.

I'm not sure if there is a better way to organize this besides adding another bug that contains both of these.
Attachment #8813458 - Attachment is obsolete: true
Attachment #8818037 - Flags: review?(mconley)
Comment on attachment 8818037 [details] [diff] [review]
track-scrolling1.patch

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

Tested that the sum is accumulated properly, and that we don't collect counts on subframes and inputs. Seems to behave as advertised.

Thanks!
Attachment #8818037 - Flags: review?(mconley) → review+
Attached patch scroll-tracking1.patch (obsolete) — Splinter Review
Same as before, but with excluding 'about:' pages moved to apply to both probes.
Attachment #8818037 - Attachment is obsolete: true
Comment on attachment 8818626 [details] [diff] [review]
scroll-tracking1.patch

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

This isn't marked as opt-out, which is what I imagine Harald needs. I'm fine with this being opt-out if that's a business requirement. data-r=me either way but please confirm with Harald.
Attachment #8818626 - Flags: feedback?(benjamin) → feedback+
Harald, are we going to have both of these scroll tracking probes be opt-out?
Flags: needinfo?(hkirschner)
> Harald, are we going to have both of these scroll tracking probes be opt-out?

I am ok to keep this opt-in while we work to understand this probe as a measure for performance improvements and how much data we need to make conclusions.
Flags: needinfo?(hkirschner)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8913967cd7e6
Add a telemetry probe to track the amount of scrolling in a page. r=mconley data-review=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/8913967cd7e6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Didin't try a FF Nightly yet but in SeaMonkey I now see an error in the log:

> Timestamp: 12/23/2016 12:22:37 PM
> Error: ReferenceError: event is not defined
> Source File: chrome://global/content/browser-content.js
> Line: 1772

Are you sure the code is ok?
Shouldn't this be:

> addEventListener("DOMWindowCreated", function(aEvent) {
>   if (aEvent.target !== content.document...
(In reply to Frank-Rainer Grahl from comment #60)
> Shouldn't this be:
> 
> > addEventListener("DOMWindowCreated", function(aEvent) {
> >   if (aEvent.target !== content.document...

Oh my, you're right. Something must have gone wrong when I spliced the patches. I'll have a fix as soon as possible. Sorry!
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f79edc3888e5
Follow up fix for missing parameter. rs=kats
Depends on: 1325645
Backed out for mass test failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6a515461cb2f8f8e8f567739ea9794533f0ba9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f79edc3888e5ce7647fd259cbceb6e806df2a715
Failure log (example): https://treeherder.mozilla.org/logviewer.html#?job_id=40997640&repo=mozilla-inbound

[task 2016-12-23T17:23:08.615021Z] 17:23:08     INFO - TEST-FAIL | docshell/test/navigation/test_sessionhistory.html | The author of the test has indicated that flaky timeouts are expected.  Reason: untriaged 
[task 2016-12-23T17:23:08.618167Z] 17:23:08     INFO - Buffered messages finished
[task 2016-12-23T17:23:08.620079Z] 17:23:08     INFO - TEST-UNEXPECTED-FAIL | docshell/test/navigation/test_sessionhistory.html | Should have persisted session history entry. - got false, expected true
[task 2016-12-23T17:23:08.621595Z] 17:23:08     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:271:5
[task 2016-12-23T17:23:08.623566Z] 17:23:08     INFO -     test@docshell/test/navigation/file_scrollRestoration.html:47:13
[task 2016-12-23T17:23:08.625508Z] 17:23:08     INFO -     setTimeout handler*@docshell/test/navigation/file_scrollRestoration.html:127:13
[task 2016-12-23T17:23:08.627405Z] 17:23:08     INFO -     EventListener.handleEvent*@docshell/test/navigation/file_scrollRestoration.html:125:7
[task 2016-12-23T17:23:08.629491Z] 17:23:08     INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(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/862c7c3c6e14
Status: RESOLVED → REOPENED
Flags: needinfo?(rhunt)
Resolution: FIXED → ---
There are at least two problems with the original patch.

1. We do content.addEventListener("unload") to track the end of viewing a page. Per [1] this disables the bfcache. This caused many of the test failures.
2. The presence of event listeners directly on the page window object seems to causes test failures in devtools.

The test failures were not caught beforehand because of me forgetting that I had not done a try run. My apologies.

Reading [1] it seems like 'pagehide' is a better event than 'unload' for this. Additionally we need to track 'pageshow', because on navigation back from a page 'DOMWindowCreated' is not fired.

[1] https://developer.mozilla.org/en-US/Firefox/Releases/1.5/Using_Firefox_1.5_caching
This patch corrects the issues of the previous patch from the comment above.

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

Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdea25f90e16090a3a49e64b93aebfc1fb827618
Attachment #8818626 - Attachment is obsolete: true
Comment on attachment 8821788 [details] [diff] [review]
track-scroll-amount.patch

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

::: toolkit/content/browser-content.js
@@ +1808,5 @@
> +  },
> +
> +  shouldIgnorePage: function() {
> +    return (content.location == "" ||
> +            content.location.protocol === "about:");

no need for parens here.
Attachment #8821788 - Flags: review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72d41fe5c0e
Add a telemetry probe to track the amount of scrolling in a page. r=jaws data-review=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/f72d41fe5c0e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 1328471
Depends on: 1328678
Comment on attachment 8821788 [details] [diff] [review]
track-scroll-amount.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Adding a new probe for measurement engagement with content.

[User impact if declined]:
None on user. Missing opportunity for us to test this probe on Beta during e10s rollout.

[Is the change risky?]:
Code has been on Nightly for a while and now on Aurora.

[Why is the change risky/not risky?]:
Telemetry probe does not touch any code but just increments a scroll counter.
Attachment #8821788 - Flags: approval-mozilla-beta?
Comment on attachment 8821788 [details] [diff] [review]
track-scroll-amount.patch

add telemetry probe for vertical scrolling, beta52+
Attachment #8821788 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Please don't add JS implemented telemetry probes to (reasonable) hot code paths.
(In reply to Olli Pettay [:smaug] from comment #73)
> Please don't add JS implemented telemetry probes to (reasonable) hot code
> paths.

Hey smaug,

Sorry I let this one through. :/ I thought since this event listener was "passive", its impact would be minimal.
I missed also that scrollY is accessed, and that flushes layout, which can be super slow operation.
This should probably be backed out.
ni'ing rhunt for what is probably going to need to be a backout of this patch from Nightly to Beta.
Flags: needinfo?(rhunt)
Actually, clearing ni, as it looks like this is being handled in bug 1338891.
Clearing the ni, as it is being handled in bug 1338891 :)
Flags: needinfo?(rhunt)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: