Bug 21261 – Add layoutTestController API so that animation layout tests don't have to delay

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Simon Fraser (smfr)]

We should add some API just for layout tests, so that animations/transitions can be moved to a specific time programatically. This would avoid having to have animation tests each take one second.

[Pierre-Olivier Latour]

I'll be taking a pass at it once Chris has checked in some animation changes.

[Pierre-Olivier Latour]

Chris: what bugzilla or radar is blocking this work?

[Pierre-Olivier Latour]

Created attachment 25039 [details]
Patch

Reviewed by Chris Marrin

[Simon Fraser (smfr)]

> diff --git a/WebCore/page/animation/AnimationController.cpp b/WebCore/page/animation/AnimationController.cpp
> index d449afe..ec29b36 100644
> --- a/WebCore/page/animation/AnimationController.cpp
> +++ b/WebCore/page/animation/AnimationController.cpp
> @@ -59,6 +60,9 @@ public:
>  
>      bool isAnimatingPropertyOnRenderer(RenderObject*, int property, bool isRunningNow) const;
>  
> +    bool setTimeForAnimationOnNode(Node*, const String&, double t);
> +    bool setTimeForTransitionOnNode(Node*, const String&, double t);

The rest of the AnimationController API deals with renderers. I think the caller should get the node's renderer and pass that in here.
Also, I think it should be more obvious that these pause the animation/transition, and a comment should indicate that they are used by the test harness, and are not intended for general use.

So I think these should be:

bool pauseAnimationAt(RenderObject*, const String& animName, double t);
bool pauseTransitionAt(RenderObject*, int propertyID, double t);

What do they do if the animation/transition is in the pause phase? Comments should say.

> diff --git a/WebKit/mac/WebView/WebFrameView.mm b/WebKit/mac/WebView/WebFrameView.mm
> index e9c0aae..202c695 100644
> --- a/WebKit/mac/WebView/WebFrameView.mm
> +++ b/WebKit/mac/WebView/WebFrameView.mm

> +- (BOOL)_setTime:(NSTimeInterval)time forNamedAnimation:(NSString *)name onNode:(DOMNode *)node

To be consistent:
- (BOOL)_pauseAnimation:(NSString*)name onNode:(DOMNode *)node atTime:(NSTimeInterval)time;
- (BOOL)_pauseTransitionOfProperty:(NSString*)cssProperty onNode:(DOMNode *)node atTime:(NSTimeInterval)time;

> diff --git a/WebKitTools/DumpRenderTree/LayoutTestController.h b/WebKitTools/DumpRenderTree/LayoutTestController.h
> index f27c2c6..c0fbca8 100644
> --- a/WebKitTools/DumpRenderTree/LayoutTestController.h
> +++ b/WebKitTools/DumpRenderTree/LayoutTestController.h
> @@ -153,6 +153,9 @@ public:
>      const std::string& testPathOrURL() const { return m_testPathOrURL; }
>      const std::string& expectedPixelHash() const { return m_expectedPixelHash; }
>      
> +    bool setAnimationTimeForElement(JSStringRef elementId, JSStringRef animationName, double time);
> +    bool setTransitionTimeForElement(JSStringRef elementId, JSStringRef propertyName, double time);

I think it's worth figuring out how to pass the element directly. We shouldn't have to require that elements used
with this API have an id.

[Pierre-Olivier Latour]

(In reply to comment #4)
> The rest of the AnimationController API deals with renderers. I think the
> caller should get the node's renderer and pass that in here.
> Also, I think it should be more obvious that these pause the
> animation/transition, and a comment should indicate that they are used by the
> test harness, and are not intended for general use.
> 
> So I think these should be:
> 
> bool pauseAnimationAt(RenderObject*, const String& animName, double t);
> bool pauseTransitionAt(RenderObject*, int propertyID, double t);

Agreed for renderer, but WebCore is not exposing any API in WebKit to go from property name to propertyID, so let's keep using the property name.
 
> What do they do if the animation/transition is in the pause phase? Comments
> should say.

We're deprecating pause anyway, but I added comment at the WebKit header level.

> To be consistent:
> - (BOOL)_pauseAnimation:(NSString*)name onNode:(DOMNode *)node
> atTime:(NSTimeInterval)time;
> - (BOOL)_pauseTransitionOfProperty:(NSString*)cssProperty onNode:(DOMNode
> *)node atTime:(NSTimeInterval)time;

ok
 
> I think it's worth figuring out how to pass the element directly. We shouldn't
> have to require that elements used
> with this API have an id.

Yes, it'd be "cleaner", but you can't access JS internals from DRT, and already existing method in DRT that take elements as parameters use their string ids.

[Pierre-Olivier Latour]

Created attachment 25052 [details]
Patch v2

[Pierre-Olivier Latour]

Created attachment 25125 [details]
patch v3

[Sam Weinig]

Comment on attachment 25125 [details]
patch v3

  > +- (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.

[Pierre-Olivier Latour]

(In reply to comment #8)
> 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.

Fixed

> You probably need to either put the new tests on the Windows skipped list or
> add Windows specific results.

Fixed

>  You should also file a bug about implementing
> the Windows version.

https://bugs.webkit.org/show_bug.cgi?id=22239

[Pierre-Olivier Latour]

Created attachment 25138 [details]
Patch v4

[Dean Jackson]

r38525 = c3c1c2f9b7435caccacdf517deda57105507132d (trunk)
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/WebView/WebFramePrivate.h
	M	WebKit/mac/WebView/WebFrame.mm
	M	WebCore/ChangeLog
	M	WebCore/WebCore.base.exp
	M	WebCore/page/animation/AnimationBase.h
	M	WebCore/page/animation/CompositeAnimation.cpp
	M	WebCore/page/animation/AnimationController.cpp
	M	WebCore/page/animation/CompositeAnimation.h
	M	WebCore/page/animation/AnimationController.h
	M	WebCore/page/animation/AnimationBase.cpp
	M	WebCore/page/animation/ImplicitAnimation.cpp
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	A	LayoutTests/platform/mac/animations/animation-drt-api-expected.txt
	A	LayoutTests/platform/mac/transitions/transition-drt-api-expected.txt
	M	LayoutTests/platform/win/Skipped
	M	LayoutTests/ChangeLog
	A	LayoutTests/animations/animation-drt-api.html
	A	LayoutTests/transitions/transition-drt-api.html
	M	WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp
	M	WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
	M	WebKitTools/DumpRenderTree/LayoutTestController.cpp
	M	WebKitTools/DumpRenderTree/LayoutTestController.h
	M	WebKitTools/ChangeLog

[Eric Seidel (no email)]

Comment on attachment 25138 [details]
Patch v4

Clearing review flag, silly bugzilla.

There is also a dupe of this somewhere, as I filed a bug asking for this for SVG animation years ago. :)

[Pierre-Olivier Latour]

> There is also a dupe of this somewhere, as I filed a bug asking for this for
> SVG animation years ago. :)

I don't think this would qualify as a dup, as this enhancement was only for CSS transitions and animations.