[chromium] Route all animate calls through CCLayerTreeHost in composited mode to simplify rate limiting logic
Created attachment 121161 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
See ChangeLog for description of what this does. One thing that's a bit ugly is that WebWidget declares animate() and layout(). Since I wanted to break up CCLayerTreeHostClient's animateAndLayout() function, the obvious names are animate() and layout() but they collide in WebViewImpl which implements both WebWidget and CCLayerTreeHostClient. The implementations for layout() are the same so I just left that alone, but the two animate() overrides require different behavior. I solved this by (somewhat arbitrarily) renaming the CCLayerTreeHostClient version to updateAnimations(). This requires a chromium-side change for the WebLayerTreeViewClient interface change which I'll land before landing this or aura will stop compiling. It looks like WebLayerTreeViewClient implementations don't currently use animateAndLayout() for anything at all. I expect that they will want to hook up with updateAnimations() but they don't immediately need a layout(), so I've left that out of the API for now. I may end up adding layout() as well in a future patch.
Chromium side here: http://codereview.chromium.org/9008078/
Comment on attachment 121161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121161&action=review Unofficial LGTM > Source/WebKit/chromium/src/WebViewImpl.cpp:1141 > + if (m_layerTreeHost) should this be m_acceleratedCompositingActive or something like that? > Source/WebKit/chromium/src/WebViewImpl.cpp:1149 > +void WebViewImpl::updateAnimations(double frameBeginTime) I think the trace event for animate has disappeared with this patch. Might just put an trace here? I assume layout is still there, I think that was explicitly traced.
Ah whoops, WebViewImpl::animate() and WebViewImpl::layout() have traces, but in the threaded path I did lose the trace for the animate call. I think I'll just move the animate trace in WebViewImpl to WebViewImpl::updateAnimations(), which all codepaths still go through. Does that sound good?
(In reply to comment #5) > (From update of attachment 121161 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121161&action=review > > Unofficial LGTM > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1141 > > + if (m_layerTreeHost) > > should this be m_acceleratedCompositingActive or something like that? Yes, I think it should be. Fixing.
Created attachment 121178 [details] Patch
Created attachment 121193 [details] Patch
I changed my mind re WebLayerTreeViewClient::layout() - i'm going to need that for the next patch (making WebViewImpl depend on WebLayerTreeView instead of CCLayerTreeHost) and since it requires a two-sided patch it's easier to just do it all at once.
Comment on attachment 121193 [details] Patch Ship it. Unofficial lgtm
Comment on attachment 121193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121193&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:1136 > +#if USE(ACCELERATED_COMPOSITING) nit: What about moving this code into WebViewImpl::updateAnimations, or if not, then perhaps WebViewImpl::updateAnimations should be renamed to indicate that it should only be called when not in accelerated compositing mode? Maybe add an assertion too? > Source/WebKit/chromium/src/WebViewImpl.h:112 > + // This also implements WebCore::CCLayerTreeHostClient::layout() micro-nit: maybe put this comment inline so the flow of function declarations is not disrupted? virtual void layout(); // Also implements CCLayerTreeHostClient::layout()
Comment on attachment 121193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121193&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:1136 >> +#if USE(ACCELERATED_COMPOSITING) > > nit: What about moving this code into WebViewImpl::updateAnimations, or if not, > then perhaps WebViewImpl::updateAnimations should be renamed to indicate that it > should only be called when not in accelerated compositing mode? Maybe add an > assertion too? currently updateAnimations() is called in composited and non-composited mode. the three possibilities are: (1) non-composited: render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate() WebViewImpl::animate() calls WebViewImpl::updateAnimations() (2) composited but render_widget scheduling (single-threaded) render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate() WebViewImpl::animate() calls CCLayerTreeHost::updateAnimations() CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations() (3) composited with compositor-driven scheduling (threaded) CCThreadProxy calls CCLayerTreeHost::updateAnimations() CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations() so i'm not sure i can fold any functions into each other >> Source/WebKit/chromium/src/WebViewImpl.h:112 >> + // This also implements WebCore::CCLayerTreeHostClient::layout() > > micro-nit: maybe put this comment inline so the flow of function declarations is not disrupted? > > virtual void layout(); // Also implements CCLayerTreeHostClient::layout() Sure, will do
Chromium implementations of new pure virtuals on WebLayerTreeViewClient landed in http://src.chromium.org/viewvc/chrome?view=rev&revision=116580.
(In reply to comment #13) > (From update of attachment 121193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121193&action=review > > >> Source/WebKit/chromium/src/WebViewImpl.cpp:1136 > >> +#if USE(ACCELERATED_COMPOSITING) > > > > nit: What about moving this code into WebViewImpl::updateAnimations, or if not, > > then perhaps WebViewImpl::updateAnimations should be renamed to indicate that it > > should only be called when not in accelerated compositing mode? Maybe add an > > assertion too? > > currently updateAnimations() is called in composited and non-composited mode. the three possibilities are: > > (1) non-composited: > render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate() > WebViewImpl::animate() calls WebViewImpl::updateAnimations() > > (2) composited but render_widget scheduling (single-threaded) > render_widget calls WebWidget::animate() which is implemented by WebViewImpl::animate() > WebViewImpl::animate() calls CCLayerTreeHost::updateAnimations() > CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations() > > (3) composited with compositor-driven scheduling (threaded) > CCThreadProxy calls CCLayerTreeHost::updateAnimations() > CCLayerTreeHost::updateAnimations() sets rate limiting logic and calls WebViewImpl::updateAnimations() > > so i'm not sure i can fold any functions into each other OK... makes sense. I hadn't understood all of the call paths. I'm happy with the code as you have it.
Committed r104248: <http://trac.webkit.org/changeset/104248>