Bug 77804 – [chromium] Add support for pinch gesture in the threaded compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Sadrul Habib Chowdhury]

The threaded compositor currently does not support pinch gestures. This patch adds that support.

[Sadrul Habib Chowdhury]

Created attachment 125475 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

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?

[Sadrul Habib Chowdhury]

Created attachment 125476 [details]
Patch

[Sadrul Habib Chowdhury]

Created attachment 125478 [details]
Patch

[Sadrul Habib Chowdhury]

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.

[Sadrul Habib Chowdhury]

Created attachment 125479 [details]
Patch

[Sadrul Habib Chowdhury]

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?

[Sadrul Habib Chowdhury]

Created attachment 125907 [details]
Patch

[Robert Kroeger]

it looks reasonable to me.

[Sadrul Habib Chowdhury]

Created attachment 126109 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[James Robinson]

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

[Sadrul Habib Chowdhury]

Created attachment 126132 [details]
Patch

[Sadrul Habib Chowdhury]

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.

[James Robinson]

Comment on attachment 126132 [details]
Patch

Cool.

[WebKit Review Bot]

Comment on attachment 126132 [details]
Patch

Clearing flags on attachment: 126132

Committed r107125: <http://trac.webkit.org/changeset/107125>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raymond]

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;

[Raymond]

Reopening to attach new patch.

[Raymond]

Created attachment 126215 [details]
Patch

[James Robinson]

Comment on attachment 126215 [details]
Patch

Thanks, Raymond. I think you are completely right.

[Sadrul Habib Chowdhury]

(In reply to comment #21)
> Created an attachment (id=126215) [details]
> Patch

Whoops! Thanks for catching this!

[WebKit Review Bot]

Comment on attachment 126215 [details]
Patch

Clearing flags on attachment: 126215

Committed r107187: <http://trac.webkit.org/changeset/107187>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.