984728 - ParseGridAutoFlow should use ParseBitmaskValues

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

While reviewing bug 981752 patch 2b, I'm realizing ParseGridAutoFlow should use ParseChoice, and then sanity-check the results of that, rather than hard-coding its own "parse a subset of these keywords in any order" process.

(I'm spinning off this as a helper-bug to avoid scope-creep over there.)

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Sorry, my bad -- ParseChoice isn't right here. It's for shorthands, and I used it for ParseFlexFlow which *is* a shorthand, and which is somewhat similar in structure to the (non-shorthand) grid-auto-flow (in that it can take row/column and an optional suffix/prefix).

It might be that ParseBitmaskValues() might be closer to what we want, but I'm not sure yet.

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Yeah, I'm pretty sure ParseBitmaskValues is exactly what we should be using in ParseGridAutoFlow. (particularly given that we end up representing the result in a bitmask)

We probably should ditch "none" from ParseGridAutoFlow's initial ParseVariant call, too, and let the ParseBitmaskValues call handle it (since the "none" keyword is in our keyword table), I think.

Comment 3

[Simon Sapin (:SimonSapin)]

Attachment: Patch

Comment 4

[Simon Sapin (:SimonSapin)]

Attachment: Patch v2

Comment 5

[Simon Sapin (:SimonSapin)]

Attachment: Patch v3

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8394633 [details] [diff] [review]
Patch v3

>+// End-of-array marker for mask arguments to ParseBitmaskValues
>+#define MASK_END_VALUE  -1

Use parens here -- i.e.:
  #define MASK_END_VALUE  (-1)

Otherwise, this macro will mysteriously be accepted in contexts where we wouldn't like it to.

e.g.: this would be treated as a valid number (when we'd prefer that it be a compiler error):
  5 MASK_END_VALUE
since it evaluates to 5 -1 which is 4.

>   nsCSSValue value;
>   if (ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE, nullptr)) {
>     AppendValue(eCSSProperty_grid_auto_flow, value);
>     return true;
>   }
[...]
>+  // Do not parse 'none' here, it's already covered above.

I would prefer that we *do* parse 'none' as part of the ParseBitmaskValues() call (and leave ParseVariant to *only* do INHERIT), for consistency in how we handle 'none' for this property. (as a keyword-table entry, which we encode in a bitmask)

This will mean we can drop the "case eCSSUnit_None:" special-case for this property in nsRuleNode.cpp, too (which we should do as part of this patch).

>+  static const int32_t mask[] = {
>+    NS_STYLE_GRID_AUTO_FLOW_COLUMN | NS_STYLE_GRID_AUTO_FLOW_ROW,
>+    NS_STYLE_GRID_AUTO_FLOW_NONE | NS_STYLE_GRID_AUTO_FLOW_COLUMN,
>+    NS_STYLE_GRID_AUTO_FLOW_NONE | NS_STYLE_GRID_AUTO_FLOW_ROW,
>+    NS_STYLE_GRID_AUTO_FLOW_NONE | NS_STYLE_GRID_AUTO_FLOW_DENSE,
>+    MASK_END_VALUE
>+  };

Instead of all of these exclusion bitfields for _NONE with every other possible value, we should just have a single check after ParseBitmaskValues() like so:

 // 'none' is only valid if it occurs alone:
 if ((bitField & NS_STYLE_GRID_AUTO_FLOW_NONE) &&
     bitfield != NS_STYLE_GRID_AUTO_FLOW_NONE) {
   return false;
 }

This will be more future-proof, and it also makes our ParseBitmaskValues() code more efficient in the usual case (because we have fewer masks to loop across on each progressive keyword that ParseBitmaskValues() gets for us.)

(I believe one of the other ParseBitmaskValues clients already does something like this, too.)

Comment 7

[Simon Sapin (:SimonSapin)]

Attachment: Patch v4

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8394882 [details] [diff] [review]
Patch v4

Looks good. Thanks!

Comment 9

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(Simon, could you give this a Try run & add checkin-needed when we're sure it's good to go?)

Comment 10

[Simon Sapin (:SimonSapin)]

https://tbpl.mozilla.org/?tree=Try&rev=ed01484cd993

Comment 11

[Simon Sapin (:SimonSapin)]

The two orange bits seem to be intermittent.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d475e5c4d9e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d475e5c4d9e