Bug 57904 – onClick does not function with <select> elements in WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Jon Lee]

<rdar://problem/9165095>

[Jon Lee]

Created attachment 88727 [details]
Patch

[Anders Carlsson]

Comment on attachment 88727 [details]
Patch

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?

[Jon Lee]

Created attachment 88890 [details]
Patch

Made adjustments based on anderca's suggestions.

[Jon Lee]

Created attachment 89021 [details]
Patch

Fixing windows build error

[Anders Carlsson]

Comment on attachment 89021 [details]
Patch

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

The indentation in WebPopupMenuProxyMac.mm is messed up.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:127
> +        if (m_client)

This could be an early return.

> Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:132
> +            NSEvent* initiatingNSEvent = m_client->currentlyProcessedMouseDownEvent()->nativeEvent();

I think you need to null check the event before getting the native event.

> Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp:328
> +    if (m_client) {

Early return.

> Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp:339
> +        const MSG* initiatingWinEvent = m_client->currentlyProcessedMouseDownEvent()->nativeEvent();

Null check.

[Anders Carlsson]

Comment on attachment 89021 [details]
Patch

Whoops, I meant to r- this.

[Jon Lee]

Created attachment 89078 [details]
Patch

early returns added, format fixed in ....Mac.mm

[Anders Carlsson]

Comment on attachment 89078 [details]
Patch

There are a lot of unneeded whitespace changes in WebPopupMenuProxyMac.mm

Also, I think the currently processed mouse down event needs to be cleared out if the web process crashes (you can do this in WebPageProxy::processDidCrash()

[Jon Lee]

Created attachment 89107 [details]
Patch

Clearing out cached events, including mouse down (relevant to this bug), and also mouse move and wheel.

[WebKit Commit Bot]

Comment on attachment 89107 [details]
Patch

Rejecting attachment 89107 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
1 line).
patching file Source/WebKit2/UIProcess/WebPopupMenuProxy.h
patching file Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm
patching file Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp
patching file Source/WebKit2/UIProcess/win/WebView.cpp
patching file Source/WebKit2/WebKit2.xcodeproj/project.pbxproj
patching file Source/WebKit2/win/WebKit2.vcproj

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Anders Carlsson', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8404382

[Jon Lee]

Created attachment 89474 [details]
Patch

Trying again-- same changes, on top of newer code.

[WebKit Review Bot]

Comment on attachment 89474 [details]
Patch

Rejecting attachment 89474 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 89474 [details]
Patch

Rejecting attachment 89474 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 89474 [details]
Patch

Rejecting attachment 89474 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
eProxy.h
patching file Source/WebKit2/UIProcess/WebPopupMenuProxy.h
patching file Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm
patching file Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp
patching file Source/WebKit2/UIProcess/win/WebView.cpp
patching file Source/WebKit2/WebKit2.xcodeproj/project.pbxproj
patching file Source/WebKit2/win/WebKit2.vcproj

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Anders Carlsson', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8403570

[Jon Lee]

Created attachment 89670 [details]
Patch

[Jon Lee]

Created attachment 89685 [details]
Patch

Trying to fold changes into Qt.

[Jon Lee]

Created attachment 89695 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 89695 [details]
Patch

Signing off on the updates since the last version.

[Maciej Stachowiak]

Comment on attachment 89695 [details]
Patch

Signing off on the updates since the last version.

[Jon Lee]

Created attachment 89721 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 89721 [details]
Patch

Rejecting attachment 89721 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-a..." exit_code: 2

Last 500 characters of output:
World.cpp
	M	Source/WebCore/html/HTMLCanvasElement.cpp
r83990 = 52d4a4ddb5bdfde702f42fbe11d1a2d0501d21b8 (refs/remotes/trunk)
	M	Source/WebCore/WebCore.exp.in
	M	Source/WebCore/dom/DocumentMarkerController.h
	M	Source/WebCore/dom/DocumentMarkerController.cpp
	M	Source/WebCore/dom/DocumentMarker.h
	M	Source/WebCore/ChangeLog
r83991 = 3e9aec703b851aa84338cad59d52e2be93b0c66e (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8457195

[WebKit Review Bot]

Comment on attachment 89107 [details]
Patch

Cleared Anders Carlsson's review+ from obsolete attachment 89107 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Jon Lee]

The patch was not landing for some reason, so Anders landed it for me, but the bug never got closed.  Setting resolution to resolved/fixed.