Bug 146142 - Make PluginProxy::handleMouseEvent() asynchronous
Summary: Make PluginProxy::handleMouseEvent() asynchronous
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sungmann Cho
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-18 22:59 PDT by Sungmann Cho
Modified: 2015-07-21 15:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.76 KB, patch)
2015-06-18 23:02 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff
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 Details
Patch (7.62 KB, patch)
2015-06-23 06:47 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff
Patch (7.10 KB, patch)
2015-07-21 11:09 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sungmann Cho 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.
Comment 1 Sungmann Cho 2015-06-18 23:02:08 PDT
Created attachment 255177 [details]
Patch
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Sungmann Cho 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
Comment 5 Alexey Proskuryakov 2015-06-22 21:31:19 PDT
EWS uses expected results from platform/mac-mavericks.
Comment 6 Alexey Proskuryakov 2015-06-22 21:32:13 PDT
Or from platform/mac if there are no Mavericks specific results.
Comment 7 Sungmann Cho 2015-06-23 06:47:30 PDT
Created attachment 255410 [details]
Patch
Comment 8 Sungmann Cho 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.
Comment 9 Darin Adler 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.
Comment 10 Anders Carlsson 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.
Comment 11 Anders Carlsson 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.
Comment 12 Sungmann Cho 2015-07-21 11:09:46 PDT
Created attachment 257187 [details]
Patch
Comment 13 Sungmann Cho 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?
Comment 14 Anders Carlsson 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.
Comment 15 WebKit Commit Bot 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>
Comment 16 WebKit Commit Bot 2015-07-21 12:53:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Sungmann Cho 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.