[webkit-reviews] review granted: [Bug 66492] [chromium] Separate platform gesture events from synthetic gestures : [Attachment 105205] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Robert Kroeger
<rjkroege at chromium.org>'s request for review:
Bug 66492: [chromium] Separate platform gesture events from synthetic gestures
https://bugs.webkit.org/show_bug.cgi?id=66492

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105205&action=review


Look good!

> Source/WebCore/page/EventHandler.cpp:2235
> +	   PlatformMouseEvent
fakeMouseMove(roundedIntPoint(synthetic.position()),
roundedIntPoint(synthetic.globalPosition()), NoButton, MouseEventMoved, /*
clickCount */ 1, synthetic.shiftKey(), synthetic.ctrlKey(), synthetic.altKey(),
synthetic.metaKey(), synthetic.timestamp());
> +	   PlatformMouseEvent
fakeMouseDown(roundedIntPoint(synthetic.position()),
roundedIntPoint(synthetic.globalPosition()), LeftButton, MouseEventPressed, /*
clickCount */ 1, synthetic.shiftKey(), synthetic.ctrlKey(), synthetic.altKey(),
synthetic.metaKey(), synthetic.timestamp());
> +	   PlatformMouseEvent
fakeMouseUp(roundedIntPoint(synthetic.position()),
roundedIntPoint(synthetic.globalPosition()), LeftButton, MouseEventReleased, /*
clickCount */ 1, synthetic.shiftKey(), synthetic.ctrlKey(), synthetic.altKey(),
synthetic.metaKey(), synthetic.timestamp());

It would be a bit cleaner to have local variables for
roundedIntPoint(synthetic.position()) and
roundedIntPoint(synthetic.globalPosition()) instead of repeating the call.

> Source/WebCore/page/EventHandler.cpp:2249
> +	   // FIXME: Implement additional synthetic gestures.

This comment does not convey enough information in my opinion. You should
mention examples of what is missing (or not have the comment).