1299066 - Convert NS_STYLE_DISPLAY_* to an enum class, avoid linear search by display in the frame constructor.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1299066: Make NS_STYLE_DISPLAY_* an enum class.

Review commit: https://reviewboard.mozilla.org/r/75230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75230/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1299066: Rename NS_STYLE_DISPLAY_* to use the new enum class.

This was generated with the following python script:

```

import sys
import re

CAMEL_CASE_REGEX = re.compile(r"(^|_|-)([A-Z])([A-Z]+)")
DISPLAY_REGEX = re.compile(r"\bNS_STYLE_DISPLAY_([^M][A-Z_]+)\b")

def to_camel_case(ident):
  return re.sub(CAMEL_CASE_REGEX,
                lambda m: m.group(2) + m.group(3).lower(), ident)

def constant_to_enum(constant):
  return "StyleDisplay::" + to_camel_case(constant) + ("_" if constant == "NONE" else "")

def process_line(line):
  return re.sub(DISPLAY_REGEX,
                lambda m: constant_to_enum(m.group(1)), line)

lines = []
with open(sys.argv[1], "r") as f:
  for line in f:
    lines.append(process_line(line))

with open(sys.argv[1], "w") as f:
  for line in lines:
    f.write(line)
```

And the following shell commands:

```
find . -name '*.cpp' -exec python display.py {} \;
find . -name '*.h' -exec python display.py {} \;
```

Still doesn't build because some files don't use the mozilla namespace, but this
should make it better for review.

Review commit: https://reviewboard.mozilla.org/r/75232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75232/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1299066: Use the new enum class, and fix remaining build bustages except in the frame constructor.

Review commit: https://reviewboard.mozilla.org/r/75234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75234/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

Review commit: https://reviewboard.mozilla.org/r/75236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75236/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Now I'm here, I'm not sure it was worth the churn, but anyway. Sorry for the massive commit message, Boris.

Also, I'll try to squash them appropriately, but thought they might be easier to review this way.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73060

::: layout/style/nsStyleStruct.h:2993
(Diff revision 1)
> -    return mozilla::StyleDisplay::Ruby == aDisplay ||
> -           mozilla::StyleDisplay::RubyBase == aDisplay ||
> +    return aDisplay >= mozilla::StyleDisplay::Ruby &&
> +           aDisplay <= mozilla::StyleDisplay::RubyTextContainer;
> -           mozilla::StyleDisplay::RubyBaseContainer == aDisplay ||
> -           mozilla::StyleDisplay::RubyText == aDisplay ||
> -           mozilla::StyleDisplay::RubyTextContainer == aDisplay;

I expect compiler be able to work out this kind of simple optimization. I don't think we should replace those equal comparisons with a range comparison, unless you can ensure in compile time that the range includes and only includes these values. Otherwise I think this is a regression on type safety.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73060

> I expect compiler be able to work out this kind of simple optimization. I don't think we should replace those equal comparisons with a range comparison, unless you can ensure in compile time that the range includes and only includes these values. Otherwise I think this is a regression on type safety.

That's fair, I'll revert them.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75236/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1299066: Make NS_STYLE_DISPLAY_* an enum class. Prefer indexing instead of linear search in the frame constructor

Review commit: https://reviewboard.mozilla.org/r/75244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75244/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1299066: Rename NS_STYLE_DISPLAY_* to use the new enum class.

This was generated with the following python script:

```

import sys
import re

CAMEL_CASE_REGEX = re.compile(r"(^|_|-)([A-Z])([A-Z]+)")
DISPLAY_REGEX = re.compile(r"\bNS_STYLE_DISPLAY_([^M][A-Z_]+)\b")

def to_camel_case(ident):
  return re.sub(CAMEL_CASE_REGEX,
                lambda m: m.group(2) + m.group(3).lower(), ident)

def constant_to_enum(constant):
  return "StyleDisplay::" + to_camel_case(constant) + ("_" if constant == "NONE" else "")

def process_line(line):
  return re.sub(DISPLAY_REGEX,
                lambda m: constant_to_enum(m.group(1)), line)

lines = []
with open(sys.argv[1], "r") as f:
  for line in f:
    lines.append(process_line(line))

with open(sys.argv[1], "w") as f:
  for line in lines:
    f.write(line)
```

And the following shell commands:

```
find . -name '*.cpp' -exec python display.py {} \;
find . -name '*.h' -exec python display.py {} \;
```

Still doesn't build because some files don't use the mozilla namespace, but this
should make it better for review.

Review commit: https://reviewboard.mozilla.org/r/75246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75246/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1299066: Use the new enum class, and fix remaining build bustages except in the frame constructor.

Review commit: https://reviewboard.mozilla.org/r/75248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75248/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Mozreview isn't funny posting just one commit, sigh.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73216

