[webkit-reviews] review granted: [Bug 216208] Move all remaining flags from ElementRareData to Node to reduce the frequency : [Attachment 408069] Fixed builds 2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 216208: Move all remaining flags from ElementRareData to Node to reduce the
frequency
https://bugs.webkit.org/show_bug.cgi?id=216208

Attachment 408069: Fixed builds 2

https://bugs.webkit.org/attachment.cgi?id=408069&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 408069
  --> https://bugs.webkit.org/attachment.cgi?id=408069
Fixed builds 2

View in context: https://bugs.webkit.org/attachment.cgi?id=408069&action=review

Great!

> Source/WebCore/dom/Element.cpp:247
> +	   clearFlag(TabIndexIsZeroOrInRareDataFlag);

I think it would be easier to reason about this if it was treated as a 4-value
enumeration rather than two flags. The technique is great but the logic is
confusing this way.

> Source/WebCore/dom/Element.cpp:269
> +    bool isOne = getFlag(TabIndexIsZeroOrInRareDataFlag);

Did you mean to name this “isZero”?

> Source/WebCore/dom/ElementRareData.h:60
> +    int tabIndex() const { return m_tabIndex; }

The name here isn’t great because this is not the tab index in the 0, -1, or
“no tab index” cases. And also 0 is a special value meaning “not stored here”,
not a tab index of 0. I think a different name could make some of that clearer.
Like maybe “unusual tab index”. And a comment somewhere explaining the special
meaning of 0? Maybe an assertion that the getter isn’t called when m_tabIndex
is 0?

> Source/WebCore/dom/ElementRareData.h:61
> +    void setTabIndex(int);

Ditto on the name.

> Source/WebCore/dom/ElementRareData.h:161
> +    int m_tabIndex { 0 };

Ditto.

> Source/WebCore/dom/Node.cpp:137
> +    case NodeRareData::UseType::PseudoElements:

Sorting the cases makes sense when an enumeration gets long like this.

> Source/WebCore/dom/Node.h:548
> +#if ENABLE(FULLSCREEN_API)

Would it be better to not put this inside an #if? I can never tell; certainly
more conditionals make the code messier.