Last Comment Bug 672175 - Clean up / reorganize / better encapsulate our scroll widget code and figure out some way to write tests for it
: Clean up / reorganize / better encapsulate our scroll widget code and figure ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
:
Mentors:
Depends on: 712483 733988 739297
Blocks: 483136 783450 719320
  Show dependency treegraph
 
Reported: 2011-07-18 04:20 PDT by Jim Mathies [:jimm]
Modified: 2012-08-16 19:19 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch WIP (54.63 KB, patch)
2011-12-20 05:25 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch (WIP) (100.79 KB, patch)
2011-12-20 05:28 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.1 Implement MouseScrollHandler for Windows (11.05 KB, patch)
2012-01-19 00:30 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.2 MouseScrollHandler should manage system settings (12.54 KB, patch)
2012-01-19 00:33 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.3 MouseScrollHandler should manage user prefs (8.12 KB, patch)
2012-01-19 00:47 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils (5.05 KB, patch)
2012-01-19 00:49 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.5 Move device specific code to MouseScrollHandler (39.68 KB, patch)
2012-01-19 01:15 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.6 Summarize native mouse wheel events by MouseScrollHandler::EventInfo (13.48 KB, patch)
2012-01-19 01:50 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.1 Implement MouseScrollHandler for Windows (7.05 KB, patch)
2012-02-16 21:59 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.2 MouseScrollHandler should manage system settings (12.57 KB, patch)
2012-02-16 22:01 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.3 MouseScrollHandler should manage user prefs (8.24 KB, patch)
2012-02-16 22:02 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils (5.05 KB, patch)
2012-02-16 22:03 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.5 Move device specific code to MouseScrollHandler (41.38 KB, patch)
2012-02-16 22:04 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.6 Summarize native mouse wheel events by MouseScrollHandler::EventInfo (13.48 KB, patch)
2012-02-16 22:07 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo (18.33 KB, patch)
2012-02-16 22:08 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review-
Details | Diff | Review
part.8 Compute modifier key state in MouseScrollHandler (3.88 KB, patch)
2012-02-16 22:09 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.9 Implement NS_QUERY_SCROLL_TARGET_INFO event dispatcher on MouseScrollHandler::EventInfo (12.94 KB, patch)
2012-02-16 22:10 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.10 Initialize mouse scroll events in MouseScrollHandler::LastEventInfo (18.29 KB, patch)
2012-02-16 22:11 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.11 Move MOZ_WM_MOUSE*WHEEL handler into MouseScrollHandler (14.62 KB, patch)
2012-02-16 22:11 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.12 Move MOZ_WM_*SCROLL handler into MouseScrollHandler (12.12 KB, patch)
2012-02-16 22:12 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.13 Compute cursor position at WM_MOUSEWHEEL and WM_MOUSEHWHEEL in MouseScrollHandler (9.00 KB, patch)
2012-02-16 22:13 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.14 Move WM_MOUSE*WHEEL and WM_*SCROLL handlers into MouseScrollHandler (26.44 KB, patch)
2012-02-16 22:14 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.1 Implement MouseScrollHandler for Windows (24.63 KB, patch)
2012-02-22 01:51 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.1 Implement MouseScrollHandler for Windows (7.55 KB, patch)
2012-02-22 01:52 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.2 MouseScrollHandler should manage system settings (12.62 KB, patch)
2012-02-22 01:52 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.3 MouseScrollHandler should manage user prefs (8.24 KB, patch)
2012-02-22 01:53 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils (5.21 KB, patch)
2012-02-22 01:53 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.5 Move device specific code to MouseScrollHandler (40.89 KB, patch)
2012-02-22 01:54 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.6 Summarize native mouse wheel events by MouseScrollHandler::EventInfo (13.49 KB, patch)
2012-02-22 01:54 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo (19.52 KB, patch)
2012-02-22 01:57 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.8 Compute modifier key state in MouseScrollHandler (3.87 KB, patch)
2012-02-22 01:58 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.9 Implement NS_QUERY_SCROLL_TARGET_INFO event dispatcher on MouseScrollHandler::EventInfo (12.95 KB, patch)
2012-02-22 01:59 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.10 Initialize mouse scroll events in MouseScrollHandler::LastEventInfo (18.18 KB, patch)
2012-02-22 01:59 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.11 Move MOZ_WM_MOUSE*WHEEL handler into MouseScrollHandler (14.64 KB, patch)
2012-02-22 02:00 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.12 Move MOZ_WM_*SCROLL handler into MouseScrollHandler (12.12 KB, patch)
2012-02-22 02:00 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.13 Compute cursor position at WM_MOUSEWHEEL and WM_MOUSEHWHEEL in MouseScrollHandler (9.18 KB, patch)
2012-02-22 02:01 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.14 Move WM_MOUSE*WHEEL and WM_*SCROLL handlers into MouseScrollHandler (26.45 KB, patch)
2012-02-22 02:01 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent() (9.24 KB, patch)
2012-02-28 22:22 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows (39.38 KB, patch)
2012-02-28 22:38 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.17 System settings of mouse wheel on Windows should be able to be overridden by prefs for testing (7.97 KB, patch)
2012-02-28 22:39 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.18 Make basic tests for MouseScrollHandler (68.91 KB, patch)
2012-02-28 22:44 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows (39.45 KB, patch)
2012-03-08 01:25 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.18 Make basic tests for MouseScrollHandler (68.97 KB, patch)
2012-03-08 01:26 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent() (9.25 KB, patch)
2012-03-20 17:57 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
roc: review+
bugs: superreview+
Details | Diff | Review
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows (39.46 KB, patch)
2012-03-20 17:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.17 System settings of mouse wheel on Windows should be able to be overridden by prefs for testing (7.97 KB, patch)
2012-03-20 17:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.18 Make basic tests for MouseScrollHandler (68.97 KB, patch)
2012-03-20 17:59 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
roc: review+
Details | Diff | Review

