[webkit-reviews] review requested: [Bug 80744] [chromium] Threaded opacity animation jump to opacity of 0 : [Attachment 132169] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 80744: [chromium] Threaded opacity animation jump to opacity of 0
https://bugs.webkit.org/show_bug.cgi?id=80744

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #16)
> (From update of attachment 132080 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=132080&action=review
>
> > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
> > +	 bool drawOpacityIsAnimating() const { return m_drawOpacityIsAnimating;
}
>
> i see code that sets this bit on RenderSurfaces, but I can't find any code
that uses this bit on render surfaces. Why do we need to track this on layers
and surfaces?
This will be used in an upcoming patch of Dana's. In fact, Dana had sent me the
logic for updating drawOpacityIsAnimating from that patch because I needed it
for layers.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:112
> >  template<typename LayerType>
> > +bool subtreeShouldBeSkipped(LayerType* layer)
>
> this specialization is only for CCLayerImpls - right? can you make it
explicitly on that type or will things explode then?
Changed it to overloads to bake in the two layer types.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:991
> > +class CCLayerTreeHostTestDoNotSkipLayersWithAnimatedOpacity : public
CCLayerTreeHostTestThreadOnly {
>
> not sure i understand this test - how is it verifying that we aren't skipping
the subtree?
If the subtree is skipped while preparing to draw, the draw opacity will not be
updated (it will remain at 1). This is what would have happened without this
patch since the LayerChromium's opacity was zero. With this patch, the presence
of the animation will make sure the layer is not skipped. I've added a comment
to the test.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1008
> > +	     m_numAnimates++;
>
> what's this used for?
D'oh. It's not used for anything. Removed.