[webkit-reviews] review denied: [Bug 106594] [chromium] Register newly-created layers for animation : [Attachment 182191] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Ali Juma <ajuma at chromium.org>'s
request for review:
Bug 106594: [chromium] Register newly-created layers for animation
https://bugs.webkit.org/show_bug.cgi?id=106594

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

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


> Source/Platform/chromium/public/WebLayerTreeView.h:154
> -    virtual void updateAnimations(double frameBeginTime) = 0;
> +    virtual void updateAnimations(double monotonicFrameBeginTime, double
wallClockFrameBegin) = 0;

this is going to busterate the build unless a chromium-side patch implementing
both changes has landed in chromium and rolled in.  please put the chromium
rev# here on this bug when you do that

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:134
> +    WebKit::ChromeClientImpl* m_chromeClient; // weak pointer

what makes sure this pointer won't outlive the object it points to? saying it's
a weak pointer doesn't add anything (that's clear from the type) - what would
be useful in a comment would be describing the lifecycle that makes it a *safe*
weak pointer

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:174
> +    , m_graphicsLayerFactory(adoptPtr(new
GraphicsLayerFactoryChromium(this)))

we have a WebViewImpl* here, why not just pass that in to
GraphicsLayerFactoryChromium and avoid the downcast, etc?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1739
> +	   double wallClockFrameBeginTime = currentTime();

ick, why are you grabbing a wall clock time here? this isn't going to be a
useful value for anyone