[webkit-reviews] review requested: [Bug 84620] [chromium] accelerated animations on backgrounded tabs should still tick without the threaded compositor. : [Attachment 139242] 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 139242: Patch
https://bugs.webkit.org/attachment.cgi?id=139242&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #4)
> (From update of attachment 138818 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=138818&action=review
>
> Lots of comments, but I do think the basic approach is sound.
>
> > Source/Platform/chromium/public/WebLayerTreeViewClient.h:80
> > +	 virtual bool tryScheduleComposite() = 0;
>
> The key thing here is changing scheduleComposite to return something
indicating whether the composite is going to happen promptly or not. We can do
the renaming later, as a refactoring-only change.
>
Switched tryScheduleComposite back to scheduleComposite.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:670
> > +bool CCLayerTreeHost::tryScheduleComposite()
>
> assert that you're not in threaded mode
Done.
>
> maybe just have the proxy go m_layerTreeHost->client->tryScheduleComposite()
instead of having it go through the lth at all?
>
> if we had a scheduleComposite on this class, lets have it go away
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:256
> > +		 m_redrawTimer = CCSingleThreadProxyRedrawTimer::create(this);
>
> dont lazily create this. make it up front to keep this code linear.
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:259
> > +	     m_redrawTimer.clear();
>
> redraw timer -> forced composite timer?
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:282
> > +	     if (m_layerTreeHost->needsAnimateLayers())
>
> This shouldn't be inside the debugscopedsetimplthread --- this is main thread
code. Only the didBeComeInvisibleOnImplThread should be in the debugScopedSet
Done.
>
> >> Source/WebKit/chromium/public/WebWidgetClient.h:86
> >> +	  virtual bool tryScheduleComposite()
> >
> > please document this method.  what does it mean for tryScheduleComposite to
return false?
> > you mean the embedder can decide that it doesn't want to schedule a
composite operation
> > right now?	what does that mean?  if this is about the page being
invisible, can't we use
> > the visibility information that the WebViewImpl already has?  why does the
embedder need
> > to be involved?
I've added documentation to explain that 'true' means that a composite is
immanent, and 'false'
means that the composite will be delayed, possibly forever.
>
> We might make this return an enum to make it clear that its not returning a
yes/no decsion but a "yes || yes_but_delayed || no" --- render_widget can
answer yes an yes_but_delayed. DRT answers no, and always has. Its just not
been a problem, before.
Being able to distinguish indefinite from undetermined doesn't seem valuable
right now. Can we maybe add this later when the need arises?