Closed Bug 1153156 Opened 5 years ago Closed 4 years ago

[e10s] Mousewheel scroll distance does not match non-e10s (with apz disabled)

Categories

(Core :: Widget: Win32, defect)

All
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
e10s + ---
firefox45 --- affected
firefox46 --- verified
firefox47 --- verified

People

(Reporter: ke5trel, Assigned: masayuki)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted][STR in comment 35])

Attachments

(9 files, 6 obsolete files)

51.15 KB, image/jpeg
Details
15.23 KB, patch
Details | Diff | Splinter Review
15.74 KB, patch
Details | Diff | Splinter Review
9.99 KB, patch
Details | Diff | Splinter Review
14.40 KB, patch
Details | Diff | Splinter Review
15.11 KB, patch
Details | Diff | Splinter Review
15.68 KB, patch
Details | Diff | Splinter Review
9.99 KB, patch
Details | Diff | Splinter Review
12.85 KB, patch
Details | Diff | Splinter Review
Steps:

1. Set mousewheel.default.delta_multiplier_y to 99, 100 (default) and 101. 
2. For each value, scroll any long website vertically with the mousewheel one step and note the amount scrolled.

Actual result:

With a multiplier of 100 the scrolling step size is much larger than for other multiplier values and deviates significantly from what it should be.

Expected result:

The scrolling step size should be proportional to the multiplier and consistent with system settings.
Sounds like you have the layers.async-pan-zoom.enabled pref set. Cc'ing dvander.
This is with a fresh profile which has layers.async-pan-zoom.enabled set to false. I tried toggling it on/off and restarting and there was no difference in the mousewheel step size, the above described behavior is the same.
Is this a recent regression? What version/build are you seeing this on?
This is on the latest Nightly (20150410030204). It is not a recent regression and has been this way for some time, possibly since the last scroll size bug (Bug 1136177) The latest Developer Edition is unaffected.

While looking through about:config I stumbled upon: 

mousewheel.system_scroll_override_on_root_content.vertical.factor = 200

When I change this to 100 it solves the problem, mousewheel.default.delta_multiplier_y behaves properly with a value of 100 and scrolling is the correct step size. When I make the same change in the Developer Edition it doesn't do anything.

"On Windows, only when the system scroll speed settings are not customized by user or mouse driver, this overrides the scrolling speed. " - https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling

I have customized the system scroll speed (12 lines instead of the default 3) so the multiplier should not apply. 

On Developer Edition the behavior is correct, the multiplier activates only when the system default scroll speed is used. On Nightly however it is the opposite, the multiplier is activating when a custom system scroll speed is used.
Summary: Mousewheel scrolling step size is inconsistently large for the default delta multiplier → Mousewheel scroll speed is being multiplied when using custom system scroll speed
If it's on nightly but not dev edition it sounds like a fairly recent regression. Can you use the mozregression tool to identify the changeset that caused this regression?

