[webkit-reviews] review denied: [Bug 57904] onClick does not function with <select> elements in WebKit2 : [Attachment 88727] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Jon Lee <jonlee at apple.com>'s
request for review:
Bug 57904: onClick does not function with <select> elements in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=57904

Attachment 88727: Patch
https://bugs.webkit.org/attachment.cgi?id=88727&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88727&action=review

> Source/WebKit2/UIProcess/API/mac/WKView.mm:946
> +- (void)Selector:(NSEvent *)theEvent \
> +{ \
> +NativeWebMouseEvent webEvent(theEvent, self); \
> +_data->_page->handleMouseEvent(webEvent); \
> +}

I think we indent the body of multi-line macros.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:782
> +    // (bug #57904) we need to keep track of the mouse down event in the
case where we

Please use full URLs when linking to bugs, that makes it easier to open them.
Also use a capital letter in the beginning of the sentence.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:785
> +    // when the mouse up message is received from WebProcess

Period at the end :)

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2146
> +NativeWebMouseEvent* WebPageProxy::currentMouseDownEvent()

Maybe this should be called "currentlyProcessedMouseDownEvent" or something
along those lines, to indicate that this is the event that's been sent to the
web process.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:131
> +	   // 57904: This code is adopted from
EventHandler::sendFakeEventsAfterWidgetTracking()

Bug URL.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:135
> +	       NSEvent *fakeEvent = nil;

No need to initialize fakeEvent to nil. You can just initialize it with the
created event.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:148
> +					     
location:[[m_client->currentMouseDownEvent()->nativeView() window]
convertScreenToBase:[NSEvent mouseLocation]]

Do we really need to store the native view in the event? Can't we just get the
window from the WKView?

> Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp:334
> +	   const MSG* initiatingWinEvent =
m_client->currentMouseDownEvent()->nativeEvent();
> +	   MSG fakeEvent = *initiatingWinEvent;
> +	   fakeEvent.message = WM_LBUTTONUP;
> +	   ::DispatchMessage(&fakeEvent);

This could use a comment. Also, is DispatchMessage the right event here or
should we use ::PostMessage instead?