Last Comment Bug 482402 - (enablesmil) Enable "svg.smil.enabled" pref by default
(enablesmil)
: Enable "svg.smil.enabled" pref by default
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 9 votes (vote)
: mozilla1.9.3a1
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 436276 436296 animateMotion 473705 473904 474049 474739 571863
Blocks: 470868 svg11tests acid3 532997
  Show dependency treegraph
 
Reported: 2009-03-09 21:49 PDT by Daniel Holbert [:dholbert]
Modified: 2012-04-04 03:34 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch: enable SMIL pref by default (779 bytes, patch)
2009-03-12 15:02 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch: enable SMIL pref by default, and enable SMIL reftests (1.43 KB, patch)
2009-10-22 15:34 PDT, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2009-03-09 21:49:17 PDT

    
Comment 1 Daniel Holbert [:dholbert] 2009-03-09 21:54:21 PDT
I'm filing this bug on default-enabling the "svg.smil.enabled" pref, once we're sure that SVG Animation will be "ready for shipping" before the next release off of Trunk.

See discussion about this topic on this newsgroup thread:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/4cf4cbdcbeeab7de
Comment 2 Daniel Holbert [:dholbert] 2009-03-09 21:56:16 PDT
Note that it hasn't yet been decided what "ready for shipping" would include, but to start, it probably should include support for animating all SVG attributes (Bug 436276) and ideally CSS properties, too (Bug 474049).
Comment 3 Bill Gianopoulos [:WG9s] 2009-03-12 13:18:21 PDT
I added 3 more bugs to list that that I thought should at least be looked at, and made them all block this bug.  Feel free to remove them if you determine they don't really block flipping the pref.
Comment 4 Helder "Lthere" Magalhães 2009-03-12 14:54:04 PDT
(In reply to comment #3)
> I added 3 more bugs to list that that I thought should at least be looked at,
> and made them all block this bug.

I took a look at the dependencies but didn't find any related issues... Did I miss something?
Comment 5 Daniel Holbert [:dholbert] 2009-03-12 14:57:49 PDT
Helder: it's easiest to just look at the "History" look in the upper-right corner of this bug page -- that shows that Bill recently added bug 436296, bug 436418, and bug 474739 as dependencies. (along with the bugs I noted in comment 2)
Comment 6 Daniel Holbert [:dholbert] 2009-03-12 14:58:17 PDT
(In reply to comment #5)
> Helder: it's easiest to just look at the "History" look
er, "History" *link*
Comment 7 Daniel Holbert [:dholbert] 2009-03-12 15:02:36 PDT
Created attachment 367113 [details] [diff] [review]
patch: enable SMIL pref by default

Here's a patch to enable the pref by default (when we're ready).
Comment 8 Helder "Lthere" Magalhães 2009-03-12 15:08:30 PDT
(In reply to comment #5)
> Helder: it's easiest to just look at the "History" look in the upper-right
> corner of this bug page

I saw the dependencies but somehow got the feeling they were lying there for a
while already -- didn't remember to check the report's history. Thanks for the
enlightenment! :-)
Comment 9 Bill Gianopoulos [:WG9s] 2009-03-12 15:30:16 PDT
I was thinking on this further.  There are really 3 buckets these bugs could
fall into.

1.  Bugs that you don't want to enable SMIL until they are fixed.

2.  Bugs that you don't want to release a beta with SMIL enabled unless they
are fixed.

3.  Bugs you don't want to do an official release with SMIL enabled.

Probably all of these should block a release and perhaps some block releasing a
beta.  I am not sure any of them warrant not enabling SMIL in a pre-alpha1
build to get more testing to make sure this does not cause other issues.

So kind of a mark all these as blocking next release, fix any that belong in bucket1 then flip the pref and prioritize the remaining bugs for where in the release cycle they need to be fixed.
Comment 10 Daniel Holbert [:dholbert] 2009-10-22 15:34:10 PDT
Created attachment 407867 [details] [diff] [review]
patch: enable SMIL pref by default, and enable SMIL reftests
Comment 11 Daniel Holbert [:dholbert] 2009-10-22 15:53:08 PDT
Landed:
http://hg.mozilla.org/mozilla-central/rev/c5b353307fb8
Comment 12 Jeff Muizelaar [:jrmuizel] 2009-10-23 06:24:49 PDT
Looks like this regressed tSVG by 1%. Does that seem possible?
Comment 13 Daniel Holbert [:dholbert] 2009-10-23 11:12:56 PDT
Definitely seems possible, if tSVG uses pages with SVG animation elements, and/or SVG animation APIs.
Comment 14 Daniel Holbert [:dholbert] 2009-10-23 11:37:41 PDT
Hmm, looks like comment 13 was off the mark -- looks like Talos doesn't use any animation.

I just grabbed latest talos from CVS using this command[1]:
> cvs -d :pserver:anonymous@cvs-mirror.mozilla.org:/cvsroot co -d talos mozilla/testing/performance/talos
...and I examined the "talos/page_load_test/svg" directory (which  "sample.config" says is used for tSVG

I couldn't find any animation-related stuff there, using grep -ir animate, grep -ir pause, grep -ir currenttime, and grep -ir "<set".  So, this isn't a result of using any SMIL elements/APIs.

So, what could the tSVG hit be from, in that case? This bug's patch can only affected code that calls NS_SMILEnabled[2] (a getter for the about:config pref), and that's not very much code.  My guess is that any perf hit would be due to calls to FlushAnimations[3].  That function is called from a number of places, and its behavior depends on SMIL being enabled because it uses "GetAnimationController()" (which returns null unless NS_SMILEnabled() is on).

With no actual animation in the document, FlushAnimations has a pretty minimal impact, but I could see it having a small hit, since it's called in every nsSVGLength2 getter function and in nsSVGElement::GetAnimatedLengthValues.

[1] source:  https://wiki.mozilla.org/User:Standard8/Talos_Notes
[2] http://mxr.mozilla.org/mozilla-central/ident?i=NS_SMILEnabled
[3] http://mxr.mozilla.org/mozilla-central/search?string=flushanimations

Note You need to log in before you can comment on or make changes to this bug.