Bug 127921 – MouseButton in PlatformMouseEvent should be forward-declarable

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Blaze Burg]

Because forward declarations are cool, y'know.

[Blaze Burg]

Created attachment 226973 [details]
WIP

[Blaze Burg]

Created attachment 226993 [details]
the patch

[WebKit Commit Bot]

Attachment 226993 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/MouseEvent.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/MouseEvent.cpp:126:  Wrong number of spaces before statement. (expected: 27)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:271:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 226996 [details]
the patch.2

[WebKit Commit Bot]

Attachment 226996 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:128:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/MouseEvent.cpp:136:  Wrong number of spaces before statement. (expected: 27)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:281:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 226996 [details]
the patch.2

Attachment 226996 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6354463874875392

New failing tests:
fast/events/init-events.html

[Build Bot]

Created attachment 227001 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226996 [details]
the patch.2

Attachment 226996 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4785806258470912

New failing tests:
fast/events/init-events.html

[Build Bot]

Created attachment 227008 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Blaze Burg]

Created attachment 227106 [details]
the patch.3

[WebKit Commit Bot]

Attachment 227106 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/MouseEvent.cpp:125:  Wrong number of spaces before statement. (expected: 27)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:126:  Wrong number of spaces before statement. (expected: 27)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MouseEvent.cpp:290:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Latest patch takes a more compatibility-minded approach. I think it's ready for review.

[Alexey Proskuryakov]

Comment on attachment 227106 [details]
the patch.3

View in context: https://bugs.webkit.org/attachment.cgi?id=227106&action=review

> Source/WebCore/ChangeLog:18
> +        No new tests. This is a refactoring.

I find the new approach more confusing than the old one.

We can't have any MouseEvent function return a value that's limited to values 0..2, because it is legitimate to have more buttons. So, there is no need to forward declare MouseButton - we only need its values, not the type itself.

It is indeed confusing that MouseEvent.button values are not the same as MouseEventInit.button values, or PlatformMouseEvent.button values. Can we fix the latter two by adding a boolean for whether a button was pressed, and removing -1/NoButton entirely?

> Source/WebCore/dom/MouseEvent.cpp:201
>      // For the Netscape "which" property, the return values for left, middle and right mouse buttons are 1, 2, 3, respectively. 
>      // So we must add 1.
> -    if (!m_buttonDown)
> +    if (!m_buttonInitialized)
>          return 0;
> +
>      return m_button + 1;

This code is incorrect (or at least incompatible with Firefox). In "no button is pressed" case, Firefox returns 1, not 0.

> Source/WebCore/dom/MouseEvent.h:125
> +    bool m_buttonInitialized;

This new name is very confusing. What does it mean for button to not be initialized?

I know that initMouseEvent function does not provide all the necessary information to initialize a MouseEvent, because it doesn't differentiate between no button and left button being pressed. But that doesn't seem to be it, and besides, that's more "ambiguous" than "uninitialized".

Note that there is also a MouseEvent.buttons property that we don't implement, and that lets one actually differentiate between left button and no button. This property is always 0 for events created with initMouseEvent, even if a non-0 button was passed to this function.

I think that implementing the buttons property would be the best way to clean up our code here.

> Source/WebCore/dom/WheelEvent.cpp:73
> -                , event.ctrlKey(), event.altKey(), event.shiftKey(), event.metaKey(), 0, 0, 0, false)
> +                , event.ctrlKey(), event.altKey(), event.shiftKey(), event.metaKey(), NoButton, 0, 0, false)

This is a behavior change that can be verified with a regression test - we'll get a different value of MouseEvent.which (but per another comment, that's a bug in MouseEvent.which implementation).

I'm curious why we pass 0 here, and not the actual mouse buttons state from PlatformWheelEvent. Can we have wheel events when mouse buttons are down?

> Source/WebCore/platform/PlatformMouseEvent.h:51
> +        NoButton = 3

The value of 3 can be easily confused with 4th button on a mouse, or with a number 3 passed to initMouseEvent. I think that we should find a way to not have a "no button" value at all.

[Alexey Proskuryakov]

> I find the new approach more confusing than the old one.

Where by "old one" I meant the code in trunk - I didn't look at the first patch posted to the bug.

[Blaze Burg]

Comment on attachment 227106 [details]
the patch.3

View in context: https://bugs.webkit.org/attachment.cgi?id=227106&action=review

>> Source/WebCore/ChangeLog:18
>> +        No new tests. This is a refactoring.
> 
> I find the new approach more confusing than the old one.
> 
> We can't have any MouseEvent function return a value that's limited to values 0..2, because it is legitimate to have more buttons. So, there is no need to forward declare MouseButton - we only need its values, not the type itself.
> 
> It is indeed confusing that MouseEvent.button values are not the same as MouseEventInit.button values, or PlatformMouseEvent.button values. Can we fix the latter two by adding a boolean for whether a button was pressed, and removing -1/NoButton entirely?

