[webkit-reviews] review granted: [Bug 79819] [chromium] Add impl-thread support for animation-timing-function : [Attachment 129374] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted vollick at chromium.org's request
for review:
Bug 79819: [chromium] Add impl-thread support for animation-timing-function
https://bugs.webkit.org/show_bug.cgi?id=79819

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

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


R=me. Several naming nits cos that's how we roll :)

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:47

> +void insertKeyframe(PassOwnPtr<Keyframe> keyframe, Vector<OwnPtr<Keyframe>
>& keyframes)
>  {
> -    if (!keyframes.size())
> -	   return true;
> -
> -    for (size_t i = 0; i < keyframes.size() - 1; ++i) {
> -	   if (keyframes[i].time > keyframes[i+1].time)
> -	       return false;
> +    OwnPtr<Keyframe> popKeyframe = keyframe;

nit: the naming here is reversed from what we normally do, although I think the
logic is correct. the normal idiom is to name the parameter of type
PassOwnPtr<> "popFoo" and then create a local called "foo" and never use the
PassOwnPtr<> value again. http://www.webkit.org/coding/RefPtr.html has an
example using PassRefPtr. this looks like the code is manipulating the
PassOwnPtr<> value, which doesn't work since it's nulled out after assignment
into a local OwnPtr<>

would you mind swapping the names?

> Source/WebCore/platform/graphics/chromium/cc/CCTimingFunction.cpp:32
> +const double kEpsilon = 1e-6;

nit: in WebKit, constants are named the same way as other variables - there's
no "k" prefix

> Source/WebCore/platform/graphics/chromium/cc/CCTimingFunction.cpp:75
> +PassOwnPtr<CCTimingFunction> CCEaseTimingFunction::create()

nit: it probably wouldn't hurt to put a link to the section of the CSS spec
that defines these seemingly magical numbers (I see you have one in the header)


> Source/WebCore/platform/graphics/chromium/cc/CCTimingFunction.h:53
> +    virtual float getValue(double t) const;

nit: in headers please expand the variable name out to 'time' just to be extra
clear