The default bug view has changed. See this FAQ.

Clean up / reorganize / better encapsulate our scroll widget code and figure out some way to write tests for it

RESOLVED FIXED in mozilla14

Status

()

Core
Widget: Win32
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jimm, Assigned: masayuki)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla14
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(19 attachments, 28 obsolete attachments)

7.55 KB, patch
Details | Diff | Splinter Review
12.62 KB, patch
Details | Diff | Splinter Review
8.24 KB, patch
Details | Diff | Splinter Review
5.21 KB, patch
Details | Diff | Splinter Review
40.89 KB, patch
Details | Diff | Splinter Review
13.49 KB, patch
Details | Diff | Splinter Review
19.52 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.87 KB, patch
Details | Diff | Splinter Review
12.95 KB, patch
Details | Diff | Splinter Review
18.18 KB, patch
Details | Diff | Splinter Review
14.64 KB, patch
Details | Diff | Splinter Review
12.12 KB, patch
Details | Diff | Splinter Review
9.18 KB, patch
Details | Diff | Splinter Review
26.45 KB, patch
Details | Diff | Splinter Review
68.97 KB, patch
Details | Diff | Splinter Review
9.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
39.46 KB, patch
Details | Diff | Splinter Review
7.97 KB, patch
Details | Diff | Splinter Review
68.97 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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
(Assignee)

Comment 1

5 years ago
I still don't have explicit idea for automated testing. However, I have an image of the  separated code. I'll post it soon.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 583124 [details] [diff] [review]
Patch WIP

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

Comment 3

5 years ago
Created attachment 583126 [details] [diff] [review]
Patch (WIP)

forgot to add new files...
Attachment #583124 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
That's good idea. I'll do it in a separate bug before this.
(Assignee)

Updated

5 years ago
Depends on: 712483
(Assignee)

Updated

5 years ago
Blocks: 719320
(Assignee)

Comment 6

5 years ago
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.
Attachment #583126 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 483136
(Assignee)

Comment 12

5 years ago
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.
Attachment #589797 - Attachment is obsolete: true
Attachment #589798 - Attachment is obsolete: true
Attachment #589799 - Attachment is obsolete: true
Attachment #589800 - Attachment is obsolete: true
Attachment #589801 - Attachment is obsolete: true
Attachment #589805 - Attachment is obsolete: true
Attachment #598134 - Flags: review?(jmathies)
(Assignee)

Comment 13

5 years ago
Created attachment 598136 [details] [diff] [review]
part.2 MouseScrollHandler should manage system settings
Attachment #598136 - Flags: review?(jmathies)
(Assignee)

Comment 14

5 years ago
Created attachment 598137 [details] [diff] [review]
part.3 MouseScrollHandler should manage user prefs
Attachment #598137 - Flags: review?(jmathies)
(Assignee)

Comment 15

5 years ago
Created attachment 598139 [details] [diff] [review]
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils
Attachment #598139 - Flags: review?(jmathies)
(Assignee)

Comment 16

5 years ago
Created attachment 598140 [details] [diff] [review]
part.5 Move device specific code to MouseScrollHandler
Attachment #598140 - Flags: review?(jmathies)
(Assignee)

Comment 17

5 years ago
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.
Attachment #598141 - Flags: review?(jmathies)
(Assignee)

Comment 18

5 years ago
Created attachment 598142 [details] [diff] [review]
part.7 Manage last wheel scroll message information by MouseScrollHandler::LastEventInfo
Attachment #598142 - Flags: review?(jmathies)
(Assignee)

Comment 19

5 years ago
Created attachment 598143 [details] [diff] [review]
part.8 Compute modifier key state in MouseScrollHandler
Attachment #598143 - Flags: review?(jmathies)
(Assignee)

Comment 20

5 years ago
Created attachment 598144 [details] [diff] [review]
part.9 Implement NS_QUERY_SCROLL_TARGET_INFO event dispatcher on MouseScrollHandler::EventInfo
Attachment #598144 - Flags: review?(jmathies)
(Assignee)

Comment 21

5 years ago
Created attachment 598145 [details] [diff] [review]
part.10 Initialize mouse scroll events in MouseScrollHandler::LastEventInfo
Attachment #598145 - Flags: review?(jmathies)
(Assignee)

Comment 22

5 years ago
Created attachment 598146 [details] [diff] [review]
part.11 Move MOZ_WM_MOUSE*WHEEL handler into MouseScrollHandler
Attachment #598146 - Flags: review?(jmathies)
(Assignee)

Comment 23

5 years ago
Created attachment 598147 [details] [diff] [review]
part.12 Move MOZ_WM_*SCROLL handler into MouseScrollHandler
Attachment #598147 - Flags: review?(jmathies)
(Assignee)

Comment 24

5 years ago
Created attachment 598148 [details] [diff] [review]
part.13 Compute cursor position at WM_MOUSEWHEEL and WM_MOUSEHWHEEL in MouseScrollHandler
Attachment #598148 - Flags: review?(jmathies)
(Assignee)

Comment 25

