Closed Bug 1238137 Opened 9 years ago Closed 8 years ago

Add telemetry for which input methods are used for scrolling

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Platform:
Unspecified
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: kats, Assigned: botond)

References

Details

(Keywords: feature)

Attachments

(7 files)

MozReview Request: Bug 1238137 - Define a telemetry histogram for tracking the input methods used to trigger scrolling. r=kats
(deleted), text/x-review-board-request
kats
: review+
benjamin
: review+
Details
MozReview Request: Bug 1238137 - Telemetry pings for APZ-driven scroll input methods. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1238137 - Telemetry pings for main thread keyboard-driven scroll input methods. r=ehsan
(deleted), text/x-review-board-request
ehsan.akhgari
: review+
Details
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=ehsan
(deleted), text/x-review-board-request
ehsan.akhgari
: review+
Details
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrollbar-driven scroll input methods. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1238137 - Telemetry pings for main thread autoscrolling. r=mconley
(deleted), text/x-review-board-request
mconley
: review+
Details
MozReview Request: Bug 1238137 - Telemetry pings for main thread touch scrolling (Windows only). r=kats
(deleted), text/x-review-board-request
kats
: review+
Details 
      No description provided.
Before we start adding support for other input methods on desktop (keyboard, touch, scrollbar dragging, scrollbar buttons, autoscroll, others?) we should figure out how much each one is used so that we can prioritize in order of importance.
When doing this we should also make sure to distinguish wheel scrolls that scroll by pixels, lines, and pages. As per the discussion in bug 1238502, page scrolling by wheel input results in more checkerboarding, so we should find out if it's a common scenario.
I'm going to have a look at this.
Assignee: nobody → botond
As a first step, we should decide more precisely what data we would like to collect.

I can think of a few options:

  (1) For each input method, what fraction of all input events that cause scrolling
      used this input method?

  (2) The above, grouped by _page_, so we can get some idea of what fractions of 
      pages fall into buckets like "scrolled mostly by wheel", "scrolled mostly by 
      keyboard", "scrolled using a mixture of input types", etc.

  (3) The above, grouped by _user_, so we can get some idea of what fractions of 
      users fall into buckets like "scroll mostly with wheel", "scroll mostly with
      keyboard", etc.

  (4) Something else entirely.

Kats, which of these, if any, did you have in mind? I was thinking of starting with (1), and perhaps exploring (3), but I'm open to input.
Flags: needinfo?(bugmail.mozilla)
(1) seems like something we should get regardless of whether we get (2) or (3) as well, so I agree - we should start with (1) and then think about other things. In terms of what we're planning to use this for, we might have:

(4) The above, grouped by jank, so we can get some idea of which input methods result in the most user-visible jank, and would benefit the most from APZ.