http://mozilla.github.io/mozregression/
i can repro since firefox17 as well as Firefox37.0.1
Component: Panning and Zooming → DOM: Events
Flags: needinfo?(masayuki)
(In reply to Kestrel from comment #4)
> mousewheel.system_scroll_override_on_root_content.vertical.factor = 200

Yes, must be related to this.

> When I change this to 100 it solves the problem,
> mousewheel.default.delta_multiplier_y behaves properly with a value of 100
> and scrolling is the correct step size. When I make the same change in the
> Developer Edition it doesn't do anything.
> 
> "On Windows, only when the system scroll speed settings are not customized
> by user or mouse driver, this overrides the scrolling speed. " -
> https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling
> 
> I have customized the system scroll speed (12 lines instead of the default
> 3) so the multiplier should not apply. 

Could you check if you don't have following hidden prefs:
mousewheel.windows.vertical_amount_override
mousewheel.windows.horizontal_amount_override

These prefs are used at testing WM_MOUSEWHEEL handler for emulating any environment's behavior. So, they should not be in your about:config.

If they are not in your prefs, could you check how much value Gecko gets from your system.

Set your environment variable as:

NSPR_LOG_FILE=c:\mousewheel.log
NSPR_LOG_MODULES=MouseScrollHandlerWidgets:1

Then, restart Firefox and turn wheel once, then, the log file should include the line starts with:

MouseScroll::SystemSettings::Init():

Could you post the lines into this bug?
Flags: needinfo?(masayuki) → needinfo?(kestrel)
I don't have those prefs and the log looks fine.

>0[575a50e800]: MouseScroll::SystemSettings::Init(): initialized, mScrollLines=12, mScrollChars=3
>0[575a50e800]: MouseScroll::LastEventInfo::InitWheelEvent: aWidget=57693b0400, aWheelEvent { refPoint: { x: 352, y: 114 }, deltaX: 0.000000, deltaY: 12.000000, lineOrPageDeltaX: 0, lineOrPageDeltaY: 12, isShift: FALSE, isControl: FALSE, isAlt: FALSE, isMeta: FALSE }, mAccumulatedDelta: 0
>0[575a50e800]: MouseScroll::HandleMouseWheelMessage: dispatching NS_WHEEL_WHEEL event

I previously thought I had eliminated e10s as a cause but I was mistaken. 

Revised steps to reproduce:

1. Change Windows system vertical scrolling speed from the default of 3 to some other value like 12.
2. Scroll in a non-e10s window.
3. Scroll in an e10s window.

Actual results:

Scrolling in the e10s window is twice as fast as the non-e10s window due to the scroll override factor still applying (mousewheel.system_scroll_override_on_root_content.vertical.factor).

Expected:

The e10s window should respect custom system scroll settings like the non-e10s window. 

nsWindow::OverrideSystemMouseScrollSpeed is responsible for applying the override factor if the system scroll speed is the default value of 3. The nsBaseWidget version of this function always applies the override regardless of system settings. Perhaps the latter is being called instead of the former?
Flags: needinfo?(kestrel)
Summary: Mousewheel scroll speed is being multiplied when using custom system scroll speed → [e10s] Mousewheel scroll speed is not respecting custom system settings
Ah, I see. I think that in the content process, nsBaseWidget::OverrideSystemMouseScrollSpeed() is called instead of nsWindow::OverrideSystemMouseScrollSpeed() since widgets in the content process is PuppetWidget, not nsWindow.
Given recent comments, I'm uncertain if a regression range is still needed here to move this bug forward. Please re-add the regressionwindow-wanted keyword if a regression range is still needed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can reproduce this and this is the sort of fit and finish that causes people to drop Firefox.
Duplicate of this bug: 1108574
Component: DOM: Events → Widget: Win32
Flags: needinfo?(jmathies)
So since no-one's working on this, is setting mousewheel.default.delta_multiplier_y to 99 or 100 the best workaround for now?
Flags: needinfo?(jmathies)
Assignee: nobody → enndeakin
Don't think I can do this one.
Assignee: enndeakin → nobody
Kats, how does this relate to APZ and what needs to be done here
Flags: needinfo?(bugmail.mozilla)
It's not related to APZ; unrelated bug according to comments 2-6.
Flags: needinfo?(bugmail.mozilla)
The triage group largely doesn't think this matters for release, but we'll defer to product to make that call.
This is fairly awful and is a primary interaction for users. I stand by my comment 11.
Kev, do you know which prefs you've changed?

I think this regresses with apz since that code doesn't respect custom mousewheel prefs, and without apz, due to comment 9.

I think both sides would have to fix this to address the problem completely.
Flags: needinfo?(kbrosnan)
Actually, some of these prefs are supported via apz, for example - 

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#1472
Windows OS mouse wheel preferences as I recall. I'll check when I get home as that is where I use Windows the most.
I believe this is functioning as expected on Nightly x64 Windows 10 for me using the 2015-10-07 build. https://hg.mozilla.org/mozilla-central/rev/3edc8d4a1e198314f5d7ebd2967b85842beef602

Changing the windows mouse scroll wheel preference changed the scroll distance. I tested numbers lower than the default of 3, numbers above 3 and page at a time. All seemed to behave as expected. 

Kestrel would you test this on an up to date nightly?
Flags: needinfo?(kbrosnan) → needinfo?(kestrel)
(In reply to Kevin Brosnan [:kbrosnan] from comment #22)
> I believe this is functioning as expected on Nightly x64 Windows 10 for me
> using the 2015-10-07 build.
> https://hg.mozilla.org/mozilla-central/rev/
> 3edc8d4a1e198314f5d7ebd2967b85842beef602
> 
> Changing the windows mouse scroll wheel preference changed the scroll
> distance. I tested numbers lower than the default of 3, numbers above 3 and
> page at a time. All seemed to behave as expected. 
> 
> Kestrel would you test this on an up to date nightly?

This shouldn't be gone without explicitly fix. The cause of this bug is, there is no path to call nsWindow::OverrideSystemMouseScrollSpeed() from content process.

If we can add new IPC for calling it, we can fix this bug simply. However, I don't like this approach since causing additional IPC for each wheel event wastes a lot of CPU time. We should move the overriding code into XP level.
(In reply to Kevin Brosnan [:kbrosnan] from comment #22)
> I believe this is functioning as expected

Compare scrolling in Notepad with Nightly and you can see that Nightly is scrolling more than the system scroll size, this is expected when the system scroll size is default (3 lines) but not when it is customized. The user may want faster scrolling across applications but this results in much faster scrolling in Nightly to the point where it can be unusable.

The original behavior can be reproduced in Nightly by using a non-e10s window with APZ disabled.
Flags: needinfo?(kestrel)
Blocks: 1215208
This is a e10s bug in our mouse wheel handling on windows. It looks like it'd be pretty easy to fix too. I think we should add this into m8.
Assignee: nobody → jmathies
This appears to be fixed. I tested with apz on and off, in each case the scroll preferences related to this setting are read in via MouseScrollHandler::SystemSettings::Init().

https://dxr.mozilla.org/mozilla-central/source/widget/windows/WinMouseScrollHandler.cpp#878

These settings get refreshed if the control panel setting changes during a session too.

I do see a weird difference in scroll handling between non-e10s and e10s. Specifically, with apz disabled and non-e10s, we pass through WheelTransaction::OverrideSystemScrollSpeed during wheel scrolling. I worked up a patch to fix this but after testing it seems it's not needed.

I'll post my patch but I don't think we need to land it. Masayuki per your comment 23 you felt something needed to change, so any comments here? I'm at a loss as to what we need to fix here.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(masayuki)
Resolution: --- → WORKSFORME
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 8680666 [details] [diff] [review]
patch

Yeah, using #ifdef XP_WIN is one of the ways. If you don't like that, you can add a method to LookAndFeel.h and override it from nsLookAndFeel for Windows.
Flags: needinfo?(masayuki)
Still not working here with latest Nightly. The APZ case seems to be new since I first reported this bug but the e10s case is still there.

Scroll speed table with custom Windows scroll size (eg 12 lines):
                          APZ
                    on            off       
      e10s on       too fast      too fast
           off      too fast      normal (matches Windows scroll size)

IMO Bug 1215208 is a duplicate of this bug.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
APZ should be disabled in non-e10s windows, so I'm surprised there's a difference between the bottom two entries in your table. Did you restart the browser after changing the APZ pref?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> Did you restart the browser after changing the APZ pref?

Yes, I restarted the browser for the APZ pref to take effect and the scrolling smoothness reflected that. I retested just to be sure.

> APZ should be disabled in non-e10s windows

I created non-e10s windows through the menu which still seem to be affected by the APZ pref. When disabling e10s globally it does what you describe and scrolling speed is normal regardless of APZ setting.
I can see a difference in distance between e10s+no-apz and non-e10s. However apz appears to fix this, so I don't think this needs to block.
Summary: [e10s] Mousewheel scroll speed is not respecting custom system settings → [e10s] Mousewheel scroll distance does not match non-e10s (with apz disabled)
(In reply to Jim Mathies [:jimm] from comment #34)
> STR:
> 
> 1) find a long bugzilla bug page to test on
> 2) set vertical scrolling to 12 lines in the control panel mouse applet
> 3) disable apz, enable e10s, restart
> 4) fling scroll using a precision mouse wheel down the bugzilla page
> 
> note the distance you scroll, repeat to get a good average distance
> 
> 5) enable apz, enabled e10s, restart
> 6) repeat step 4
> 
> with e10s: scroll distance is about ~ 1.2 times that of non-e10s.

