[webkit-reviews] review denied: [Bug 35329] Enhance CSS parser for Paged Media (Iteration 1) : [Attachment 49359] Enhance CSS parser for Paged Media (Iteration 1)

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Yuzo Fujishima
<yuzo at google.com>'s request for review:
Bug 35329: Enhance CSS parser for Paged Media (Iteration 1)
https://bugs.webkit.org/show_bug.cgi?id=35329

Attachment 49359: Enhance CSS parser for Paged Media (Iteration 1)
https://bugs.webkit.org/attachment.cgi?id=49359&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
I'm not good at CSS parser stuff so we need CSS guru's reviews eventually, but
I'd like to do preliminary reviews for this patch. Please consider following
comments.

> +	   Note: http://dev.w3.org/csswg/css3-page/#syntax-page-selector says
margin_box contains ruleset, but I believe this is a bug.
> +	   margin_box contains property declarations as far as I can tell from
the examples there. I've sent a question about this to
> +	   www-style at w3.org.

I think this kind of comments should be in bugzilla, not in ChangeLog.

> +	   This change contains no tests because it doesn't change the
rendering yet.

I think we can add tests for invalid styles by something like

@page {
  div {
    background-color: red; /* this must be ignored */
  }
}
div {
  background-color: green;
  @page {
    background-color: red; /* this must be ignored */
  }
  background-color: red; /* this must be ignored */
}

I think having bunch of valid @page rules in tests would be also nice because
we can ensure the grammar introduced in this change won't crash.

> +%token <string> TOPLEFTCORNER_SYM
> +%token <string> TOPLEFT_SYM
> +%token <string> TOPCENTER_SYM
> +%token <string> TOPRIGHT_SYM
> +%token <string> TOPRIGHTCORNER_SYM
> +%token <string> BOTTOMLEFTCORNER_SYM
> +%token <string> BOTTOMLEFT_SYM
> +%token <string> BOTTOMCENTER_SYM
> +%token <string> BOTTOMRIGHT_SYM
> +%token <string> BOTTOMRIGHTCORNER_SYM
> +%token <string> LEFTTOP_SYM
> +%token <string> LEFTMIDDLE_SYM
> +%token <string> LEFTBOTTOM_SYM
> +%token <string> RIGHTTOP_SYM
> +%token <string> RIGHTMIDDLE_SYM
> +%token <string> RIGHTBOTTOM_SYM

> +%type <string> margin_sym

I'm not sure why they are strings. I guess they should be integers.

> +declarations_and_margins:
> +    declaration_list {
> +    }
> +    |
> +    decl_followed_by_margin {
> +    }
> +    |
> +    decl_followed_by_margin maybe_space declaration_list {
> +    }
> +    ;
> +
> +decl_followed_by_margin:
> +    margin_box {
> +    }
> +    | declaration_list maybe_space margin_box {
> +    }
> +    | decl_followed_by_margin maybe_space margin_box {
> +    }
> +    | decl_followed_by_margin maybe_space declaration_list maybe_space
margin_box {
> +    }
> +    ;

I'm not sure, but cannot we use simpler syntax like

declarations_and_margins:
  declarations_and_margins maybe_space margin_box {
  }
  declarations_and_margins maybe_space declaration_list {
  }
  | /* empty */ {
  }
  ;

?

> +margin_box:
> +    margin_sym
{static_cast<CSSParser*>(parser)->startDeclarationsForMarginBox();} maybe_space


I think we need line breaks.

> +CSSRule* CSSParser::createPageRule(CSSSelector* /* pageSelector */)
> +{
> +// FIXME: Create page rule here, using:
> +//	     - pageSelector->pseudoType(): the page pseudo-class, i.e., :left,
:right, or :first
> +//	     - pageSelector->m_tag:  the page name
> +//	     - m_parsedProperties: the page properties

Wrong indentation.

> +// FIXME: Implement margin at-rule here, using:
> +//	     - margin: margin box names excluding '@', e.g., "top-left-corner",
"top-left", ...
> +//	     - m_parsedProperties: properties at
[m_numParsedPropertiesBeforeMarginBox, m_numParsedProperties) are for this
at-rule.

Ditto.

> +    ASSERT(m_numParsedPropertiesBeforeMarginBox >= 0);

On my mac, compiler warns for this expression because
m_numParsedPropertiesBeforeMarginBox is unsigned.

  /Users/hamaji/WebKit/WebCore/css/CSSParser.cpp:5189: warning: comparison of
unsigned expression >= 0 is always true