Kestrel Reporter
	Description • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 1 • 9 years ago

Sounds like you have the layers.async-pan-zoom.enabled pref set. Cc'ing dvander.

	Kestrel Reporter
	Comment 2 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 3 • 9 years ago

Is this a recent regression? What version/build are you seeing this on?

	Kestrel Reporter
	Comment 4 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 5 • 9 years ago

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/

	Alice0775 White
	Comment 6 • 9 years ago

i can repro since firefox17 as well as Firefox37.0.1

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 9 years ago

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

	Kestrel Reporter
	Comment 8 • 9 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 9 years ago

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.

	u279076
	Comment 10 • 9 years ago

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.

	Kevin Brosnan [Ex-Mozilla]
	Comment 11 • 9 years ago

I can reproduce this and this is the sort of fit and finish that causes people to drop Firefox.

	avada
	Comment 13 • 9 years ago

So since no-one's working on this, is setting mousewheel.default.delta_multiplier_y to 99 or 100 the best workaround for now?

	Neil Deakin
	Comment 14 • 9 years ago

Don't think I can do this one.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 15 • 9 years ago

Kats, how does this relate to APZ and what needs to be done here

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 16 • 9 years ago

It's not related to APZ; unrelated bug according to comments 2-6.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 17 • 9 years ago

The triage group largely doesn't think this matters for release, but we'll defer to product to make that call.

	Kevin Brosnan [Ex-Mozilla]
	Comment 18 • 9 years ago

This is fairly awful and is a primary interaction for users. I stand by my comment 11.

	Jim Mathies [:jimm]
	Comment 19 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 20 • 9 years ago

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

	Kevin Brosnan [Ex-Mozilla]
	Comment 21 • 9 years ago

Windows OS mouse wheel preferences as I recall. I'll check when I get home as that is where I use Windows the most.

	Kevin Brosnan [Ex-Mozilla]
	Comment 22 • 9 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 23 • 9 years ago

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

	Kestrel Reporter
	Comment 24 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 25 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 27 • 9 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 29 • 9 years ago

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.

	Kestrel Reporter
	Comment 30 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 31 • 9 years ago

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?

	Kestrel Reporter
	Comment 32 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 33 • 9 years ago

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.

	Jim Mathies [:jimm]
	Comment 35 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 36 • 9 years ago

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

	Kestrel Reporter
	Comment 37 • 9 years ago

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

	vkrishnamoorthy
	Comment 38 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 39 • 9 years ago

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

	Julien L.
	Comment 41 • 9 years ago

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

	Julien L.
	Comment 42 • 9 years ago

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! :)

	Julien L.
	Comment 43 • 9 years ago

e10s is coming in firefox 45 or 46 and it will be great to fix that bug. That's the only one I get :)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 44 • 8 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 45 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 46 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 47 • 8 years ago

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

	avada
	Comment 48 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 49 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06978ccce9a8

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 50 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6055b50837f4

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 51 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86c9ee5f0a5f

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 52 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 53 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 54 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 55 • 8 years ago

This patch makes the new implementation APZC-aware. I'll request to review this to mstange after smaug agrees with my approach.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 56 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 57 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 58 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 59 • 8 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 60 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 61 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 62 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 63 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 64 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 65 • 8 years ago

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

	Markus Stange [:mstange]
	Comment 66 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 67 • 8 years ago

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

	Jim Mathies [:jimm]
	Comment 68 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 69 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 73 • 8 years ago

I'll request approvals for Aurora and Beta (if e10s may be enabled in those branches).

	Carsten Book [:Tomcat]
	Comment 74 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5c4d4ee8924b
https://hg.mozilla.org/mozilla-central/rev/0e3407fff0c1
https://hg.mozilla.org/mozilla-central/rev/d3f1f3501ce2
https://hg.mozilla.org/mozilla-central/rev/8773a723f7a2

	Carsten Book [:Tomcat]
	Comment 75 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5c4d4ee8924b
https://hg.mozilla.org/mozilla-central/rev/0e3407fff0c1
https://hg.mozilla.org/mozilla-central/rev/d3f1f3501ce2
https://hg.mozilla.org/mozilla-central/rev/8773a723f7a2

	avada
	Comment 76 • 8 years ago

Scrolled distance with "alt" pressed is still bugged. So I guess that's a different issue from this.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 79 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 80 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 81 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 82 • 8 years ago

Comment on attachment 8712540 [details] [diff] [review]
part.4 Make APZC system scroll speed overriding aware (r=mstange)

Approval Request Comment: See above.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 83 • 8 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 84 • 8 years ago

Approval Request Comment: See above.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 85 • 8 years ago

Approval Request Comment: See above.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 86 • 8 years ago

Approval Request Comment: See above.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 87 • 8 years ago

Approval Request Comment: See above.

(sorry for the spam, I forgot to do qrefresh)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 88 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 89 • 8 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 90 • 8 years ago

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.

	Sylvestre Ledru [:Sylvestre]
	Comment 91 • 8 years ago

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)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 92 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 93 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 94 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 95 • 8 years ago

Andrei, can someone from your team test on aurora using the STR in comment 35? Thanks!

	Carsten Book [:Tomcat]
	Comment 96 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/786ed66f993a
https://hg.mozilla.org/releases/mozilla-aurora/rev/b9a8fc7ef9a5
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec4502e3c0d4
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f8f3e46273a

	Jim Mathies [:jimm]
	Comment 97 • 8 years ago

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

	Andrei Vaida [:avaida]
	Comment 98 • 8 years ago

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

	Petruta Horea [:phorea], Desktop QA
	Comment 99 • 8 years ago

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!

	Kestrel Reporter
	Comment 100 • 8 years ago

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.

	Petruta Horea [:phorea], Desktop QA
	Comment 101 • 8 years ago

Thanks a lot! 
I'm updating the status flags.

	avada
	Comment 102 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 103 • 8 years ago

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

	avada
	Comment 104 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 105 • 8 years ago

No, because nobody (except you?) have found such bug.