WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
Patch
(6.70 KB, patch)
2020-09-12 15:48 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1
2020-09-12 15:01:11 PDT
Created
attachment 408613
[details]
Patch
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
Created
attachment 408619
[details]
Patch
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
<
rdar://problem/68783520
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug