RESOLVED FIXED 146142
Make PluginProxy::handleMouseEvent() asynchronous
https://bugs.webkit.org/show_bug.cgi?id=146142
Summary Make PluginProxy::handleMouseEvent() asynchronous
Sungmann Cho
Reported 2015-06-18 22:59:41 PDT
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.
Attachments
Patch (5.76 KB, patch)
2015-06-18 23:02 PDT, Sungmann Cho
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (570.29 KB, application/zip)
2015-06-18 23:38 PDT, Build Bot
no flags
Patch (7.62 KB, patch)
2015-06-23 06:47 PDT, Sungmann Cho
no flags
Patch (7.10 KB, patch)
2015-07-21 11:09 PDT, Sungmann Cho
no flags
Sungmann Cho
Comment 1 2015-06-18 23:02:08 PDT
Build Bot
Comment 2 2015-06-18 23:38:50 PDT
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
Build Bot
Comment 3 2015-06-18 23:38:52 PDT
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
Sungmann Cho
Comment 4 2015-06-19 17:43:24 PDT
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
Alexey Proskuryakov
Comment 5 2015-06-22 21:31:19 PDT
EWS uses expected results from platform/mac-mavericks.
Alexey Proskuryakov
Comment 6 2015-06-22 21:32:13 PDT
Or from platform/mac if there are no Mavericks specific results.
Sungmann Cho
Comment 7 2015-06-23 06:47:30 PDT
Sungmann Cho
Comment 8 2015-06-23 06:51:22 PDT
@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.
Darin Adler
Comment 9 2015-07-20 18:31:00 PDT
Anders, what do you think about this patch? It seems OK to me but I am not sure.
Anders Carlsson
Comment 10 2015-07-21 10:38:38 PDT
(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.
Anders Carlsson
Comment 11 2015-07-21 10:39:41 PDT
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.
Sungmann Cho
Comment 12 2015-07-21 11:09:46 PDT
Sungmann Cho
Comment 13 2015-07-21 11:20:09 PDT
(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?
Anders Carlsson
Comment 14 2015-07-21 11:35:44 PDT
Comment on attachment 257187 [details] Patch We can probably do the same thing for the mouseEnter/mouseLeave events too.
WebKit Commit Bot
Comment 15 2015-07-21 12:53:47 PDT
Comment on attachment 257187 [details] Patch Clearing flags on attachment: 257187 Committed r187114: <http://trac.webkit.org/changeset/187114>
WebKit Commit Bot
Comment 16 2015-07-21 12:53:53 PDT
All reviewed patches have been landed. Closing bug.
Sungmann Cho
Comment 17 2015-07-21 15:50:55 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.