Closed Bug 1214170 Opened 4 years ago Closed 4 years ago

APZ support for prefs that modify mousewheel behaviour

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: ernstp, Assigned: dvander)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
OS: Unspecified → Linux
Blocks: gtk3
> 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.
Blocks: apz-linux
No longer blocks: gtk3
Summary: Mousewheel acceleration not working with GTK3 Firefox → Mousewheel acceleration not working with Firefox nightly on Linux
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
OS: Linux → All
Hardware: Unspecified → All
Summary: Mousewheel acceleration not working with Firefox nightly on Linux → Support mousewheel acceleration prefs with APZ
What about other ways to modify the scroll speed, like mousewheel.default.delta_multiplier_x and  mousewheel.withnokey.numlines ?
There's a whole page dedicated to this on the wiki: https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Version: 44 Branch → Trunk
Modified title to expand scope as per comment 4.
Summary: Support mousewheel acceleration prefs with APZ → APZ support for prefs that modify mousewheel behaviour
Assignee: nobody → dvander
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8691065 - Flags: review?(bugmail.mozilla)
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 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
Attachment #8691065 - Flags: review?(bugmail.mozilla) → review+
(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.
Okay. It seems pretty obscure so I'll defer it for now, and add some telemetry to see how prevalent it is.
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
Attachment #8691807 - Flags: review?(gfritzsche)
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.
Attachment #8691807 - Flags: review?(vladan.bugzilla)
(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 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.
Attachment #8691807 - Flags: review?(vladan.bugzilla)
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).
Attachment #8691807 - Attachment is obsolete: true
Attachment #8693413 - Flags: review?(bugmail.mozilla)
Don't need this anymore, there are no more prefs that disable APZ.
Attachment #8693414 - Flags: review?(bugmail.mozilla)
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
Attachment #8693413 - Flags: review?(bugmail.mozilla) → review+
Attachment #8693414 - Flags: review?(bugmail.mozilla) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/fffdbaaac8a1
https://hg.mozilla.org/mozilla-central/rev/81783ea82024
https://hg.mozilla.org/mozilla-central/rev/608d89fa125b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Duplicate of this bug: 1171398
You need to log in before you can comment on or make changes to this bug.