5 years ago
Created attachment 598149 [details] [diff] [review]
part.14 Move WM_MOUSE*WHEEL and WM_*SCROLL handlers into MouseScrollHandler
Attachment #598149 - Flags: review?(jmathies)
The patches seem to be inconsistent in using MouseScrollHandler::GetInstance() vs. MouseScrollHandler::sInstance; is that intentional?
(Assignee)

Comment 27

5 years ago
(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.
(Reporter)

Comment 28

5 years ago
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.
Attachment #598134 - Flags: review?(jmathies) → review+
(Reporter)

Comment 29

5 years ago
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.
Attachment #598136 - Flags: review?(jmathies) → review+
(Reporter)

Comment 30

5 years ago
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.
Attachment #598137 - Flags: review?(jmathies) → review+
(Reporter)

Comment 31

5 years ago
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.
Attachment #598139 - Flags: review?(jmathies) → review+
(Reporter)

Comment 32

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

Comment 33

5 years ago
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.
Attachment #598140 - Flags: review?(jmathies) → review+
(Reporter)

Updated

5 years ago
Attachment #598141 - Flags: review?(jmathies) → review+
(Reporter)

Comment 34

5 years ago
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?
Attachment #598142 - Flags: review?(jmathies) → review-
(Reporter)

Updated

5 years ago
Attachment #598143 - Flags: review?(jmathies) → review+
(Reporter)

Comment 35

5 years ago
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. :)
Attachment #598144 - Flags: review?(jmathies) → review+
(Reporter)

Updated

5 years ago
Attachment #598145 - Flags: review?(jmathies) → review+
(Reporter)

Comment 36

5 years ago
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.
(Reporter)

Updated

5 years ago
Attachment #598146 - Flags: review?(jmathies) → review+
(Reporter)

Updated

5 years ago
Attachment #598147 - Flags: review?(jmathies) → review+
(Reporter)

Comment 37

5 years ago
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' ?
Attachment #598148 - Flags: review?(jmathies) → review+
(Reporter)

Updated

5 years ago
Attachment #598149 - Flags: review?(jmathies) → review+
And MOZ_ASSERT instead of NS_ASSERTION for new assertions, please.
(Assignee)

Comment 39

5 years ago
Created attachment 599526 [details] [diff] [review]
part.1 Implement MouseScrollHandler for Windows
Attachment #598134 - Attachment is obsolete: true
(Assignee)

Comment 40

5 years ago
Created attachment 599527 [details] [diff] [review]
part.1 Implement MouseScrollHandler for Windows
Attachment #599526 - Attachment is obsolete: true
(Assignee)

Comment 41

5 years ago
Created attachment 599528 [details] [diff] [review]
part.2 MouseScrollHandler should manage system settings
Attachment #598136 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 599529 [details] [diff] [review]
part.3 MouseScrollHandler should manage user prefs
Attachment #598137 - Attachment is obsolete: true
(Assignee)

Comment 43

5 years ago
Created attachment 599531 [details] [diff] [review]
part.4 Move HasRegistryKey() in nsWindow.cpp to WinUtils
Attachment #598139 - Attachment is obsolete: true
(Assignee)

Comment 44

5 years ago
Created attachment 599532 [details] [diff] [review]
part.5 Move device specific code to MouseScrollHandler
Attachment #598140 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
Created attachment 599533 [details] [diff] [review]
part.6 Summarize native mouse wheel events by MouseScrollHandler::EventInfo
Attachment #598141 - Attachment is obsolete: true
(Assignee)

Comment 46

5 years ago
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.
Attachment #598142 - Attachment is obsolete: true
Attachment #599535 - Flags: review?(jmathies)
(Assignee)

Comment 47

5 years ago
Created attachment 599536 [details] [diff] [review]
part.8 Compute modifier key state in MouseScrollHandler
Attachment #598143 - Attachment is obsolete: true
(Assignee)

Comment 48

5 years ago
Created attachment 599537 [details] [diff] [review]
part.9 Implement NS_QUERY_SCROLL_TARGET_INFO event dispatcher on MouseScrollHandler::EventInfo
Attachment #598144 - Attachment is obsolete: true
(Assignee)

Comment 49

5 years ago
Created attachment 599538 [details] [diff] [review]
part.10 Initialize mouse scroll events in MouseScrollHandler::LastEventInfo
(Assignee)

Comment 50

5 years ago
Created attachment 599539 [details] [diff] [review]
part.11 Move MOZ_WM_MOUSE*WHEEL handler into MouseScrollHandler
Attachment #598145 - Attachment is obsolete: true
Attachment #598146 - Attachment is obsolete: true
(Assignee)

Comment 51

5 years ago
Created attachment 599540 [details] [diff] [review]
part.12 Move MOZ_WM_*SCROLL handler into MouseScrollHandler
Attachment #598147 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
Created attachment 599541 [details] [diff] [review]
part.13 Compute cursor position at WM_MOUSEWHEEL and WM_MOUSEHWHEEL in MouseScrollHandler
Attachment #598148 - Attachment is obsolete: true
(Assignee)

