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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted vollick at chromium.org's request
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 132495: Patch
https://bugs.webkit.org/attachment.cgi?id=132495&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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

>
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

>
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

> 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

> 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

> Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:228
> +    // The the float animation should have started at time 1 and should be
done.

typo "The the"

> Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:248
> +    // Anims with id 1 should both start now.

Animations please