1297867 - Measure the amount of scrolling in "session fragment"

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[:Harald Kirschner :digitarald]

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

Still requires agreement of which unit to track.

Comment 1

[:Harald Kirschner :digitarald]

Tracked in https://docs.google.com/spreadsheets/d/1G33YEBL2-hSaF0-HrHPi0HVwANoTpYHsnevmMd1eZ1U/edit#gid=0

Comment 2

[:Harald Kirschner :digitarald]

:kats for input on unit and implementation

Comment 3

[Georg Fritzsche [:gfritzsche]]

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?

Comment 4

[:Harald Kirschner :digitarald]

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[:Harald Kirschner :digitarald]

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[:Harald Kirschner :digitarald]

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

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Ok.

Do you have somebody in mind to work on this already? Or should I try to find some cycles for it?

Comment 10

[:Harald Kirschner :digitarald]

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

Comment 11

[:Harald Kirschner :digitarald]

:kats, while it seems to be a rather straightforward metric from the outside, how do you estimate the complexity and effort involved?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 13

[:Harald Kirschner :digitarald]

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

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll take it into trello and get back to you.

Comment 15

[:Harald Kirschner :digitarald]

r? for data collection process

Comment 16

[:Harald Kirschner :digitarald]

Would you recommend to measure this in platform or frontend side?

Comment 17

[Kartikaya Gupta (email:[e-mail])]

I would lean towards platform.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

First pass: Total number of CSS pixels scrolled in the session of Firefox.  Histogram, probably exponential, not normalized for the length of the session.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Ryan, let's see if we can do this in 52, but 53 for sure.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Should this bug live in Core::Graphics?

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll move it, and if Harald needs it elsewhere, he can move it back.

Comment 22

[:Harald Kirschner :digitarald]

Taking it out of the engagement bucket for now as it tracks performance related interaction.

Comment 23

[Chris Peterson [:cpeterson]]

(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?

Comment 24

[:Harald Kirschner :digitarald]

Engagement metrics were in the telemetry component and their data review process is currently being in reworked.

Comment 25

[Ryan Hunt [:rhunt]]

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.

Comment 26

[Ryan Hunt [:rhunt]]

Attachment: Part 1: Track whether an APZ callback originated from user input or not

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.

Comment 27

[Ryan Hunt [:rhunt]]

Attachment: Part 2: Add a function for converting scalar app units to CSS pixels

Just a helper function for converting units

Comment 28

[Ryan Hunt [:rhunt]]

Attachment: Part 3: Add a telemetry histogram for the total css pixels scrolled

Data policy review.

Comment 29

[Ryan Hunt [:rhunt]]

Attachment: Part 4: Track the amount of CSS pixels scrolled by user input in a subsession

The code to do the actual scroll tracking. All the different locations for the probes and explanations are in the comment before the patches.

Comment 30

[Ryan Hunt [:rhunt]]

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.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 32

[:Harald Kirschner :digitarald]

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.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 35

[Ryan Hunt [:rhunt]]

Attachment: scroll-tracking.patch

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.

Comment 36

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 37

[Ryan Hunt [:rhunt]]

Harald, do we want to include scrolling a textarea and/or tabbing between elements?

Comment 38

[:Harald Kirschner :digitarald]

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

Comment 39

[Ryan Hunt [:rhunt]]

Attachment: scroll-tracking.patch

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

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8813458 [details] [diff] [review]
scroll-tracking.patch

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

LGTM, thanks!

Comment 41

[Kartikaya Gupta (email:[e-mail])]

You still need data collection review for the telemetry probe - see https://wiki.mozilla.org/Firefox/Data_Collection

Comment 42

[Ryan Hunt [:rhunt]]

Comment on attachment 8813458 [details] [diff] [review]
scroll-tracking.patch

Feedback for data collection review.

Comment 43

[Benjamin Smedberg]

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.

Comment 44

[Ryan Hunt [: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.

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.

Comment 45

[:Harald Kirschner :digitarald]

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

Comment 46

[Ryan Hunt [: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.

Comment 47

[:Harald Kirschner :digitarald]

Thank you for summarizing it again, you got it down!

Comment 48

[Benjamin Smedberg]

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.

Comment 49

[Ryan Hunt [:rhunt]]

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.

Comment 50

[Ryan Hunt [:rhunt]]

Attachment: track-scrolling1.patch

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.

Comment 51

[Mike Conley (: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!

Comment 52

[Ryan Hunt [:rhunt]]

Attachment: scroll-tracking1.patch

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

Comment 53

[Ryan Hunt [:rhunt]]

Comment on attachment 8818626 [details] [diff] [review]
scroll-tracking1.patch

feedback? for data review

Comment 54

[Benjamin Smedberg]

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.

Comment 55

[Ryan Hunt [:rhunt]]

Harald, are we going to have both of these scroll tracking probes be opt-out?

Comment 56

[:Harald Kirschner :digitarald]

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

Comment 57

[Pulsebot]

Pushed by [e-mail]:
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

Comment 58

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

https://hg.mozilla.org/mozilla-central/rev/8913967cd7e6

Comment 59

[Frank-Rainer Grahl (:frg)]

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?

Comment 60

[Frank-Rainer Grahl (:frg)]

Shouldn't this be:

> addEventListener("DOMWindowCreated", function(aEvent) {
>   if (aEvent.target !== content.document...

Comment 61

[Ryan Hunt [:rhunt]]

(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!

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f79edc3888e5
Follow up fix for missing parameter. rs=kats

Comment 63

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 64

[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/862c7c3c6e14

Comment 65

[Ryan Hunt [:rhunt]]

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

Comment 66

[Ryan Hunt [:rhunt]]

Attachment: track-scroll-amount.patch

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

Comment 67

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

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.

Comment 68

[Pulsebot]

Pushed by [e-mail]:
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

Comment 69

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

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

Comment 70

[:Harald Kirschner :digitarald]

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.

Comment 71

[Julien Cristau [:jcristau]]

Comment on attachment 8821788 [details] [diff] [review]
track-scroll-amount.patch

add telemetry probe for vertical scrolling, beta52+

Comment 72

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d382faa1a2e2

Comment 73

[Olli Pettay [:smaug][[e-mail]]]

Please don't add JS implemented telemetry probes to (reasonable) hot code paths.

Comment 74

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

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

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

I missed also that scrollY is accessed, and that flushes layout, which can be super slow operation.
This should probably be backed out.

Comment 76

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

ni'ing rhunt for what is probably going to need to be a backout of this patch from Nightly to Beta.

Comment 77

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

Actually, clearing ni, as it looks like this is being handled in bug 1338891.

Comment 78

[Ryan Hunt [:rhunt]]

Clearing the ni, as it is being handled in bug 1338891 :)