[webkit-reviews] review granted: [Bug 108961] [GTK][AC] Implement opacity animation with clutter ac backend : [Attachment 186675] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 6 04:24:19 PST 2013


Gustavo Noronha (kov) <gns at gnome.org> has granted ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 108961: [GTK][AC] Implement opacity animation with clutter ac backend
https://bugs.webkit.org/show_bug.cgi?id=108961

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

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186675&action=review


Looks OK in general, just a few nits.

> Source/WebCore/ChangeLog:9
> +	   Almost implementations of GraphicsLayerClutter are based on mac
port's one.

This sentence seems to be missing a word. "Almost all"?

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:32
>  
> +#include <wtf/text/CString.h>

No blank line.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:45
> +
> +#include <limits.h>

Ditto.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:53
> +// If we send a duration of 0 to CA, then it will use the default duration

Talking about CA here doesn't make much sense =)

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:678
> +    // CoreAnimation does not handle the steps() timing function. Fall back

Another CA-related comment here.

> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:398
> +	   notImplemented();
> +    else if (m_animatedPropertyType == BackgroundColor)
> +	   notImplemented();

Make these ASSERT_NOT_REACHED() for now? You should be refusing to create these
kinds of animations, so they should really not be reached.


More information about the webkit-reviews mailing list