Bug 109370 - [chromium] Apply page scale to all WebInputEvent types
Summary: [chromium] Apply page scale to all WebInputEvent types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandre Elias
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-09 21:26 PST by Alexandre Elias
Modified: 2013-02-11 20:24 PST (History)
9 users (show)

See Also:


Attachments
Patch (8.96 KB, patch)
2013-02-09 21:31 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (16.01 KB, patch)
2013-02-10 19:20 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (28.57 KB, patch)
2013-02-11 17:52 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (28.66 KB, patch)
2013-02-11 18:11 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2013-02-09 21:26:55 PST
[chromium] Apply page scale to all WebInputEvent types
Comment 1 Alexandre Elias 2013-02-09 21:31:43 PST
Created attachment 187461 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-09 21:38:24 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2013-02-09 23:33:14 PST
Comment on attachment 187461 [details]
Patch

What about doing these conversions in WebInputEventConversions.cpp?  It already deals with the general problem of turning a WebInputEvent into something that WebCore understands
Comment 4 WebKit Review Bot 2013-02-10 06:28:35 PST
Comment on attachment 187461 [details]
Patch

Attachment 187461 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16470776

New failing tests:
fast/frames/frame-set-scaling-hit.html
fast/events/touch/gesture/touch-gesture-scroll-div-scaled.html
fast/frames/frame-set-rotation-hit.html
fast/events/page-scaled-mouse-click-iframe.html
fast/events/page-scaled-mouse-click.html
fast/events/touch/page-scaled-touch-gesture-click.html
platform/chromium/plugins/transformed-events.html
fast/events/touch/touch-scaled-scrolled.html
Comment 5 Alexandre Elias 2013-02-10 19:20:29 PST
Created attachment 187504 [details]
Patch

Move everything to WebInputEventConversions.cpp
Comment 6 Alexandre Elias 2013-02-10 19:21:45 PST
Good idea, it's much cleaner this way.  I also did the reverse conversion in the PlatformEvent -> WebInputEvent builders.
Comment 7 James Robinson 2013-02-10 19:27:42 PST
Comment on attachment 187504 [details]
Patch

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

It seems pretty easy to miss a scale or run a scale backwards through here without noticing.  Can you add some cases to WebInputEventConversionTest.cpp to make sure we don't accidentally lose or invert any of the scales you've added?

Why are webkitMovementX/Y unscaled in WebMouseEventBuilder?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:436
>      webEvent.globalX = event.screenX();
>      webEvent.globalY = event.screenY();

why are these not scaled?
Comment 8 WebKit Review Bot 2013-02-10 20:05:37 PST
Comment on attachment 187504 [details]
Patch

Attachment 187504 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16492050

New failing tests:
platform/chromium/plugins/transformed-events.html
Comment 9 Alexandre Elias 2013-02-11 17:52:21 PST
Created attachment 187736 [details]
Patch

Add unit test; add missing reverse-mouse scales; change widget->parent() to widget->root() for robustness; fix disambiguation scaling; revert int->float conversion since plugins reinterpret the bytes as ints
Comment 10 Alexandre Elias 2013-02-11 17:55:09 PST
OK, this should be ready for landing.

> Why are webkitMovementX/Y unscaled in WebMouseEventBuilder?
> 

That was a mistake; I'm now scaling them.

> > Source/WebKit/chromium/src/WebInputEventConversion.cpp:436
> >      webEvent.globalX = event.screenX();
> >      webEvent.globalY = event.screenY();
> 
> why are these not scaled?

This one is intentional.  According to comments, these refer to location relative to the top-left corner of the physical screen.  It doesn't make sense to apply page scale to this.
Comment 11 WebKit Review Bot 2013-02-11 18:00:35 PST
Comment on attachment 187736 [details]
Patch

Attachment 187736 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16497497
Comment 12 James Robinson 2013-02-11 18:04:44 PST
Comment on attachment 187736 [details]
Patch

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

R=me, one question for your consideration

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:178
> +        m_area = IntSize(e.data.tap.width / scale, e.data.tap.height / scale);

do we want to use an explicit conversion here so we know what sort of rounding we're getting?  Maybe go to a FloatSize first and then use enclosing/enclosed/whateverIntSize?
Comment 13 James Robinson 2013-02-11 18:05:00 PST
(In reply to comment #11)
> (From update of attachment 187736 [details])
> Attachment 187736 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16497497

Looks like you need some 'u' suffixes
Comment 14 WebKit Review Bot 2013-02-11 18:06:40 PST
Comment on attachment 187736 [details]
Patch

Attachment 187736 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16501462
Comment 15 Alexandre Elias 2013-02-11 18:11:23 PST
Created attachment 187741 [details]
Patch

Switch to expandedIntSize for size conversions; fix signed/unsigned integer warning
Comment 16 WebKit Review Bot 2013-02-11 20:24:46 PST
Comment on attachment 187741 [details]
Patch

Clearing flags on attachment: 187741

Committed r142571: <http://trac.webkit.org/changeset/142571>
Comment 17 WebKit Review Bot 2013-02-11 20:24:50 PST
All reviewed patches have been landed.  Closing bug.