Bug 71221 - CSSRule: Devirtualize type() and isFooRule()
Summary: CSSRule: Devirtualize type() and isFooRule()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-31 08:01 PDT by Andreas Kling
Modified: 2011-10-31 10:42 PDT (History)
2 users (show)

See Also:


Attachments
Patch (18.39 KB, patch)
2011-10-31 08:37 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-10-31 08:01:24 PDT
SSIA
Comment 1 Andreas Kling 2011-10-31 08:37:40 PDT
Created attachment 113053 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-31 08:39:39 PDT
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.
Comment 3 Antti Koivisto 2011-10-31 08:41:08 PDT
Comment on attachment 113053 [details]
Patch

r=me
Comment 4 Andreas Kling 2011-10-31 08:53:33 PDT
Committed r98859: <http://trac.webkit.org/changeset/98859>
Comment 5 Luke Macpherson 2011-10-31 09:53:08 PDT
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.
Comment 6 Andreas Kling 2011-10-31 10:05:03 PDT
(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 :/
Comment 7 Luke Macpherson 2011-10-31 10:42:27 PDT
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.