[webkit-reviews] review denied: [Bug 127921] MouseButton in PlatformMouseEvent should be forward-declarable : [Attachment 227106] the patch.3

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Brian Burg <bburg at apple.com>'s
request for review:
Bug 127921: MouseButton in PlatformMouseEvent should be forward-declarable
https://bugs.webkit.org/show_bug.cgi?id=127921

Attachment 227106: the patch.3
https://bugs.webkit.org/attachment.cgi?id=227106&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.