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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 127687: Patch
https://bugs.webkit.org/attachment.cgi?id=127687&action=review

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


> Source/WebCore/platform/graphics/chromium/LayerChromium.h:218
> +    bool addAnimation(const KeyframeValueList&, const IntSize& boxSize,
const Animation*, const String& animationName, double timeOffset);

we know that we aren't sticking with strings so I think landing some code using
strings is simply creating unnecessary work for us.  Keep in mind this is a
large, complex codebase with lots of people contributing that we ship to
millions of users on a weekly and daily basis.	Any churn that we can avoid is
worth its weight in gold.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:78
> +   
layer->animationController()->synchronizeAnimations(ccLayerImpl->animationContr
oller());

I'd rather you do this inside LayerChromium::pushPropertiesTo().  Ideally the
synchronizer has as little logic as possible.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:47
> +CCActiveAnimation::~CCActiveAnimation() { }

more newlines, please

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:48
> +    typedef size_t GroupID;

why do we need to bump this? are we expecting to go through > 2 billion
groupids?

>
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:29
> +#include "GraphicsLayer.h" // for KeyframeValueList

this is something that we're going to have to wrap in our own API, although we
don't need to immediately. have you put much thought into how that would look?

>
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:50
> +						 const IntSize& /*boxSize*/,

in the implementation if you aren't going to use a parameter simply omit the
name completely, no need to comment it out

>
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:85
> +void
CCLayerAnimationController::removeAnimationsCompletedOnImplThread(CCLayerAnimat
ionControllerImpl* controllerImpl)
> +{

any functions with specific thread requirements should ideally have thread
ASSERT()s to keep us honest

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:29
> +#include "PlatformString.h"
> +#include "Vector.h"

these should be <wtf/text/WTFString.h> and <wtf/Vector.h>, respectively
(although we may not end up needing the first one at all)

>
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp
:48
> +CCLayerAnimationControllerImpl::~CCLayerAnimationControllerImpl() { }

expand

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:328
> +TEST_F(TreeSynchronizerAnimationTest, syncNewAnimation)

I don't think it provides us much value to have this test both the tree
synchronizer and the synchronizeAnimations() function. I think a much better
strategy would be to move the synchronizeAnimations() call inside of
LayerChromium::pushPropertiesTo() function and rely on our other
TreeSynchronizer tests to verify that the call happens. Then, test
synchronizerAnimations() independently of TreeSynchronizer.  That will cut down
on the number of extra fixtures / friends and boilerplate you need to generate
without reducing our test coverage.