Tidy up data memebers of FrameView and related classes to shrink class sizes
Created attachment 360983 [details] Patch
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.
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
(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.
Comment on attachment 360983 [details] Patch Clearing flags on attachment: 360983 Committed r240901: <https://trac.webkit.org/changeset/240901>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47765771>
(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.
(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.
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.
Will do those in bug 194203.