[webkit-reviews] review denied: [Bug 89250] [chromium] Make sure events are transformed correctly for plugins. : [Attachment 167383] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied sadrul at chromium.org's request for
review:
Bug 89250: [chromium] Make sure events are transformed correctly for plugins.
https://bugs.webkit.org/show_bug.cgi?id=89250

Attachment 167383: Patch
https://bugs.webkit.org/attachment.cgi?id=167383&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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."