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
Simon Fraser (smfr)
Comment 1 2019-02-02 15:36:37 PST
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
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.