[webkit-reviews] review denied: [Bug 69803] Add compile-time asserts for RenderStyle::(Non)InheritedFlags size. : [Attachment 110960] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 69803: Add compile-time asserts for RenderStyle::(Non)InheritedFlags size.
https://bugs.webkit.org/show_bug.cgi?id=69803

Attachment 110960: Patch
https://bugs.webkit.org/attachment.cgi?id=110960&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.