Description Jim Mathies [:jimm] 2011-07-18 04:20:51 PDT
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
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-19 16:55:24 PST
I still don't have explicit idea for automated testing. However, I have an image of the  separated code. I'll post it soon.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-20 05:25:25 PST
Created attachment 583124 [details] [diff] [review]
Patch WIP

I'll improve |MouseScrollHandler::HandleInternalMouseWheelMessage()| and separate to multiple patches.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-20 05:28:55 PST
Created attachment 583126 [details] [diff] [review]
Patch (WIP)

forgot to add new files...
Comment 4 Jim Mathies [:jimm] 2011-12-20 06:03:44 PST
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.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-12-20 15:42:22 PST
That's good idea. I'll do it in a separate bug before this.
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-19 00:30:00 PST
Created attachment 589797 [details] [diff] [review]
part.1 Implement MouseScrollHandler for Windows

The handler should be a singleton class due to reducing memory size on environment which doesn't have mouse wheel.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-19 00:33:00 PST
Created attachment 589798 [details] [diff] [review]
part.2 MouseScrollHandler should manage system settings

The system settings should be stored in MouseScrollHandler::SystemSettings. It's going to be private by later patch.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-19 00:47:50 PST
Created attachment 589799 [details] [diff] [review]
part.3 MouseScrollHandler should manage user prefs

Similar to SystemSettings, MouseScrollHandler::UserPrefs should manage user prefs and it's going to be private by later patch.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-19 00:49:09 PST
Created attachment 589800 [details] [diff] [review]
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils

Just moving a static method in nsWindow.cpp to WinUtils.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-19 01:15:11 PST
Created attachment 589801 [details] [diff] [review]
part.5 Move device specific code to MouseScrollHandler

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.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-01-19 01:50:38 PST
Created attachment 589805 [details] [diff] [review]
part.6 Summarize native mouse wheel events by MouseScrollHandler::EventInfo

