Bug 92025 – SVG animation not working for elements inserted after parsing is finished

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

[Florin Malita]

Created attachment 153859 [details]
Fill animation not working

Animation is activated from Document::implicitClose(), but deferred element insertion doesn't seem to hit that path.

[Florin Malita]

Created attachment 155829 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 155829 [details]
Patch

Looks sane, r=me.

[Dirk Schulze]

Dicsussing this patch on IRC now. At least the layout test needs an update.

[Dirk Schulze]

Comment on attachment 155829 [details]
Patch

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

> LayoutTests/svg/animations/deferred-insertion.html:16
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

We discussed the patch online during your review Niko. I am fine with the patch as well, but window.layoutTestController needs to be replaced.

[Florin Malita]

Created attachment 155847 [details]
Patch for landing

[Florin Malita]

Thanks for reviewing this Niko & Dirk.

[Nikolas Zimmermann]

(In reply to comment #4)
> We discussed the patch online during your review Niko. I am fine with the patch as well, but window.layoutTestController needs to be replaced.
Good catch!

[WebKit Review Bot]

Comment on attachment 155847 [details]
Patch for landing

Clearing flags on attachment: 155847

Committed r124369: <http://trac.webkit.org/changeset/124369>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

I think this change may have introduced a performance regression:
http://webkit-perf.appspot.com/graph.html#tests=[[1610803,2001,173262]]&sel=none&displayrange=7&datatype=running

Pageload time went from 88ms to 92ms average. None of the boolean checks added in this patch are more than just local variable reads, so I don't know what's up.

[Florin Malita]

(In reply to comment #10)
> I think this change may have introduced a performance regression:
> http://webkit-perf.appspot.com/graph.html#tests=[[1610803,2001,173262]]&sel=none&displayrange=7&datatype=running
> 
> Pageload time went from 88ms to 92ms average. None of the boolean checks added in this patch are more than just local variable reads, so I don't know what's up.

This seems unlikely not only because it's a lightweight test, but also because that code path is only hit on <svg> element insertion. And for this particular perftest, the conditional gets shortcut on the first test (document()->parsing() is true) as the SVG element is inserted during parsing.

Have you ruled out all the other changes in range?

[Florin Malita]

Created attachment 156402 [details]
crawfish2_ganson.svg perftest run

I ran the perftest locally, see attached graph. First 40 runs are without this patch, last 40 with it - I cannot see a difference.