Comment on attachment 49359[details]
Enhance CSS parser for Paged Media (Iteration 1)
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@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
Attachment 49365[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:73: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 49367[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:73: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 50382[details]
Enhance CSS parser for Paged Media (Iteration 1)
I do not speak yacc well enough to review this. Darin Adler has historically reviewed a lot of these changes in the past. Perhaps I should learn yacc and review this. :)
Comment on attachment 50382[details]
Enhance CSS parser for Paged Media (Iteration 1)
> + CSSRule* createMarginAtRule(CSSSelector::MarginBox& marginBox);
Why marginBox is passed by reference? Will we modify this value inside this function?
> + enum MarginBox {
I this good naming? I would say MarginBoxPosition, MarginBoxType or something like this, but I'm not sure.
> + TopLeftCorner,
> + TopLeft,
> + TopCenter,
> + TopRight,
> + TopRightCorner,
> + BottomLeftCorner,
> + BottomLeft,
> + BottomCenter,
> + BottomRight,
> + BottomRightCorner,
> + LeftTop,
> + LeftMiddle,
> + LeftBottom,
> + RightTop,
> + RightMiddle,
> + RightBottom,
CSSSelector::TopLeftCorner sounds a bit unclear to me. How about adding MarginBox prefixes for them (e.g., MarginBoxTopLeftCorner)?
Thank you for the review.
(In reply to comment #24)
> (From update of attachment 50382[details])
> > + CSSRule* createMarginAtRule(CSSSelector::MarginBox& marginBox);
>
> Why marginBox is passed by reference? Will we modify this value inside this
> function?
Changed to pass-by-value.
>
> > + enum MarginBox {
>
> I this good naming? I would say MarginBoxPosition, MarginBoxType or something
> like this, but I'm not sure.
Renamed it MarginBoxType.
>
> > + TopLeftCorner,
> > + TopLeft,
> > + TopCenter,
> > + TopRight,
> > + TopRightCorner,
> > + BottomLeftCorner,
> > + BottomLeft,
> > + BottomCenter,
> > + BottomRight,
> > + BottomRightCorner,
> > + LeftTop,
> > + LeftMiddle,
> > + LeftBottom,
> > + RightTop,
> > + RightMiddle,
> > + RightBottom,
>
> CSSSelector::TopLeftCorner sounds a bit unclear to me. How about adding
> MarginBox prefixes for them (e.g., MarginBoxTopLeftCorner)?
Added "MarginBox" suffix, e.g., TopLeftCornerMarginBox because
it is a little bit easier to read. I have no strong preference, though.
I can use prefix if you prefer.
Yuzo
I could reproduce the bison error on my snow leopard by using bison in
http://www.opensource.apple.com/release/mac-os-x-103/
It seems just adding the following line fixes this issue.
%type <boolean> declarations_and_margins
2010-02-23 22:48 PST, Yuzo Fujishima
2010-02-24 02:16 PST, Yuzo Fujishima
2010-02-24 02:24 PST, Yuzo Fujishima
2010-02-24 02:29 PST, Yuzo Fujishima
2010-03-01 02:09 PST, Yuzo Fujishima
2010-03-01 02:21 PST, Yuzo Fujishima
2010-03-01 22:51 PST, Yuzo Fujishima
2010-03-03 18:13 PST, Yuzo Fujishima
2010-03-04 04:26 PST, Yuzo Fujishima
2010-03-10 02:11 PST, Yuzo Fujishima
2010-04-07 19:22 PDT, Yuzo Fujishima