ARRGH! wrong str, here's the correct steps:

STR:

1) find a long bugzilla bug page to test on
2) set vertical scrolling to 12 lines in the control panel mouse applet
3) DISABLE apz, DISABLE e10s, restart
4) fling scroll using a precision mouse wheel down the bugzilla page

note the distance you scroll, repeat to get a good average distance

5) ENABLE apz, ENABLE e10s, restart
6) repeat step 4

with e10s: scroll distance is about ~ 1.2 times that of non-e10s.
Whiteboard: gfx-noted → [gfx-noted][STR in comment 35]
(In reply to Kestrel from comment #32)
> > APZ should be disabled in non-e10s windows
> 
> I created non-e10s windows through the menu which still seem to be affected
> by the APZ pref. When disabling e10s globally it does what you describe and
> scrolling speed is normal regardless of APZ setting.

Ah, my bad. I misremembered - apparently in bug 1162064 we did end up letting APZ work in non-e10s windows as long as the global e10s pref was enabled.
(In reply to Jim Mathies [:jimm] from comment #35)
> with e10s: scroll distance is about ~ 1.2 times that of non-e10s.

I get exactly two times (2x) the difference with APZ or e10s enabled. I tested with a single mousewheel step and window.scrollY to precisely measure the scroll distance.
Product wanted triage - This has user impact and should be fixed. Do we know the level of effort? that can help decide if we should include this for m8
Keywords: productwanted
(In reply to Kestrel from comment #37)
> (In reply to Jim Mathies [:jimm] from comment #35)
> > with e10s: scroll distance is about ~ 1.2 times that of non-e10s.
> 
> I get exactly two times (2x) the difference with APZ or e10s enabled. I
> tested with a single mousewheel step and window.scrollY to precisely measure
> the scroll distance.

With what setting? Also is there any chance you can post your test case to the bug? I was not seeing 2x with a global scroll lines setting of 12.
Duplicate of this bug: 1215208
Blocks: apz-desktop
Assignee: jmathies → nobody
(In reply to Jim Mathies [:jimm] from comment #33)
> I can see a difference in distance between e10s+no-apz and non-e10s. However
> apz appears to fix this, so I don't think this needs to block.

I'm sorry but on my computer I don't see any difference in distance between e10s+"layers.async-pan-zoom.enabled" set to True and e10s+"layers.async-pan-zoom.enabled" set to False.

For me it's the same. 
I hope I understood what you tried to explain...
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
I'm sorry but why anybody is working on this bug???

With apz and e10s (by default) and nightly 45, the scrolling is still too fast on my computer when I set vertical scrolling to 6 lines in the control panel. 
I must disable e10s in order to have a normal speed of scrolling (or set the vertical scrolling in the control panel to 3).
Thanks for answering! :)
Flags: needinfo?(jmathies)
e10s is coming in firefox 45 or 46 and it will be great to fix that bug. That's the only one I get :)
I'll take this bug. I fixed bug 1235686 which tells us current system settings override approach is wrong since ::SystemParametersInfo(SPI_GETWHEELSCROLLLINES) may return different value if it's hooked by touchpad utility and used asynchronously.

I'll rewrite the code. I'm thinking that we need to set system settings to new member of WidgetWheelEvent.
Assignee: nobody → masayuki
Status: REOPENED → ASSIGNED
Just for the record, I'm not sure how much we care about the e10s non-apz scenario, since it seems like they will ship together. APZ at least is riding 46, and is only enabled if e10s is enabled, so from 46 onwards you should never have e10s without APZ.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #45)
> Just for the record, I'm not sure how much we care about the e10s non-apz
> scenario, since it seems like they will ship together. APZ at least is
> riding 46, and is only enabled if e10s is enabled, so from 46 onwards you
> should never have e10s without APZ.

Can APZ be enabled even if HWA is disabled?

Anyway, APZ must have same bug related to bug 1235686. I strongly believe that we should record native scroll amount to WidgetWheelEvent and overriding code should be implemented in it.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #46)
>
> Can APZ be enabled even if HWA is disabled?
> 

Yes, provided that e10s doesn't get disabled because HWA is disabled.

> Anyway, APZ must have same bug related to bug 1235686. I strongly believe
> that we should record native scroll amount to WidgetWheelEvent and
> overriding code should be implemented in it.

ok. looking at the patches for bug 1235686 quickly it looks like they are all the windows widget code so they probably affected APZ as well. I'm not sure what code the fix for this bug will touch. If this bug affects APZ then by all means go ahead and fix it. I'm just saying that if it only affects e10s without apz that configuration will cease to exist soon.
Not sure about normal scrolling. But when I press alt it definitely doesn't match. I set the with alt multipliers to 300 and it starts fast but actually gets slower as I scroll. Slower than the default.
The strategy to fix this bug is, moving nsIWidget::OverrideSystemMouseScrollSpeed() into WidgetWheelEvent and set necessary information before dispatching WidgetWheelEvent. Then, any process (even if it's in too strong sandbox) can work exactly same as current chrome process's impelementation.

First of all, this patch moves nsBaseWidget::OverrideSystemMouseScrollSpeed() to WidgetWheelEvent and removes the method from nsIWidget.

For easier to review following patches, this patch just use #if 0 for nsWindow::OverrideSystemMouseScrollSpeed() of Windows.
Attachment #8680666 - Attachment is obsolete: true
Attachment #8710951 - Flags: superreview?(bugs)
Attachment #8710951 - Flags: review?(bugs)
This patch adds mAllowToOverrideSystemScrollSpeed to WidgetWheelEvent as a bool member. This is the simplest way to reimplement the function with WidgetWheelEvent. I don't like to add raw system settings to every WidgetWheelEvent and check it in XP level code.

This patch moves only first half of nsWindow::OverrideSystemMouseScrollSpeed() to MouseScrollHandler. Now, pref is checked by WidgetWheelEvent::OverriddenDelta(X|Y)(). Therefore, MouseScrollHandler::SystemSettings::IsOverridingSystemScrollSpeedAllowed() should check only if current system settings for both direction (vertical and horizontal) are default values (both are 3).

If you think this should be reviewed by jimm too, please request additional review.
Attachment #8710953 - Flags: review?(bugs)
This moves the second half of nsWindow::OverrideSystemMouseScrollSpeed(). This prevents too fast scroll by overriding the system settings. We decided that if multiplied scroll amount is larger than multiplied scroll amount of system default settings, i.e., big delta value comes, the raw delta value should be used instead of multiplied (overridden) delta value.

This patch perhaps should be reviewed by jimm too.
Attachment #8710955 - Flags: review?(bugs)
This patch makes the new implementation APZC-aware. I'll request to review this to mstange after smaug agrees with my approach.
[Tracking Requested - why for this release]:

This will fix odd mouse scroll speed in e10s mode even though APZ will be enabled at enabling e10s always. According to bug 1235686, current design has a problem. Current design is, checking if the system scroll speed settings are customized by expensive mouse's utility or not. If it's changed, we don't override system settings because such expensive mouse utility may have its own scroll speed acceleration. Otherwise, i.e., they are system default, the scroll amount is multiplied by 2 for making users feel Firefox faster. However, retrieving system settings *asynchronously* may cause retrieving different value because the API may be hooked by last used pointing device's utility. The patches will fix this issue with judging if the wheel event may be overridden before dispatch to content.
Comment on attachment 8710951 [details] [diff] [review]
part.1 Move nsBaseWidget::OverrideSystemMouseScrollSpeed() to WidgetWheelEvent

>+#if 0
> NS_IMETHODIMP
> nsWindow::OverrideSystemMouseScrollSpeed(double aOriginalDeltaX,
>                                          double aOriginalDeltaY,
>                                          double& aOverriddenDeltaX,
>                                          double& aOverriddenDeltaY)
I assume this method will be removed or reused in the following patches
Attachment #8710951 - Flags: superreview?(bugs)
Attachment #8710951 - Flags: superreview+
Attachment #8710951 - Flags: review?(bugs)
Attachment #8710951 - Flags: review+
Comment on attachment 8710953 [details] [diff] [review]
part.2 Make WidgetWheelEvent store if overriding system scroll speed is allowed and it shouldn't be allowed if scroll speed isn't system default settings on Windows

>+MouseScrollHandler::SystemSettings::IsOverridingSystemScrollSpeedAllowed()
>+{
>+  // The default vertical and horizontal scrolling speed is 3, this is defined
>+  // on the document of SystemParametersInfo in MSDN.
>+  const uint32_t kSystemDefaultScrollingSpeed = 3;
so we have this defined here

>@@ -3700,62 +3700,16 @@ nsWindow::OverrideSystemMouseScrollSpeed
>                                          double aOriginalDeltaY,
>                                          double& aOverriddenDeltaX,
>                                          double& aOverriddenDeltaY)
> {
>   // The default vertical and horizontal scrolling speed is 3, this is defined
>   // on the document of SystemParametersInfo in MSDN.
>   const uint32_t kSystemDefaultScrollingSpeed = 3;
and here. Could we please have it in just one place. Put some static variable to nsWindow?

>-  // Otherwise, we should check whether the user customized the system settings
>-  // or not.  If the user did it, we should respect the will.
>-  UINT systemSpeed;
>-  if (!::SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0, &systemSpeed, 0)) {
>-    return NS_ERROR_FAILURE;
>-  }
>-  // The default vertical scrolling speed is 3, this is defined on the document
>-  // of SystemParametersInfo in MSDN.
>-  if (systemSpeed != kSystemDefaultScrollingSpeed) {
>-    return NS_OK;
>-  }
>-
>-  // Only Vista and later, Windows has the system setting of horizontal
>-  // scrolling by the mouse wheel.
>-  if (IsVistaOrLater()) {
>-    if (!::SystemParametersInfo(SPI_GETWHEELSCROLLCHARS, 0, &systemSpeed, 0)) {
>-      return NS_ERROR_FAILURE;
>-    }
>-    // The default horizontal scrolling speed is 3, this is defined on the
>-    // document of SystemParametersInfo in MSDN.
>-    if (systemSpeed != kSystemDefaultScrollingSpeed) {
>-      return NS_OK;
>-    }
>-  }

So you're removing all these SystemParametersInfo calls...
ok, they are used also in InitScrollLines() and InitScrollChars()

Yes, jimm should take a look at too. It is hard to see if the new code misses some checks which the old one has.
Attachment #8710953 - Flags: review?(bugs) → review+
Comment on attachment 8710955 [details] [diff] [review]
part.3 Don't allow to override system scroll speed if the wheel event causes too fast scroll

>+  if (aWheelEvent.deltaMode != nsIDOMWheelEvent::DOM_DELTA_LINE) {
I assume this check is currently somewhere, but could you say where?
Attachment #8710955 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #59)
> Comment on attachment 8710955 [details] [diff] [review]
> part.3 Don't allow to override system scroll speed if the wheel event causes
> too fast scroll
> 
> >+  if (aWheelEvent.deltaMode != nsIDOMWheelEvent::DOM_DELTA_LINE) {
> I assume this check is currently somewhere, but could you say where?

Sorry, what did you mean? Currently, we allow/perform to override system scroll settings only when the scroll amount is system default. If they are default, the delta mode is always DOM_DELTA_LINE because if it's DOM_DELTA_PAGE, the system settings are customized to so.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #58)
> Comment on attachment 8710953 [details] [diff] [review]
> part.2 Make WidgetWheelEvent store if overriding system scroll speed is
> allowed and it shouldn't be allowed if scroll speed isn't system default
> settings on Windows
> 
> >+MouseScrollHandler::SystemSettings::IsOverridingSystemScrollSpeedAllowed()
> >+{
> >+  // The default vertical and horizontal scrolling speed is 3, this is defined
> >+  // on the document of SystemParametersInfo in MSDN.
> >+  const uint32_t kSystemDefaultScrollingSpeed = 3;
> so we have this defined here
> 
> >@@ -3700,62 +3700,16 @@ nsWindow::OverrideSystemMouseScrollSpeed
> >                                          double aOriginalDeltaY,
> >                                          double& aOverriddenDeltaX,
> >                                          double& aOverriddenDeltaY)
> > {
> >   // The default vertical and horizontal scrolling speed is 3, this is defined
> >   // on the document of SystemParametersInfo in MSDN.
> >   const uint32_t kSystemDefaultScrollingSpeed = 3;
> and here. Could we please have it in just one place. Put some static
> variable to nsWindow?

Ah, the constant will be reimplemented as static inline method in part.3, so, don't mind the duplication.
Comment on attachment 8710953 [details] [diff] [review]
part.2 Make WidgetWheelEvent store if overriding system scroll speed is allowed and it shouldn't be allowed if scroll speed isn't system default settings on Windows

See comment 52 for the strategy of these patches how to fix this bug and see comment 53 for the detail of this patch.
Attachment #8710953 - Flags: review?(jmathies)
Comment on attachment 8710955 [details] [diff] [review]
part.3 Don't allow to override system scroll speed if the wheel event causes too fast scroll

See comment 54 for the detail of this patch.
Attachment #8710955 - Flags: review?(jmathies)
Comment on attachment 8710956 [details] [diff] [review]
part.4 Make APZC system scroll speed overriding aware

Could you review this, mstange?

This patch makes the preceding changes APZC-aware.

By the patches, system scroll overriding is implemented by WidgetWheelEvent and sets if the instance is allowed to be overridden to WidgetWheelEvent::mAllowToOverrideSystemScrollSpeed. So, ScrollWheelInput should store it and AsyncPanZoomController should use new methods of WidgetWheelEvent and check the mAllowToOverrideSystemScrollSpeed value.
Attachment #8710956 - Flags: review?(mstange)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #60)
> Sorry, what did you mean? Currently, we allow/perform to override system
> scroll settings only when the scroll amount is system default. If they are
> default, the delta mode is always DOM_DELTA_LINE because if it's
> DOM_DELTA_PAGE, the system settings are customized to so.
ok, so we do have effectively the same check currently, but it is just totally different kind of code.
Flags: needinfo?(bugs)
Comment on attachment 8710956 [details] [diff] [review]
part.4 Make APZC system scroll speed overriding aware

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

Looks good!

It looks like APZ only applies the override when we're scrolling a root content scroll frame. Is that intentional or was it a short term solution? Does non-APZ scrolling have the same behavior?

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1636,1 @@
>      // Only apply delta multipliers if we're increasing the delta.

You're removing the "if" conditions that this comment describes, so let's remove the comment, too.
Attachment #8710956 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #66)
> Comment on attachment 8710956 [details] [diff] [review]
> part.4 Make APZC system scroll speed overriding aware
> 
> Review of attachment 8710956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> It looks like APZ only applies the override when we're scrolling a root
> content scroll frame. Is that intentional or was it a short term solution?
> Does non-APZ scrolling have the same behavior?

Same behavior because we don't want faster scroll on _small_ scrollable elements like <select>. If we have a better idea to decide if a scrollable element is small or big, we should take such idea, though.

> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1636,1 @@
> >      // Only apply delta multipliers if we're increasing the delta.
> 
> You're removing the "if" conditions that this comment describes, so let's
> remove the comment, too.

Ah, indeed. Thank you for your review!
Flags: needinfo?(jmathies)
Attachment #8710953 - Flags: review?(jmathies) → review+
Comment on attachment 8710955 [details] [diff] [review]
part.3 Don't allow to override system scroll speed if the wheel event causes too fast scroll

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

::: widget/MouseEvents.h
@@ +638,5 @@
>    double OverriddenDeltaY() const;
>  
> +  // Compute the overridden delta value.  This may be useful for suppressing
> +  // too fast scroll by system scroll speed overriding when widget sets
> +  // mAllowToOverrideSystemScrollSpeed. 

nit - whitespace
Attachment #8710955 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4d4ee8924ba418a132f87bebd3ea6932900730
Bug 1153156 part.1 Move nsBaseWidget::OverrideSystemMouseScrollSpeed() to WidgetWheelEvent r=smaug, sr=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e3407fff0c1d255b216f3fe6dea5957c3c975a4
Bug 1153156 part.2 Make WidgetWheelEvent store if overriding system scroll speed is allowed and it shouldn't be allowed if scroll speed isn't system default settings on Windows r=smaug+jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f1f3501ce23f10b472720b88d77e77128bd234
Bug 1153156 part.3 Don't allow to override system scroll speed if the wheel event causes too fast scroll r=smaug+jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/8773a723f7a2361313704075a1b1aba20ff9457a
Bug 1153156 part.4 Make APZC system scroll speed overriding aware r=mstange
I'll request approvals for Aurora and Beta (if e10s may be enabled in those branches).
Attachment #8710956 - Attachment is obsolete: true
Scrolled distance with "alt" pressed is still bugged. So I guess that's a different issue from this.
Blocks: e10s
No longer blocks: 1215208
OS: Windows 8.1 → Windows
Hardware: x86_64 → All
See Also: 1215208
Comment on attachment 8712536 [details] [diff] [review]
part.1 Move nsBaseWidget::OverrideSystemMouseScrollSpeed() to WidgetWheelEvent (r=smaug, sr=smaug)

Approval Request Comment
[Feature/regressing bug #]: Regression of e10s without APZ.
[User impact if declined]: Without this bug fix, system scroll speed overriding doesn't work in e10s mode without APZ. The feature makes users feel slow at scrolling by mouse wheel.
[Describe test coverage new/current, TreeHerder]: Landed onto m-c.
[Risks and why]: Low because the implementation moved from nsIWidget to WidgetWheelEvent and computing if it may be overridden by the feature *before* dispatching WidgetWheelEvent from parent process's widget. 
[String/UUID change made/needed]: Changing nsIWidget.h.
Attachment #8712536 - Flags: approval-mozilla-aurora?
Comment on attachment 8712537 [details] [diff] [review]
part.2 Make WidgetWheelEvent store if overriding system scroll speed is allowed and it shouldn't be allowed if scroll speed isn't system default settings on Windows (r=smaug+jimm)

Approval Request Comment: See previous comment.

And I forgot to mention about this fact, if e10s won't be enabled in the release, this bug fix isn't necessary.
Attachment #8712537 - Flags: approval-mozilla-aurora?
Comment on attachment 8712538 [details] [diff] [review]
part.3 Don't allow to override system scroll speed if the wheel event causes too fast scroll (r=smaug+jimm)

Approval Request Comment: See above.
Attachment #8712538 - Flags: approval-mozilla-aurora?
Comment on attachment 8712540 [details] [diff] [review]
part.4 Make APZC system scroll speed overriding aware (r=mstange)

Approval Request Comment: See above.
Attachment #8712540 - Flags: approval-mozilla-aurora?
Approval Request Comment: See above.

If Beta 45 will enable e10s mode at its release, this bug should be fixed because users may feel slow if they disable APZ.
Attachment #8714612 - Flags: approval-mozilla-beta?
Approval Request Comment: See above.
Attachment #8712538 - Attachment is obsolete: true
Attachment #8712538 - Flags: approval-mozilla-aurora?
Attachment #8714616 - Flags: approval-mozilla-beta?
Approval Request Comment: See above.
Attachment #8714617 - Flags: approval-mozilla-beta?
Approval Request Comment: See above.

(sorry for the spam, I forgot to do qrefresh)
Attachment #8714617 - Attachment is obsolete: true
Attachment #8714617 - Flags: approval-mozilla-beta?
Attachment #8714618 - Flags: approval-mozilla-beta?
Comment on attachment 8712538 [details] [diff] [review]
part.3 Don't allow to override system scroll speed if the wheel event causes too fast scroll (r=smaug+jimm)

Approval Request Comment: See comment 79. Accidentally, I marked this as obsolete when I attach the patch for beta. Sorry for the spam.
Attachment #8712538 - Attachment is obsolete: false
Attachment #8712538 - Flags: approval-mozilla-aurora?
My understanding is we aren't aiming to release e10s in 45 (now in beta) but we are hoping to for 46.  
But, I also have the impression that we have to have APZ enabled along with e10s (for other issues than this).   

Jim, kats, are we planning for both options in 46 release -- e10s enabled without apz, and with apz?
Flags: needinfo?(jmathies)
Flags: needinfo?(bugmail.mozilla)
Thus far APZ is ready to ship in 46. It's possible that we uncover blockers that cause APZ to get pulled from 46, and in that case it may be desirable to ship e10s without APZ. I don't know if e10s would also want to wait another cycle if that were to happen. Jim or Brad might be able to answer that.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8714612 [details] [diff] [review]
part.1 Move nsBaseWidget::OverrideSystemMouseScrollSpeed() to WidgetWheelEvent (r=smaug, sr=smaug) for Beta

Sorry but I am not taking these changes in beta.
Rational:
* e10s won't be enabled in 45
* the experiment ends in 3 beta, we still need to stabilize the 45 release without e10s
* 45 is an esr
* the 4 patches are pretty large (more than 50k)
Attachment #8714612 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8714615 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8714616 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8714618 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Sylvestre Ledru [:sylvestre] from comment #91)
> Comment on attachment 8714612 [details] [diff] [review]
> part.1 Move nsBaseWidget::OverrideSystemMouseScrollSpeed() to
> WidgetWheelEvent (r=smaug, sr=smaug) for Beta
> 
> Sorry but I am not taking these changes in beta.
> Rational:
> * e10s won't be enabled in 45

Thank you. Sounds good to me. Especially, e10s won't be enabled in 45. I have some trouble in e10s-TSF for now. I'll do the best for 46.
Comment on attachment 8712536 [details] [diff] [review]
part.1 Move nsBaseWidget::OverrideSystemMouseScrollSpeed() to WidgetWheelEvent (r=smaug, sr=smaug)

Seems a bit risky but let's uplift this to aurora. Even if we ship e10s and apz in 46, we won't be shipping it to all users. We still want to support those users.  So I think this is a good risk.
 
If this causes regressions, please be ready to back out.
Attachment #8712536 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8712537 [details] [diff] [review]
part.2 Make WidgetWheelEvent store if overriding system scroll speed is allowed and it shouldn't be allowed if scroll speed isn't system default settings on Windows (r=smaug+jimm)

OK to uplift to aurora.
Attachment #8712537 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8712538 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8712540 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Andrei, can someone from your team test on aurora using the STR in comment 35? Thanks!
Flags: needinfo?(andrei.vaida)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #89)
> My understanding is we aren't aiming to release e10s in 45 (now in beta) but
> we are hoping to for 46.  
> But, I also have the impression that we have to have APZ enabled along with
> e10s (for other issues than this).   
> 
> Jim, kats, are we planning for both options in 46 release -- e10s enabled
> without apz, and with apz?

e10s currently isn't latched to apz. We were blocking on both back in december but that relationship didn't last long. I believe apz is targeting 46, and e10s is targeting 46 so both features will likley roll out together anyway.
Flags: needinfo?(jmathies)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #95)
> Andrei, can someone from your team test on aurora using the STR in comment
> 35? Thanks!

Petruta is currently looking into this, she'll follow up with test results as soon as possible.
Flags: needinfo?(andrei.vaida) → needinfo?(petruta.rasa)
Tested with Nightly 47.0a1 2016-02-08 and Aurora 46.0a2 2016-02-09 under Win 7 64-bit. 
I verified the approximate scroll distance on some pages and also used window.scrollY for more precision - in both cases the scroll distance was improved.

Kestrel, could you please confirm for your environment as well with latest Nightly or Aurora? Thank you!
Flags: needinfo?(petruta.rasa) → needinfo?(kestrel)
I tested various combinations of scroll lines, e10s, apz and delta multipliers, scrolling behaves as expected in all cases like it did before the regression.

Verified fixed on Nightly 47.0a1 (2016-02-09) and Aurora 46.0a2 (2016-02-09) using Windows 10 x64.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kestrel)
(In reply to Kestrel from comment #100)
> I tested various combinations of scroll lines, e10s, apz and delta
> multipliers, scrolling behaves as expected in all cases like it did before
> the regression.

Try testing with these settings:

general.smoothScroll.mouseWheel.durationMaxMS;1500
mousewheel.default.delta_multiplier_x;150
mousewheel.default.delta_multiplier_y;150
mousewheel.with_alt.delta_multiplier_x;300
mousewheel.with_alt.delta_multiplier_y;300
mousewheel.withcontrolkey.action;3

If I press "alt" and turn the wheel quickly and continously the scrolling slows down to a halt.
Sometimes it's abnormally slow whichever way I turn the wheel if I press "alt".
(In reply to avada from comment #102)
> general.smoothScroll.mouseWheel.durationMaxMS;1500
> mousewheel.default.delta_multiplier_x;150
> mousewheel.default.delta_multiplier_y;150
> mousewheel.with_alt.delta_multiplier_x;300
> mousewheel.with_alt.delta_multiplier_y;300
> mousewheel.withcontrolkey.action;3
> 
> If I press "alt" and turn the wheel quickly and continously the scrolling
> slows down to a halt.
> Sometimes it's abnormally slow whichever way I turn the wheel if I press
> "alt".

When you set mutiliper_* except 100, it's different bug even if there are some bugs because this is a bug for overriding system scroll speed settings which is available only when the settings are 100.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #103)

> 
> When you set mutiliper_* except 100, it's different bug even if there are
> some bugs because this is a bug for overriding system scroll speed settings
> which is available only when the settings are 100.

Do you have a bug number?
No, because nobody (except you?) have found such bug.
You need to log in before you can comment on or make changes to this bug.