Bug 146142 – Make PluginProxy::handleMouseEvent() asynchronous

• Component: WebKit2
• Status: RESOLVED FIXED

[Sungmann Cho]

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.

[Sungmann Cho]

Created attachment 255177 [details]
Patch

[Build Bot]

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]

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]

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]

EWS uses expected results from platform/mac-mavericks.

[Alexey Proskuryakov]

Or from platform/mac if there are no Mavericks specific results.

[Sungmann Cho]

Created attachment 255410 [details]
Patch

[Sungmann Cho]

@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]

Anders, what do you think about this patch? It seems OK to me but I am not sure.

[Anders Carlsson]

(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 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]

Created attachment 257187 [details]
Patch

[Sungmann Cho]

(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 on attachment 257187 [details]
Patch

We can probably do the same thing for the mouseEnter/mouseLeave events too.

[WebKit Commit Bot]

Comment on attachment 257187 [details]
Patch

Clearing flags on attachment: 257187

Committed r187114: <http://trac.webkit.org/changeset/187114>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sungmann Cho]

(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.