Bug 77804 - [chromium] Add support for pinch gesture in the threaded compositor
Summary: [chromium] Add support for pinch gesture in the threaded compositor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sadrul Habib Chowdhury
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-03 21:07 PST by Sadrul Habib Chowdhury
Modified: 2012-02-08 23:52 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.37 KB, patch)
2012-02-03 21:11 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (5.26 KB, patch)
2012-02-03 21:19 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (5.41 KB, patch)
2012-02-03 21:25 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2012-02-03 21:35 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (11.14 KB, patch)
2012-02-07 13:07 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (11.15 KB, patch)
2012-02-08 10:24 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (11.43 KB, patch)
2012-02-08 12:16 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (1.24 KB, patch)
2012-02-08 18:34 PST, Raymond
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sadrul Habib Chowdhury 2012-02-03 21:07:33 PST
The threaded compositor currently does not support pinch gestures. This patch adds that support.
Comment 1 Sadrul Habib Chowdhury 2012-02-03 21:11:18 PST
Created attachment 125475 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-03 21:13:26 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2012-02-03 21:17:07 PST
Comment on attachment 125475 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:161
> +        ASSERT_NOT_REACHED();

should this have a FIXME too about PlatformGestureEvent?
Comment 4 Sadrul Habib Chowdhury 2012-02-03 21:19:03 PST
Created attachment 125476 [details]
Patch
Comment 5 Sadrul Habib Chowdhury 2012-02-03 21:25:49 PST
Created attachment 125478 [details]
Patch
Comment 6 Sadrul Habib Chowdhury 2012-02-03 21:26:24 PST
Comment on attachment 125475 [details]
Patch

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

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:161
>> +        ASSERT_NOT_REACHED();
> 
> should this have a FIXME too about PlatformGestureEvent?

Good point. Added.
Comment 7 Sadrul Habib Chowdhury 2012-02-03 21:35:54 PST
Created attachment 125479 [details]
Patch
Comment 8 Sadrul Habib Chowdhury 2012-02-03 21:37:05 PST
A quick explanation: the approach in this patch is very simple: it simply adds the necessary enums for pinch-gestures, and processes them appropriately from the threaded compositor. Unlike scroll-gesture events, the compositor always processes the pinch-gestures, and so the pinches don't reach the main thread.

When not using the MT compositor, I have added checks in the appropriate places (WebViewImpl, WebPopupMenuImpl) so that the events are ignored. I am not entirely sure if pinch should do anything when the compositor isn't used. Also, it would require updating PlatformGestureEvent. So I have kept that for a separate CL. Does that sound good?
Comment 9 Sadrul Habib Chowdhury 2012-02-07 13:07:02 PST
Created attachment 125907 [details]
Patch
Comment 10 Robert Kroeger 2012-02-07 16:00:29 PST
it looks reasonable to me.
Comment 11 Sadrul Habib Chowdhury 2012-02-08 10:24:03 PST
Created attachment 126109 [details]
Patch
Comment 12 Darin Fisher (:fishd, Google) 2012-02-08 11:12:49 PST
Comment on attachment 126109 [details]
Patch

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

LGTM, but...

> Source/WebKit/chromium/public/WebInputEvent.h:112
> +        GesturePinchBegin,

OK

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:177
> +    } else if (event.type == WebInputEvent::GesturePinchBegin) {

jamesr should probably review this part and the test.
Comment 13 James Robinson 2012-02-08 11:58:02 PST
Comment on attachment 126109 [details]
Patch

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

R=me with one request for documentation and one very minor style nit.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:196
> +        m_inputHandlerClient->pinchGestureUpdate(gestureEvent.deltaX, IntPoint(gestureEvent.x, gestureEvent.y));

I suppose this means that pinches are 1-dimensional and we're using deltaX to represent the amount - could you please document this somewhere (such as in WebInputEvent where it defines WebGestureEvent)?

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:252
> +    gesture.deltaX = 1.5f;

don't put the trailing "f" on this. see "Floating point literals" in http://www.webkit.org/coding/coding-style.html
Comment 14 Sadrul Habib Chowdhury 2012-02-08 12:16:00 PST
Created attachment 126132 [details]
Patch
Comment 15 Sadrul Habib Chowdhury 2012-02-08 12:27:56 PST
Comment on attachment 126109 [details]
Patch

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

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:196
>> +        m_inputHandlerClient->pinchGestureUpdate(gestureEvent.deltaX, IntPoint(gestureEvent.x, gestureEvent.y));
> 
> I suppose this means that pinches are 1-dimensional and we're using deltaX to represent the amount - could you please document this somewhere (such as in WebInputEvent where it defines WebGestureEvent)?

Indeed; we do use deltaX to represent the scaling/magnification amount. I have added a comment explaining this in WebInputEvent.h

>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:252
>> +    gesture.deltaX = 1.5f;
> 
> don't put the trailing "f" on this. see "Floating point literals" in http://www.webkit.org/coding/coding-style.html

Fixed.
Comment 16 James Robinson 2012-02-08 12:31:09 PST
Comment on attachment 126132 [details]
Patch

Cool.
Comment 17 WebKit Review Bot 2012-02-08 13:13:16 PST
Comment on attachment 126132 [details]
Patch

Clearing flags on attachment: 126132

Committed r107125: <http://trac.webkit.org/changeset/107125>
Comment 18 WebKit Review Bot 2012-02-08 13:13:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Raymond 2012-02-08 18:12:12 PST
Comment on attachment 126132 [details]
Patch

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

buid failed on my machine due to gcc warnning.

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:64
> +        m_pinchStarted = m_pinchStarted = false;

I guess this should be m_pinchStarted = m_pinchEnded = false;
Comment 20 Raymond 2012-02-08 18:22:41 PST
Reopening to attach new patch.
Comment 21 Raymond 2012-02-08 18:34:50 PST
Created attachment 126215 [details]
Patch
Comment 22 James Robinson 2012-02-08 20:06:08 PST
Comment on attachment 126215 [details]
Patch

Thanks, Raymond. I think you are completely right.
Comment 23 Sadrul Habib Chowdhury 2012-02-08 20:37:08 PST
(In reply to comment #21)
> Created an attachment (id=126215) [details]
> Patch

Whoops! Thanks for catching this!
Comment 24 WebKit Review Bot 2012-02-08 23:52:52 PST
Comment on attachment 126215 [details]
Patch

Clearing flags on attachment: 126215

Committed r107187: <http://trac.webkit.org/changeset/107187>
Comment 25 WebKit Review Bot 2012-02-08 23:52:59 PST
All reviewed patches have been landed.  Closing bug.