914432 - Add support for multiple prefs to enable/disable values in the same property keyword table

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

Add support for multiple prefs to enable/disable values in the same property keyword table.  This is needed for display:contents (bug 907396).
The existing mechanism (to enable/disable flexbox) only allows for one
pref per keyword table.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

As far as I can tell, the only problem is nsCSSProps::FindIndexOfKeyword
which bails out at the first encountered eCSSKeyword_UNKNOWN.  The fix
is to continue until we see the real sentinel.
I also cleaned up ValueToKeywordEnum slightly to make it more readable
and wrote the new FindIndexOfKeyword in a similar style so that the two
methods can be compared.

Daniel, since you already looked at this problem, do you remember if
there were any other issues?

FYI, bug 907396 will later add:
   // The next two entries are controlled by the layout.css.flexbox.enabled pref.
   eCSSKeyword_flex,               NS_STYLE_DISPLAY_FLEX,
   eCSSKeyword_inline_flex,        NS_STYLE_DISPLAY_INLINE_FLEX,
+  // The next entry is controlled by the layout.css.display_contents.enabled
+  // pref.
+  eCSSKeyword_contents,           NS_STYLE_DISPLAY_CONTENTS,
   eCSSKeyword_UNKNOWN,-1
 };
 
https://tbpl.mozilla.org/?tree=Try&rev=4f6876e531f1
I've manually tested all four combinations of enable/disable for the two prefs
and appears to work as intended.

Comment 2

[Daniel Holbert [:dholbert]]

I posted what amounts to mostly the same patch back on bug 796212, as a final patch, but ended up not landing it after chatting with bz in the last few comments there.

He was worried that it'd slow down keyword-table lookups (bug 796212 comment 19); I pointed out that it only adds overhead in keyword-table lookups *that fail* (bug 796212 comment 22), which makes it less problematic; he agreed (bug 796212 comment 23); but we ended up not landing it since it wasn't needed, and neither of us was convinced that we wouldn't end up with a better long-term idea.

So anyway: this sort of solution should be fine, but it's worth making sure you're not adding additional overhead for the usual-case codepaths.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 801943 [details] [diff] [review]
fix

>+static bool IsKeyValSentinel(nsCSSKeyword aKey, int32_t aValue)
>+{
>+  return aValue == -1 && aKey == eCSSKeyword_UNKNOWN;
>+}
>+

Nit: It might be marginally more optimal to do the "aKey == eCSSKeyword_UNKNOWN" check first, because that means we won't have to bother even looking at aValue in the usual case where we've got a mismatched keyword which is not UNKNOWN.  (We already have to look at the keyword, because we're going to compare it against the passed-in aKeyword, up in FindIndexOfKeyword)

I suspect this will be even more likely to be true (i.e. we'll be less likely to bother reading aTable[i + 1] when we don't need to) if you declare IsKeyValSentinel as inline instead of static... might be worth doing that.

> int32_t
> nsCSSProps::FindIndexOfKeyword(nsCSSKeyword aKeyword, const int32_t aTable[])
> {
>-  int32_t index = 0;
>-  while (eCSSKeyword_UNKNOWN != nsCSSKeyword(aTable[index])) {
>-    if (aKeyword == nsCSSKeyword(aTable[index])) {
>-      return index;
>+  if (eCSSKeyword_UNKNOWN == aKeyword) {
>+    // NOTE: we can have keyword tables where eCSSKeyword_UNKNOWN is used
>+    // not only for the sentinel, but also in the middle of the table to
>+    // knock out values that have been disabled by prefs, e.g. kDisplayKTable.
>+    // So we deal with eCSSKeyword_UNKNOWN up front to avoid returning a valid
>+    // index in the loop below.
>+    return -1;
>+  }

I'm not sure this initial check is worth it.  Do we actually rely on FindIndexOfKeyword(eCSSKeyword_UNKNOWN) having any particular behavior?  It seems like any such calls would be bogus -- and if we don't have any such calls, it'd be nice to avoid having to do this check for all keyword lookups.

If we can, I'd suggest replacing this clause with an assertion, e.g.:
  MOZ_ASSERT(aKeyword != eCSSKeyword_UNKNOWN,
             "Why is someone looking for UNKNOWN in our keyword table?");
...and a version of your comment explaining that UNKNOWN can occur various places in our keyword table, so callers shouldn't expect useful results if they pass it as an argument.

Comment 4

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Nit: It might be marginally more optimal to do the "aKey ==
> eCSSKeyword_UNKNOWN" check first,

OK, I'll do that, thanks.

> if you declare
> IsKeyValSentinel as inline instead of static... 

I fully expect the compiler to inline this function where its used
without explicitly specifying 'inline'.  I chose 'static' to avoid
an external link symbol, does 'inline' also guarantee that?

Perhaps 'static inline' is what we want, but I still feel like 'inline'
is somewhat redundant in this case... and in general I leave it to the
compiler to decide what to inline.

> I'm not sure this initial check is worth it.  Do we actually rely on
> FindIndexOfKeyword(eCSSKeyword_UNKNOWN) having any particular behavior? 

Yes, I actually tried the very assertion you suggested and saw that it
was used.  Here's one example:
http://hg.mozilla.org/mozilla-central/annotate/680c89c76100/layout/style/nsCSSParser.cpp#l6936
(where we call FindKeyword four times with eCSSKeyword_UNKNOWN before
we end up in the else-part at the end)

I didn't investigate further than that.  It might worth dealing with
eCSSKeyword_UNKNOWN up front in FindKeyword though, and forbidding it
in FindIndexOfKeyword.  It looks like that's doable since the only
other caller of FindIndexOfKeyword is our pref code in nsLayoutUtils:
http://mxr.mozilla.org/mozilla-central/search?string=FindIndexOfKeyword

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #4)
> It might worth dealing with
> eCSSKeyword_UNKNOWN up front in FindKeyword though, and forbidding it
> in FindIndexOfKeyword.

(I don't have strong feelings on which one's responsible for the check; it works out pretty much the same, either way.)

I do think it might make sense to fix up the call-site you found, so that it stops trying to look up UNKNOWN in a keyword table 4 times, but that's orthogonal to this bug.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #0)
> This is needed for display:contents (bug 907396).

I think this is no longer technically needed (or at least, it won't be needed for long), since AIUI "display:contents" is going to be renamed to its own "box" property in bug 917313.

Regardless, though, it's probably still a good idea to fix this bug, since we're bound to hit a situation like this at some point in the not-too-distant-future.

Comment 7

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

Comment on attachment 801943 [details] [diff] [review]
fix

r=dbaron; sorry for the delay

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e2909a86db2d