Bug 69803

Summary: Add compile-time asserts for RenderStyle::(Non)InheritedFlags size.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: New BugsAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, hyatt, macpherson, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Luke Macpherson 2011-10-10 17:10:01 PDT
Add compile-time asserts for RenderStyle::(Non)InheritedFlags size.
Comment 1 Luke Macpherson 2011-10-10 17:13:34 PDT
Created attachment 110443 [details]
Patch
Comment 2 Luke Macpherson 2011-10-10 18:46:52 PDT
Eric requested something like this on https://bugs.webkit.org/show_bug.cgi?id=64583, and it would be good to have regardless of what happens to that patch.
Comment 3 David Levin 2011-10-11 11:31:23 PDT
Comment on attachment 110443 [details]
Patch

I'm probably not the person to review this, but it is clear that there is a need to fix the Windows build failure.
Comment 4 Eric Seidel (no email) 2011-10-11 12:18:30 PDT
Comment on attachment 110443 [details]
Patch

Yup.  These are super useful.  I think normally we put the outside of the class itself, but this also seems OK, since it's right next to where the equals is.  You'd have to modify both.
Comment 5 Luke Macpherson 2011-10-11 15:13:38 PDT
I don't know why this fails on windows, and don't have a windows build environment to test on, so I'm going to throw a few patches at the build bots and see what happens. The size really shouldn't be > 8 bytes, but maybe windows doesn't round up to a 4-byte alignment?
Comment 6 Luke Macpherson 2011-10-11 15:54:47 PDT
Created attachment 110596 [details]
Patch
Comment 7 Luke Macpherson 2011-10-13 22:50:41 PDT
Created attachment 110960 [details]
Patch
Comment 8 WebKit Review Bot 2011-10-13 22:53:43 PDT
Attachment 110960 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Last 3072 characters of output:
endering/style/RenderStyle.h:206:  _white_space is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:207:  _box_direction is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:213:  _pointerEvents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:214:  _insideLink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:250:  _effectiveDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:251:  _originalDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:252:  _overflowX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:253:  _overflowY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:254:  _vertical_align is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:255:  _clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:256:  _position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:257:  _floating is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:258:  _table_layout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:261:  _page_break_after is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:262:  _page_break_inside is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:264:  _styleType is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:268:  _pseudoBits is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:269:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 29 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Luke Macpherson 2011-10-16 14:52:11 PDT
So, interestingly, I think this picked up a real bug on the windows build. These structs were much larger on windows (I don't remember the exact size). rearranging the order of the fields on windows got it down to ~13 bytes, and changing them to unsigned char (instead of unsigned) got them down to the same size as on mac.

The style errors here are existing naming errors where I have simply changed the type from unsigned to unsigned char.
Comment 10 Darin Adler 2011-10-16 20:12:48 PDT
Comment on attachment 110960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110960&action=review

These assertions should be in the .cpp file so they are only compiled once. And if you have some need to change these all to “unsigned char”, then please explain why.

> Source/WebCore/rendering/style/RenderStyle.h:172
> +            COMPILE_ASSERT((sizeof(InheritedFlags) <= 8), InheritedFlags_does_not_grow);

I’m not sure I understand why this is in the == operator, and further, why it is in the header file, not the .cpp file.

> Source/WebCore/rendering/style/RenderStyle.h:218
> +        unsigned char _empty_cells : 1; // EEmptyCell
> +        unsigned char _caption_side : 2; // ECaptionSide
> +        unsigned char _list_style_type : 7; // EListStyleType
> +        unsigned char _list_style_position : 1; // EListStylePosition
> +        unsigned char _visibility : 2; // EVisibility
> +        unsigned char _text_align : 4; // ETextAlign
> +        unsigned char _text_transform : 2; // ETextTransform
> +        unsigned char _text_decorations : ETextDecorationBits;
> +        unsigned char _cursor_style : 6; // ECursor
> +        unsigned char _direction : 1; // TextDirection
> +        unsigned char _border_collapse : 1; // EBorderCollapse
> +        unsigned char _white_space : 3; // EWhiteSpace
> +        unsigned char _box_direction : 1; // EBoxDirection (CSS3 box_direction property, flexible box layout module)
>          // 34 bits
>          
>          // non CSS2 inherited
> -        unsigned m_rtlOrdering : 1; // Order
> +        unsigned char m_rtlOrdering : 1; // Order
>          bool _force_backgrounds_to_white : 1;
> -        unsigned _pointerEvents : 4; // EPointerEvents
> -        unsigned _insideLink : 2; // EInsideLink
> +        unsigned char _pointerEvents : 4; // EPointerEvents
> +        unsigned char _insideLink : 2; // EInsideLink
>          // 43 bits
>  
>          // CSS Text Layout Module Level 3: Vertical writing support
> -        unsigned m_writingMode : 2; // WritingMode
> +        unsigned char m_writingMode : 2; // WritingMode

Why are you adding “char” to all of these? It seems unrelated to the assertion and not an improvement.
Comment 11 Darin Adler 2011-10-16 20:13:39 PDT
OK, I see now, the change from "unsigned" to "unsigned char" is a Windows-specific fix or perhaps compiler bug workaround.

This needs to be mentioned in the change log.
Comment 12 Luke Macpherson 2011-10-16 21:48:04 PDT
Created attachment 111207 [details]
Patch
Comment 13 WebKit Review Bot 2011-10-16 21:51:20 PDT
Attachment 111207 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Last 3072 characters of output:
endering/style/RenderStyle.h:205:  _white_space is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:206:  _box_direction is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:212:  _pointerEvents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:213:  _insideLink is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:248:  _effectiveDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:249:  _originalDisplay is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:250:  _overflowX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:251:  _overflowY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:252:  _vertical_align is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:253:  _clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:254:  _position is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:255:  _floating is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:256:  _table_layout is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:258:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:259:  _page_break_after is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _page_break_inside is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:262:  _styleType is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:266:  _pseudoBits is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/rendering/style/RenderStyle.h:267:  _unicodeBidi is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 29 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 WebKit Review Bot 2011-10-17 21:08:04 PDT
Comment on attachment 111207 [details]
Patch

Clearing flags on attachment: 111207

Committed r97712: <http://trac.webkit.org/changeset/97712>
Comment 15 WebKit Review Bot 2011-10-17 21:08:09 PDT
All reviewed patches have been landed.  Closing bug.