Comment 53

5 years ago
Created attachment 599542 [details] [diff] [review]
part.14 Move WM_MOUSE*WHEEL and WM_*SCROLL handlers into MouseScrollHandler
Attachment #598149 - Attachment is obsolete: true
(Reporter)

Comment 54

5 years ago
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!
Attachment #599535 - Flags: review?(jmathies) → review+
(Assignee)

Comment 55

5 years ago
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.
Attachment #601531 - Flags: review?(roc)
(Assignee)

Comment 56

5 years ago
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.
Attachment #601532 - Flags: review?(jmathies)
(Assignee)

Comment 57

5 years ago
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
Attachment #601533 - Flags: review?(jmathies)
(Assignee)

Comment 58

5 years ago
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.
Attachment #601534 - Flags: review?(roc)
Attachment #601534 - Flags: review?(jmathies)
(Assignee)

Comment 59

5 years ago
Comment on attachment 601531 [details] [diff] [review]
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent()

Oops, roc is now offline. Smaug, could you review this?
Attachment #601531 - Flags: review?(roc) → review?(bugs)
(Assignee)

Comment 60

5 years ago
Comment on attachment 601534 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

and also this...
Attachment #601534 - Flags: review?(roc) → review?(bugs)
(Assignee)

Comment 61

5 years ago
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.
Whiteboard: [inbound] but don't close this bug until part.15 - part.18 are also landed on m-c
Target Milestone: --- → mozilla13
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
(Reporter)

Updated

5 years ago
Attachment #601533 - Flags: review?(jmathies) → review+
(Reporter)

Comment 63

5 years ago
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.
Attachment #601532 - Flags: review?(jmathies) → review+
(Reporter)

Comment 64

5 years ago
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.
Attachment #601534 - Flags: review?(jmathies) → review+
(Assignee)

Comment 65

5 years ago
(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!
Depends on: 733988

Comment 66

5 years ago
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.
(Assignee)

Comment 68

5 years ago
Created attachment 603991 [details] [diff] [review]
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows
Attachment #601532 - Attachment is obsolete: true
(Assignee)

Comment 69

5 years ago
Created attachment 603992 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler
Attachment #601534 - Attachment is obsolete: true
Attachment #603992 - Flags: review?(bugs)
Attachment #601534 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Whiteboard: [inbound] but don't close this bug until part.15 - part.18 are also landed on m-c → don't close this bug until part.15 - part.18 are also landed on m-c
(Assignee)

Comment 70

5 years ago
Comment on attachment 601531 [details] [diff] [review]
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent()

roc:

do you have time to review this?
Attachment #601531 - Flags: review?(roc)
(Assignee)

Comment 71

5 years ago
Comment on attachment 603992 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

and also this.
Attachment #603992 - Flags: review?(roc)
(Assignee)

Comment 72

5 years ago
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.
(Assignee)

Comment 73

5 years ago
Created attachment 607798 [details] [diff] [review]
part.15 Add new API nsIDOMWindowUtils.sendNativeMouseScrollEvent()

float is replaced with double.
Attachment #601531 - Attachment is obsolete: true
Attachment #607798 - Flags: review?(roc)
Attachment #607798 - Flags: review?(bugs)
Attachment #601531 - Flags: review?(roc)
Attachment #601531 - Flags: review?(bugs)
(Assignee)

Comment 74

5 years ago
Created attachment 607799 [details] [diff] [review]
part.16 Implement nsIWidget::SynthesizeNativeMouseScrollEvent() on Windows

float is replaced with double.
(Assignee)

Comment 75

5 years ago
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.
Attachment #601533 - Attachment is obsolete: true
Attachment #603991 - Attachment is obsolete: true
(Assignee)

Comment 76

5 years ago
Created attachment 607801 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

float is replaced with double.
Attachment #603992 - Attachment is obsolete: true
Attachment #607801 - Flags: review?(roc)
Attachment #607801 - Flags: review?(bugs)
Attachment #603992 - Flags: review?(roc)
Attachment #603992 - Flags: review?(bugs)
Comment on attachment 603992 [details] [diff] [review]
part.18 Make basic tests for MouseScrollHandler

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

rubber-stamp=me
Attachment #603992 - Attachment is obsolete: false
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."
Attachment #607798 - Flags: review?(roc) → review+
Attachment #607801 - Flags: review?(roc) → review+
(Assignee)

Comment 79

5 years ago
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.
Attachment #607798 - Flags: review?(bugs) → superreview?(bugs)
(Assignee)

Updated

5 years ago
Attachment #607801 - Flags: review?(bugs)
Attachment #607798 - Flags: superreview?(bugs) → superreview+
(Assignee)

Comment 80

5 years ago
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
Whiteboard: don't close this bug until part.15 - part.18 are also landed on m-c → [inbound]
Target Milestone: mozilla13 → mozilla14
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Depends on: 739297
(Assignee)

Updated

5 years ago
Blocks: 783450
You need to log in before you can comment on or make changes to this bug.