Enhance CSS parser for Paged Media. First, extend the grammar and define skeletal methods for creating page rules and margin at-rules.
Created attachment 49359 [details] Enhance CSS parser for Paged Media (Iteration 1)
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
Created attachment 49365 [details] Enhance CSS parser for Paged Media (Iteration 1)
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.
Created attachment 49367 [details] Enhance CSS parser for Paged Media (Iteration 1)
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.
Created attachment 49368 [details] Enhance CSS parser for Paged Media (Iteration 1)
Hi, Hamaji-san, Thank you for the review. (In reply to comment #2) > (From update of attachment 49359 [details]) > 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. OK, deleted. > > > + 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. Added tests. > > > +%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. > Changed to use CSSSelector::MarginBox instead. > > +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 */ { > } > ; > > ? Made them simpler and shorter. > > > +margin_box: > > + margin_sym {static_cast<CSSParser*>(parser)->startDeclarationsForMarginBox();} maybe_space > > I think we need line breaks. > Done. > > +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. > Fixed. > > +// 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. Fixed. > > > + 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 Oops. Thanks for the good catch. Changed to use a constant representing an invalid value (actually UNIT_MAX). Yuzo
Created attachment 49713 [details] Enhance CSS parser for Paged Media (Iteration 1)
Changed not to use unnamed namespace.
Created attachment 49716 [details] Enhance CSS parser for Paged Media (Iteration 1)
Changed to include limits.h instead of climits.
Created attachment 49781 [details] Enhance CSS parser for Paged Media (Iteration 1)
Simplified the grammar. Fixed indentation.
Created attachment 49970 [details] Enhance CSS parser for Paged Media (Iteration 1)
1-line fix in the patch. Sunk floating selector.
Created attachment 50004 [details] Enhance CSS parser for Paged Media (Iteration 1)
1-line fix: use nullAtom for selector local name for no identifier no pseudo-class case.
Created attachment 50382 [details] Enhance CSS parser for Paged Media (Iteration 1)
Ping?
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. :)
Eric, thank you for your review. Darin, I'd appreciate it if you could take a look at this and other patches that I CC to you shortly. Yuzo
Darin, Would you mind reviewing this? Or can you recommend a reviewer for this? Yuzo
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)?
Created attachment 52820 [details] Enhance CSS parser for Paged Media (Iteration 1)
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
Hyatt, could you look at this patch please?
Comment on attachment 52820 [details] Enhance CSS parser for Paged Media (Iteration 1) r=me
Committed r58299: <http://trac.webkit.org/changeset/58299>
Committed r58301: <http://trac.webkit.org/changeset/58301>
Rolled out r58299 by r58301 because it causes: Tiger Intel Release : compiled failed. /bin/sh -c \"/Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/WebKitBuild/WebCore.build/Release/Derived\ Sources.build/Script-DD041FBD09D9DDBE0010AF2A.sh\" bison -d -p cssyy WebCore/css/CSSGrammar.y -o CSSGrammar.cpp WebCore/css/CSSGrammar.y:790: type clash (`' `boolean') on default action make[1]: *** [CSSGrammar.cpp] Error 1
The Tiger compilation error needs to be fixed.
r58299 also causes the following crash: perl ./WebKitTools/Scripts/run-webkit-tests --no-launch-safari --no-new-test-results --no-sample-on-timeout --results-directory layout-test-results --use-remote-links-to-tests --debug --exit-after-n-failures 20 --leaks in dir /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-leaks/build (timeout 1200 secs) watching logfiles {} argv: ['perl', './WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--no-new-test-results', '--no-sample-on-timeout', '--results-directory', 'layout-test-results', '--use-remote-links-to-tests', '--debug', '--exit-after-n-failures', '20', '--leaks'] environment: HOME=/Users/buildbot LOGNAME=buildbot PATH=/usr/bin:/bin:/usr/sbin:/sbin PWD=/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-leaks/build SHELL=/bin/bash TMPDIR=/var/folders/dR/dRbf9KVoHs0rNZjRONbHTU+++TI/-Tmp-/ USER=buildbot VERSIONER_PYTHON_PREFER_32_BIT=no VERSIONER_PYTHON_VERSION=2.6 closing stdin using PTY: False Running build-dumprendertree ... java/lc3/ConvertNumber/number-011.html -> crashed ...
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
Committed r58374: <http://trac.webkit.org/changeset/58374>
This caused bug 38272.
This also caused bug 38697.