1214170 - APZ support for prefs that modify mousewheel behaviour

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Ernst Sjostrand]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151012030612

Steps to reproduce:

Using Firefox nightly with GTK3 on Linux (Ubuntu 15.10), I notice that mousewheel acceleration no longer works. (mousewheel.acceleration.*)
I'm guessing this is because of the GTK3 switch.

Comment 1

[AnAkkk]

Probably related to https://bugzilla.mozilla.org/show_bug.cgi?id=1207700

Comment 2

[Ernst Sjostrand]

> I doubt it.  Bug 1143856 seems more likely.
> If toggling layers.async-pan-zoom.enabled resolves the issue, then file a new bug blocking Bug 1143856
> rather than commenting in that bug.

That was indeed correct, toggling layers.async-pan-zoom.enabled did resolve it.

Comment 3

[Botond Ballo [:botond]]

Indeed, it looks like the mousewheel.acceleration.* prefs are checked and followed by the main-thread scrolling code [1], but not by the APZ code.

[1] https://dxr.mozilla.org/mozilla-central/rev/ccf288f658211b6cfab33c458aaf033baed2375b/dom/events/WheelHandlingHelper.cpp#315

Comment 4

[Ernst Sjostrand]

What about other ways to modify the scroll speed, like mousewheel.default.delta_multiplier_x and  mousewheel.withnokey.numlines ?

Comment 5

[Ernst Sjostrand]

There's a whole page dedicated to this on the wiki: https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling

Comment 6

[Botond Ballo [:botond]]

Modified title to expand scope as per comment 4.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, mousewheel.acceleration support

This patch implements mousewheel.acceleration pref support in APZ. The "sScrollSeriesCounter" in WheelHandlingHelper.cpp has been replicated in WheelBlockState, and similarly starts at 0 and resets if 80ms elapse in between two events in the transaction.

This counter gets affixed to ScrollWheelInput events, so APZC::GetScrollWheelDelta() can do the acceleration.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

This might be all we need. The other multiplier prefs are blacklisted for APZ, so changing them will go through main-thread scrolling, and they should function fine there.

kats, do we want to keep doing that, or should we take this bug to add full support for those as well?

Comment 9

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

Comment on attachment 8691065 [details] [diff] [review]
part 1, mousewheel.acceleration support

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

r=me with comments addressed

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +10,5 @@
>  #include "mozilla/MouseEvents.h"
>  #include "mozilla/SizePrintfMacros.h"       // for PRIuSIZE
>  #include "mozilla/layers/APZCTreeManager.h" // for AllowedTouchBehavior
>  #include "OverscrollHandoffState.h"
> +#include "AsyncScrollBase.h"                // for kScrollSeriesTimeoutMs

Move this up to alphabetical order

::: gfx/layers/apz/src/InputBlockState.h
@@ +248,5 @@
> +
> +  /**
> +   * Adjust a scroll delta for custom acceleration preferences.
> +   */
> +  double AccelerateScrollAmount(double delta) const;

This function signature seems left over, it doesn't have an implementation and isn't called.

::: gfx/thebes/gfxPrefs.h
@@ +384,5 @@
>    // This and code dependent on it should be removed once containerless scrolling looks stable.
>    DECL_GFX_PREF(Once, "layout.scroll.root-frame-containers",   LayoutUseContainersForRootFrames, bool, true);
>  
>    // This affects whether events will be routed through APZ or not.
> +  DECL_GFX_PREF(Live, "mousewheel.acceleration.factor",        MouseWheelAccelerationFactor, int32_t, -1);

The comment above this line only belongs on the mousewheel.system_scroll_override_on_root_content.enabled pref. Move the two new prefs up into a separate block separated by new lines

