[webkit-reviews] review granted: [Bug 216445] Use an OptionSet<> for LayoutBox::BaseTypeFlags : [Attachment 408613] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 216445: Use an OptionSet<> for LayoutBox::BaseTypeFlags
https://bugs.webkit.org/show_bug.cgi?id=216445

Attachment 408613: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 408613
  --> https://bugs.webkit.org/attachment.cgi?id=408613
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(InitialContainingBlo
ckFlag); }

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.