Bug 72996 – [chromium] Add page-scale animation support to Impl thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

[chromium] Add page-scale animation support to Impl thread

[Alexandre Elias]

Created attachment 116303 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116303&action=review

> Source/WebKit/chromium/public/WebCompositor.h:47
> +    virtual void handlePageScaleAnimationEvent(const WebPageScaleAnimationEvent&) = 0;

since WebPageScaleAnimationEvent inherits from WebInputEvent, you don't need this method.  just use handleInputEvent.

> Source/WebKit/chromium/public/WebInputEvent.h:371
> +class WebPageScaleAnimationEvent : public WebInputEvent {

this seems like it could/should be represented as a type of gesture event.  or maybe
we should derive from WebGestureEvent and call this a WebPageScaleGestureEvent?

[Alexandre Elias]

The reason it was hard to reuse handleInputEvent is that these events are sent from the render process via a side channel, after the target rect is computed based on the page structure.  InputEventFilter assumes it sends all events, and crashes if it receives a didHandle/didNotHandle when its queue is empty.  I could special-case my event type to not send the didHandle/didNotHandle, or return an enum from the handleInputEvent requesting that the caller does it, or fix the filter to deal with it properly (though I don't know much about it so I didn't want to introduce new bugs).  Which do you prefer?

I'm fine with deriving from WebGestureEvent although the generic fields of WebGestureEvent would be mostly unused in this one.

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> The reason it was hard to reuse handleInputEvent is that these events are sent from the render process via a side channel, after the target rect is computed based on the page structure.  InputEventFilter assumes it sends all events, and crashes if it receives a didHandle/didNotHandle when its queue is empty.  I could special-case my event type to not send the didHandle/didNotHandle, or return an enum from the handleInputEvent requesting that the caller does it, or fix the filter to deal with it properly (though I don't know much about it so I didn't want to introduce new bugs).  Which do you prefer?
> 
> I'm fine with deriving from WebGestureEvent although the generic fields of WebGestureEvent would be mostly unused in this one.

I think I need to know more about what you are trying to do then.  It seems very odd to send WebInputEvent subclasses through something other than handleInputEvent.  It seems equally odd to not be sending them through the normal HandleInputEvent IPC mechanism.

[Alexandre Elias]

Created attachment 116427 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 116427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116427&action=review

These WebKit API changes look good to me... just one question about the event structure:

> Source/WebKit/chromium/public/WebInputEvent.h:381
> +    double durationMs;

I observe that the Fling gesture does not similarly have the embedder provide a duration.  Why does page scaling not work similarly?  Or, why does Fling not work like page scaling?

[Alexandre Elias]

There are two completely different kinds of page scale events.  This one is for double-tap gestures, where the user taps on a page element and then a zoom animation occurs bringing it forward.  The other one is still in Clank-branch only, and represent pinch gestures


Double-tap input events are unlike the other events in that they need to be augmented by information about the document structure.  If a user double-taps on an image for instance, we need to get its size to know the target page scale to animate to.  The input event is passed first to the WebKit thread to gather this data.  Since the ViewMsg channel is from browser -> compositor thread, I didn't know how to use the normal input handler from there.

Now I figured out I can post a ViewMsg_HandleInputEvent task directly to InputEventHandler::OnMessageReceived from RenderViewImpl.  However, now I'm concerned about this:

  // TODO(darin): Change RenderWidgetHost to always require an ACK before
  // sending the next input event.  This way we can nuke this queue.

I'm not sending from RenderWidgetHost so if the queue goes away, this approach would stop working.

[Alexandre Elias]

Never mind my last comment, I talked offline with Darin and we agreed that we shouldn't be using this kind of "rich" input event.  I'll avoid creating a public API for them and send them through WebCompositorImpl from WebCore.

[Adrienne Walker]

Comment on attachment 116427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116427&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:91
> +    IntSize scrollTotal = m_scrollLayerImpl->scrollPosition() + m_scrollLayerImpl->scrollDelta() - IntPoint();

nit: use toSize() here instead of "- IntPoint()".

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:248
> +    IntSize scrollTotal = m_scrollLayerImpl->scrollPosition() + m_scrollLayerImpl->scrollDelta() - IntPoint();

nit: same as other IntPoint() comment.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:250
> +    m_pageScaleAnimation = adoptPtr(new CCPageScaleAnimation(scrollTotal, m_pageScale, m_viewportSize, currentTimeMs()));

What happens if startPageScaleAnimation is called again in the middle of a page scale animation? Should we early out or maybe call zoomElsewhere?

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:106
> +void CCPageScaleAnimation::zoomElsewhere(const IntSize& finalScroll, float finalPageScale, double duration, double time)

Where does this get used?

[Alexandre Elias]

Created attachment 116459 [details]
Patch

[Alexandre Elias]

I stripped out all the input event stuff from this CL, so this no longer adds any public APIs.  We'll do it more cleanly later.

For what happens if an animation already exists, good point.  On second thought, I don't think zoomElsewhere is a good idea so I deleted that method.  We can implicitly destroy and recreate the animation object if it already exists.  I adjusted the logic to make sure page scale deltas are treated properly, which should cover both in-progress double-tap and pinch zooms.  I tested that it works correctly by sending extremely slow animations.

[Alexandre Elias]

Created attachment 116469 [details]
Patch

[James Robinson]

Comment on attachment 116469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116469&action=review

This is a great start. Left some comments that should be addressed before landing, IMO

> Source/WebCore/ChangeLog:12
> +        No new tests. (https://bugs.webkit.org/show_bug.cgi?id=71529 filed.)

I think it'd be really helpful to land at least some skeleton tests with this patch, at least to show how the tests will be constructed and to verify that there aren't any crazy explosions when the class is used in a straightforward way.

> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:57
> +        bool anchorPoint,
> +        float pageScale,
> +        double durationMs) = 0;

the line wrapping here is a bit strange - could you align with the opening "(" perhaps?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:114
> +    m_pageScaleAnimation = adoptPtr(new CCPageScaleAnimation(scrollTotal, scaleTotal, m_viewportSize, currentTimeMs()));

don't use currentTimeMs() - we use monotonicallyIncreasingTime() for all animation logic

even better would be taking a timestamp passed in from the caller so the caller can adjust the time appropriately to reflect the event time or the next frame time or whatnot

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:392
> +    if (!didMove || m_pinchGestureActive || m_pageScaleAnimation.get()) {

you don't need the .get() to null check, all WebKit smart pointers have an operator bool() that does the right thing

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:16
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

we use a 2-clause license now. see the license header on (say) http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLCanvasElement.cpp, modulo the date lines

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:50
> +    m_scrollStart = scrollStart;
> +    m_pageScaleStart = pageScaleStart;
> +    m_windowSize = windowSize;
> +    m_anchorMode = false;
> +    m_scrollEnd = scrollStart;
> +    m_pageScaleEnd = pageScaleStart;
> +    m_startTime = startTime;

should be done with initializer list syntax

    : m_scrollStart(scrollStart)
    , m_pageScaleStart(pageScaleStart)

etc

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:119
> +        return 1.0f;

nit: we just say '1' for constants like this (see 'Floating point literals' section in http://www.webkit.org/coding/coding-style.html)

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:126
> +    if (ratio <= 0.0f)

0

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:128
> +    if (ratio >= 1.0f)

1

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:156
> +    if (ratio <= 0.0f)

0

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:158
> +    if (ratio >= 1.0f)

1

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:172
> +} // namespace WebKit

WebCore, no?

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:8
> + *     * Redistributions of source code must retain the above copyright

wrong license header here too

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:47
> +    CCPageScaleAnimation(const IntSize& scrollStart, float pageScaleStart, const IntSize& windowSize, double startTime);

what unit are all the times in this class' interface in? can you tag the parameter and function names with the appropriate suffix?

another note: if it's intended that this class will be stored in an OwnPtr<> it's better to make the c'tor private and expose a static PassOwnPtr<> create function to ensure that the caller puts the object into a smart pointer and doesn't leak it

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:74
> +private:

nit: newline before the private: please

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:92
> +} // namespace WebKit

WebCore, I think

[Nat Duca]

Comment on attachment 116469 [details]
Patch

This needs unit tests, As written, its not easily testable, writing unit tests will improve that situation likely.

Alex, what do you think the methods on the following class would bee:
 class CCAnimation {

  }

Assume that CCScrollAnimation derives from it and ideally the CCLayerTreeHOstImpl is not directly aware of the scrollanimation type.

[Nat Duca]

https://bugs.webkit.org/show_bug.cgi?id=73233

[Alexandre Elias]

Created attachment 118182 [details]
Patch

[Alexandre Elias]

OK, I still haven't written the unit tests as they depend on the target stuff still being worked on in https://bugs.webkit.org/show_bug.cgi?id=73233

Other than that, most of James's comments addressed, with one nit:

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:114
> > +    m_pageScaleAnimation = adoptPtr(new CCPageScaleAnimation(scrollTotal, scaleTotal, m_viewportSize, currentTimeMs()));
> 
> don't use currentTimeMs() - we use monotonicallyIncreasingTime() for all animation logic

currentTimeMs() is a CCLayerTreeHostImpl function that does monotonicallyIncreasingTime() * 1000, so it looks to me like it was intended for things like animations.

[Alexandre Elias]

Created attachment 118184 [details]
Patch

[James Robinson]

(In reply to comment #18)
> currentTimeMs() is a CCLayerTreeHostImpl function that does monotonicallyIncreasingTime() * 1000, so it looks to me like it was intended for things like animations.

Oh my.  CurrentTime.h puts a function called currentTimeMS() into the global namespace that exactly what we don't want to do.  This seems just a touch error-prone to me.

We can address that in a different patch, though.

[James Robinson]

Comment on attachment 118184 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 118184 [details]
Patch

Clearing flags on attachment: 118184

Committed r102308: <http://trac.webkit.org/changeset/102308>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.