The threaded compositor currently does not support pinch gestures. This patch adds that support.
Created attachment 125475 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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?
Created attachment 125476 [details] Patch
Created attachment 125478 [details] Patch
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.
Created attachment 125479 [details] Patch
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?
Created attachment 125907 [details] Patch
it looks reasonable to me.
Created attachment 126109 [details] Patch
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 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
Created attachment 126132 [details] Patch
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 on attachment 126132 [details] Patch Cool.
Comment on attachment 126132 [details] Patch Clearing flags on attachment: 126132 Committed r107125: <http://trac.webkit.org/changeset/107125>
All reviewed patches have been landed. Closing bug.
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;
Reopening to attach new patch.
Created attachment 126215 [details] Patch
Comment on attachment 126215 [details] Patch Thanks, Raymond. I think you are completely right.
(In reply to comment #21) > Created an attachment (id=126215) [details] > Patch Whoops! Thanks for catching this!
Comment on attachment 126215 [details] Patch Clearing flags on attachment: 126215 Committed r107187: <http://trac.webkit.org/changeset/107187>