Bug 194197 – Tidy up data members of FrameView and related classes to shrink class sizes

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Tidy up data memebers of FrameView and related classes to shrink class sizes

[Simon Fraser (smfr)]

Created attachment 360983 [details]
Patch

[EWS Watchlist]

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 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)]

(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 on attachment 360983 [details]
Patch

Clearing flags on attachment: 360983

Committed r240901: <https://trac.webkit.org/changeset/240901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47765771>

[Chris Dumez]

(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)]

(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 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)]

Will do those in bug 194203.