Bug 106594 – [chromium] Register newly-created layers for animation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ali Juma]

[chromium] Register newly-created layers for animation

[Ali Juma]

Created attachment 182191 [details]
Patch

[Ali Juma]

This is needed for enabling accelerated animations for orphaned layers. The overall plan for this can be found at https://chromiumcodereview.appspot.com/11783037

This requires Bug 103635 and https://chromiumcodereview.appspot.com/11783101 to land first.

[James Robinson]

Comment on attachment 182191 [details]
Patch

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

> Source/Platform/chromium/public/WebLayerTreeView.h:154
> -    virtual void updateAnimations(double frameBeginTime) = 0;
> +    virtual void updateAnimations(double monotonicFrameBeginTime, double wallClockFrameBegin) = 0;

this is going to busterate the build unless a chromium-side patch implementing both changes has landed in chromium and rolled in.  please put the chromium rev# here on this bug when you do that

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:134
> +    WebKit::ChromeClientImpl* m_chromeClient; // weak pointer

what makes sure this pointer won't outlive the object it points to? saying it's a weak pointer doesn't add anything (that's clear from the type) - what would be useful in a comment would be describing the lifecycle that makes it a *safe* weak pointer

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:174
> +    , m_graphicsLayerFactory(adoptPtr(new GraphicsLayerFactoryChromium(this)))

we have a WebViewImpl* here, why not just pass that in to GraphicsLayerFactoryChromium and avoid the downcast, etc?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1739
> +        double wallClockFrameBeginTime = currentTime();

ick, why are you grabbing a wall clock time here? this isn't going to be a useful value for anyone

[Ali Juma]

Created attachment 189364 [details]
Patch

[Ali Juma]

Starting orphaned animations before they get added to the tree is no longer part of the plan. This means the changes involving times are no longer needed.

(In reply to comment #3)
> (From update of attachment 182191 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182191&action=review
> 
> > Source/Platform/chromium/public/WebLayerTreeView.h:154
> > -    virtual void updateAnimations(double frameBeginTime) = 0;
> > +    virtual void updateAnimations(double monotonicFrameBeginTime, double wallClockFrameBegin) = 0;
> 
> this is going to busterate the build unless a chromium-side patch implementing both changes has landed in chromium and rolled in.  please put the chromium rev# here on this bug when you do that

This change has been removed.

 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:134
> > +    WebKit::ChromeClientImpl* m_chromeClient; // weak pointer
> 
> what makes sure this pointer won't outlive the object it points to? saying it's a weak pointer doesn't add anything (that's clear from the type) - what would be useful in a comment would be describing the lifecycle that makes it a *safe* weak pointer

Done (for m_webView, which we replaced m_chromeClient as a result of the comment below).


> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:174
> > +    , m_graphicsLayerFactory(adoptPtr(new GraphicsLayerFactoryChromium(this)))
> 
> we have a WebViewImpl* here, why not just pass that in to GraphicsLayerFactoryChromium and avoid the downcast, etc?

Done.

 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1739
> > +        double wallClockFrameBeginTime = currentTime();
> 
> ick, why are you grabbing a wall clock time here? this isn't going to be a useful value for anyone

This change has been removed.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 189364 [details]
Patch

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:120
> +    GraphicsLayerFactoryChromium(WebKit::WebViewImpl* webView)

explicit

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:136
> +    // GraphicsLayerFactoryChromium's lifetime is bounded by that of its owning
> +    // ChromeClientImpl, whose own lifetime is bounded by that of m_webView.

Hmm - you'll have to remind me.  Why doesn't the WebViewImpl just own this object directly?

[Ali Juma]

(In reply to comment #7)
> (From update of attachment 189364 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189364&action=review
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:120
> > +    GraphicsLayerFactoryChromium(WebKit::WebViewImpl* webView)
> 
> explicit
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:136
> > +    // GraphicsLayerFactoryChromium's lifetime is bounded by that of its owning
> > +    // ChromeClientImpl, whose own lifetime is bounded by that of m_webView.
> 
> Hmm - you'll have to remind me.  Why doesn't the WebViewImpl just own this object directly?

WebViewImpl could own this directly. As I understand it, the only reason it's currently owned by ChromeClientImpl is that this seemed to be a logical place for it since ChromeClientImpl defines a ::graphicsLayerFactory method.

[Ali Juma]

Created attachment 189777 [details]
Patch

[Ali Juma]

(In reply to comment #7)
> (From update of attachment 189364 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189364&action=review
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:120
> > +    GraphicsLayerFactoryChromium(WebKit::WebViewImpl* webView)
> 
> explicit

Done.

> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:136
> > +    // GraphicsLayerFactoryChromium's lifetime is bounded by that of its owning
> > +    // ChromeClientImpl, whose own lifetime is bounded by that of m_webView.
> 
> Hmm - you'll have to remind me.  Why doesn't the WebViewImpl just own this object directly?

WebViewImpl now owns this object directly.

[James Robinson]

Comment on attachment 189777 [details]
Patch

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

R=me - one change suggested before landing. You don't need to ask me for another review after moving that, just ask any friendly neighborhood WebKit committer to help you out.  Thanks for the iterations and explanations!

> Source/WebKit/chromium/src/WebViewImpl.cpp:212
> +class GraphicsLayerFactoryChromium : public GraphicsLayerFactory {

WebViewImpl.cpp is kind of gigantic, so I would consider moving this class out to its own header/cpp just to cut down on churn.

[Ali Juma]

Created attachment 189805 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189805 [details]
Patch for landing

Clearing flags on attachment: 189805

Committed r143803: <http://trac.webkit.org/changeset/143803>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.