Scroll handling has become unwieldly in windows/widget to the extent that it's getting hard to understand what's going on or review code. We also have zero test coverage on this and code related to wheel interaction with plugins. I'd like to look at: 1) isolating this code in a module in widget 2) better comment/break the code down into smaller, easier to understand chunks/methods 3) figure out how to write widget/plugin tests against it 4) better define how we expect scroll to work

I still don't have explicit idea for automated testing. However, I have an image of the separated code. I'll post it soon.

I'll improve |MouseScrollHandler::HandleInternalMouseWheelMessage()| and separate to multiple patches.

forgot to add new files...

This will be a nice chunk of code removed from nsWindow. Thanks Masayuki. One suggestion - consider moving helpers like 'FindTopmostWindowAtPoint' and friends into nsToolkit or a utility class where they would be easy to find and re-use.

That's good idea. I'll do it in a separate bug before this.

The handler should be a singleton class due to reducing memory size on environment which doesn't have mouse wheel.

The system settings should be stored in MouseScrollHandler::SystemSettings. It's going to be private by later patch.

Similar to SystemSettings, MouseScrollHandler::UserPrefs should manage user prefs and it's going to be private by later patch.

Just moving a static method in nsWindow.cpp to WinUtils.

This patch separates the managing code of devices' information on the environment to MouseScrollHandler::Device. And it has Elantech, TrackPoint and UltraNav classes. They manage only a device. These information are static due to WM_KEYDOWN/WM_KEYUP are needed to handle even if MouseScrollHandler isn't instanced. I cannot test all of this patch. I don't have these devices. It should be fine. Most of this patch are just moved from nsWindow except: 1. Using nsCommandEvent for forward and back when Elantech dispatched the strange key messages. We shouldn't dispatch actual key events for them with fake modifier key state. 2. "Software\\Lenovo\\UltraNav" check is moved to UltraNavi::IsObsoleteDriverInstalled() rather than TrackPoint::IsDriverInstalled(). It must be OEM product of Synaptics.

By capsuling the native message data into MouseScrollHandler::EventInfo, the handling code becomes easier to read.

I'm still working on the automated tests, however, I don't see any regression when I use the patched build for a week. Let's start of the reviews. I think that the NSPR log must be useful when developers try to check the behavior of some odd drivers, especially when they don't have them.

This patch should make a lot of conditions in nsWindow::OnMouseWheelInternal() easier to read.

The patches seem to be inconsistent in using MouseScrollHandler::GetInstance() vs. MouseScrollHandler::sInstance; is that intentional?

