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

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	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 150289: Patch
https://bugs.webkit.org/attachment.cgi?id=150289&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #6)
> (From update of attachment 150248 [details])
> 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.
Fixed.
>
> > 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.
Sweet. Removed.
>
> > 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.
Done.
>
> > 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)
Removed.
>
> > 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.
This is gone now due to the the next point.
>
> 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.
Done.
>
> > 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
Removed.
>
> > Source/WebKit/chromium/src/WebAnimationCurve.cpp:45
> > +PassOwnPtr<WebCore::CCTimingFunction>
getTimingFunction(WebTimingFunctionType type)
>
> this isn't really a getter - maybe createTimingFunction() ?
Done (and moved somewhere visible by both the float and keyframe animation
curves).
>
> > Source/WebKit/chromium/src/WebAnimationCurve.cpp:48
> > +	 case WebTimingFunctionEase: return
WebCore::CCEaseTimingFunction::create();
>
> put return statements on their own lines, please
Done.
>
> > 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.
Done.
>
> the compiler will check that we have a case: for every possible value enum
value so the assert isn't all that useful
Removed.
>
> > Source/WebKit/chromium/src/WebAnimationCurve.cpp:72
> > +#if WEBKIT_IMPLEMENTATION
>
> no #if WEBKIT_IMPLEMENTATION in .cpp files
Removed.
>
> > Source/WebKit/chromium/src/WebAnimationCurve.cpp:105
> > +#if WEBKIT_IMPLEMENTATION
>
> no #if
Removed.
>
> >> 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
Whoops -- I remembered there being a problem before, and just assumed it was
the same thing. Switched.
>
> >> 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"
Fixed.