Last Comment Bug 771594 - CSS parser should pay attention to the _pref field for CSS properties
: CSS parser should pay attention to the _pref field for CSS properties
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-06 11:20 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-07-14 10:03 PDT (History)
3 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?


Attachments
Allow preference control over what CSS properties we parse. (27.36 KB, patch)
2012-07-07 10:34 PDT, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
dholbert: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 11:20:27 PDT
Should let us pref things on and off more easily.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 23:49:28 PDT
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 [:bz] (still a bit busy) 2012-07-07 10:34:36 PDT
Created attachment 639969 [details] [diff] [review]
Allow preference control over what CSS properties we parse.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-07-13 16:17:11 PDT
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 [:bz] (still a bit busy) 2012-07-13 16:35:36 PDT
> 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 [:bz] (still a bit busy) 2012-07-13 16:36:21 PDT
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] 2012-07-13 16:54:49 PDT
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 [:bz] (still a bit busy) 2012-07-13 17:00:09 PDT
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc62d5247fe
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-07-14 10:03:02 PDT
https://hg.mozilla.org/mozilla-central/rev/2bc62d5247fe

Note You need to log in before you can comment on or make changes to this bug.