Bug 77872 - [chromium] Add support for starting page/scale animations on CC impl thread from WebViewImpl
Summary: [chromium] Add support for starting page/scale animations on CC impl thread f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: W. James MacLean
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-06 07:27 PST by W. James MacLean
Modified: 2012-02-09 21:52 PST (History)
9 users (show)

See Also:


Attachments
Patch (9.64 KB, patch)
2012-02-06 07:28 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (11.44 KB, patch)
2012-02-08 09:06 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2012-02-08 11:45 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (13.12 KB, patch)
2012-02-09 10:15 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (15.33 KB, patch)
2012-02-09 11:44 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (15.63 KB, patch)
2012-02-09 13:17 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (15.62 KB, patch)
2012-02-09 13:21 PST, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch for landing (16.06 KB, patch)
2012-02-09 14:05 PST, W. James MacLean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description W. James MacLean 2012-02-06 07:27:30 PST
[chromium] Add support for WebInputEvent::GestureDoubleTap on CC impl thread
Comment 1 W. James MacLean 2012-02-06 07:28:50 PST
Created attachment 125641 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-06 08:10:46 PST
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
Comment 3 W. James MacLean 2012-02-06 08:15:18 PST
(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.
Comment 4 James Robinson 2012-02-06 15:19:53 PST
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.
Comment 5 Alexandre Elias 2012-02-06 15:37:47 PST
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 6 James Robinson 2012-02-06 16:15:38 PST
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.
Comment 7 W. James MacLean 2012-02-07 06:41:51 PST
(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.
Comment 8 W. James MacLean 2012-02-08 09:06:33 PST
Created attachment 126095 [details]
Patch
Comment 9 W. James MacLean 2012-02-08 09:08:00 PST
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?
Comment 10 Alexandre Elias 2012-02-08 10:09:14 PST
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.
Comment 11 W. James MacLean 2012-02-08 10:22:46 PST
(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.
Comment 12 W. James MacLean 2012-02-08 11:45:30 PST
Created attachment 126127 [details]
Patch
Comment 13 W. James MacLean 2012-02-08 11:48:01 PST
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?
Comment 14 Alexandre Elias 2012-02-08 11:56:12 PST
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.
Comment 15 W. James MacLean 2012-02-08 11:58:57 PST
(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 ...
Comment 16 W. James MacLean 2012-02-09 10:15:18 PST
Created attachment 126326 [details]
Patch
Comment 17 Alexandre Elias 2012-02-09 11:03:50 PST
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.
Comment 18 W. James MacLean 2012-02-09 11:44:22 PST
Created attachment 126340 [details]
Patch
Comment 19 W. James MacLean 2012-02-09 11:45:25 PST
(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.
Comment 20 Alexandre Elias 2012-02-09 11:49:57 PST
LGTM.  James, could you take a look?
Comment 21 James Robinson 2012-02-09 11:54:35 PST
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 22 David Levin 2012-02-09 12:01:32 PST
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
Comment 23 W. James MacLean 2012-02-09 13:17:06 PST
Created attachment 126356 [details]
Patch
Comment 24 W. James MacLean 2012-02-09 13:21:46 PST
Created attachment 126359 [details]
Patch
Comment 25 W. James MacLean 2012-02-09 13:24:16 PST
(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 26 James Robinson 2012-02-09 13:48:58 PST
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 27 James Robinson 2012-02-09 13:55:45 PST
Comment on attachment 126359 [details]
Patch

I think you should fix the ChangeLogs before landing. They're highly misleading in this patch.
Comment 28 W. James MacLean 2012-02-09 14:05:08 PST
Created attachment 126368 [details]
Patch for landing
Comment 29 WebKit Review Bot 2012-02-09 21:52:12 PST
Comment on attachment 126368 [details]
Patch for landing

Clearing flags on attachment: 126368

Committed r107357: <http://trac.webkit.org/changeset/107357>
Comment 30 WebKit Review Bot 2012-02-09 21:52:18 PST
All reviewed patches have been landed.  Closing bug.