::: layout/base/nsCSSFrameConstructor.h:735
(Diff revision 2)
>      const FrameConstructionData mData;
>    };
>  
> +  struct FrameConstructionDataByDisplay {
> +#ifdef DEBUG
> +    const mozilla::StyleDisplay mDisplay;

I assume for commit purposes this is being squashed with the commits that actually define the enum type and whatnot, right?  Want to avoid bisect footguns.

::: layout/base/nsCSSFrameConstructor.cpp:4316
(Diff revision 2)
>  
>  #define SIMPLE_XUL_CREATE(_tag, _func)            \
>    { &nsGkAtoms::_tag, SIMPLE_XUL_FCDATA(_func) }
>  #define SCROLLABLE_XUL_CREATE(_tag, _func)            \
>    { &nsGkAtoms::_tag, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SIMPLE_XUL_INT_CREATE(_int, _func)      \
> +#define SIMPLE_XUL_DISPLAY_CREATE(_int, _func)      \

s/_int/_display/ here, please.

::: layout/base/nsCSSFrameConstructor.cpp:4318
(Diff revision 2)
>    { &nsGkAtoms::_tag, SIMPLE_XUL_FCDATA(_func) }
>  #define SCROLLABLE_XUL_CREATE(_tag, _func)            \
>    { &nsGkAtoms::_tag, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SIMPLE_XUL_INT_CREATE(_int, _func)      \
> -  { _int, SIMPLE_XUL_FCDATA(_func) }
> -#define SCROLLABLE_XUL_INT_CREATE(_int, _func)                          \
> +#define SIMPLE_XUL_DISPLAY_CREATE(_int, _func)      \
> +  FCDATA_FOR_DISPLAY(_int, SIMPLE_XUL_FCDATA(_func))
> +#define SCROLLABLE_XUL_DISPLAY_CREATE(_int, _func)                          \

And here too, s/_int/_display/

::: layout/base/nsCSSFrameConstructor.cpp:4320
(Diff revision 2)
>    { &nsGkAtoms::_tag, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SIMPLE_XUL_INT_CREATE(_int, _func)      \
> -  { _int, SIMPLE_XUL_FCDATA(_func) }
> -#define SCROLLABLE_XUL_INT_CREATE(_int, _func)                          \
> -  { _int, SCROLLABLE_XUL_FCDATA(_func) }
> -#define SCROLLABLE_ABSPOS_CONTAINER_XUL_INT_CREATE(_int, _func)         \
> +#define SIMPLE_XUL_DISPLAY_CREATE(_int, _func)      \
> +  FCDATA_FOR_DISPLAY(_int, SIMPLE_XUL_FCDATA(_func))
> +#define SCROLLABLE_XUL_DISPLAY_CREATE(_int, _func)                          \
> +  FCDATA_FOR_DISPLAY(_int, SCROLLABLE_XUL_FCDATA(_func))
> +#define SCROLLABLE_ABSPOS_CONTAINER_XUL_DISPLAY_CREATE(_int, _func)         \

And here.

::: layout/base/nsCSSFrameConstructor.cpp:4530
(Diff revision 2)
>  
> -  // Processing by display here:
> -  return FindDataByInt(aDisplay->mDisplay, aElement, aStyleContext,
> -                       sXULDisplayData, ArrayLength(sXULDisplayData));
> +  if (aDisplay->mDisplay < StyleDisplay::Box) {
> +    return nullptr;
> +  }
> +
> +  MOZ_ASSERT(aDisplay->mDisplay <= StyleDisplay::Popup,

Please add a comment in nsStyleConsts.h about how if values are inserted in the XUL bits or added at the end this code needs to be updated.

::: layout/base/nsCSSFrameConstructor.cpp:4742
(Diff revision 2)
>        return &sNonScrollableGridData;
>      }
>    }
>  
> -  static const FrameConstructionDataByInt sDisplayData[] = {
> -    // To keep the hash table small don't add inline frames (they're
> +  // To keep the hash table small don't add inline frames (they're

Please keep this comment next to the StyleDisplay::Inline bit.

::: layout/base/nsCSSFrameConstructor.cpp:4840
(Diff revision 2)
>  
> -  return FindDataByInt(aDisplay->mDisplay,
> -                       aElement, aStyleContext, sDisplayData,
> -                       ArrayLength(sDisplayData));
> +  const FrameConstructionDataByDisplay& data =
> +    sDisplayData[size_t(aDisplay->mDisplay)];
> +
> +  MOZ_ASSERT(data.mDisplay == aDisplay->mDisplay,
> +             "Someone messed the order in the display values");

"messed up"

::: layout/style/nsStyleConsts.h:490
(Diff revision 2)
>             NS_STYLE_WRITING_MODE_SIDEWAYS_MASK)
>  
>  // See nsStyleDisplay
> +//
> +// NOTE: Order is important! If you change it, make sure to take a look at
> +// the FrameConstructorDataByDisplay stuff, and ensure it's still correct!

And XUL stuff, right?

r=me

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8786243 [details]
Bug 1299066: Make NS_STYLE_DISPLAY_* an enum class. Prefer indexing instead of linear search in the frame constructor

https://reviewboard.mozilla.org/r/75244/#review73864

::: layout/style/nsStyleConsts.h:518
(Diff revision 1)
> -#define NS_STYLE_DISPLAY_POPUP                  27
> -#define NS_STYLE_DISPLAY_GROUPBOX               28
> +  Contents,
> +  WebkitBox,
> +  WebkitInlineBox,
> +  Box,
> +  InlineBox,
> +#ifdef MOZ_XUL // XXX why not removing this?

I'm not sure what the state of --disable-xul builds are, but I just tried and the build fails pretty early on.  Certainly they're not going to be useful for Firefox.

In bug 540222 comment 5 Mats suggests that the MOZ_XUL blocks are still good for documentation, but I'm not sure it's really worth it.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8786244 [details]
Bug 1299066: Rename NS_STYLE_DISPLAY_* to use the new enum class.

https://reviewboard.mozilla.org/r/75246/#review73866

::: layout/style/nsRuleNode.cpp:202
(Diff revision 1)
>  nsRuleNode::EnsureBlockDisplay(uint8_t& display,
>                                 bool aConvertListItem /* = false */)
>  {
>    // see if the display value is already a block
>    switch (display) {
> -  case NS_STYLE_DISPLAY_LIST_ITEM :
> +  case StyleDisplay::ListItem :

Nit: feel free to drop the spaces before the ":"s while touching these lines.

::: layout/style/nsRuleNode.cpp:6253
(Diff revision 1)
>        // XXX We may also need to promote ruby display vals; see bug 1179349.
>  
>        // It's okay to cache this change in the rule tree for the same
>        // reasons as floats in the previous condition.
> -      if (display->mDisplay == NS_STYLE_DISPLAY_INLINE) {
> -          display->mDisplay = NS_STYLE_DISPLAY_INLINE_BLOCK;
> +      if (display->mDisplay == StyleDisplay::Inline) {
> +          display->mDisplay = StyleDisplay::InlineBlock;

This line has confusing indentation.  Fix it up while you're here?

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8786245 [details]
Bug 1299066: Use the new enum class, and fix remaining build bustages except in the frame constructor.

https://reviewboard.mozilla.org/r/75248/#review73876

Let's not add "mozilla::" to the .cpp files.  Most .cpp files have a |using namespace mozilla;| at the top.  For those that don't, and we would otherwise need mozilla::StyleDisplay, just add the using statement.

r=me with these comments addressed.

::: dom/base/nsRange.cpp:3504
(Diff revision 1)
>              !IsLastNonemptyRowGroupOfTable(f->GetParent())) {
>            result.Append('\n');
>          }
>          break;
> +      default:
> +        ; // Nothing

Nit: might prefer:

  default:
    break;  // do nothing

just so we don't leave a fall through in case we add a new case below it later.

::: layout/generic/nsFrame.cpp:10788
(Diff revision 1)
>      // This array must exactly match the NS_STYLE_DISPLAY constants.
>      const char *const displayTypes[] = {
>        "none", "block", "inline", "inline-block", "list-item", "marker",
>        "run-in", "compact", "table", "inline-table", "table-row-group",
>        "table-column", "table-column-group", "table-header-group",
>        "table-footer-group", "table-row", "table-cell", "table-caption",
>        "box", "inline-box",
>  #ifdef MOZ_XUL
>        "grid", "inline-grid", "grid-group", "grid-line", "stack",
>        "inline-stack", "deck", "popup", "groupbox",
>  #endif
>      };

I guess this array needs to be updated with your previous patch.

::: layout/style/nsRuleNode.cpp:280
(Diff revision 1)
>        break;
>      case StyleDisplay::Stack:
>        display = StyleDisplay::InlineStack;
>        break;
> +    default:
> +      ; // Nothing

As above.

::: layout/style/nsRuleNode.cpp:6213
(Diff revision 1)
>          case StyleDisplay::InlineFlex:
>            display->mDisplay = StyleDisplay::Flex;
>            conditions.SetUncacheable();
>            break;
> +        default:
> +          ; // Nothing

And here.

::: layout/style/nsStyleStruct.h:2830
(Diff revision 1)
>                                  //         and float:left/right; otherwise equal
>                                  //         to mDisplay

Re-align these two comment lines?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8786237 [details]
Bug 1299066: Fix the frame constructor breakage, avoid linear search in the frame constructor (prefer indexing).

https://reviewboard.mozilla.org/r/75236/#review73216

> I assume for commit purposes this is being squashed with the commits that actually define the enum type and whatnot, right?  Want to avoid bisect footguns.

Sure

> And XUL stuff, right?

Sure, the XUL array is also a FrameConstructorDataByDisplay array, but I'll clarify.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8786245 [details]
Bug 1299066: Use the new enum class, and fix remaining build bustages except in the frame constructor.

https://reviewboard.mozilla.org/r/75248/#review73876

Unfortunately we can't on some of the files (the frame ones concretely), because some compilers on try for some reason get confused with the `StyleDisplay` method :/

> I guess this array needs to be updated with your previous patch.

It was already broken, but yeah, I fixed it anyway.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8786243 [details]
Bug 1299066: Make NS_STYLE_DISPLAY_* an enum class. Prefer indexing instead of linear search in the frame constructor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75244/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c212e496d0ce
Make NS_STYLE_DISPLAY_* an enum class. Prefer indexing instead of linear search in the frame constructor r=heycam,bz

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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