Summary: | Use an OptionSet<> for LayoutBox::BaseTypeFlags | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
Component: | Layout and Rendering | Assignee: | 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
Simon Fraser (smfr)
2020-09-12 14:58:32 PDT
Created attachment 408613 [details]
Patch
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 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. Created attachment 408619 [details]
Patch
Committed r266985: <https://trac.webkit.org/changeset/266985> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408619 [details]. |