First, the reason why I stuck my finger in this butter: forward-declaring MouseButton (or whatever enum PlatformMouseEvent uses) is necessary for replay code to no be a burden on build times. If it's not forward declared we'll have to drag in PlatformMouseEvent.h into every cpp file that has use of replay inputs, or revert to blindly casting between shorts and enums. If it's possible to make the code simpler when enabling that, then great! But because of MSVC's inability to directly compare ushorts with enum MouseButton: ushort, I was compelled to make things more type safe (sadly, this usually means less simple in C++).

As far as I can tell from reading all WebCore uses of MouseButton, the are no uses of values other than none, left, right, middle internally. On every port but EFL, the PlatformMouseEvent factory methods explicitly switch on the native values and then pick one of the four enum values.
The event constructors can set arbitrary button values on MouseEvent (and this patch goes to annoying lengths to preserve that). But for internal uses, why permit more values than are actually used?

I agree that we could get rid of NoButton by adding a flag, but unless I am mistaken this won't simplify the code.

>> Source/WebCore/dom/MouseEvent.cpp:201
>>      return m_button + 1;
> 
> This code is incorrect (or at least incompatible with Firefox). In "no button is pressed" case, Firefox returns 1, not 0.

There is no specification so I prefer to not change the behavior. But if you insist...

>> Source/WebCore/dom/MouseEvent.h:125
>> +    bool m_buttonInitialized;
> 
> This new name is very confusing. What does it mean for button to not be initialized?
> 
> I know that initMouseEvent function does not provide all the necessary information to initialize a MouseEvent, because it doesn't differentiate between no button and left button being pressed. But that doesn't seem to be it, and besides, that's more "ambiguous" than "uninitialized".
> 
> Note that there is also a MouseEvent.buttons property that we don't implement, and that lets one actually differentiate between left button and no button. This property is always 0 for events created with initMouseEvent, even if a non-0 button was passed to this function.
> 
> I think that implementing the buttons property would be the best way to clean up our code here.

The uninitialized/initialized name comes directly from the specification of MouseEvent.button. IMO, m_buttonDown is misleading, because we don't actually know if the button is down when initMouseEvent passes the value '0'.

>> Source/WebCore/dom/WheelEvent.cpp:73
>> +                , event.ctrlKey(), event.altKey(), event.shiftKey(), event.metaKey(), NoButton, 0, 0, false)
> 
> This is a behavior change that can be verified with a regression test - we'll get a different value of MouseEvent.which (but per another comment, that's a bug in MouseEvent.which implementation).
> 
> I'm curious why we pass 0 here, and not the actual mouse buttons state from PlatformWheelEvent. Can we have wheel events when mouse buttons are down?

Oops, it should be LeftButton.

>> Source/WebCore/platform/PlatformMouseEvent.h:51
>> +        NoButton = 3
> 
> The value of 3 can be easily confused with 4th button on a mouse, or with a number 3 passed to initMouseEvent. I think that we should find a way to not have a "no button" value at all.

NoButton = 3 is never exposed to the web, so I'm not sure what the issue is. We can easily change NoButton to 6 or 99 later if/when MouseEvent.buttons is implemented and ports start populating PlatformMouseEvent with buttons other than none,left,middle,right. And because internal checks of the button now use enum values, there shouldn't be any accidental comparison with the raw value '3'.

[Alexey Proskuryakov]

> First, the reason why I stuck my finger in this butter: forward-declaring MouseButton (or whatever enum PlatformMouseEvent uses) is necessary for replay code to no be a burden on build times.

Replaying should almost certainly work in terms of PlatformMouseEvents, not MouseEvents. But if it works with MouseEvents, it should just use an unsigned, not artificially limit the whole range to what WebCore has special handling for internally.

> > This code is incorrect (or at least incompatible with Firefox). In "no button is pressed" case, Firefox returns 1, not 0.
>
>There is no specification so I prefer to not change the behavior. But if you insist...

We definitely shouldn't change the behavior as part of refactoring, but adding a FIXME stating that our behavior is not compatible with Firefox - which de facto defines this property with its historic behavior - would be appropriate.

> NoButton = 3 is never exposed to the web, so I'm not sure what the issue is.

Consider what happens when 3 is passed to initMouseEvent - you get a different behavior than when 4 is passed, which is obviously incorrect.

Again, I think that any substantial refactoring in this area should implement event.buttons, as otherwise we are nearly guaranteed to move in the wrong direction.

[Blaze Burg]

I landed a MSVC-only workaround to enable forward-declaring rather than spend more effort on a real refactoring here. Going to close this bug unless someone has a better name for the refactoring proposed by Alexei.