Should let us pref things on and off more easily.

Hmm. How much do we care about dynamic changes? Making them work for normal properties is not too bad, I think, but making them work for aliases is a pain. In particular, I was going to allow LookupProperty() to work no matter what, since that can be used on things we already parsed as prop names, in theory, and just cut things off in the parser, but for aliases the parser only sees the real prop id, so I have to actually break change LookupProperty. Of course if changing LookupProperty in general is OK, that seems like the way to go. David, thoughts?

Comment on attachment 639969 [details] [diff] [review] Allow preference control over what CSS properties we parse. >diff --git a/content/smil/nsSMILAnimationController.cpp b/content/smil/nsSMILAnimationController.cpp >diff --git a/content/smil/nsSMILCompositor.cpp b/content/smil/nsSMILCompositor.cpp >diff --git a/content/svg/content/src/nsSVGElement.cpp b/content/svg/content/src/nsSVGElement.cpp Maybe have someone who knows the SMIL code better have a look at these? \>+ static bool prefObserversInited = false; >+ if (!prefObserversInited) { >+ prefObserversInited = true; >+ >+ #define OBSERVE_PROP(pref_, id_) \ >+ if (pref_[0]) { \ >+ Preferences::AddBoolVarCache(&gPropertyEnabled[eCSSProperty_##id_], \ >+ pref_); \ >+ } >+ #define CSS_PROP(name_, id_, method_, flags_, pref_, parsevariant_, \ >+ kwtable_, stylestruct_, stylestructoffset_, animtype_) \ >+ OBSERVE_PROP(pref_, id_) >+ #include "nsCSSPropList.h" >+ #undef CSS_PROP I feel like this would be clearer if all the preprocessor stuff were indented another 2 spaces. Then again, I suppose I've written a bunch of stuff in this style. > nsCSSProperty >-nsCSSProps::LookupProperty(const nsACString& aProperty) >+nsCSSProps::LookupProperty(const nsACString& aProperty, >+ EnabledState aEnabled) > { > NS_ABORT_IF_FALSE(gPropertyTable, "no lookup table, needs addref"); > > nsCSSProperty res = nsCSSProperty(gPropertyTable->Lookup(aProperty)); > // Check eCSSAliasCount against 0 to make it easy for the > // compiler to optimize away the 0-aliases case. > if (eCSSAliasCount != 0 && res == eCSSProperty_UNKNOWN) { > for (const CSSPropertyAlias *alias = gAliases, > *alias_end = ArrayEnd(gAliases); > alias < alias_end; ++alias) { >- if (aProperty.LowerCaseEqualsASCII(alias->name)) { >+ if (aProperty.LowerCaseEqualsASCII(alias->name) && alias->enabled) { You need to look at aEnabled here too, before failing if !alias->enabled. (Twice.) Or was this what comment 1 (which I don't completely understand, but which I *think* predates the idea of passing a parameter to LookupProperty) was about? r=dbaron with that

> You need to look at aEnabled here too, before failing if !alias->enabled. Good catch! Definitely need to do that. Comment 1 is obsoleted by passing the parameter to LookupProperty.

Comment on attachment 639969 [details] [diff] [review] Allow preference control over what CSS properties we parse. Daniel, can you look over the SMIL changes here, please?

Comment on attachment 639969 [details] [diff] [review] Allow preference control over what CSS properties we parse. > nsISMILAttr* > nsSMILCompositor::CreateSMILAttr() > { > if (mKey.mIsCSS) { > nsCSSProperty propId = >- nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName)); >+ nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName), >+ nsCSSProps::eAny); I think we want to pass eEnabled here, just like the other SMIL chunks of this patch. (so that we'll only allow ourselves to animate enabled properties) (We aren't guaranteed that mKey.mAttributeName is a valid (or enabled) CSS property at this point. Now, if we've hit the code touched by the first chunk of this patch, _then_ we do have that guarantee, because we already would have called LookupProperty there -- but we could've skipped over that chunk entirely, if the animation explicitly has <animate attributeType="CSS" ...>) With that, r=me on the SVG/SMIL chunks.

Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc62d5247fe