[webkit-reviews] review requested: [Bug 84620] [chromium] accelerated animations on backgrounded tabs should still tick without the threaded compositor. : [Attachment 139840] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 84620: [chromium] accelerated animations on backgrounded tabs should still
tick without the threaded compositor.
https://bugs.webkit.org/show_bug.cgi?id=84620

Attachment 139840: Patch
https://bugs.webkit.org/attachment.cgi?id=139840&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #10)
> (From update of attachment 139242 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=139242&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:342
> > +bool CCLayerTreeHost::needsAnimateLayers() const
>
> who uses this accessor?
The single threaded proxy uses it to determine if it needs to schedule a commit
when backgrounded.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:44
> > +static const double forcedCompositeTickRate = 0.1;
>
> 60hz please
I've added code to switch between 60hz and 10hz depending on whether or now
we're backgrounded.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:58
> > +	     m_proxy->compositeImmediately();
>
> Iirw, we're not supposed to composite when we're not visible (due to
backbuffer dropping stuff). So, you're going to need to make sure
compositeImmediately() doens't actually draw or swap.
Good catch. I've updated doComposite to bail if we're invisible.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:259
> > +	 if (!m_layerTreeHost->client()->scheduleComposite() &&
m_layerTreeHost->needsAnimateLayers())
>
> stash schedComp return value in a bool to help things along.
Done.
>
> >> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:281
> >> +	      {
> >
> > This { should be at the end of the previous line  [whitespace/braces] [4]
>
> Make the stylebot happy if its not already, please. You can pull this out to
a helper function if necessary.
Made a helper function.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:273
> >	 }
>
> Where's the single thread unit test that verifies that we behave correctly in
this new case?
The animation tests in CCLayerTreeHostTest.cpp now run in single threaded mode.
In particular,
CCLayerTreeHostTestTickAnimationWhileBackgrounded.runSingleThread covers the
new case. Please let me know if it's insufficient.
> Also, where's your WebWigetHost changes? Does DRT compile with this change?
Yep, the changes are there and DRT does compile.