Closed
Bug 473705
Opened 16 years ago
Closed 16 years ago
Enable SVG Animation (SMIL) in builds by default
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
(Whiteboard: [evang-wanted])
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
Bug 216462 (which is landing in the next day or so) adds basic SVG Animation support, but it's disabled in builds by default for now.
I'm filing this bug on enabling it by default, once it's ready.
(For now, you can enable it in builds by adding "ac_add_options --enable-smil" to your .mozconfig)
Comment 1•16 years ago
|
||
How is this gonna get real world exposure being disabled? It landed on m-c and probably has no chance of landing on 1.9.1, so it should be enabled by default on trunk. Just my .02.
Assignee | ||
Updated•16 years ago
|
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Comment 2•16 years ago
|
||
This probably needs to block 410460, the acid3 tracking bug.. im not sure how to do that
Comment 3•16 years ago
|
||
Indeed. Too bad it is not enabled by default right now. It would have been great to have it for shiretoko, but as it is already going for beta3...
Comment 4•16 years ago
|
||
(In reply to comment #1)
> How is this gonna get real world exposure being disabled?
I also agree that making this available only at compilation-time makes this pretty restrictive. I'm a strong advocate of an approach based on enabling the code but disabling it's action by default through a preference. I'm convinced that this would dramatically raise the user base for beta testing, as there are many curious users willing to help but who aren't experienced enough (and/or willing) to go through the compilation hassle. ;-)
Assignee | ||
Comment 5•16 years ago
|
||
(In reply to comment #1, comment #4)
> How is this gonna get real world exposure being disabled?
This question was brought up to some extent in a thread that roc started on mozilla.dev.platform, linked here:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/4cf4cbdcbeeab7de
Basically, this feature isn't really ready yet for large-scale "real world exposure", which is why it's currently default-disabled. It's still fairly experimental, and it's incomplete in many areas.
However, it seems like it's probably best to default-disable it at run-time via an about:config pref, rather than at build-time, so that interested users can play around without needing custom builds (as suggested in comment 4). So, I've filed bug 473904 on adding an about:config pref, and once that's done, we can hopefully start building with SMIL support by default.
(In reply to comment #3)
> It would have been
> great to have it for shiretoko, but as it is already going for beta3...
Shiretoko / Firefox 3.1 is a separate code branch from the mozilla-central trunk, and it's way past being feature-frozen, so we're not planning on landing SMIL support (even in a disabled state) on that branch. This is a post-Firefox-3.1 feature.
Comment 6•16 years ago
|
||
How about just keeping it active on trunk and disabling it on any future branches (after branching) if it's deemed not ready for release?
about:config "prefs" are not really prefs, putting it there would make it just as disabled as by compile-time switch but with extra code bloat.
Comment 7•16 years ago
|
||
(In reply to comment #5)
> However, it seems like it's probably best to default-disable it at run-time via
> an about:config pref, rather than at build-time, so that interested users can
> play around without needing custom builds (as suggested in comment 4). So,
> I've filed bug 473904 on adding an about:config pref, and once that's done, we
> can hopefully start building with SMIL support by default.
Great! :-) I'm sure that this will really help towards a much broader audience being able to early try out this new feature: changing a preference is at the fingertips of any mortal (naturally, when aware of the possibility). ;-)
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #6)
> How about just keeping it active on trunk and disabling it on any future
> branches (after branching) if it's deemed not ready for release?
I'm pretty sure it should to be disabled-by-default (either via about:config or via a build option) until we're confident it'll be "ready" (whatever we decide that means) for the next release. This is the basic issue roc brought up & beltzner agreed on, in the first two posts in the newsgroup thread I linked above -- see that thread for more info.
If you disagree, please reply on that newsgroup thread, rather than on this bug. (so that we minimize parallel discussions about the same topic)
Comment 9•16 years ago
|
||
Definitely best to leave off until ready to stay on for good. We don't need another debacle like Fx2 Places.
Assignee | ||
Comment 10•16 years ago
|
||
This patch enables smil by default, with a "--disable-smil" flag available for anyone who doesn't want to build with it.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Comment 11•16 years ago
|
||
We should probably also catch any disabling of MOZ_SVG (via "--disable-svg") and have that force MOZ_SMIL to be disabled as well. (even if "--disable-smil" isn't explicitly specified).
(Right now, I think a build with SMIL support but with SVG support disabled would be pretty broken & almost certainly wouldn't compile.)
Assignee | ||
Comment 12•16 years ago
|
||
This patch...
(a) default-enables SMIL in builds
(b) changes the build option from --enable-smil to --disable-smil
(c) force-disables MOZ_SMIL whenever MOZ_SVG is disabled (per comment 11)
Attachment #357419 -
Attachment is obsolete: true
Attachment #366476 -
Flags: superreview?
Attachment #366476 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #366476 -
Flags: superreview?(roc)
Attachment #366476 -
Flags: superreview?
Attachment #366476 -
Flags: review?(roc)
Attachment #366476 -
Flags: review?
Attachment #366476 -
Flags: superreview?(roc)
Attachment #366476 -
Flags: superreview+
Attachment #366476 -
Flags: review?(roc)
Attachment #366476 -
Flags: review+
Assignee | ||
Comment 13•16 years ago
|
||
patch v2 landed: http://hg.mozilla.org/mozilla-central/rev/ca2d45349d89
(Note that SMIL is currently still disabled at _run time_ by default via the "svg.smil.enabled" pref added in Bug 473904, in accordance with comment 5 & the newsgroup thread linked from that comment)
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Blocks: enablesmil
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Looks like the tests aren't passing:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236660394.1236670091.1407.gz
John Daggett backed out this patch due to failing mochitests. Is SMIL enabled in the mochitest profile?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 16•16 years ago
|
||
It's not specified explicitly either way:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#227
Assignee | ||
Comment 17•16 years ago
|
||
Thanks for catching those testfailures -- sorry that I didn't catch it sooner myself & back out.
Here's a patch that fixes the mochitest failures by enabling the SMIL about:config pref at the beginning of each test, and clearing it just before the end of each test. Once the tree is greenish, this should be fine for re-landing.
Assignee | ||
Updated•16 years ago
|
Attachment #366534 -
Attachment description: followup patch: mochitest fixes (toggle pref using pref service) → followup patch: mochitest fixes (toggle smil pref in each test)
You could also just flip the pref in the file in comment 16. That seems simpler, especially as more SMIL tests are added.
Comment 19•16 years ago
|
||
Also if you flip the pref per comment 16 you won't need to revert anything when SMIL is enabled by default.
Assignee | ||
Comment 20•16 years ago
|
||
Here's a patch that toggles the pref in automation.py.in, per comment 16 / comment 18 / comment 19.
roc, do you think this is the right way to go?
Attachment #366622 -
Flags: superreview?(roc)
Attachment #366622 -
Flags: review?(roc)
The svg.smil.enabled pref is false by default right? Don't you want to set it to true here?
Assignee | ||
Comment 22•16 years ago
|
||
erm, yes. Thanks for that correction. :)
Attachment #366622 -
Attachment is obsolete: true
Attachment #366634 -
Flags: superreview?(roc)
Attachment #366634 -
Flags: review?(roc)
Attachment #366622 -
Flags: superreview?(roc)
Attachment #366622 -
Flags: review?(roc)
Assignee | ||
Comment 23•16 years ago
|
||
My build just finished, & I tested the fixed "alternate followup patch" & confirmed that we pass the SMIL mochitests with it.
Attachment #366634 -
Flags: superreview?(roc)
Attachment #366634 -
Flags: superreview+
Attachment #366634 -
Flags: review?(roc)
Attachment #366634 -
Flags: review+
Assignee | ||
Comment 24•16 years ago
|
||
Pushed mochitest fix: http://hg.mozilla.org/mozilla-central/rev/d21a0036ef5c
Re-pushed "patch v2": http://hg.mozilla.org/mozilla-central/rev/a22156e4d71a
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Attachment #366534 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Comment 25•16 years ago
|
||
So for what it's worth, this added about 100KB of additional codesize on Mac (hard to tell for Linux because the test has been broken for the last several days).
Was that about how much was expected?
I didn't have a particular expectation, but there is a significant amount of new code being built, yes.
Updated•15 years ago
|
Keywords: dev-doc-needed
Comment 27•15 years ago
|
||
Last update here was from March 15th - do we know if we want to drive this into 3.6?
Assignee | ||
Comment 28•15 years ago
|
||
Well, this shouldn't currently affect Firefox behavior, since SMIL is preffed off by default (see bug 482402). And for those users interested in SVG/SMIL (and those interested in seeing high Acid 3 scores), I think it's handy to be able to be able to just flip the pref and play.
Would the reason to remove it be that we want to make the binary a little smaller, per comment 25?
I actually think we should disable compilation of SMIL on branch. It's extra code that's unused, and adds scriptable API that doesn't work.
Comment 30•15 years ago
|
||
I am not at all sure what the right answer here is, but it seems to me that removing this creates yet another code fork. Removing this code would also kind of subvert the entire idea of landing code on the trunk first before the branch becuase it makes code that has landed on the trunk less likely to even be able to build on the branch each time something like this that has been on the trunk for a long time and is now being removed from the branch.
Comment 31•15 years ago
|
||
There's no need to remove code, it's a build option.
Comment 32•15 years ago
|
||
Actually I think the answer I ma really interested in, but fail to understand, is exactly what is the point of the Gecko 1.9.2/Firefox 3.6 release? Seems to be a big yawn that everyone will ignore and just run trunk.
Assignee | ||
Comment 33•15 years ago
|
||
(In reply to comment #29)
> I actually think we should disable compilation of SMIL on branch. It's extra
> code that's unused, and adds scriptable API that doesn't work.
FWIW, if SMIL is preffed off (as it is by default), I believe the scriptable API should behave exactly as if SMIL were disabled in builds. (see the patch that added the pref in bug 473904) If there's a case where that's not true, that's a bug.
Comment 34•15 years ago
|
||
Bug 512594 is for turning off SMIL on 1.9.2 branch.
Updated•15 years ago
|
Keywords: dev-doc-needed
Updated•15 years ago
|
Whiteboard: [evang-wanted]
You need to log in
before you can comment on or make changes to this bug.
Description
•