The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla16
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-kilimanjaro:?)

Details

Attachments

(1 attachment)

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?
Created attachment 639969 [details] [diff] [review]
Allow preference control over what CSS properties we parse.
Attachment #639969 - Flags: review?(dbaron)
Whiteboard: [need review]

Updated

5 years ago
blocking-kilimanjaro: --- → ?
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
Attachment #639969 - Flags: review?(dbaron) → review+
> 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?
Attachment #639969 - Flags: review?(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.
Attachment #639969 - Flags: review?(dholbert) → review+
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc62d5247fe
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/2bc62d5247fe
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.