[webkit-reviews] review granted: [Bug 75874] [chromium] Plumb from GraphicsLayer to the cc thread animation code : [Attachment 128023] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted vollick at chromium.org's request
for review:
Bug 75874: [chromium] Plumb from GraphicsLayer to the cc thread animation code
https://bugs.webkit.org/show_bug.cgi?id=75874

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=128023&action=review


Great, R=me!  I have a few nits, but nothing serious.

Can you confirm whether those test failures from the cr-linux EWS bot are due
to this patch and if so what's going on?  If it's nothing to do with this feel
free to upload another patch with 'Reviewed by James Robinson.' lines in the
ChangeLogs instead of 'Reviewed by NOBODY (OOPS!)' without a "review?" flag and
ask any committer to set commit-queue+.

>
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:82
> +// Looking at GraphicsLayerCA, this appears to be the analog to
suspendAnimations, which is for testing.

note: FYI, i think this might someday be used by explicit JavaScript animation
APIs. such APIs don't exist today, but dino at apple has been working on something
in this area.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:61
> +    , m_needsAnimateLayers(true)

shouldn't this initially be false and be set true by the first tree sync?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:593
> +    if (!m_needsAnimateLayers || !m_rootLayerImpl.get())

nit: all WebKit smart pointer type (OwnPtr/RefPtr) have operator bool()
overrides that do the expected thing and the style is to prefer these to null
checking the raw pointer. this means to null-check a pointer just write
"!m_rootLayerImpl" not "!m_rootLayerImpl.get()"

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:104
> +    const bool synchronizedAnimations() const { return
m_synchronizedAnimations; }

const on a bool return type is meaningless - it's returned by value so the
caller gets their own copy anyway