Bug 77872 – [chromium] Add support for starting page/scale animations on CC impl thread from WebViewImpl

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Add support for WebInputEvent::GestureDoubleTap on CC impl thread

[W. James MacLean]

Created attachment 125641 [details]
Patch

[WebKit Review Bot]

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

[W. James MacLean]

(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.

[James Robinson]

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.

[Alexandre Elias]

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.

[James Robinson]

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.

[W. James MacLean]

(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.

[W. James MacLean]

Created attachment 126095 [details]
Patch

[W. James MacLean]

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?

[Alexandre Elias]

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.

[W. James MacLean]

(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.

[W. James MacLean]

Created attachment 126127 [details]
Patch

[W. James MacLean]

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?

[Alexandre Elias]

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.

[W. James MacLean]

(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 ...

[W. James MacLean]

Created attachment 126326 [details]
Patch

[Alexandre Elias]

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.

[W. James MacLean]

Created attachment 126340 [details]
Patch

[W. James MacLean]

(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.

[Alexandre Elias]

LGTM.  James, could you take a look?

[James Robinson]

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*"

[David Levin]

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

[W. James MacLean]

Created attachment 126356 [details]
Patch

[W. James MacLean]

Created attachment 126359 [details]
Patch

[W. James MacLean]

(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.

[James Robinson]

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.

[James Robinson]

Comment on attachment 126359 [details]
Patch

I think you should fix the ChangeLogs before landing. They're highly misleading in this patch.

[W. James MacLean]

Created attachment 126368 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 126368 [details]
Patch for landing

Clearing flags on attachment: 126368

Committed r107357: <http://trac.webkit.org/changeset/107357>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.