Bug 78422 – Convert svg/animations to use SMIL methods for driving the timeline

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Convert svg/animations to use SMIL methods for driving the timeline.
This allows us to remove the home-brewn sampleSVGAnimationAtTime method from DRTs LayoutTestController, and the support code all around in SVGSMILElement & SMILTimeContainer.
Doing that, exposes two bugs in our SMIL implementation, that need to be resolved so that it works.

[Nikolas Zimmermann]

Created attachment 126652 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 126652 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126652&action=review

> Source/WebKit/chromium/public/WebFrame.h:-629
> -    virtual bool pauseSVGAnimation(const WebString& animationId,

Chromium WebKit API change looks good to me.

[Nikolas Zimmermann]

FYIL new-run-webkit-tests svg/animations --randomize-order --iterations=100
All 9000 tests ran as expected.
I don't see any flakiness anymore, even with -g.
Note that this patch also doesn't need any rebaselines, it's all cross platform.

[Nikolas Zimmermann]

CC'ing Dirk/Zoltan/Antti for review.

[Eric Seidel (no email)]

Comment on attachment 126652 [details]
Patch

Seems like we should be able to do this in smaller (more reviewable) pieces.

[Dirk Schulze]

Comment on attachment 126652 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126652&action=review

> Source/WebCore/ChangeLog:19
> +          via beginElement/endElement - get removed if the begin/endTImes list is updated. Why?

typo: s/endTImes/endTimes/

> Source/WebCore/ChangeLog:30
> +        - document.getElementsByTagName('animateMotion')[0], always returned 'object SVGElement', instead of

Normally this would be another bug report :)

> Source/WebCore/ChangeLog:32
> +          from SVGANimationElement were not available. Fix that by removing the last hacks from svgtags.in,

s/SVGANimationElement/SVGAnimationElement/

> Source/WebCore/ChangeLog:70
> +        * svg/svgtags.in: Enable animateMotion/hkern/mpath JS interfaces, which were not enabled, despite their IDLs existed.

Another bug report.. normally ;) Do you test them?

> LayoutTests/svg/animations/resources/SVGAnimationTestCase.js:77
> +    // We still have to name this function runRepaintTest(), as svg/dynamic-updates/

? where is repaintTest?

[Nikolas Zimmermann]

(In reply to comment #7)
> typo: s/endTImes/endTimes/
> 
> > Source/WebCore/ChangeLog:30
> > +        - document.getElementsByTagName('animateMotion')[0], always returned 'object SVGElement', instead of
> 
> Normally this would be another bug report :)
This is covered by animate-path-nested-transforms.js & animate-mpath-insert.js.

> s/SVGANimationElement/SVGAnimationElement/
Fixed.

> Another bug report.. normally ;) Do you test them?
See above, both covered by svg/animations/ tests already.

> 
> > LayoutTests/svg/animations/resources/SVGAnimationTestCase.js:77
> > +    // We still have to name this function runRepaintTest(), as svg/dynamic-updates/
Oh, that comment is outdated, will fix.

[Nikolas Zimmermann]

