Bug 72996 - [chromium] Add page-scale animation support to Impl thread
Summary: [chromium] Add page-scale animation support to Impl thread
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: Alexandre Elias
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-22 18:10 PST by Alexandre Elias
Modified: 2011-12-07 20:35 PST (History)
7 users (show)

See Also:


Attachments
Patch (27.59 KB, patch)
2011-11-22 18:14 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (26.64 KB, patch)
2011-11-23 14:19 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (20.76 KB, patch)
2011-11-23 17:22 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (20.49 KB, patch)
2011-11-23 19:33 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (20.83 KB, patch)
2011-12-07 00:39 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (21.34 KB, patch)
2011-12-07 01:24 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2011-11-22 18:10:40 PST
[chromium] Add page-scale animation support to Impl thread
Comment 1 Alexandre Elias 2011-11-22 18:14:19 PST
Created attachment 116303 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-22 18:17:35 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-11-22 20:34:52 PST
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?
Comment 4 Alexandre Elias 2011-11-22 20:53:43 PST
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.
Comment 5 Darin Fisher (:fishd, Google) 2011-11-23 08:40:33 PST
(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.
Comment 6 Alexandre Elias 2011-11-23 14:19:50 PST
Created attachment 116427 [details]
Patch
Comment 7 Darin Fisher (:fishd, Google) 2011-11-23 14:23:33 PST
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?
Comment 8 Alexandre Elias 2011-11-23 14:48:41 PST
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.
Comment 9 Alexandre Elias 2011-11-23 15:06:00 PST
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.
Comment 10 Adrienne Walker 2011-11-23 16:15:13 PST
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?
Comment 11 Alexandre Elias 2011-11-23 17:22:29 PST
Created attachment 116459 [details]
Patch
Comment 12 Alexandre Elias 2011-11-23 17:25:18 PST
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.
Comment 13 Alexandre Elias 2011-11-23 19:33:03 PST
Created attachment 116469 [details]
Patch
Comment 14 James Robinson 2011-11-27 12:49:28 PST
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
Comment 15 Nat Duca 2011-11-28 10:22:44 PST
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.
Comment 17 Alexandre Elias 2011-12-07 00:39:17 PST
Created attachment 118182 [details]
Patch
Comment 18 Alexandre Elias 2011-12-07 00:51:25 PST
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.
Comment 19 Alexandre Elias 2011-12-07 01:24:05 PST
Created attachment 118184 [details]
Patch
Comment 20 James Robinson 2011-12-07 17:22:10 PST
(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.
Comment 21 James Robinson 2011-12-07 18:20:36 PST
Comment on attachment 118184 [details]
Patch

R=me
Comment 22 WebKit Review Bot 2011-12-07 20:35:48 PST
Comment on attachment 118184 [details]
Patch

Clearing flags on attachment: 118184

Committed r102308: <http://trac.webkit.org/changeset/102308>
Comment 23 WebKit Review Bot 2011-12-07 20:35:54 PST
All reviewed patches have been landed.  Closing bug.