WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
194197
Tidy up data members of FrameView and related classes to shrink class sizes
https://bugs.webkit.org/show_bug.cgi?id=194197
Summary
Tidy up data members of FrameView and related classes to shrink class sizes
Simon Fraser (smfr)
Reported
2019-02-02 15:33:44 PST
Tidy up data memebers of FrameView and related classes to shrink class sizes
Attachments
Patch
(24.05 KB, patch)
2019-02-02 15:36 PST
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1
2019-02-02 15:36:37 PST
Created
attachment 360983
[details]
Patch
EWS Watchlist
Comment 2
2019-02-02 15:39:46 PST
Attachment 360983
[details]
did not pass style-queue: ERROR: Source/WebCore/page/FrameView.h:850: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 3
2019-02-02 16:07:02 PST
Comment on
attachment 360983
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=360983&action=review
> Source/WebCore/ChangeLog:3 > + Tidy up data memebers of FrameView and related classes to shrink class sizes
Typo for members
> Source/WebCore/platform/ScrollTypes.h:119 > +enum ScrollbarOrientation : uint8_t {
Ditto
> Source/WebCore/platform/ScrollTypes.h:130 > +enum ScrollbarControlSize : uint8_t {
Could be bool instead of uint8_t.
> Source/WebCore/platform/ScrollTypes.h:135 > +enum class ScrollbarExpansionState : uint8_t {
Ditto
> Source/WebCore/platform/ScrollTypes.h:140 > +enum class ScrollEventAxis : uint8_t {
Ditto.
> Source/WebCore/platform/ScrollTypes.h:173 > +enum class ScrollbarStyle : uint8_t {
Ditto
> Source/WebCore/platform/ScrollTypes.h:190 > +enum class ScrollClamping : uint8_t {
Ditto
> Source/WebCore/platform/ScrollTypes.h:195 > +enum ScrollBehaviorForFixedElements : uint8_t {
Ditto
Simon Fraser (smfr)
Comment 4
2019-02-02 16:13:25 PST
(In reply to Chris Dumez from
comment #3
)
> Comment on
attachment 360983
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=360983&action=review
> > > Source/WebCore/ChangeLog:3 > > + Tidy up data memebers of FrameView and related classes to shrink class sizes > > Typo for members > > > Source/WebCore/platform/ScrollTypes.h:119 > > +enum ScrollbarOrientation : uint8_t { > > Ditto > > > Source/WebCore/platform/ScrollTypes.h:130 > > +enum ScrollbarControlSize : uint8_t { > > Could be bool instead of uint8_t.
Could be but I don't think it's clearer.
WebKit Commit Bot
Comment 5
2019-02-02 16:16:50 PST
Comment on
attachment 360983
[details]
Patch Clearing flags on attachment: 360983 Committed
r240901
: <
https://trac.webkit.org/changeset/240901
>
WebKit Commit Bot
Comment 6
2019-02-02 16:16:52 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 7
2019-02-02 16:17:27 PST
<
rdar://problem/47765771
>
Chris Dumez
Comment 8
2019-02-02 16:26:38 PST
(In reply to Simon Fraser (smfr) from
comment #4
)
> (In reply to Chris Dumez from
comment #3
) > > Comment on
attachment 360983
[details]
> > Patch > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=360983&action=review
> > > > > Source/WebCore/ChangeLog:3 > > > + Tidy up data memebers of FrameView and related classes to shrink class sizes > > > > Typo for members > > > > > Source/WebCore/platform/ScrollTypes.h:119 > > > +enum ScrollbarOrientation : uint8_t { > > > > Ditto > > > > > Source/WebCore/platform/ScrollTypes.h:130 > > > +enum ScrollbarControlSize : uint8_t { > > > > Could be bool instead of uint8_t. > > Could be but I don't think it's clearer.
Doesn’t it allow for better packing? It is just an underlying type, not sure why we worry about clarity.
Simon Fraser (smfr)
Comment 9
2019-02-02 17:38:28 PST
(In reply to Chris Dumez from
comment #8
)
> (In reply to Simon Fraser (smfr) from
comment #4
) > > (In reply to Chris Dumez from
comment #3
) > > > Comment on
attachment 360983
[details]
> > > Patch > > > > > > View in context: > > >
https://bugs.webkit.org/attachment.cgi?id=360983&action=review
> > > > > > > Source/WebCore/ChangeLog:3 > > > > + Tidy up data memebers of FrameView and related classes to shrink class sizes > > > > > > Typo for members > > > > > > > Source/WebCore/platform/ScrollTypes.h:119 > > > > +enum ScrollbarOrientation : uint8_t { > > > > > > Ditto > > > > > > > Source/WebCore/platform/ScrollTypes.h:130 > > > > +enum ScrollbarControlSize : uint8_t { > > > > > > Could be bool instead of uint8_t. > > > > Could be but I don't think it's clearer. > > Doesn’t it allow for better packing? It is just an underlying type, not sure > why we worry about clarity.
sizeof(bool) == sizeof(uint8_t) so bool won't save any more space. uint8_t tells me explicitly that it's bits. 'bool' makes me wonder what sizeof(bool) is for this compiler.
Darin Adler
Comment 10
2019-02-02 19:16:23 PST
Comment on
attachment 360983
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=360983&action=review
A few comments related to the code touched here, not to the patch itself.
> Source/WebCore/page/FrameView.cpp:192 > + , m_mediaType("screen")
Spotted this since it’s one of the few ones we are keeping. 1) Maybe we should change this to "screen"_s? 2) Can we put this in the class definition, even though it's a string literal? I think there’s no reason we can’t.
> Source/WebCore/rendering/Pagination.h:33 > + Pagination() = default;
Could we just omit this entirely instead?
> Source/WebCore/rendering/Pagination.h:42 > return mode != other.mode || behavesLikeColumns != other.behavesLikeColumns || pageLength != other.pageLength || gap != other.gap;
Would be nicer to write this so it calls the operator== so we didn’t have to have two lists of all the data members.
Simon Fraser (smfr)
Comment 11
2019-02-03 09:21:02 PST
Will do those in
bug 194203
.
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