Enable SVG Animation (SMIL) in builds by default

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
SVG
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [evang-wanted])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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)
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

8 years ago
Flags: wanted1.9.2?
Flags: blocking1.9.2?

Comment 2

8 years ago
This probably needs to block 410460, the acid3 tracking bug.. im not sure how to do that

Comment 3

8 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...

Updated

8 years ago
Blocks: 410460
(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)

Updated

8 years ago
Depends on: 473904
(Assignee)

Comment 5

8 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

8 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.
(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

8 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)
Definitely best to leave off until ready to stay on for good. We don't need another debacle like Fx2 Places.
(Assignee)

Comment 10

8 years ago
Created attachment 357419 [details] [diff] [review]
patch: switch from --enable-smil to --disable-smil

This patch enables smil by default, with a "--disable-smil" flag available for anyone who doesn't want to build with it.
(Assignee)

Updated

8 years ago
Assignee: nobody → dholbert
(Assignee)

Comment 11

8 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

8 years ago
Created attachment 366476 [details] [diff] [review]
patch v2: also disable smil when SVG is disabled

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

8 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

8 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

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 482402
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 → ---
It's not specified explicitly either way:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#227
(Assignee)

Comment 17

8 years ago
Created attachment 366534 [details] [diff] [review]
followup patch: mochitest fixes (toggle smil pref in each test)

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

8 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

8 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

8 years ago
Created attachment 366622 [details] [diff] [review]
alternate followup patch: enable pref when setting up mochitest profile

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

8 years ago
Created attachment 366634 [details] [diff] [review]
alternate followup patch: enable pref when setting up mochitest profile

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

8 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

8 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
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #366534 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Flags: wanted1.9.2?
Flags: blocking1.9.2?
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.
Depends on: 483584
Keywords: dev-doc-needed
Last update here was from March 15th - do we know if we want to drive this into 3.6?
(Assignee)

Comment 28

8 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.
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.
There's no need to remove code, it's a build option.
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

8 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.
Bug 512594 is for turning off SMIL on 1.9.2 branch.
Keywords: dev-doc-needed
Blocks: 512594
Blocks: 541203
Whiteboard: [evang-wanted]
You need to log in before you can comment on or make changes to this bug.