771594 - CSS parser should pay attention to the _pref field for CSS properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Should let us pref things on and off more easily.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow preference control over what CSS properties we parse.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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 5

[Boris Zbarsky [:bzbarsky]]

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 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Thanks!

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bc62d5247fe