[webkit-reviews] review denied: [Bug 70021] [CSS Regions]Parse @-webkit-region rule : [Attachment 110849] Patch 2

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Mihnea Ovidenie <mihnea at adobe.com>'s
request for review:
Bug 70021: [CSS Regions]Parse @-webkit-region rule
https://bugs.webkit.org/show_bug.cgi?id=70021

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=110849&action=review


Looks good. Some minor nits.

> Source/WebCore/css/CSSGrammar.y:784
> +    WEBKIT_REGION_STYLE_RULE_SYM WHITESPACE simple_selector_list maybe_space
'{' maybe_space block_rule_list save_block {

Why limit to simple selectors?	Don't you want selector_list instead? I don't
think there's any reason to limit to simple selectors here.

> Source/WebCore/css/CSSRegionStyleRule.cpp:40
> +    , m_lstCSSRules(rules)

I'd prefer this be m_ruleList.

> Source/WebCore/css/CSSRegionStyleRule.cpp:42
> +    for (unsigned idx = 0; idx < m_lstCSSRules->length(); ++idx)

You probably copied this from other crufty old CSS code, but fix the variable
name to just be "index"

> Source/WebCore/css/CSSRegionStyleRule.cpp:51
> +    for (unsigned idx = 0; idx < m_lstCSSRules->length(); ++idx)

Same here.

> Source/WebCore/css/CSSRegionStyleRule.cpp:64
> +    // First add the selectors.
> +    for (CSSSelector* s = m_selectorList.first(); s; s =
CSSSelectorList::next(s)) {
> +	   if (s != m_selectorList.first())
> +	       result += ", ";
> +	   result += s->selectorText();
> +    }

I'd add a selectorText() helper to CSSSelectorList and put this code there.

> Source/WebCore/css/CSSRegionStyleRule.cpp:70
> +	   for (unsigned idx = 0; idx < m_lstCSSRules->length(); ++idx) {

"index"