Bug 89250 – [chromium] Make sure events are transformed correctly for plugins.

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[chromium] Browser Plugin/Pepper: Make mouse events to transform properly in WebPluginContainer.

Created attachment 147912 [details]
Patch

[Robert Kroeger]

Comment on attachment 147912 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:355
> +WebMouseEventBuilder::WebMouseEventBuilder(const Widget* widget, const RenderObject* renderer, const MouseEvent& event)

I suspect that using a RenderObject here is a layering violation and you need to do something different.

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:598
> +    WebMouseEventBuilder webEvent(this, m_element->renderer(), *event);

As I understand how things work, the events should be transformed appropriately by the time they arrive in this object -- so you should look into why this code is not receiving appropriately transformed events.

Comment on attachment 147912 [details]
Patch

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

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:355
>> +WebMouseEventBuilder::WebMouseEventBuilder(const Widget* widget, const RenderObject* renderer, const MouseEvent& event)
> 
> I suspect that using a RenderObject here is a layering violation and you need to do something different.

I tried avoiding renderer(), but couldn't since seems like only it has the transformation info (I tried calling other mapping function in element/widget {similar to absoluteToLocal one below}).
I'm also not sure RenderObject would be always nonNULL at the point when event arrives?

>> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:598
>> +    WebMouseEventBuilder webEvent(this, m_element->renderer(), *event);
> 
> As I understand how things work, the events should be transformed appropriately by the time they arrive in this object -- so you should look into why this code is not receiving appropriately transformed events.

This event handler gets called from WebCore, (HTMLPluginElement.defaultEventHandler), do you think it should be already transformed at that point?
I'm currently looking into HTMLIFrameElement's event handling code path and trying figure out where in the chain the events gets transformed, do you think HTMLPluginElement should follow the same pattern for transforming events?

[Ryosuke Niwa]

Comment on attachment 147912 [details]
Patch

We need a change log here.

[Sadrul Habib Chowdhury]

Created attachment 167335 [details]
Patch

[Sadrul Habib Chowdhury]

I have updated the original patch from lazyboy@ to also properly transform touch and gesture events, done some refactoring, and added a test.

[Ryosuke Niwa]

Comment on attachment 167335 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:412
> +    IntPoint p = view->contentsToWindow(
> +        IntPoint(event.absoluteLocation().x(), event.absoluteLocation().y()));

Nit: This fits in one line. Also, please don't use one-letter variable name like p.

> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:8
> +* 10, 10: Pressed
> +* 10, 10: Pressed
> +* 10, 10: Pressed
> +Plugin received event: TouchMove
> +* 20, 15: Moved
> +* 20, 15: Moved
> +* 20, 15: Moved

It's not obvious at all that these outputs are correct.
Please include js-test-pre.js and use shouldBe(), etc...

[Sadrul Habib Chowdhury]

Created attachment 167383 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 167335 [details]
Patch

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

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:412
>> +        IntPoint(event.absoluteLocation().x(), event.absoluteLocation().y()));
> 
> Nit: This fits in one line. Also, please don't use one-letter variable name like p.

Fixed.

>> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:8
>> +* 20, 15: Moved
> 
> It's not obvious at all that these outputs are correct.
> Please include js-test-pre.js and use shouldBe(), etc...

This output comes form the plugin's printf statements, and from what I can see, those unfortunately cannot be tested from the JS (the other plugin related event-tests also do not use shouldBe)

[Tony Chang]

I think our use of TestWebPlugin when not testing the WebPlugin API is suboptimal.  The test doesn't seem specific to Chromium-- it could also run in Safari or other WebKit browsers.

