Bug 106418 – Implement CSSGroupingRule for @host @-rules and @supports.

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

[Takashi Sakamoto]

These implementations is almost the same and now CSSHostRule, which is also the same as the implementations, will be added.
So we need to refactor these implementations.

[Takashi Sakamoto]

Created attachment 181865 [details]
Patch

[Pablo Flouret]

Nice. You probably want to name the class CSSGroupingRule, as that's the name on the spec (http://dev.w3.org/csswg/css3-conditional/#cssgroupingrule)

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Nice. You probably want to name the class CSSGroupingRule, as that's the name on the spec (http://dev.w3.org/csswg/css3-conditional/#cssgroupingrule)

I like this suggestion.

[Alexey Proskuryakov]

> class CSSMediaRule and class WebKitCSSRegionRule can share most of their implementations

I have doubts whether these classes are semantically close enough to share a public base class. This refactoring looks like it makes code much more confusing.

Also, inlining bodies of these large functions just so that you could use templates is not good.

[Dimitri Glazkov (Google)]

Comment on attachment 181865 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> +        is "const" method, needs static_cast and const_cast.

Does it need to be a const method?

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> > class CSSMediaRule and class WebKitCSSRegionRule can share most of their implementations
> 
> I have doubts whether these classes are semantically close enough to share a public base class. This refactoring looks like it makes code much more confusing.

I think the idea of sharing the code for all grouping rules is a good one. Not sure about this specific refactoring yet.

> Also, inlining bodies of these large functions just so that you could use templates is not good.

[Dimitri Glazkov (Google)]

Comment on attachment 181865 [details]
Patch

I agree with Alexey, this patch looks sad because of gratuitous templating. The thing is, you almost don't need these templates and I am confident this refactoring can be done without them.

[Antti Koivisto]

Please don't try to use this for WebKitCSSRegionRule if it really doesn't share the interface. Also definitely no templates.

Please explain in ChangeLog the real reason for doing this (@supports rules).

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> Please explain in ChangeLog the real reason for doing this (@supports rules).

and @host rules.

[Takashi Sakamoto]

Created attachment 182065 [details]
WIP

[Takashi Sakamoto]

Created attachment 182080 [details]
Patch

[Takashi Sakamoto]

Thank you for reviewing.

I uploaded a new patch. The differences between the previous one and this are:

- No template,
- Add @supports and @host @-rules to ChangeLog.
- Add the spec URLs to ChangeLog.

Talking about @region,

interface CSSRegionStyleRule : CSSRule {
  attribute DOMString selectorText;
  readonly attribute CSSRuleList cssRules;
  unsigned long insertRule(DOMString rule,  unsigned long index)
    raises(DOMException);
  void deleteRule(unsigned long index)
    raises(DOMException);
};

c.f. http://dev.w3.org/csswg/css3-regions/#the-at-region-style-rule

I think, this is the same as:

interface CSSGroupingRule : CSSRule {
    readonly attribute CSSRuleList cssRules;
    unsigned long insertRule (DOMString rule, unsigned long index);
    void deleteRule (unsigned long index);
}

http://www.w3.org/TR/2012/WD-css3-conditional-20121213/#the-cssgroupingrule-interface

Best regards,
Takashi Sakamoto

[Build Bot]

Comment on attachment 182080 [details]
Patch

Attachment 182080 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15766738

[Antti Koivisto]

Comment on attachment 182080 [details]
Patch

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-host-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.

[Takashi Sakamoto]

Created attachment 182265 [details]
WIP for checking Windows BuildBot

[Takashi Sakamoto]

Comment on attachment 182080 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/ChangeLog:14
>> +        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.

I see. I added the reason to the top of the description.

>> Source/WebCore/WebCore.vcproj/WebCore.vcproj:36179
>> +					<Tool
> 
> You don't need this stuff (probably why windows build is failing).

I found I missed CSSAllInOne.cpp. I'm now checking whether only the mistake (CSSAllInOne) causes windows build failure or not.

>> Source/WebCore/css/CSSGroupingRule.cpp:46
>> +CSSGroupingRule::CSSGroupingRule(StyleRuleBlock* groupRule, CSSStyleSheet* parent)
> 
> For consistency we should probably rename StyleRuleBlock -> StyleRuleGroup in another patch.

I see. If no bug for the renaming, I will file a new bug.

>> 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.

I see. Thanks. Done.

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

Done.

[Takashi Sakamoto]

Created attachment 182268 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 182268 [details]
Patch for landing

Clearing flags on attachment: 182268

Committed r139409: <http://trac.webkit.org/changeset/139409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.