[webkit-reviews] review requested: [Bug 87873] [chromium] Single thread proxy should not tick animations unless the layer renderer has been initialized : [Attachment 145124] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 87873: [chromium] Single thread proxy should not tick animations unless the
layer renderer has been initialized
https://bugs.webkit.org/show_bug.cgi?id=87873

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

------- Additional Comments from vollick at chromium.org
 (In reply to comment #14)
> (From update of attachment 145103 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=145103&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:97
> > +	 virtual void didStopAnimationTimer() { }
>
> This is an odd part of the API, especially since the notion of animation
timer is CCSingleThreadProxy specific.	Is this just a test hook?  Why not
simply have a test that calls finishAllRendering and then ASSERT()s that the
timer doesn't fire?
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:126
> > +	 virtual bool initializeLayerRenderer(PassRefPtr<GraphicsContext3D>,
TextureUploaderOption);
>
> Also a weird test hook - we can make initialization fail by passing a
GraphicsContext3D in that's not useful (say, make the first
makeContextCurrent() call return false) - that's a much more realistic test and
we do it in other places
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:133
> > +	     // We're done rendering and should not process any future
animation events.
>
> I'd much prefer that we cancel the timer when initialization fails.  The fact
that WebViewImpl calls finishAllRendering() is a strange historical artifact of
how shutdown used to work and isn't really a concern of the compositor - in
fact I've been planning to delete that call.
Done.