941 bytes, patch
|Details | Diff | Splinter Review|
954 bytes, patch
|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)
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.
This probably needs to block 410460, the acid3 tracking bug.. im not sure how to do that
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...
(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. ;-)
(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.
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). ;-)
(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.
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.
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.)
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)
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)
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?
It's not specified explicitly either way: http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#227
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.
You could also just flip the pref in the file in comment 16. That seems simpler, especially as more SMIL tests are added.
Also if you flip the pref per comment 16 you won't need to revert anything when SMIL is enabled by default.
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?
The svg.smil.enabled pref is false by default right? Don't you want to set it to true here?
Created attachment 366634 [details] [diff] [review] alternate followup patch: enable pref when setting up mochitest profile erm, yes. Thanks for that correction. :)
My build just finished, & I tested the fixed "alternate followup patch" & confirmed that we pass the SMIL mochitests with it.
Pushed mochitest fix: http://hg.mozilla.org/mozilla-central/rev/d21a0036ef5c Re-pushed "patch v2": http://hg.mozilla.org/mozilla-central/rev/a22156e4d71a
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.
Last update here was from March 15th - do we know if we want to drive this into 3.6?
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.
(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.