RESOLVED FIXED 216445
Use an OptionSet<> for LayoutBox::BaseTypeFlags
https://bugs.webkit.org/show_bug.cgi?id=216445
Summary Use an OptionSet<> for LayoutBox::BaseTypeFlags
Simon Fraser (smfr)
Reported 2020-09-12 14:58:32 PDT
Use an OptionSet<> for LayoutBox::BaseTypeFlags
Attachments
Patch (6.64 KB, patch)
2020-09-12 15:01 PDT, Simon Fraser (smfr)
darin: review+
Patch (6.70 KB, patch)
2020-09-12 15:48 PDT, Simon Fraser (smfr)
no flags
Simon Fraser (smfr)
Comment 1 2020-09-12 15:01:11 PDT
Simon Fraser (smfr)
Comment 2 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?
Darin Adler
Comment 3 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.
Simon Fraser (smfr)
Comment 4 2020-09-12 15:48:04 PDT
EWS
Comment 5 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].
Radar WebKit Bug Importer
Comment 6 2020-09-12 18:31:15 PDT
Note You need to log in before you can comment on or make changes to this bug.