::: layout/generic/AsyncScrollBase.h
@@ +90,5 @@
> +// Helper for accelerated wheel deltas.
> +static inline double
> +ComputeAcceleratedWheelDelta(double aDelta, int32_t aCounter, int32_t aFactor)
> +{
> +  if (!aDelta) {

Add a comment here that says "this may be called from threads other than the main thread" just in case somebody decides to change it in the future to rely on main thread objects

Comment 10

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

(In reply to David Anderson [:dvander] from comment #8)
> This might be all we need. The other multiplier prefs are blacklisted for
> APZ, so changing them will go through main-thread scrolling, and they should
> function fine there.
> 
> kats, do we want to keep doing that, or should we take this bug to add full
> support for those as well?

Depends on how complicated the implementation would be, I guess. For now we need to leave the main-thread implementation in place so if it would add a lot of complexity to reimplement it in APZ then I say let's wait. If/when it gets to the point that we can remove the main-thread implementation and move the complexity into APZ we can reconsider it.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Okay. It seems pretty obscure so I'll defer it for now, and add some telemetry to see how prevalent it is.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: telemetry for mousewheel prefs

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb6fb453cca

Comment 14

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/772ebbbb852446775bb64fda8c5542daa381633a
Backed out changeset ebb6fb453cca (bug 1214170) for build bustage on a CLOSED TREE

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8691807 [details] [diff] [review]
telemetry for mousewheel prefs

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

This will collect data from all user populations, do we really need data from release users here?
Does it need to be in the environment data or could it be submitted in a histogram instead?
What questions are you trying to answer with this data & do we really need all the prefs values to answer them?
Who will monitor this data and how?

The changes look simple enough, but this needs review from a data peer, ideally with info for the above question provided:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8691807 [details] [diff] [review]
telemetry for mousewheel prefs

I just want to know how often these prefs are changed by the user. It doesn't need to ride the train very far, I'd be happy to get data in aurora and back it out after a few weeks.

Comment 17

[Georg Fritzsche [:gfritzsche]]

(In reply to David Anderson [:dvander] from comment #16)
> Comment on attachment 8691807 [details] [diff] [review]
> telemetry for mousewheel prefs
> 
> I just want to know how often these prefs are changed by the user. It
> doesn't need to ride the train very far, I'd be happy to get data in aurora
> and back it out after a few weeks.

That sounds like it doesn't need to be in the environment - could you just add a single flag histogram instead that tells you whether any of those prefs were changed/non-default by the user?
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

That way you will also get it displayed automatically on the telemetry.mozilla.org dashboards, although it will give you changes "per session", not per individual user.

Comment 18

[Vladan Djeric (:vladan)]

Comment on attachment 8691807 [details] [diff] [review]
telemetry for mousewheel prefs

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

The environment block is supposed to consist of configurations & characterstics that significantly affect performance and other behavior. When Telemetry detects that one of these prefs has changed during a session, it immedially starts a new Telemetry subsession. In general, we try keep the list of prefs to a minimum.

I think histograms might indeed be better for these measurements. They'll be easier to visualize via dashes too.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, delta multiplier support

Actually I think this is super easy, so we might as well just support it. We just need to communicate the new multiplier to APZ, and we can just compute it before handing the event off on the main thread.

This patch also fixes an existing micro-bug where APZ applied the system delta override to pixel delta events (it should only apply to line events).

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, remove "bad prefs" warning

Don't need this anymore, there are no more prefs that disable APZ.

Comment 21

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

Comment on attachment 8693413 [details] [diff] [review]
part 2, delta multiplier support

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

LGTM, did you check if this passes all tests?

::: widget/InputData.h
@@ +570,5 @@
>                     Modifiers aModifiers,
>                     ScrollMode aScrollMode,
>                     ScrollDeltaType aDeltaType,
>                     const ScreenPoint& aOrigin,
> +                   double aDeltaX, double aDeltaY)

Undo this change, leave the params as one-per-line

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

I didn't see any new failures on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eac8bc7bf2b so attempting to re-land this today.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fffdbaaac8a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/81783ea82024
https://hg.mozilla.org/integration/mozilla-inbound/rev/608d89fa125b

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fffdbaaac8a1
https://hg.mozilla.org/mozilla-central/rev/81783ea82024
https://hg.mozilla.org/mozilla-central/rev/608d89fa125b