Bug 71221 – CSSRule: Devirtualize type() and isFooRule()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

SSIA

[Andreas Kling]

Created attachment 113053 [details]
Patch

[WebKit Review Bot]

Attachment 113053 [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

Source/WebCore/css/CSSStyleRule.h:64:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSStyleSelector.cpp:233:  The parameter name "rule" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 113053 [details]
Patch

r=me

[Andreas Kling]

Committed r98859: <http://trac.webkit.org/changeset/98859>

[Luke Macpherson]

Comment on attachment 113053 [details]
Patch

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

> Source/WebCore/css/CSSRule.h:118
> +    unsigned m_type : 31; // Plenty of space for additional flags here.

I would set this to just the number of bits required (4), delete the comment, and add a COMPILE_ASSERT on the overall size of the of CSSRule.
Reason for that is that I would not be surprised if this change causes visual c++ builds to increase the size of CSSRule, even though it will be ok on gcc.

[Andreas Kling]

(In reply to comment #5)
> (From update of attachment 113053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113053&action=review
> 
> > Source/WebCore/css/CSSRule.h:118
> > +    unsigned m_type : 31; // Plenty of space for additional flags here.
> 
> I would set this to just the number of bits required (4), delete the comment, and add a COMPILE_ASSERT on the overall size of the of CSSRule.
> Reason for that is that I would not be surprised if this change causes visual c++ builds to increase the size of CSSRule, even though it will be ok on gcc.

Hm, do you know the specifics about this? I had problems with MSVC and bit fields when trying to make a size guard for InlineBox (see InlineBox.cpp), but since I never use that compiler, I didn't learn much from it :/

[Luke Macpherson]

Comment on attachment 113053 [details]
Patch

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

>>> Source/WebCore/css/CSSRule.h:118
>>> +    unsigned m_type : 31; // Plenty of space for additional flags here.
>> 
>> I would set this to just the number of bits required (4), delete the comment, and add a COMPILE_ASSERT on the overall size of the of CSSRule.
>> Reason for that is that I would not be surprised if this change causes visual c++ builds to increase the size of CSSRule, even though it will be ok on gcc.
> 
> Hm, do you know the specifics about this? I had problems with MSVC and bit fields when trying to make a size guard for InlineBox (see InlineBox.cpp), but since I never use that compiler, I didn't learn much from it :/

I don't use MSVC either, but borrowed a colleague's box to run some experiments a few weeks ago. I didn't figure out exactly what the compiler was doing, but I did find that reordering fields reduced the size of RenderStyle.
I think the size of the type that it is reading into impacts the way it tries to align the field. Presumably they are trying to allow the compiler to mask the data out without shifting?
The easiest way to get things to pack as expected was to use unsigned char (instead of unsigned) for bitfields that were less than 8 bits in size.