Bug 216445

Summary: Use an OptionSet<> for LayoutBox::BaseTypeFlags
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, darin, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
darin: review+
Patch none

Description Simon Fraser (smfr) 2020-09-12 14:58:32 PDT
Use an OptionSet<> for LayoutBox::BaseTypeFlags
Comment 1 Simon Fraser (smfr) 2020-09-12 15:01:11 PDT
Created attachment 408613 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-09-12 15:01:52 PDT
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?
Comment 3 Darin Adler 2020-09-12 15:22:16 PDT
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.
Comment 4 Simon Fraser (smfr) 2020-09-12 15:48:04 PDT
Created attachment 408619 [details]
Patch
Comment 5 EWS 2020-09-12 18:30:17 PDT
Committed r266985: <https://trac.webkit.org/changeset/266985>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408619 [details].
Comment 6 Radar WebKit Bug Importer 2020-09-12 18:31:15 PDT
<rdar://problem/68783520>