But that's more complex to measure so we should definitely start with (1). CCing Avi as well as he might have thoughts on this.
Flags: needinfo?(bugmail.mozilla)
(In reply to Botond Ballo [:botond] from comment #4)
>   (1) For each input method, what fraction of all input events that cause
> scrolling
>       used this input method?

Collecting fractions is hard for telemetry (since at the time of collection we don't know what 100% is). I think collecting overall traveled pixels per input event type might be easier to collect and would also provide better correlation to what we're ultimately after -  the not well defined "amount of use".
(In reply to Avi Halachmi (:avih) from comment #6)
> (In reply to Botond Ballo [:botond] from comment #4)
> >   (1) For each input method, what fraction of all input events that cause
> > scrolling
> >       used this input method?
> 
> Collecting fractions is hard for telemetry (since at the time of collection
> we don't know what 100% is).

I'm not very experienced with Telemetry, but it seems to me that having a histogram of type 'enumerated', with a different enum value for each input type, should give us the data we want.

That said...

> I think collecting overall traveled pixels per
> input event type might be easier to collect and would also provide better
> correlation to what we're ultimately after -  the not well defined "amount
> of use".

... I can definitely see this being useful as well. Thanks for suggesting it!
Update on the progress here:

   - I plan to, as a first milestone, implement idea (1) from comment 4 by
     using an 'enumerated' histogram with a different enum value representing
     each input type, and accumulating an entry in the relevant bucket
     whenever an input event of that type is generated.

   - For input events handled by APZ, I will do said accumulation as the
     event is handled by APZ.

   - For input events handled by the main thread, I will do said accumulation
     as the event is handled by the main thread. I'm currently surveying
     the relevant main thread code to see where exactly these probes should
     be added.

   - Note that this will mean that for scrolling in the content process,
     the APZ-driven data will accumulate in the parent process, while the
     {main thread}-driven data will accumulate in the child process. I believe
     this is fine, as the Telemetry backend will aggregate the two datasets 
     into a single one. (It also gives us the ability to view the datasets
     independently; in this case, that just wouldn't be very meaningful.)
As mentioned in the discussion today, the plan above sounds fine.

Re bug 1180706 comment 4: turns out touch scrolling does work in Windows without APZ, so we should make sure to have separate probes for APZ touch scrolling and main-thread touch scrolling (which would probably go in nsWinGesture.cpp somewhere).
Having investigated (most of) the relevant main thread scrolling code in some detail, I'm going to summarize my findings, and my approach for adding the probes:

  - Keyboard-driven scrolling

      - This goes through the interface nsISelectionController.

        This interface has methods that do scrolling:
          ScrollLine       (roughly, "scroll up or down by a line")
          ScrollCharacter  (roughly, "scroll left or right by a character")
          ScrollPage       (roughly, "scroll up or down by a page")
          CompleteScroll   (roughly, "scroll up or down to the end of the scroll range")

        methods that move the caret by a logical amount:
          LineMove
          CharacterMove
          WordMove
          PageMove
          CompleteMove
          IntraLineMove
        (these methods are also used if a caret selection is active,
        in which case the methods can extend the selection)

        and a method that moves the caret in a physical direction:
          PhysicalMove

        Note that the methods that move the caret can also trigger scrolling
        if the new position of the caret needs to be scrolled into view.

      - This interface has two implementations: nsPresShell and 
        nsTextInputSelectionImpl. The latter is used for elements that are
        editable, such as text boxes and content-editable divs; the former
        is used for all other scroll frames.

        The implementations are very similar. The scrolling methods
        first find a target scroll frame (in the case of nsPresShell, it
        finds the scroll frame enclosing the focused element; the editor one
        is already associated with a single scroll frame), and then scroll
        it via the nsIScrollableFrame API. The caret-move methods generally
        call nsSelection::MoveCaret, which moves the caret and then calls
        ScrollIntoView to scroll the new position of the caret into view
        if necessary.

        Note that caret-move operations are meaningful for non-editable
        scroll frames, too, due to the "caret browsing" feature.

      - The nsISelectionController methods are generally triggered by
        commands such as "cmd_moveUp", "cmd_scrollLineDown", etc., with
        bindings defined in nsGlobalWindowCommands.cpp and 
        nsEditorCommands.cpp; the latter defines the associations used for
        editable elements, and the former defines the associations used
        elsewhere.

        The mapping from the commands to the nsISelectionController methods
        is generally one-to-one for scroll commands (like "cmd_scrollLineUp")
        and logical move commands (like "cmd_lineNext"). It gets a bit more
        interesting for physical move commands (like "cmd_moveUp"):

          - In an editable context, these call PhysicalMove.

          - In a non-editable context:

              - If caret browsing is disabled, they call a scroll method
                like ScrollLine.

              - If caret browsing is enabled, they call PhysicalMove,
                falling back to the scroll method if the caret wasn't
                able to move.

      - The triggers for the "cmd_XXX" commands are generally defined by
        the XBL and widget layers. The triggers are all keyboard-based;
        the only exception I could find is that cmd_scrollTop/Bottom
        (which is associated with CompleteScroll) can be triggered by a 
        "MozSwipeGesture" event processed in browser-gestureSupport.js.

      - Based on the above, I would suggest adding the telemetry probes to
        the implementations of the scrolling methods (ScrollLine etc.), and 
        to the places where the caret-move methods call ScrollIntoView 
        and that results in actual scrolling.

        Some implications of this choice:

          - We may get a small number of false positives, such as for
            MozSwipeGestures triggering cmd_scrollTop/Bottom, and
            hypothetically for other non-keyboard triggers that I may 
            have missed or that may be added in the future.

            If we wanted to weed out these false positives, we could
            do so by putting an RAII class onto the stack in a place
            like nsIWidget::DispatchEvent(WidgetKeyboardEvent&), and
            restrict the data collection to the periods during which
            such a class is present on the stack. This could however
            introduce false *negatives* if in some places the call
            to the scroll / caret-move method is not in the same
            event loop iteration as the DispatchEvent() call. This is
            not the case when a key binding is connected directly to
            the command of interest via XBL, but there is also some
            JS code that invokes commands (such as Finder.jsm, forms.js, 
            Keyboard.jsm) for which this may be the case (I haven't
            done an exhaustive survey).
            
            I think for now it's sufficient to record unconditionally
            in the scroll / caret-move methods. We can revisit this
            choice later if needed.

          - We may also get false positives if an add-on invokes an
            nsISelectionController method or a cmd_XXX command as a 
            result of something other than a keyboard trigger. Note
            however that an add-on could also generate key events
            without an actual keyboard trigger, so our telemetry
            logic can't be 100% accurate anyways.

  - Scrollbar dragging is implemented in nsSliderFrame::HandleEvent which
    calls nsIScrollableFrame::ScrollTo via nsIScrollbarMediator::ThumbMoved.
    Collecting telemetry should be a simple matter of adding a probe to the
    relevant part of nsSliderFrame::HandleEvent.

  - Autoscrolling is implemented in JS (autoscrollLoop function in 
    browser-content.js). Collecting telemetry should be a simple matter of 
    adding a probe in that function. (Note that telemetry probes can be
    in both JS and C++ code.)

  - Scrollbar button clicking remains to be investigated. (In my default setup
    (GTK 3) scrollbar tracks do not have buttons at the end.)

  - Main-thread touch scrolling (which Kats pointed out in comment 9 exists
    on Windows) remains to be investigated.
Posting what I have so far; this is a work in progress.
(In reply to Botond Ballo [:botond] from comment #10)
>   - Scrollbar button clicking remains to be investigated. (In my default setup
>     (GTK 3) scrollbar tracks do not have buttons at the end.)

I was able to get Firefox under GTK3 to show scrollbar buttons using the technique described at [1].

The scrolling in response to clicking the button happens in nsScrollbarButtonFrame::HandleButtonPress.

[1] http://unix.stackexchange.com/a/247758
Attachment #8715604 - Flags: review?(bugmail.mozilla)
Attachment #8715605 - Flags: review?(bugmail.mozilla)
Attachment #8715606 - Flags: review?(bugmail.mozilla)
Attachment #8715607 - Flags: review?(bugmail.mozilla)
Posted patches for review. A few notes:

  - I haven't tested the Windows main thread touch gesture pings, just made an
    educated guess as to where it needs to be by code reading, but I will test
    it before landing.

  - For the JS call site, I just redeclared the value of the relevant constant
    instead of declaring it in some way such that a single declaration of the
    value will make it available in both C++ and JS; the work required to do
    the latter just didn't seem worth it given that there's only one JS call
    site. Let me know if you disagree with this.
Comment on attachment 8715604 [details]
MozReview Request: Bug 1238137 - Define a telemetry histogram for tracking the input methods used to trigger scrolling. r=kats

Flagging :ally for data collection review.

I'm only flagging the first patch, but the review request applies to all the patches together. (They're pretty short :))

The tl;dr for the change is "we'd like to understand what input methods (mouse wheel, keyboard, touch, etc.) people use to scroll things, and we're going to do it by collecting telemetry from the pre-release population".
Attachment #8715604 - Flags: feedback?(a.m.naaktgeboren)
Comment on attachment 8715604 [details]
MozReview Request: Bug 1238137 - Define a telemetry histogram for tracking the input methods used to trigger scrolling. r=kats

https://reviewboard.mozilla.org/r/33567/#review30537

::: gfx/layers/apz/util/ScrollInputMethods.h:24
(Diff revision 1)
> +  ApzPanGesture,     // pan gesture events (generally triggered by trackpad)

Add a ApzScrollbarDrag here as well (note that the autoscrolling one will get bumped to 14, so you'll have to update the JS). Add the corresponding telemetry accumulate call in AsyncPanZoomController::HandleDragEvent. It's not actually hooked up yet but it will be at some point and I'd rather not forget about it.
Attachment #8715604 - Flags: review?(bugmail.mozilla) → review+
Attachment #8715605 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8715605 [details]
MozReview Request: Bug 1238137 - Telemetry pings for APZ-driven scroll input methods. r=kats

https://reviewboard.mozilla.org/r/33569/#review30539

LGTM. As mentioned in the previous patch, add one for ApzScrollbarDrag as well.
Comment on attachment 8715606 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread keyboard-driven scroll input methods. r=ehsan

https://reviewboard.mozilla.org/r/33571/#review30541

Looks ok to me, but somebody who knows this code should review. Maybe masayuki? Or ehsan?
Attachment #8715606 - Flags: review?(bugmail.mozilla)
Attachment #8715607 - Flags: review?(bugmail.mozilla)
Comment on attachment 8715607 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=ehsan

https://reviewboard.mozilla.org/r/33573/#review30543

Ditto - LGTM but somebody else should review this.
Comment on attachment 8716502 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrollbar-driven scroll input methods. r=kats

https://reviewboard.mozilla.org/r/33851/#review30545

::: layout/xul/nsScrollbarButtonFrame.cpp:142
(Diff revision 1)
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SCROLL_INPUT_METHODS,

I would move this one down inside the if (sb) block, after the switch (pressedButtonAction). That way the "case 3:" return false condition is excluded from the telemetry count, as is the case where sb is null.

Also, looking at the code, it looks like this covers not just clicking on the actual scrollbar button, but also clicking on the scrollbar track above or below the thumb. You may want to rename the enum value or update the comment accordingly.
Attachment #8716502 - Flags: review?(bugmail.mozilla)
Comment on attachment 8716503 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread autoscrolling. r=mconley

https://reviewboard.mozilla.org/r/33853/#review30547

Also LGTM but somebody who owns this file should review.
Attachment #8716503 - Flags: review?(bugmail.mozilla)
Comment on attachment 8716504 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread touch scrolling (Windows only). r=kats

https://reviewboard.mozilla.org/r/33855/#review30549
Attachment #8716504 - Flags: review?(bugmail.mozilla) → review+
I still have concerns that collecting the number of events could end up with an unknown quantity of bias towards events which tend to induce shorter distance per event (in larger-to-lower bias order: trackpad/touch, line scrolls, page scrolls, autoscroll?).

How do we correlate event type to average-traveled-distance per event? or average-scroll-duration per event?

I'd imagine one of the above, or a combination of those, would correlate best to what this bug is about: measuring "how much each one is used", and I fear that counting only events might give us a skewed picture.
(In reply to Avi Halachmi (:avih) from comment #30)
> I still have concerns that collecting the number of events could end up with
> an unknown quantity of bias towards events which tend to induce shorter
> distance per event (in larger-to-lower bias order: trackpad/touch, line
> scrolls, page scrolls, autoscroll?).
> 
> How do we correlate event type to average-traveled-distance per event? or
> average-scroll-duration per event?

I plan to explore collecting "total pixels scrolled" for each input type as a follow-up. We can then derived "average" figures by dividing the "total pixels scrolled" by the "number of events".
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27)
> Also, looking at the code, it looks like this covers not just clicking on
> the actual scrollbar button, but also clicking on the scrollbar track above
> or below the thumb. You may want to rename the enum value or update the
> comment accordingly.

What makes you believe nsScrollbarButtonFrame::HandleButtonPress() handles clicking on the scrollbar track above or below the thumb? Local testing shows that that goes through nsSliderFrame::HandleEvent() (a different code path than dragging [1]). 

(Also, intuitively, I would expect a frame named "nsScrollbarButtonFrame" to correspond to the scrollbar's button only, not to other parts of the track.)

That said, it's a fair observation that scrolling caused by clicking on the track isn't covered by any of the current input methods. I'll add an input method for that.

[1] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/layout/xul/nsSliderFrame.cpp#585
Ah, it was the code below that made me think that, the bit where it calls m->ScrollByPage. But I guess in some cases clicking on the button can cause scrolling by page amounts?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33)
> Ah, it was the code below that made me think that, the bit where it calls
> m->ScrollByPage. But I guess in some cases clicking on the button can cause
> scrolling by page amounts?

Yeah. In particular, middle-clicking the button is often mapped to scrolling by a page, and right-clicking to scrolling to the end. I didn't distinguish between those in the telemtry, but I could if you'd like.
Comment on attachment 8715604 [details]
MozReview Request: Bug 1238137 - Define a telemetry histogram for tracking the input methods used to trigger scrolling. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33567/diff/1-2/
Attachment #8715604 - Flags: feedback?(a.m.naaktgeboren)
Comment on attachment 8715605 [details]
MozReview Request: Bug 1238137 - Telemetry pings for APZ-driven scroll input methods. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33569/diff/1-2/
Attachment #8715606 - Attachment description: MozReview Request: Bug 1238137 - Telemetry pings for main thread keyboard-driven scroll input methods. r=kats → MozReview Request: Bug 1238137 - Telemetry pings for main thread keyboard-driven scroll input methods. r=ehsan
Attachment #8715606 - Flags: review?(ehsan)
Comment on attachment 8715606 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread keyboard-driven scroll input methods. r=ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33571/diff/1-2/
Comment on attachment 8715607 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33573/diff/1-2/
Attachment #8715607 - Attachment description: MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=kats → MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=ehsan
Attachment #8715607 - Flags: review?(ehsan)
Attachment #8716502 - Flags: review?(bugmail.mozilla)
Comment on attachment 8716502 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrollbar-driven scroll input methods. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33851/diff/1-2/
Comment on attachment 8716503 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread autoscrolling. r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33853/diff/1-2/
Attachment #8716503 - Attachment description: MozReview Request: Bug 1238137 - Telemetry pings for main thread autoscrolling. r=kats → MozReview Request: Bug 1238137 - Telemetry pings for main thread autoscrolling. r=mconley
Attachment #8716503 - Flags: review?(mconley)
Comment on attachment 8716504 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread touch scrolling (Windows only). r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33855/diff/1-2/
Addressed review comments and chose new reviewers where requested.
Comment on attachment 8715604 [details]
MozReview Request: Bug 1238137 - Define a telemetry histogram for tracking the input methods used to trigger scrolling. r=kats

Restoring flag for data collection review request (originally set in comment 22). MozReview isn't good about maintaining those.
Attachment #8715604 - Flags: feedback?(a.m.naaktgeboren)
Comment on attachment 8715606 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread keyboard-driven scroll input methods. r=ehsan

https://reviewboard.mozilla.org/r/33571/#review30695

This could use a better commit message.  :-)
Attachment #8715606 - Flags: review?(ehsan) → review+
Attachment #8715607 - Flags: review?(ehsan)
Comment on attachment 8715607 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=ehsan

https://reviewboard.mozilla.org/r/33573/#review30699

I don't like adding a bool argument and piping it all the way as you're doing here.  It should be much cleaner to add a SCROLL_FOR_CARET_MOVE flag here <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsISelectionController.idl#65> and use it instead.

This is r-, I refuse to be bound to the MozReview limits.  ;-)
Comment on attachment 8716502 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrollbar-driven scroll input methods. r=kats

https://reviewboard.mozilla.org/r/33851/#review30705
Attachment #8716502 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8715606 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread keyboard-driven scroll input methods. r=ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33571/diff/2-3/
Attachment #8715607 - Flags: review?(ehsan)
Comment on attachment 8715607 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33573/diff/2-3/
Comment on attachment 8716502 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrollbar-driven scroll input methods. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33851/diff/2-3/
Comment on attachment 8716503 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread autoscrolling. r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33853/diff/2-3/
Comment on attachment 8716504 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread touch scrolling (Windows only). r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33855/diff/2-3/
(In reply to :Ehsan Akhgari (Away 2/10-2/19) from comment #44)
> Comment on attachment 8715606 [details]
> MozReview Request: Bug 1238137 - Telemetry pings for main thread
> keyboard-driven scroll input methods. r=ehsan
> 
> https://reviewboard.mozilla.org/r/33571/#review30695
> 
> This could use a better commit message.  :-)

I added some more detail to the commit message. Let me know if this is better, or if I misunderstood in what respect it's lacking.

(In reply to :Ehsan Akhgari (Away 2/10-2/19) from comment #45)
> I don't like adding a bool argument and piping it all the way as you're
> doing here.  It should be much cleaner to add a SCROLL_FOR_CARET_MOVE flag
> here
> <https://dxr.mozilla.org/mozilla-central/source/dom/base/
> nsISelectionController.idl#65> and use it instead.

Done. Thanks for the suggestion! (I considered doing this initially, but I wasn't sure if changing an IDL interface for a telemetry-only purpose would be acceptable.)
Comment on attachment 8716503 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread autoscrolling. r=mconley

https://reviewboard.mozilla.org/r/33853/#review30749

LGTM - thanks!
Attachment #8716503 - Flags: review?(mconley) → review+
Comment on attachment 8715607 [details]
MozReview Request: Bug 1238137 - Telemetry pings for main thread scrolling to bring the caret into view after moving it in response to keyboard input. r=ehsan

https://reviewboard.mozilla.org/r/33573/#review30905
Attachment #8715607 - Flags: review?(ehsan) → review+
(In reply to Botond Ballo [:botond] from comment #20)
>   - I haven't tested the Windows main thread touch gesture pings, just made an
>     educated guess as to where it needs to be by code reading, but I will test
>     it before landing.

Confirmed this with a Windows build.
This is ready to land pending the data collection review.
Not sure if ally is checking for f? flags but her departure email in November said to use needinfo, so let's try both :)
Flags: needinfo?(a.m.naaktgeboren)
Blocks: 1247454
Blocks: 1247456
I plan to close this bug after the posted patches land, and explore additional telemetry collection in follow-ups.

I filed two such follow-ups:

  Distance scrolled: bug 1247454
  Jank:              bug 1247456
Comment on attachment 8715604 [details]
MozReview Request: Bug 1238137 - Define a telemetry histogram for tracking the input methods used to trigger scrolling. r=kats

https://reviewboard.mozilla.org/r/33567/#review31655

::: toolkit/components/telemetry/Histograms.json:9921
(Diff revision 2)
> +    "description": "Count of scroll actions triggered by different input methods."

Could you expand on this description? Please either list the possible values in this field, or reference the ScrollInputMethod enumeration so that people looking at this histogram on telemetry.mozilla.org and future auto-generated docs know what it means?

data-review=me with that change
Attachment #8715604 - Flags: review+
Flags: needinfo?(a.m.naaktgeboren)
Attachment #8715604 - Flags: feedback?(a.m.naaktgeboren)
Thanks for the data review! Made the requested change and landed.
Blocks: 1284348
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: