[webkit-reviews] review granted: [Bug 106418] Implement CSSGroupingRule for @host @-rules and @supports. : [Attachment 182080] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 106418: Implement CSSGroupingRule for @host @-rules and @supports.
https://bugs.webkit.org/show_bug.cgi?id=106418

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=182080&action=review


Looks good.

> Source/WebCore/ChangeLog:14
> +	   Since @supports and @host @-rules are dervied from CSSGroupingRule:
> +	  
http://www.w3.org/TR/2012/WD-css3-conditional/#the-cssgroupingrule-interface
> +	  
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#css-ho
st-rule-interface
> +	   we need CSSGroupingRule (WK102344 and WK104822).
> +	   Since @media is also derived from CSSGroupingRule and @region has
the
> +	   same interface as CSSGroupingRule, modify to use CSSGroupingRule:
> +	   http://dev.w3.org/csswg/css3-regions/#the-at-region-style-rule

This is bit confusing. The reason you are adding CSSGroupingRule is to share
code between CSSMediaRule, CSSSupportsRule and CSSHostRule. It would be good to
spell it out.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:36179
> +				<FileConfiguration
> +					Name="Debug|Win32"
> +					ExcludedFromBuild="true"
> +					>
> +					<Tool

You don't need this stuff (probably why windows build is failing).

> Source/WebCore/css/CSSGroupingRule.cpp:46
> +CSSGroupingRule::CSSGroupingRule(StyleRuleBlock* groupRule, CSSStyleSheet*
parent)

For consistency we should probably rename StyleRuleBlock -> StyleRuleGroup in
another patch.

> Source/WebCore/css/CSSMediaRule.cpp:50
> +    return static_cast<const
StyleRuleMedia*>(m_groupRule.get())->mediaQueries();

You might want to add inlined accessors that do the casting.

> Source/WebCore/css/WebKitCSSRegionRule.cpp:55
> +    result.append(static_cast<const
StyleRuleRegion*>(m_groupRule.get())->selectorList().selectorsText());

For this too.