Instead of using WebPlugin, would we be able to extend the NPAPI plugin in Tools/DumpRenderTree/TestNetscapePlugIn?  This would also make it possible to run these tests in Chromium or content_shell (which we'll want to do eventually anyway).

[Sadrul Habib Chowdhury]

(In reply to comment #10)
> I think our use of TestWebPlugin when not testing the WebPlugin API is suboptimal.  The test doesn't seem specific to Chromium-- it could also run in Safari or other WebKit browsers.
> 
> Instead of using WebPlugin, would we be able to extend the NPAPI plugin in Tools/DumpRenderTree/TestNetscapePlugIn?  This would also make it possible to run these tests in Chromium or content_shell (which we'll want to do eventually anyway).

I think it'd be nice to move a number of the plugin event tests into a general non chromium place, and I can start looking into it. Would it be acceptable to do that in a separate subsequent patch, though?

[Tony Chang]

Comment on attachment 167383 [details]
Patch

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

(In reply to comment #11)
> I think it'd be nice to move a number of the plugin event tests into a general non chromium place, and I can start looking into it. Would it be acceptable to do that in a separate subsequent patch, though?

Sure, although the code for the NPAPI plugin is already there (well, it doesn't know about touch events and will probably just report them as mouse move events, but that doesn't seem like a big deal).

> Source/WebKit/chromium/ChangeLog:11
> +        space of the page containing the plugin. Before the events are
> +        dispatched to the plugin, it is necessary to convert them into the
> +        plugin's own coordinate system.

The bug is only for touch events and gesture events, right?  You're just refactoring the code already used by mousemove and scroll wheel events, right?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:42
> +#include "RenderObject.h"

This is OK because it's not exposed to the public API.  There are other files in Source/WebKit/chromium/src that use RenderObject.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:405
> +static void updateWebMouseEventFromWebCoreMouseEvent(WebMouseEvent* webEvent, const MouseEvent& event, const Widget* widget, const WebCore::RenderObject* renderObject)

Normally out params come last.  WebKit style also normally uses pass by reference for out params.  Actually, why don't you just make this a protected member function so you don't have to pass |webEvent|?

> Source/WebKit/chromium/src/WebInputEventConversion.h:102
> +// Converts a WebCore::MouseEvent to a corresponding WebMouseEvent. widget and
> +// renderObject are the Widget and RenderObject corresponding to the event.

I would just remove the second sentence.  It doesn't tell you anything that the code doesn't already tell you.

> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:4
> +* 10, 10: Pressed
> +* 10, 10: Pressed
> +* 10, 10: Pressed

Why does it print the point 3 times?

> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:8
> +* 20, 15: Moved
> +* 20, 15: Moved
> +* 20, 15: Moved

Why does it print the point 3 times?

> LayoutTests/platform/chromium/plugins/transformed-events.html:17
> +        document.write("This test does not work in manual mode.");

I'm not sure what manual mode is.  I would say, "This test requires DumpRenderTree."

[Sadrul Habib Chowdhury]

Comment on attachment 167383 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:11
>> +        plugin's own coordinate system.
> 
> The bug is only for touch events and gesture events, right?  You're just refactoring the code already used by mousemove and scroll wheel events, right?

No. The fix is also for mouse and wheel events.

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:405
>> +static void updateWebMouseEventFromWebCoreMouseEvent(WebMouseEvent* webEvent, const MouseEvent& event, const Widget* widget, const WebCore::RenderObject* renderObject)
> 
> Normally out params come last.  WebKit style also normally uses pass by reference for out params.  Actually, why don't you just make this a protected member function so you don't have to pass |webEvent|?

This is currently called from WebMouseEventBuilder, and WebMouseWheelEventBuilder (one is not a subclass of the other). So it would have to be added to both, and we would end up with code duplication again.

So I have kept this here, but I have updated to pass by reference, and move the out param last.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:416
> +    IntPoint localPoint = convertLocationForRenderObject(event.absoluteLocation(), renderObject);

This is the fix for mouse and wheel events.

>> Source/WebKit/chromium/src/WebInputEventConversion.h:102
>> +// renderObject are the Widget and RenderObject corresponding to the event.
> 
> I would just remove the second sentence.  It doesn't tell you anything that the code doesn't already tell you.

Done.

>> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:4
>> +* 10, 10: Pressed
> 
> Why does it print the point 3 times?

The touch-event details printout is a bit ... awkward. Each touch-event maintains three lists of touch-points ('active', 'changed' and 'target'). The details prints out the details about each finger in each list. In this case, all three lists contain a single touch-point. So it prints it out three times.

>> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:8
>> +* 20, 15: Moved
> 
> Why does it print the point 3 times?

Ditto.

>> LayoutTests/platform/chromium/plugins/transformed-events.html:17
>> +        document.write("This test does not work in manual mode.");
> 
> I'm not sure what manual mode is.  I would say, "This test requires DumpRenderTree."

Done.

[Sadrul Habib Chowdhury]

Created attachment 167421 [details]
Patch

[Tony Chang]

Comment on attachment 167421 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:405
> +static void updateWebMouseEventFromWebCoreMouseEvent(const MouseEvent& event, const Widget& widget, const WebCore::RenderObject& renderObject, WebMouseEvent& webEvent)

Nit: I would have kept Widget and RenderObject as pointers, but this is OK.

> LayoutTests/platform/chromium/plugins/transformed-events.html:40
> +        // Test gesture events.
> +        eventSender.gestureTapDown(positionX, positionY);
> +        eventSender.gestureTap(positionX, positionY);

Can you add tests for scroll events too?  You said they're also not translating the points properly?

[Tony Chang]

Comment on attachment 167421 [details]
Patch

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

> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:4
> +* 10, 10: Pressed
> +* 10, 10: Pressed
> +* 10, 10: Pressed

You could explain why there are three outputs perhaps in the ChangeLog or in the html file.

[Sadrul Habib Chowdhury]

Comment on attachment 167421 [details]
Patch

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

>> LayoutTests/platform/chromium/plugins/transformed-events-expected.txt:4
>> +* 10, 10: Pressed
> 
> You could explain why there are three outputs perhaps in the ChangeLog or in the html file.

Fixed (explained in ChangeLog)

>> LayoutTests/platform/chromium/plugins/transformed-events.html:40
>> +        eventSender.gestureTap(positionX, positionY);
> 
> Can you add tests for scroll events too?  You said they're also not translating the points properly?

Done.

[Sadrul Habib Chowdhury]

Created attachment 167652 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167652 [details]
Patch for landing

Clearing flags on attachment: 167652

Committed r130765: <http://trac.webkit.org/changeset/130765>