| Summary: | SVG animation not working for elements inserted after parsing is finished | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Florin Malita <fmalita> | ||||||||||
| Component: | SVG | Assignee: | Florin Malita <fmalita> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | krit, pdr, schenney, stevebrooks000, webkit.review.bot, zimmermann | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 155829 [details]
Patch
Comment on attachment 155829 [details]
Patch
Looks sane, r=me.
Dicsussing this patch on IRC now. At least the layout test needs an update. 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. Created attachment 155847 [details]
Patch for landing
Thanks for reviewing this Niko & Dirk. (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! Comment on attachment 155847 [details] Patch for landing Clearing flags on attachment: 155847 Committed r124369: <http://trac.webkit.org/changeset/124369> All reviewed patches have been landed. Closing bug. 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. (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? 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.
|
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.