[chromium] Add support for WebInputEvent::GestureDoubleTap on CC impl thread
Created attachment 125641 [details] Patch
Comment on attachment 125641 [details] Patch Attachment 125641 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11429431 New failing tests: compositing/checkerboard.html
(In reply to comment #2) > (From update of attachment 125641 [details]) > Attachment 125641 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11429431 > > New failing tests: > compositing/checkerboard.html This failure is due to border texel issues on the root layer. This issue has been around a long time, but it would be nice if it wasn't a blocker for enable GESTURE events on DoubleTap zoom. Without the border texels on, text on the root layer looks awful during zoom.
What platforms set ENABLE(GESTURE_EVENTS)? I don't think it's OK to turn border texels on for the root layer indiscriminately since it changes the way we render text.
Hmm, I don't think this is the right way to pipe through double-taps. I discussed this offline with Darin a month ago and we agreed on the following: - Input events should be kept simple and shouldn't contain data that was computed from the document contents like target page scale. A double-tap event should just contain its screen position. - When threaded compositing is enabled, all input events should follow the normal input filter to go directly into the impl thread. - Therefore, the way to implement it looks like this: "raw" double-tap event goes to impl thread. Impl thread specifies didNotHandle (as it has insufficient information to start the animation) and it falls back to WebKit thread input handling. WebKit thread now calls getBlockBounds() etc, and calls a method on WebCompositorImpl that schedules a callback on the impl thread that starts a pageScaleAnimation.
Comment on attachment 125641 [details] Patch Alex's summary matches my understanding. I think this has to be main-thread triggered in order to correctly find the touch target, etc - there just isn't anything we can do on the impl thread without help.
(In reply to comment #6) > (From update of attachment 125641 [details]) > Alex's summary matches my understanding. I think this has to be main-thread triggered in order to correctly find the touch target, etc - there just isn't anything we can do on the impl thread without help. OK. I was assuming the events coming in had already been processed on the main-thread, so I'll revise this as suggested and re-submit.
Created attachment 126095 [details] Patch
Alexandre: I still need to figure out 1) Why I'm getting scroll asserts in single thread mode, 2) what's the correct way to compute the input anchor point. Suggestions?
For 2), note that startPageScaleAnimation supports receiving either an anchor or an target scroll position, based on a boolean. When double-tap zooming out to minimum scale, the on-screen double-tap position is used as anchor. When double-tap zooming in onto a div, the target scroll position is used instead and some code in CCPageScaleAnimation.cpp solves some linear equations to infer the anchor from the starting and target scroll positions. The way computeScaleAndScrollForHitRect is currently written in https://bugs.webkit.org/show_bug.cgi?id=72909 , it doesn't modify the scroll position if double-tap zooming out. So you could initialize the scroll position to (-1, -1), then if it remains the same after computeScaleAndScrollForHitRect, set the anchorPoint boolean to true and use the double-tap position; otherwise set anchorPoint boolean to false and use the scroll position it gives you.
(In reply to comment #10) > For 2), note that startPageScaleAnimation supports receiving either an anchor or an target scroll position, based on a boolean. When double-tap zooming out to minimum scale, the on-screen double-tap position is used as anchor. When double-tap zooming in onto a div, the target scroll position is used instead and some code in CCPageScaleAnimation.cpp solves some linear equations to infer the anchor from the starting and target scroll positions. > > The way computeScaleAndScrollForHitRect is currently written in https://bugs.webkit.org/show_bug.cgi?id=72909 , it doesn't modify the scroll position if double-tap zooming out. So you could initialize the scroll position to (-1, -1), then if it remains the same after computeScaleAndScrollForHitRect, set the anchorPoint boolean to true and use the double-tap position; otherwise set anchorPoint boolean to false and use the scroll position it gives you. Hmm, ok. Then I'll need to plumb the anchorPoint Boolean through.
Created attachment 126127 [details] Patch
Revised patch to: 1) plumb useAnchor bool through, end to end 2) defer computation of scroll until Varun's second patch (the one after https://bugs.webkit.org/show_bug.cgi?id=72909 that ties all this together in handleGestureEvent()). Since this is primarily a plumbing patch, what constitutes a reasonable unit test?
Looks good, except that it seems that you could get rid of CCLayerTreeHostImpl::zoomAnimation and just call startPageScaleAnimation directly. For testing, adding one to CCLayerTreeHostTest would work. Take a look at CCLayerTreeHostTestSetVisible for a somewhat similar example.
(In reply to comment #14) > Looks good, except that it seems that you could get rid of CCLayerTreeHostImpl::zoomAnimation and just call startPageScaleAnimation directly. > > For testing, adding one to CCLayerTreeHostTest would work. Take a look at CCLayerTreeHostTestSetVisible for a somewhat similar example. Will do ...
Created attachment 126326 [details] Patch
LGTM except for some details about the point parameters. Since the meaning depends on the boolean, use a more generic name like "targetPosition". Also, I think it should be possible to store an IntSize directly in the callback object.
Created attachment 126340 [details] Patch
(In reply to comment #17) > LGTM except for some details about the point parameters. Since the meaning depends on the boolean, use a more generic name like "targetPosition". Also, I think it should be possible to store an IntSize directly in the callback object. Done. Style checker sorta forced me to do a bit of cleanup in CrossThreadCopier.h ... tried to do something reasonable without changing too much.
LGTM. James, could you take a look?
Comment on attachment 126340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126340&action=review I really don't like inventing new terminology for zoom animation when we already have page scale and page scale animations. Stick to the terminology we already have. > Source/WebCore/platform/CrossThreadCopier.h:44 > +class IntSize; this whole file is indented incorrectly. I don't think fixing the indentation for just a few lines at the top is an improvement - i'd rather be consistent within this file than have some lines do one thing and some lines do another. I would stick to the existing indentation in this case. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:191 > + void zoomAnimation(const IntSize&, bool, float, double); you need to name parameters when the meaning is not clear from the types. I've no idea what the size, bool, float, and double could mean from this signature. For time-based parameters please tag the name with the unit as well (seconds or milliseconds). If it's milliseconds, use an integer type. I think we want to converge on floating-point seconds everywhere if possible. > Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:60 > + virtual void zoomAnimation(const IntSize&, bool, float, double) = 0; again here, you need to provide useful parameter names in the header > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:146 > +void CCThreadProxy::requestZoomAnimationOnImplThread(IntSize targetPosition, bool useAnchor, float scale, double duration) > +{ > + ASSERT(CCProxy::isImplThread()); > + if (m_layerTreeHostImpl) > + m_layerTreeHostImpl->startPageScaleAnimation(targetPosition, useAnchor, scale, monotonicallyIncreasingTime() * 1000.0, duration); why do we have to have inconsistent naming? what's wrong with calling it pageScaleAnimation everywhere? > Source/WebKit/chromium/src/WebViewImpl.h:319 > + void zoomAnimation(const WebCore::IntPoint&, bool, float); again - these parameters need useful names > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:958 > + static void requestZoomAnimation(void * self) no space between "void" and "*", it's a "void*"
Comment on attachment 126340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126340&action=review > Source/WebCore/platform/CrossThreadCopier.h:45 > +class IntRect; btw, IntSize should be after IntRect (and when you do that you won't get a style error here since you won't be modifying the first line). > Source/WebKit/chromium/src/WebViewImpl.cpp:630 > + m_layerTreeHost->zoomAnimation(IntSize(scroll.x(), scroll.y()), useAnchor, newScale, 500.0); As an outsider, if I looked at this code the 500.0 is a total mystery to me (what it is and why that value was chosen). > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:941 > +// This test verifies that zoomAnimation events propagate correctly from CCLayerTreeHost to Consider removing "This test" > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:948 > + { bad indent
Created attachment 126356 [details] Patch
Created attachment 126359 [details] Patch
(In reply to comment #24) > Created an attachment (id=126359) [details] > Patch Sorry about the patch-spam ... missed a comment. (In reply to comment #21) > (From update of attachment 126340 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126340&action=review > > I really don't like inventing new terminology for zoom animation when we already have page scale and page scale animations. Stick to the terminology we already have. OK, done. > > Source/WebCore/platform/CrossThreadCopier.h:44 > > +class IntSize; > > this whole file is indented incorrectly. I don't think fixing the indentation for just a few lines at the top is an improvement - i'd rather be consistent within this file than have some lines do one thing and some lines do another. > > I would stick to the existing indentation in this case. Done. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:191 > > + void zoomAnimation(const IntSize&, bool, float, double); > > you need to name parameters when the meaning is not clear from the types. I've no idea what the size, bool, float, and double could mean from this signature. Done. > For time-based parameters please tag the name with the unit as well (seconds or milliseconds). If it's milliseconds, use an integer type. I think we want to converge on floating-point seconds everywhere if possible. Done. > > Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:60 > > + virtual void zoomAnimation(const IntSize&, bool, float, double) = 0; > > again here, you need to provide useful parameter names in the header Done. > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:146 > > +void CCThreadProxy::requestZoomAnimationOnImplThread(IntSize targetPosition, bool useAnchor, float scale, double duration) > > +{ > > + ASSERT(CCProxy::isImplThread()); > > + if (m_layerTreeHostImpl) > > + m_layerTreeHostImpl->startPageScaleAnimation(targetPosition, useAnchor, scale, monotonicallyIncreasingTime() * 1000.0, duration); > > why do we have to have inconsistent naming? what's wrong with calling it pageScaleAnimation everywhere? Done. > > Source/WebKit/chromium/src/WebViewImpl.h:319 > > + void zoomAnimation(const WebCore::IntPoint&, bool, float); > > again - these parameters need useful names Done. > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:958 > > + static void requestZoomAnimation(void * self) > > no space between "void" and "*", it's a "void*" Done. (In reply to comment #22) >> (From update of attachment 126340 [details]) >> View in context: https://bugs.webkit.org/attachment.cgi?id=126340&action=review >> Source/WebCore/platform/CrossThreadCopier.h:45 >> +class IntRect; >btw, IntSize should be after IntRect (and when you do that you won't get a >style error here since you won't be modifying the first line). Alphabet memory failure - fixed! >> Source/WebKit/chromium/src/WebViewImpl.cpp:630 >> + m_layerTreeHost->zoomAnimation(IntSize(scroll.x(), scroll.y()), useAnchor, newScale, 500.0); > >As an outsider, if I looked at this code the 500.0 is a total mystery to me >(what it is and why that value was chosen). Fixed. >> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:941 >> +// This test verifies that zoomAnimation events propagate correctly from CCLayerTreeHost to > >Consider removing "This test" Done. >> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:948 >> + { > >bad indent Done.
Comment on attachment 126359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126359&action=review R=me. Could probably use a better short description in the ChangeLogs > Source/WebCore/ChangeLog:3 > + [chromium] Add support for WebInputEvent::GestureDoubleTap on CC impl thread this seems out of date now, this patch seems to be hooking up a path from WebViewImpl to the impl thread to run a page scale animation.
Comment on attachment 126359 [details] Patch I think you should fix the ChangeLogs before landing. They're highly misleading in this patch.
Created attachment 126368 [details] Patch for landing
Comment on attachment 126368 [details] Patch for landing Clearing flags on attachment: 126368 Committed r107357: <http://trac.webkit.org/changeset/107357>
All reviewed patches have been landed. Closing bug.