[chromium] Register newly-created layers for animation
Created attachment 182191 [details] Patch
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.
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
Created attachment 189364 [details] Patch
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.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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?
(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.
Created attachment 189777 [details] Patch
(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.
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.
Created attachment 189805 [details] Patch for landing
Comment on attachment 189805 [details] Patch for landing Clearing flags on attachment: 189805 Committed r143803: <http://trac.webkit.org/changeset/143803>
All reviewed patches have been landed. Closing bug.