Bug 21261 - Add layoutTestController API so that animation layout tests don't have to delay
Summary: Add layoutTestController API so that animation layout tests don't have to delay
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 22239 22368 22391
  Show dependency treegraph
 
Reported: 2008-09-30 17:47 PDT by Simon Fraser (smfr)
Modified: 2009-03-02 11:51 PST (History)
4 users (show)

See Also:


Attachments
Patch (32.26 KB, patch)
2008-11-10 19:01 PST, Pierre-Olivier Latour
simon.fraser: review-
Details | Formatted Diff | Diff
Patch v2 (33.13 KB, patch)
2008-11-11 13:12 PST, Pierre-Olivier Latour
no flags Details | Formatted Diff | Diff
patch v3 (32.33 KB, patch)
2008-11-13 11:25 PST, Pierre-Olivier Latour
sam: review+
Details | Formatted Diff | Diff
Patch v4 (32.69 KB, patch)
2008-11-13 14:43 PST, Pierre-Olivier Latour
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-09-30 17:47:50 PDT
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.
Comment 1 Pierre-Olivier Latour 2008-10-09 15:13:08 PDT
I'll be taking a pass at it once Chris has checked in some animation changes.
Comment 2 Pierre-Olivier Latour 2008-10-30 15:08:00 PDT
Chris: what bugzilla or radar is blocking this work?
Comment 3 Pierre-Olivier Latour 2008-11-10 19:01:47 PST
Created attachment 25039 [details]
Patch

Reviewed by Chris Marrin
Comment 4 Simon Fraser (smfr) 2008-11-10 20:23:41 PST
> 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.
Comment 5 Pierre-Olivier Latour 2008-11-11 12:47:30 PST
(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.
Comment 6 Pierre-Olivier Latour 2008-11-11 13:12:06 PST
Created attachment 25052 [details]
Patch v2
Comment 7 Pierre-Olivier Latour 2008-11-13 11:25:57 PST
Created attachment 25125 [details]
patch v3
Comment 8 Sam Weinig 2008-11-13 11:45:15 PST
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.
Comment 9 Pierre-Olivier Latour 2008-11-13 14:40:08 PST
(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
Comment 10 Pierre-Olivier Latour 2008-11-13 14:43:27 PST
Created attachment 25138 [details]
Patch v4
Comment 11 Dean Jackson 2008-11-17 14:06:49 PST
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
Comment 12 Eric Seidel (no email) 2008-11-20 14:35:39 PST
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. :)
Comment 13 Pierre-Olivier Latour 2008-11-20 16:19:47 PST
> 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.