[webkit-reviews] review requested: [Bug 81106] [chromium] Infrastructure to allow animating layers to be only partially updated : [Attachment 132518] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 81106: [chromium] Infrastructure to allow animating layers to be only
partially updated
https://bugs.webkit.org/show_bug.cgi?id=81106

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

------- Additional Comments from vollick at chromium.org
I've r?'d again because of some of the renaming I did in response to these
reviews.

(In reply to comment #12)
> (From update of attachment 132495 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=132495&action=review
>
> some nits for you :)
>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:548
> > +	 m_opacity = animatedOpacity;
>
> I think these deserve a comment saying why it's not using setOpacity().
Good call. Added.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:63
> > +	 virtual float animatedOpacity() const { return m_opacity; }
>
> Is this needed, since both threads set m_opacity now?
It definitely doesn't need to be called opacity, but it does need to be part of
the interface.
I've made opacity() and transform() part of the interface again, and renamed
setAnimatedTransform to setTransformFromAnimation (likewise for opacity). At
the moment,
it looks like these special setters are still required.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:65
> > +	 virtual const TransformationMatrix& animatedTransform() const { return
m_transform; }
>
> ditto?
Fixed.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:215
> >	 // We may have added an animation during the tree sync. This will
cause hostImpl to visit its controllers.
>
> both sides visit them now i guess?
Thanks for catching this. I've updated the comments.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:394
> >	 // We may have added an animation during the tree sync. This will
cause hostImpl to visit its controllers.
>
> ditto?
Fixed.

(In reply to comment #12)
> (From update of attachment 132495 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=132495&action=review
>
> some nits for you :)
>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:548
> > +	 m_opacity = animatedOpacity;
>
> I think these deserve a comment saying why it's not using setOpacity().
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:63
> > +	 virtual float animatedOpacity() const { return m_opacity; }
>
> Is this needed, since both threads set m_opacity now?
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:65
> > +	 virtual const TransformationMatrix& animatedTransform() const { return
m_transform; }
>
> ditto?
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:215
> >	 // We may have added an animation during the tree sync. This will
cause hostImpl to visit its controllers.
>
> both sides visit them now i guess?
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:394
> >	 // We may have added an animation during the tree sync. This will
cause hostImpl to visit its controllers.
>
> ditto?

(In reply to comment #13)
> (From update of attachment 132495 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=132495&action=review
>
> Seems pretty reasonable. Dana's comments are good, I've added some more in a
similar vein.
>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:608
> > +	 case CCAnimationEvent::Started: {
> > +	     m_layerAnimationDelegate->notifyAnimationStarted(wallClockTime);
> > +	     break;
>
> we normally don't put braces on case : sections unless we need block scoping
for a local var
>
Fixed.
> >
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:185
> > +void CCLayerAnimationController::add(PassOwnPtr<CCActiveAnimation> anim)
>
> don't abbreviate parameter/variable names. 'animation' would be fine here
Done.
>
> >
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:410
> > +		 } // switch
> > +	     } // if running
> > +	 } // for each animation
>
> we don't typically comment ends of blocks like this. this function doesn't
seem long enough to need these, but if there are functions with enough nesting
levels to require comments that's when we would normally break bits out into
helper functions
Make sense. Removed.
>
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:181
> > +	 EXPECT_EQ(0.5f, dummy.animatedOpacity());
>
> don't think you need the trailing "f"s for this and the other floating point
constants in this patch
Fixed.
>
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:201
> > +	 controller->animate(0.5, events.get()); // second anim starts NOW.
>
> if it's worth having a comment, it's probably worth expanding out to a
sentence with complete words
You're right -- that's an awful comment. Fixed.
>
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:228
> > +	 // The the float animation should have started at time 1 and should be
done.
>
> typo "The the"
>
Fixed.
> > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:248
> > +	 // Anims with id 1 should both start now.
>
> Animations please
Fixed.