[webkit-reviews] review denied: [Bug 90303] [chromium] Create a WebKit::Web* wrapper for the cc animation classes : [Attachment 150248] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied vollick at chromium.org's request
for review:
Bug 90303: [chromium] Create a WebKit::Web* wrapper for the cc animation
classes
https://bugs.webkit.org/show_bug.cgi?id=90303

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

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


Looks pretty close!  I think the compile error is probably release-only, but
still a valid-ish complaint.

> Source/Platform/chromium/public/WebAnimationCurve.h:51
> +struct WebFloatKeyframe {

one header per top-level type in the WebKit API, sorry -
http://trac.webkit.org/wiki/ChromiumWebKitAPI.

> Source/WebKit/chromium/src/WebAnimation.cpp:43
> +struct WebAnimationPrivate {
> +    OwnPtr<CCActiveAnimation> animation;
> +};

I think this indirection is not necessary - it's fine to forward declare a
WebCore class outside of #if WEBKIT_IMPLEMENTATION and have a
WebPrivateOwnPtr<WebCore::Type> for that type, so long as the header does not
have any inline functions that try to create/destroy such an object.  See
WebLayer for instance, it just has a WebPrivatePtr<WebCore::LayerChromium>
member.  The FooPrivate indirection is useful for when you have additional
state you want to store along with the wrapped class.

> Source/WebKit/chromium/src/WebAnimation.cpp:46
> +int WebAnimation::iterations() const { return
m_private->animation->iterations(); }
> +void WebAnimation::setIterations(int n) {
m_private->animation->setIterations(n); }

nit: I'd prefer that you expand these out, it's more typing but it makes the
code a lot more readable IMO.

> Source/WebKit/chromium/src/WebAnimation.cpp:57
> +#if WEBKIT_IMPLEMENTATION

the .cpp is all WEBKIT_IMPLEMENTATION, no need for a guard here. #if WEBKIT..
should only appear in headers that may be used by both WebKit and non-WebKit
code (i.e. public headers)

> Source/WebKit/chromium/src/WebAnimation.cpp:70
> +    case WebAnimation::WebAnimationTransform: return
CCActiveAnimation::Transform;
> +    case WebAnimation::WebAnimationOpacity: return
CCActiveAnimation::Opacity;

nit: I would expand out the return statements to their own line, this is a bit
hard to read.

For enums, we often make sure that the enum values match, add a compile-time
check to AssertMatchingEnums.cpp, and then just static_cast<> between the two
enum types (http://trac.webkit.org/wiki/ChromiumWebKitAPI, near the bottom) -
it's much shorter code and a bit easier to read.

> Source/WebKit/chromium/src/WebAnimationCurve.cpp:43
> +struct WebFloatAnimationCurvePrivate {
> +    OwnPtr<WebCore::CCKeyframedFloatAnimationCurve> curve;
> +};
> +
> +struct WebTransformAnimationCurvePrivate {
> +    OwnPtr<WebCore::CCKeyframedTransformAnimationCurve> curve;
> +};

probably don't need these wrappers either

> Source/WebKit/chromium/src/WebAnimationCurve.cpp:45
> +PassOwnPtr<WebCore::CCTimingFunction>
getTimingFunction(WebTimingFunctionType type)

this isn't really a getter - maybe createTimingFunction() ?

> Source/WebKit/chromium/src/WebAnimationCurve.cpp:48
> +    case WebTimingFunctionEase: return
WebCore::CCEaseTimingFunction::create();

put return statements on their own lines, please

> Source/WebKit/chromium/src/WebAnimationCurve.cpp:54
> +    ASSERT_NOT_REACHED();

i think you may still need a return nullptr; statement here to make the
compiler happy since ASSERT_NOT_REACHED() is compiled away to nothing in
release builds.

the compiler will check that we have a case: for every possible value enum
value so the assert isn't all that useful

> Source/WebKit/chromium/src/WebAnimationCurve.cpp:72
> +#if WEBKIT_IMPLEMENTATION

no #if WEBKIT_IMPLEMENTATION in .cpp files

> Source/WebKit/chromium/src/WebAnimationCurve.cpp:105
> +#if WEBKIT_IMPLEMENTATION

no #if

>> Source/WebKit/chromium/tests/WebAnimationCurveTest.cpp:31
>> +
> 
> Alphabetical sorting problem.  [build/include_order] [4]

I think the style bot is actually correct here - you should sort all the
includes (<public/... and "cc/..." together, and I'm pretty sure the
<public/WebTran...> includes will sort below the "cc/..." ones

>> Source/WebKit/chromium/tests/WebAnimationTest.cpp:30
>> +
> 
> Alphabetical sorting problem.  [build/include_order] [4]

same here - <public/WebAnimationCurve.h> should be sorted below
"cc/CCActiveAnimation.h"