[webkit-reviews] review granted: [Bug 216208] Move all remaining flags from ElementRareData to Node to reduce the frequency : [Attachment 408104] Updated per Darin's review comments

[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 408104: Updated per Darin's review comments

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




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 408104
  --> https://bugs.webkit.org/attachment.cgi?id=408104
Updated per Darin's review comments

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

> Source/WebCore/dom/Element.cpp:244
> +void Element::setTabIndexExplicitly(Optional<int> tabIndex)

This function can leave behind a stale leftover unusual tab index stored in the
rare data that should never be looked at. I suppose that’s OK.

> Source/WebCore/dom/Element.cpp:274
> +	   RELEASE_ASSERT(hasRareData());
> +	   return elementRareData()->unusualTabIndex();

Why is this RELEASE_ASSERT needed? Doesn’t seem valuable to me.

> Source/WebCore/dom/ElementRareData.h:233
> +    ASSERT(m_unusualTabIndex && m_unusualTabIndex != -1); // Common values
of 0 and -1 are stored as TabIndexState in Node.

The main purpose of this assertion is to catch cases where we are getting the
index before it was set. Otherwise, it’s a waste to assert the same thing in
both the setter and getter.

So it could assert only the 0 case; it’s not so much checking for an invalid
index as checking that we don’t ask for an index when one is not there.

> Source/WebCore/dom/Node.h:557
> +    static constexpr uint32_t s_tabIndexStateBitMask = 3 <<
s_tabIndexStateBitOffset;

I think this needs to be 3U to get rid of the warning (treated as an error)
that is causing the build failure on Windows. Hope that works!

> Source/WebCore/dom/Node.h:563
> +    enum class TabIndexState : uint8_t {
> +	   NotSet = 0,
> +	   Zero = 1,
> +	   NegativeOne = 2,
> +	   InRareData = 3,
> +    };

I don’t think you need the four explicit numeric values here.