Summary: | [chromium] Add support for pinch gesture in the threaded compositor | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sadrul Habib Chowdhury <sadrul> | ||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Sadrul Habib Chowdhury <sadrul> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | fishd, jamesr, rgbbones, rjkroege, webkit.review.bot, wjmaclean | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Sadrul Habib Chowdhury
2012-02-03 21:07:33 PST
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> All reviewed patches have been landed. Closing bug. |