By capsuling the native message data into MouseScrollHandler::EventInfo, the handling code becomes easier to read.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 21:59:40 PST
Created attachment 598134 [details] [diff] [review]
part.1 Implement MouseScrollHandler for Windows

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.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:01:52 PST
Created attachment 598136 [details] [diff] [review]
part.2 MouseScrollHandler should manage system settings
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:02:28 PST
Created attachment 598137 [details] [diff] [review]
part.3 MouseScrollHandler should manage user prefs
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:03:37 PST
Created attachment 598139 [details] [diff] [review]
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:04:18 PST
Created attachment 598140 [details] [diff] [review]
part.5 Move device specific code to MouseScrollHandler
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:07:23 PST
Created attachment 598141 [details] [diff] [review]
part.6 Summarize native mouse wheel events by MouseScrollHandler::EventInfo

This patch should make a lot of conditions in nsWindow::OnMouseWheelInternal() easier to read.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:08:35 PST
Created attachment 598142 [details] [diff] [review]
part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:09:23 PST
Created attachment 598143 [details] [diff] [review]
part.8 Compute modifier key state in MouseScrollHandler
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:10:26 PST
Created attachment 598144 [details] [diff] [review]
part.9 Implement NS_QUERY_SCROLL_TARGET_INFO event dispatcher on MouseScrollHandler::EventInfo
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:11:21 PST
Created attachment 598145 [details] [diff] [review]
part.10 Initialize mouse scroll events in MouseScrollHandler::LastEventInfo
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:11:59 PST
Created attachment 598146 [details] [diff] [review]
part.11 Move MOZ_WM_MOUSE*WHEEL handler into MouseScrollHandler
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:12:39 PST
Created attachment 598147 [details] [diff] [review]
part.12 Move MOZ_WM_*SCROLL handler into MouseScrollHandler
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:13:22 PST
Created attachment 598148 [details] [diff] [review]
part.13 Compute cursor position at WM_MOUSEWHEEL and WM_MOUSEHWHEEL in MouseScrollHandler
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-16 22:14:39 PST
Created attachment 598149 [details] [diff] [review]
part.14 Move WM_MOUSE*WHEEL and WM_*SCROLL handlers into MouseScrollHandler
Comment 26 :Ms2ger 2012-02-17 03:31:27 PST
The patches seem to be inconsistent in using MouseScrollHandler::GetInstance() vs. MouseScrollHandler::sInstance; is that intentional?
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-17 08:58:46 PST
(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 28 Jim Mathies [:jimm] 2012-02-21 07:54:33 PST
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 29 Jim Mathies [:jimm] 2012-02-21 07:59:48 PST
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 30 Jim Mathies [:jimm] 2012-02-21 08:04:30 PST
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 31 Jim Mathies [:jimm] 2012-02-21 08:07:46 PST
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.
Comment 32 Jim Mathies [:jimm] 2012-02-21 08:15:48 PST
nit - in Part 1, ProcessMessage is missing proper parameter names. Might as well clean those up while we have the chance.
Comment 33 Jim Mathies [:jimm] 2012-02-21 08:23:21 PST
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 34 Jim Mathies [:jimm] 2012-02-21 08:54:52 PST
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 35 Jim Mathies [:jimm] 2012-02-21 09:03:18 PST
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 36 Jim Mathies [:jimm] 2012-02-21 09:09:19 PST
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 37 Jim Mathies [:jimm] 2012-02-21 11:44:27 PST
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' ?
Comment 38 :Ms2ger 2012-02-21 13:06:36 PST
And MOZ_ASSERT instead of NS_ASSERTION for new assertions, please.
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:51:45 PST
Created attachment 599526 [details] [diff] [review]
part.1 Implement MouseScrollHandler for Windows
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:52:15 PST
Created attachment 599527 [details] [diff] [review]
part.1 Implement MouseScrollHandler for Windows
Comment 41 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:52:47 PST
Created attachment 599528 [details] [diff] [review]
part.2 MouseScrollHandler should manage system settings
Comment 42 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:53:17 PST
Created attachment 599529 [details] [diff] [review]
part.3 MouseScrollHandler should manage user prefs
Comment 43 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:53:50 PST
Created attachment 599531 [details] [diff] [review]
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils
Comment 44 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:54:25 PST
Created attachment 599532 [details] [diff] [review]
part.5 Move device specific code to MouseScrollHandler
Comment 45 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:54:59 PST
Created attachment 599533 [details] [diff] [review]
part.6 Summarize native mouse wheel events by MouseScrollHandler::EventInfo
Comment 46 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:57:54 PST
Created attachment 599535 [details] [diff] [review]
part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo

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 47 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:58:28 PST
Created attachment 599536 [details] [diff] [review]
part.8 Compute modifier key state in MouseScrollHandler
Comment 48 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:59:02 PST
Created attachment 599537 [details] [diff] [review]
part.9 Implement NS_QUERY_SCROLL_TARGET_INFO event dispatcher on MouseScrollHandler::EventInfo
Comment 49 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 01:59:30 PST
Created attachment 599538 [details] [diff] [review]
part.10 Initialize mouse scroll events in MouseScrollHandler::LastEventInfo
Comment 50 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 02:00:00 PST
Created attachment 599539 [details] [diff] [review]
part.11 Move MOZ_WM_MOUSE*WHEEL handler into MouseScrollHandler
Comment 51 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 02:00:30 PST
Created attachment 599540 [details] [diff] [review]
part.12 Move MOZ_WM_*SCROLL handler into MouseScrollHandler
Comment 52 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 02:01:01 PST
Created attachment 599541 [details] [diff] [review]
part.13 Compute cursor position at WM_MOUSEWHEEL and WM_MOUSEHWHEEL in MouseScrollHandler
Comment 53 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-22 02:01:35 PST
Created attachment 599542 [details] [diff] [review]
part.14 Move WM_MOUSE*WHEEL and WM_*SCROLL handlers into MouseScrollHandler
Comment 54 Jim Mathies [:jimm] 2012-02-22 06:30:27 PST
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!
Comment 55 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-28 22:22:28 PST
Created attachment 601531 [details] [diff] [review]
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent()

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.
Comment 56 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-28 22:38:22 PST
Created attachment 601532 [details] [diff] [review]
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows

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.
Comment 57 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-28 22:39:38 PST
Created attachment 601533 [details] [diff] [review]
part.17 System settings of mouse wheel on Windows should be able to be overridden by prefs for testing
Comment 58 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-02-28 22:44:40 PST
Created attachment 601534 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

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 59 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-02 02:21:47 PST
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 60 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-02 02:22:36 PST
Comment on attachment 601534 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

and also this...
Comment 63 Jim Mathies [:jimm] 2012-03-07 07:21:10 PST
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 64 Jim Mathies [:jimm] 2012-03-07 07:24:37 PST
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.
Comment 65 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-07 16:24:59 PST
(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!
Comment 66 H. Hofer 2012-03-08 00:50:25 PST
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').
Comment 67 Cameron McCormack (:heycam) 2012-03-08 00:51:46 PST
Bug 733988 will fix that.
Comment 68 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-08 01:25:20 PST
Created attachment 603991 [details] [diff] [review]
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows
Comment 69 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-08 01:26:15 PST
Created attachment 603992 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler
Comment 70 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-15 20:25:04 PDT
Comment on attachment 601531 [details] [diff] [review]
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent()

roc:

do you have time to review this?
Comment 71 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-15 20:25:19 PDT
Comment on attachment 603992 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

and also this.
Comment 72 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-16 04:44:59 PDT
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.
Comment 73 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-20 17:57:15 PDT
Created attachment 607798 [details] [diff] [review]
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent()

float is replaced with double.
Comment 74 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-20 17:58:04 PDT
Created attachment 607799 [details] [diff] [review]
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows

float is replaced with double.
Comment 75 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-20 17:58:55 PDT
Created attachment 607800 [details] [diff] [review]
part.17 System settings of mouse wheel on Windows should be able to be overridden by prefs for testing

float is replaced with double.
Comment 76 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-20 17:59:49 PDT
Created attachment 607801 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

float is replaced with double.
Comment 77 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 18:01:32 PDT
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 78 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 18:02:54 PDT
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 79 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-03-20 18:09:50 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.