(In reply to comment #6)
> (From update of attachment 126652 [details])
> Seems like we should be able to do this in smaller (more reviewable) pieces.
Point taken - but I already discussed with Dirk that he'll review it as-is.

[Nikolas Zimmermann]

Committed r107682: <http://trac.webkit.org/changeset/107682>

[Nikolas Zimmermann]

Needs a follow-up change, for some last minute changes to lead to assertions in svg/animations/. Sorry.

[Nikolas Zimmermann]

Created attachment 126938 [details]
Follow-up patch

[WebKit Review Bot]

Attachment 126938 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
 + 6612fe4...3db9340 master     -> origin/master  (forced update)
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 107691 = 6612fe401de446ba8e6f5ae9e558e9f8593ff272
r107688 = 8a7f22a3d3816472d1f57c4f18232bbd60ff4597
r107689 = 818888131b08465aa4d43d5e865741d09553627e
r107690 = fe093cab606d262debc8805bb541892699e59265
r107691 = c38205f7c7b217af975be90407f3d5bc69157e8c
r107692 = 3db9340249163e9ff8eca4792746293bbc60277d
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168765 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Committed r107693: <http://trac.webkit.org/changeset/107693>

[Nikolas Zimmermann]

Reopening to attach new patch.

[Nikolas Zimmermann]

Created attachment 126955 [details]
Cleanup patch

[WebKit Review Bot]

Attachment 126955 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168768 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 126955 [details]
Cleanup patch

The "cleanup patch" does some house-keeping, and updates chromium expectations.

[WebKit Review Bot]

Comment on attachment 126955 [details]
Cleanup patch

Attachment 126955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11518262

New failing tests:
svg/animations/animate-text-nested-transforms.html

[Nikolas Zimmermann]

Created attachment 126977 [details]
Cleanup patch - v2

[WebKit Review Bot]

Attachment 126977 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 126977 [details]
Cleanup patch - v2

Attachment 126977 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11521280

New failing tests:
svg/animations/animate-text-nested-transforms.html

[Nikolas Zimmermann]

(In reply to comment #22)
> (From update of attachment 126977 [details])
> Attachment 126977 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11521280
> 
> New failing tests:
> svg/animations/animate-text-nested-transforms.html

Please ignore this failure for review, I'll skip it again, before landing, then check later on with the flakiness dashboard what's happening here. (The test is already marked as failing in trunk, I tried to revert it)

[Dirk Schulze]

Comment on attachment 126977 [details]
Cleanup patch - v2

View in context: https://bugs.webkit.org/attachment.cgi?id=126977&action=review

What is that for a style problem?

> LayoutTests/ChangeLog:13
> +        to expect values like "199.98". Use "200" instead. That doesn't reduce the

Can there still be rounding issues between platforms?

> LayoutTests/ChangeLog:-400
> -        (g.setAttribute.rect.createSVGElement.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.g.appendChild.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.g.appendChild.rootSVGElement.appendChild.startSample):

What is that? Your change log is broken, or is it really a previous commit that caused that?

> LayoutTests/ChangeLog:-408
> -        (rootSVGElement.setAttribute.text.createSVGElement.text.setAttribute.text.textContent.string_appeared_here.text.setAttribute.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.text.appendChild.rootSVGElement.appendChild.startSample):

Ditto.

> LayoutTests/svg/animations/script-tests/animate-path-nested-transforms.js:20
> +animateMotion.setAttribute("dur", "40s")

40s? Why not 4?

> LayoutTests/svg/animations/script-tests/animate-path-nested-transforms.js:41
> +        ["animation", 39.999, endSample]

Why not 3.999?

> LayoutTests/svg/animations/script-tests/animate-text-nested-transforms.js:13
> +animateMotion.setAttribute("dur", "40s")

Ditto.

[Dirk Schulze]

Ah, your ChangeLog seems indeed be broken, why does it work with the other bots?

[Nikolas Zimmermann]

(In reply to comment #24)
> (From update of attachment 126977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126977&action=review
> 
> What is that for a style problem?
Bot is wedged, see webkit-dev.

> 
> > LayoutTests/ChangeLog:13
> > +        to expect values like "199.98". Use "200" instead. That doesn't reduce the
> 
> Can there still be rounding issues between platforms?
I don't think so - you'll never know though.

> > LayoutTests/ChangeLog:-400
> > -        (g.setAttribute.rect.createSVGElement.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.g.appendChild.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.g.appendChild.rootSVGElement.appendChild.startSample):
> 
> What is that? Your change log is broken, or is it really a previous commit that caused that?
Well, prepare-ChangeLog is broken, I have to fix up the ChangeLog manually again.

> 
> > LayoutTests/ChangeLog:-408
> > -        (rootSVGElement.setAttribute.text.createSVGElement.text.setAttribute.text.textContent.string_appeared_here.text.setAttribute.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.text.appendChild.rootSVGElement.appendChild.startSample):
> 
> Ditto.
Thanks.

> 
> > LayoutTests/svg/animations/script-tests/animate-path-nested-transforms.js:20
> > +animateMotion.setAttribute("dur", "40s")
> 
> 40s? Why not 4?
Ah, I talked with Rob about this on IRC. The animation runs so quick, that 0.001s may result in quite large differences from the start value (eg. rect beginning, x=50, animating to x=5000 in 1s). So I raised the duration - to make it slower. It doesn't matter at all for testing - it doesn't make the testcase slower.

> Why not 3.999?
See above.

[Nikolas Zimmermann]

Style bot is still wedged - I will upload a new version anyway to turn cr-linux ews green.

[Nikolas Zimmermann]

Created attachment 127127 [details]
Cleanup patch - v3

[Simon Hausmann]

Comment on attachment 127127 [details]
Cleanup patch - v3

rs=me

[Nikolas Zimmermann]

Comment on attachment 127127 [details]
Cleanup patch - v3

Thanks Simon, landed patch in r107791.