| Summary: | Make PluginProxy::handleMouseEvent() asynchronous | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sungmann Cho <sungmann.cho> | ||||||||||
| Component: | WebKit2 | Assignee: | Sungmann Cho <sungmann.cho> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | andersca, buildbot, commit-queue, darin, rniwa, sam | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 255177 [details]
Patch
Comment on attachment 255177 [details] Patch Attachment 255177 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5146996457340928 New failing tests: plugins/mouse-events.html Created attachment 255179 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Hmm... plugins/mouse-events.html failed. But I'm just wondering why the expectation file in EWS differ from the one in ToT? // plugins/mouse-events-expected.txt in EWS (taken from the above result attachment) Test for bug 11517: Flash clicks/interactivity not working properly. PLUGIN: mouseDown at (12, 12) PLUGIN: mouseUp at (12, 12) PLUGIN: mouseDown at (22, 22) PLUGIN: mouseUp at (32, 22) PLUGIN: getFocusEvent PASS successfullyParsed is true TEST COMPLETE // plugins/mouse-events-expected.txt in ToT (https://trac.webkit.org/browser/trunk/LayoutTests/plugins/mouse-events-expected.txt) Test for bug 11517: Flash clicks/interactivity not working properly. PLUGIN: getFocusEvent PLUGIN: mouseDown at (12, 12) PLUGIN: mouseUp at (12, 12) PLUGIN: mouseDown at (22, 22) PLUGIN: mouseUp at (32, 22) PASS successfullyParsed is true TEST COMPLETE EWS uses expected results from platform/mac-mavericks. Or from platform/mac if there are no Mavericks specific results. Created attachment 255410 [details]
Patch
@Alexey Proskuryakov: Thank you for pointing that out! platform/mac-wk2/plugins/mouse-events-expected.txt was introduced by webkit.org/b/116665 to avoid flakey tests. I think from now on we can share the common expectations. Anders, what do you think about this patch? It seems OK to me but I am not sure. (In reply to comment #9) > Anders, what do you think about this patch? It seems OK to me but I am not > sure. I think it's a good patch. I'm going to review it. Comment on attachment 255410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255410&action=review Patch looks good overall, but I'd like to see a new patch without the comment. > Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp:374 > + // Always let the web process think that we've handled this mouse event, even before the plug-in handles it. > + // This is a workaround for <rdar://problem/9299901>. If the web process send a HandleMouseEvent synchronously > + // like the other mouse events, the plug-in process spawns a nested run loop when showing a context menu, > + // so eventually the unresponsiveness timer kicks in in the UI process. > + // > + // FIXME: We should come up with a better way to do this. I don't think this comment here is needed anymore. Created attachment 257187 [details]
Patch
(In reply to comment #11) > Comment on attachment 255410 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255410&action=review > > Patch looks good overall, but I'd like to see a new patch without the > comment. > > > Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp:374 > > + // Always let the web process think that we've handled this mouse event, even before the plug-in handles it. > > + // This is a workaround for <rdar://problem/9299901>. If the web process send a HandleMouseEvent synchronously > > + // like the other mouse events, the plug-in process spawns a nested run loop when showing a context menu, > > + // so eventually the unresponsiveness timer kicks in in the UI process. > > + // > > + // FIXME: We should come up with a better way to do this. > > I don't think this comment here is needed anymore. Thanks Anders. I removed the comment. Would you please take a look again? Comment on attachment 257187 [details]
Patch
We can probably do the same thing for the mouseEnter/mouseLeave events too.
Comment on attachment 257187 [details] Patch Clearing flags on attachment: 257187 Committed r187114: <http://trac.webkit.org/changeset/187114> All reviewed patches have been landed. Closing bug. (In reply to comment #14) > Comment on attachment 257187 [details] > Patch > > We can probably do the same thing for the mouseEnter/mouseLeave events too. OK. I'll investigate it. Thanks. |
PluginProxy::handleMouseEvent() forwards the generated mouse event to PluginControllerProxy using a synchronous message, but the recipient always reply immediately with the same value("true") even before handling the received message. So I think PluginProxy::handleMouseEvent() is perfectly OK to process its messages asynchronously.