Bug 216445 – Use an OptionSet<> for LayoutBox::BaseTypeFlags

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Use an OptionSet<> for LayoutBox::BaseTypeFlags

[Simon Fraser (smfr)]

Created attachment 408613 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 408613 [details]
Patch

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

> Source/WebCore/layout/layouttree/LayoutContainerBox.cpp:40
> +    : Box(attributes, WTFMove(style), baseTypeFlags | OptionSet<BaseTypeFlag>(ContainerBoxFlag))

Is there a better way to do this?

[Darin Adler]

Comment on attachment 408613 [details]
Patch

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

> Source/WebCore/layout/layouttree/LayoutBox.h:113
> +    bool isInitialContainingBlock() const { return OptionSet<BaseTypeFlag>::fromRaw(m_baseTypeFlags).contains(InitialContainingBlockFlag); }

I think we should make a getter that does OptionSet<BaseTypeFlag>::fromRaw(m_baseTypeFlags) so we don’t have to repeat it 5 times. Can be private and can call it flags() or baseTypeFlags().

>> Source/WebCore/layout/layouttree/LayoutContainerBox.cpp:40
>> +    : Box(attributes, WTFMove(style), baseTypeFlags | OptionSet<BaseTypeFlag>(ContainerBoxFlag))
> 
> Is there a better way to do this?

Doesn’t it work without the explicit constructor call?

    baseTypeFlags | ContainerBoxFlag

I’d think it would because of the OptionSet constructor that takes a value of the underlying enumeration. If not (and I don’t know why not) we could:

1) Add an overload of the | operator that takes underlying enumeration type. But why is this needed?

2) Add a free function version of add. Then we could just write: add(baseTypeFlags, ContainerBoxFlag). Pretty sure that would work even if the operator doesn’t.

> Source/WebCore/layout/layouttree/LayoutContainerBox.h:42
> +    ContainerBox(Optional<ElementAttributes>, RenderStyle&&, OptionSet<BaseTypeFlag> = { Box::ContainerBoxFlag });

Not new, but no need for the "Box::" here.

[Simon Fraser (smfr)]

Created attachment 408619 [details]
Patch

[EWS]

Committed r266985: <https://trac.webkit.org/changeset/266985>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408619 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68783520>