(In reply to Ms2ger from comment #26) > The patches seem to be inconsistent in using > MouseScrollHandler::GetInstance() vs. MouseScrollHandler::sInstance; is that > intentional? Yes, from instance methods (not static methods), using sInstance. Otherwise, using GetInstance(). If it causes crash, there is invalid instance of MouseScrollHandler or its internal class.

Comment on attachment 598134 [details] [diff] [review] part.1 Implement MouseScrollHandler for Windows Review of attachment 598134 [details] [diff] [review]: ----------------------------------------------------------------- Lets tighten up all the lines in the nsWindow if (!sInstanceCount) {} block by removing all the empty lines between calls.

Comment on attachment 598136 [details] [diff] [review] part.2 MouseScrollHandler should manage system settings Review of attachment 598136 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/WinMouseScrollHandler.cpp @@ +130,5 @@ > + if (!::SystemParametersInfo(SPI_GETWHEELSCROLLCHARS, 0, &mScrollChars, 0)) { > + PR_LOG(gMouseScrollLog, PR_LOG_ALWAYS, > + ("MouseScroll::SystemSettings::Init(): ::SystemParametersInfo(" > + "SPI_GETWHEELSCROLLCHARS) failed, %s", > + nsUXThemeData::sIsVistaOrLater ? sIsVistaOrLater has been removed, use WinUtils. Please post a rollup merged to tip so I can build this and test.

Comment on attachment 598137 [details] [diff] [review] part.3 MouseScrollHandler should manage user prefs Review of attachment 598137 [details] [diff] [review]: ----------------------------------------------------------------- This is looking really nice.

Comment on attachment 598139 [details] [diff] [review] part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils Review of attachment 598139 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/WinUtils.cpp @@ +123,5 @@ > } > > /* static */ > +bool > +WinUtils::HasRegistryKey(HKEY aRoot, const PRUnichar* aKeyName) NS_ASSERTIONs for NULL aRoot and aKeyName. ::: widget/windows/WinUtils.h @@ +92,5 @@ > PRUnichar* aBuffer, > DWORD aBufferLength); > > /** > + * Checks whether the registry key exists on the environment. This comment should mention that the call looks in both the 32 bit and 64 bit hives.

nit - in Part 1, ProcessMessage is missing proper parameter names. Might as well clean those up while we have the chance.

Comment on attachment 598140 [details] [diff] [review] part.5 Move device specific code to MouseScrollHandler Review of attachment 598140 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/WinMouseScrollHandler.cpp @@ +407,5 @@ > + > + static HMODULE hPSAPI = ::LoadLibraryW(L"psapi.dll"); > + static GetProcessImageFileNameProc pGetProcessImageFileName = > + reinterpret_cast<GetProcessImageFileNameProc>( > + ::GetProcAddress(hPSAPI, "GetProcessImageFileNameW")); According to MSDN, GetProcessImageFileName is XP and up, so I think we can drop the use of GetProcAddress.

Comment on attachment 598142 [details] [diff] [review] part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo Review of attachment 598142 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/WinMouseScrollHandler.h @@ +92,5 @@ > PRInt32 mDelta; > + // The window handle which is handling the event. > + HWND mWnd; > + // Timestamp of the event. > + PRUint32 mTime; Please switch this out with the use of TimeStamp and TimeDuration. http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h @@ +105,5 @@ > + > + /** > + * CanContinueTransaction() checks whether the new event can continue the > + * last transaction or not. Note that if there is no transaction, this > + * returns TRUE. nit - 'true' @@ +122,5 @@ > + void RecordEvent(const EventInfo& aEvent); > + > + // The remaining native delta value (i.e., not handled by previous > + // message handler). > + // XXX will be protected. What does this XXX comment mean?

Comment on attachment 598144 [details] [diff] [review] part.9 Implement NS_QUERY_SCROLL_TARGET_INFO event dispatcher on MouseScrollHandler::EventInfo Review of attachment 598144 [details] [diff] [review]: ----------------------------------------------------------------- I'm really glad this is mostly copy paste. :)

Comment on attachment 598142 [details] [diff] [review] part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo Review of attachment 598142 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/WinMouseScrollHandler.cpp @@ +194,5 @@ > + (mWnd == aNewEvent.GetWindowHandle() && > + IsPositive() == aNewEvent.IsPositive() && > + mIsVertical == aNewEvent.IsVertical() && > + mIsPage == aNewEvent.IsPage() && > + now - aNewEvent.GetTime() <= 1500); Also in part.7, can we move that 1500 up to the top off the file and add a good comment explaining why we went with this value.

Comment on attachment 598148 [details] [diff] [review] part.13 Compute cursor position at WM_MOUSEWHEEL and WM_MOUSEHWHEEL in MouseScrollHandler Review of attachment 598148 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/WinMouseScrollHandler.cpp @@ +1226,5 @@ > + ******************************************************************************/ > + > +/* static */ > +bool > +MouseScrollHandler::Device::SetPoint::MayBeGetMeesagePosBroken(UINT aMessage, nit - 'MaybeGetMeesagePosBroken', although lets rename this to something more reasonable. 'IsGetMessagePosResponseValid' ?

And MOZ_ASSERT instead of NS_ASSERTION for new assertions, please.

If you want to test these patches, you can use tryserver build: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-b286150b8031/ But I repost all patches in case you want to build by yourself. If you thought that these patches should land without tests for other developers, let me know. Otherwise, I'll post the tests next week.

Comment on attachment 599535 [details] [diff] [review] part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo Review of attachment 599535 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/WinMouseScrollHandler.cpp @@ +67,5 @@ > > +// The duration until timeout of events transaction. The value is 1.5 sec, > +// it's just a magic number, it was suggested by Logitech's engineer, see > +// bug 605648 comment 90. > +#define DEFAULT_TIMEOUT_DURATION 1500 great!

You might not like aAdditionalFlags. However, for testing some code for some strange devices, tests need to send some additional flags. See part.16 patch for the detail. And there are three delta values, aDeltaX, aDeltaY and aDeltaZ. Because on Mac, an event can have these three delta values.

Implements the SynthesizeNativeMouseScrollEvent() API of nsIWidget on Windows. First, this patch doesn't use SendInput() API because it *cannot* emulate strange devices' behavior. Therefore, this patch uses ::SentMessage() for that. For testing modifier key of the dispatched DOM events, this patch calls ::SetKeyboardState() before calling ::SendMessage(). And after handling the sent native message and the internal message for native message, recovers the original key states by calling ::SetKeyboardState(). For managing them, this patch checks the message handling state by mSynthesizingEvent which is allocated when somebody synthesizes first native event. So, this is never allocated on released build without addons which are using it. Finally, this patch fixes a bug. That is, handlers of internal messages which are posted by native message handlers cannot get the correct modifier key state at native message received if one or more modifier keys messages are received between the native message and the posted internal message. So, the message optimization ensures no key messages are received during the time.

Since native message event handlers post internal messages and the internal message handlers dispatch events, we cannot test the behavior synchronously. Therefore, these tests are in array and executed by asynchronously. We should append more tests for some strange devices. However, it should be done after D3E wheel event implementation because it would make our widget code simpler.

Comment on attachment 601531 [details] [diff] [review] part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent() Oops, roc is now offline. Smaug, could you review this?

Comment on attachment 601534 [details] [diff] [review] part.18 Make basic tests for MouseScrollHandler and also this...

https://hg.mozilla.org/integration/mozilla-inbound/rev/a752fae4b8df https://hg.mozilla.org/integration/mozilla-inbound/rev/927dae404b95 https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8d586b3ad2 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e18d335242a https://hg.mozilla.org/integration/mozilla-inbound/rev/611579ad9098 https://hg.mozilla.org/integration/mozilla-inbound/rev/70317ec19c19 https://hg.mozilla.org/integration/mozilla-inbound/rev/c95c3c5eed99 https://hg.mozilla.org/integration/mozilla-inbound/rev/981782dbfac1 https://hg.mozilla.org/integration/mozilla-inbound/rev/57afbe7cf885 https://hg.mozilla.org/integration/mozilla-inbound/rev/40c7eec54521 https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ad777954d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a48199569fee https://hg.mozilla.org/integration/mozilla-inbound/rev/e4dfd3a36f20 https://hg.mozilla.org/integration/mozilla-inbound/rev/26165b0572ed landed only part.1 - part.14 on m-i. PLEASE don't close this bug until automated tests (part.15 - part.18) are also landed on m-c.

https://hg.mozilla.org/mozilla-central/rev/a752fae4b8df https://hg.mozilla.org/mozilla-central/rev/927dae404b95 https://hg.mozilla.org/mozilla-central/rev/8a8d586b3ad2 https://hg.mozilla.org/mozilla-central/rev/8e18d335242a https://hg.mozilla.org/mozilla-central/rev/611579ad9098 https://hg.mozilla.org/mozilla-central/rev/70317ec19c19 https://hg.mozilla.org/mozilla-central/rev/c95c3c5eed99 https://hg.mozilla.org/mozilla-central/rev/981782dbfac1 https://hg.mozilla.org/mozilla-central/rev/57afbe7cf885 https://hg.mozilla.org/mozilla-central/rev/40c7eec54521 https://hg.mozilla.org/mozilla-central/rev/b1ad777954d3 https://hg.mozilla.org/mozilla-central/rev/a48199569fee https://hg.mozilla.org/mozilla-central/rev/e4dfd3a36f20 https://hg.mozilla.org/mozilla-central/rev/26165b0572ed

Comment on attachment 601532 [details] [diff] [review] part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows Review of attachment 601532 [details] [diff] [review]: ----------------------------------------------------------------- All in all looks really good. Have you run this and all the tests through try a few times to make sure there aren't any random orange problems? ::: widget/windows/WinMouseScrollHandler.cpp @@ +86,5 @@ > + if (SynthesizingEvent::IsSynthesizing()) { > + return sInstance->mSynthesizingEvent->GetCursorPoint(); > + } > + DWORD pos = ::GetMessagePos(); > + return MAKEPOINTS(pos); nit - might as well combine these two lines. @@ +123,5 @@ > return sInstance; > } > > +MouseScrollHandler::MouseScrollHandler() : > + mIsWaitingInternalMessage(false), mSynthesizingEvent(nsnull) nit - wrapping @@ +301,5 @@ > } > > /* static */ > +void > +MouseScrollHandler::InitEvent(nsWindow* aWindow, check for the validity of nsWindow* aWindow. @@ +1635,5 @@ > +{ > + if (mStatus == SENDING_MESSAGE && mMessage == aMessage && > + mWParam == aWParam && mLParam == aLParam) { > + mStatus = NATIVE_MESSAGE_RECEIVED; > + if (aWindow->GetWindowHandle() == mWnd) { same here, just to be safe.

Comment on attachment 601534 [details] [diff] [review] part.18 Make basic tests for MouseScrollHandler Review of attachment 601534 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/tests/window_mouse_scroll_win.html @@ +44,5 @@ > +const CTRL_R = 0x0800; > +const ALT_L = 0x1000; > +const ALT_R = 0x2000; > + > +const DOM_PAGE_SCROLL_DELTA = 32768; curious, where does this constant come from? @@ +66,5 @@ > +const kPixelEnabledPref = "mousewheel.enable_pixel_scrolling"; > +const kEmulateWheelByWMSCROLLPref = "mousewheel.emulate_at_wm_scroll"; > +const kVAmountPref = "mousewheel.windows.vertical_amount_override"; > +const kHAmountPref = "mousewheel.windows.horizontal_amount_override"; > +const kTimeoutPref = "mousewheel.windows.transaction.timeout"; nit - you lined up the previous consts, but not here for some reason.

(In reply to Jim Mathies [:jimm] from comment #63) > Comment on attachment 601532 [details] [diff] [review] > part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows > > Review of attachment 601532 [details] [diff] [review]: > ----------------------------------------------------------------- > > All in all looks really good. Have you run this and all the tests through > try a few times to make sure there aren't any random orange problems? Yes, I tested 20 times on the 4 builds but I don't see new orange. > > ::: widget/windows/WinMouseScrollHandler.cpp > @@ +86,5 @@ > > + if (SynthesizingEvent::IsSynthesizing()) { > > + return sInstance->mSynthesizingEvent->GetCursorPoint(); > > + } > > + DWORD pos = ::GetMessagePos(); > > + return MAKEPOINTS(pos); > > nit - might as well combine these two lines. According to MSDN, MAKEPOINTS is macro. So, I think we shouldn't pass any methods and APIs for its argument. (In reply to Jim Mathies [:jimm] from comment #64) > Comment on attachment 601534 [details] [diff] [review] > part.18 Make basic tests for MouseScrollHandler > > Review of attachment 601534 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/tests/window_mouse_scroll_win.html > @@ +44,5 @@ > > +const CTRL_R = 0x0800; > > +const ALT_L = 0x1000; > > +const ALT_R = 0x2000; > > + > > +const DOM_PAGE_SCROLL_DELTA = 32768; > > curious, where does this constant come from? the modifier flags are defined in: http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#1301 the DOM_PAGE_SCROLL_DELTA is... Oh, I thought it's WHEEL_PAGESCROLL value. But I research the value again, then, it's UINT_MAX. So, the value is wrong. However, the API's delta is float. So, the value cannot be used. Hmm, it's ok for now because our code assumes that over 120 means page scroll, and such crazy value must not be there actually. So, it's no problem... FYI: Mac's delta value is float, therefore, it's float. Thank you, jimm, for your review!

Since the landing in comment 62, when building with --disable-accessibility (and i have to), i'm getting: > c:/.../mozilla/widget/windows/WinMouseScrollHandler.cpp(1217) : error C2653: 'nsGkAtoms' : is not a class or namespace name > c:/.../mozilla/widget/windows/WinMouseScrollHandler.cpp(1217) : error C2065: 'onAppCommand' : undeclared identifier > c:/.../mozilla/widget/windows/WinMouseScrollHandler.cpp(1218) : error C2653: 'nsGkAtoms' : is not a class or namespace name > c:/.../mozilla/widget/windows/WinMouseScrollHandler.cpp(1218) : error C2065: 'Forward' : undeclared identifier > c:/.../mozilla/widget/windows/WinMouseScrollHandler.cpp(1218) : error C2653: 'nsGkAtoms' : is not a class or namespace name > c:/.../mozilla/widget/windows/WinMouseScrollHandler.cpp(1218) : error C2065: 'Back' : undeclared identifier There seems to be an #include "nsGkAtoms.h" missing either in 'WinMouseScrollHandler.cpp' or in 'widget/windows/nsWindow.h' (like 'widget/gtk2/nsWindow.h').

Bug 733988 will fix that.

Comment on attachment 601531 [details] [diff] [review] part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent() roc: do you have time to review this?

Comment on attachment 603992 [details] [diff] [review] part.18 Make basic tests for MouseScrollHandler and also this.

Comment on attachment 601531 [details] [diff] [review] part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent() Hmm, the delta values should be double rather than float. See bug 719320 comment 4. It may cause some trouble in other platform's tests. I'll update them next week.

float is replaced with double.

float is replaced with double.

float is replaced with double.

float is replaced with double.

Comment on attachment 603992 [details] [diff] [review] part.18 Make basic tests for MouseScrollHandler Review of attachment 603992 [details] [diff] [review]: ----------------------------------------------------------------- rubber-stamp=me

Comment on attachment 607798 [details] [diff] [review] part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent() Review of attachment 607798 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsIWidget.h @@ +1369,5 @@ > PRUint32 aModifierFlags) = 0; > > /** > + * Utility method intended for testing. Dispatches native mouse scroll > + * events may even move the mouse cursor. "Dispatching native mouse scroll events may move the mouse cursor."

Comment on attachment 607798 [details] [diff] [review] part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent() Thank you, roc. changing the review requests to smaug to sr requests.

https://hg.mozilla.org/integration/mozilla-inbound/rev/78b2188530e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/7326d36d774e https://hg.mozilla.org/integration/mozilla-inbound/rev/a14cc80e5c15 https://hg.mozilla.org/integration/mozilla-inbound/rev/2943b3d6edfe

https://hg.mozilla.org/mozilla-central/rev/78b2188530e5 https://hg.mozilla.org/mozilla-central/rev/7326d36d774e https://hg.mozilla.org/mozilla-central/rev/a14cc80e5c15 https://hg.mozilla.org/mozilla-central/rev/2943b3d6edfe you don't need to annotate [inbound] in the whiteboard.