[webkit-reviews] review granted: [Bug 21261] Add layoutTestController API so that animation layout tests don't have to delay : [Attachment 25125] patch v3

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Pierre-Olivier Latour <pol at apple.com>'s
request for review:
Bug 21261: Add layoutTestController API so that animation layout tests don't
have to delay
https://bugs.webkit.org/show_bug.cgi?id=21261

Attachment 25125: patch v3
https://bugs.webkit.org/attachment.cgi?id=25125&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
  > +- (BOOL)_pauseAnimation:(NSString*)name onNode:(DOMNode *)node
atTime:(NSTimeInterval)time
> +{
> +    WebCore::Frame *frame = core(self);
> +    if (!frame)
> +	   return false;
> +
> +    AnimationController* controller = frame->animation();
> +    if (!controller)
> +	   return false;
> +
> +    WebCore::Node *coreNode = [node _node];
> +    if (!coreNode || !coreNode->renderer())
> +	   return false;
> +
> +    return controller->pauseAnimationAtTime(coreNode->renderer(), name,
time);
The * should be on the other side for c++ types like Node and Frame.  You also
are inconsistent with prefixing WebCore types with WebCore::, if it is not
necessary, we shouldn't use it anywhere.

> +- (BOOL)_pauseTransitionOfProperty:(NSString*)name onNode:(DOMNode*)node
atTime:(NSTimeInterval)time
> +{
> +    WebCore::Frame *frame = core(self);
> +    if (!frame)
> +	   return false;
> +
> +    AnimationController* controller = frame->animation();
> +    if (!controller)
> +	   return false;
> +
> +    WebCore::Node *coreNode = [node _node];
> +    if (!coreNode || !coreNode->renderer())
> +	   return false;
> +
> +    return controller->pauseTransitionAtTime(coreNode->renderer(), name,
time);

Same here.

> +bool LayoutTestController::pauseAnimationAtTimeOnElementWithId(JSStringRef
animationName, double time, JSStringRef elementId)
> +{
> +    return false; // FIXME: Implement this on Windows
> +}
> +
> +bool LayoutTestController::pauseTransitionAtTimeOnElementWithId(JSStringRef
propertyName, double time, JSStringRef elementId)
> +{
> +    return false; // FIXME: Implement this on Windows
> +}

You probably need to either put the new tests on the Windows skipped list or
add Windows specific results.  You should also file a bug about implementing
the Windows version.

r=me.