Bug 35329

Summary: Enhance CSS parser for Paged Media (Iteration 1)
Product: WebKit Reporter: Yuzo Fujishima <yuzo>
Component: CSSAssignee: Yuzo Fujishima <yuzo>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, douglas, hamaji, hayato, hyatt, mitz, m.kurz+webkitbugs, peter.linss, simon.fraser, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 15548, 35782    
Attachments:
Description Flags
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1)
none
Enhance CSS parser for Paged Media (Iteration 1) hyatt: review+

Description Yuzo Fujishima 2010-02-23 22:26:43 PST
Enhance CSS parser for Paged Media.

First, extend the grammar and define skeletal methods for creating page rules and margin at-rules.
Comment 1 Yuzo Fujishima 2010-02-23 22:48:08 PST
Created attachment 49359 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 2 Shinichiro Hamaji 2010-02-24 00:21:07 PST
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
Comment 3 Yuzo Fujishima 2010-02-24 02:16:49 PST
Created attachment 49365 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 4 WebKit Review Bot 2010-02-24 02:19:39 PST
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.
Comment 5 Yuzo Fujishima 2010-02-24 02:24:00 PST
Created attachment 49367 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 6 WebKit Review Bot 2010-02-24 02:27:11 PST
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 7 Yuzo Fujishima 2010-02-24 02:29:00 PST
Created attachment 49368 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 8 Yuzo Fujishima 2010-02-24 02:33:27 PST
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
Comment 9 Yuzo Fujishima 2010-03-01 02:09:46 PST
Created attachment 49713 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 10 Yuzo Fujishima 2010-03-01 02:10:14 PST
Changed not to use unnamed namespace.
Comment 11 Yuzo Fujishima 2010-03-01 02:21:55 PST
Created attachment 49716 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 12 Yuzo Fujishima 2010-03-01 02:25:28 PST
Changed to include limits.h instead of climits.
Comment 13 Yuzo Fujishima 2010-03-01 22:51:09 PST
Created attachment 49781 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 14 Yuzo Fujishima 2010-03-01 22:51:59 PST
Simplified the grammar.
Fixed indentation.
Comment 15 Yuzo Fujishima 2010-03-03 18:13:42 PST
Created attachment 49970 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 16 Yuzo Fujishima 2010-03-03 18:14:51 PST
1-line fix in the patch. Sunk floating selector.
Comment 17 Yuzo Fujishima 2010-03-04 04:26:16 PST
Created attachment 50004 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 18 Yuzo Fujishima 2010-03-04 04:27:48 PST
1-line fix: use nullAtom for selector local name for no identifier no pseudo-class case.
Comment 19 Yuzo Fujishima 2010-03-10 02:11:55 PST
Created attachment 50382 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 20 Yuzo Fujishima 2010-03-14 23:43:33 PDT
Ping?
Comment 21 Eric Seidel 2010-03-15 15:56:07 PDT
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 22 Yuzo Fujishima 2010-03-16 03:16:54 PDT
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
Comment 23 Yuzo Fujishima 2010-03-31 23:41:25 PDT
Darin,

Would you mind reviewing this?
Or can you recommend a reviewer for this?

Yuzo
Comment 24 Shinichiro Hamaji 2010-04-07 01:12:41 PDT
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)?
Comment 25 Yuzo Fujishima 2010-04-07 19:22:51 PDT
Created attachment 52820 [details]
Enhance CSS parser for Paged Media (Iteration 1)
Comment 26 Yuzo Fujishima 2010-04-07 19:27:27 PDT
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
Comment 27 Darin Adler 2010-04-20 17:40:17 PDT
Hyatt, could you look at this patch please?
Comment 28 Dave Hyatt 2010-04-21 12:36:36 PDT
Comment on attachment 52820 [details]
Enhance CSS parser for Paged Media (Iteration 1)

r=me
Comment 29 Yuzo Fujishima 2010-04-27 02:35:35 PDT
Committed r58299: <http://trac.webkit.org/changeset/58299>
Comment 30 Yuzo Fujishima 2010-04-27 03:55:03 PDT
Committed r58301: <http://trac.webkit.org/changeset/58301>
Comment 31 Yuzo Fujishima 2010-04-27 03:59:04 PDT
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
Comment 32 Yuzo Fujishima 2010-04-27 04:00:03 PDT
The Tiger compilation error needs to be fixed.
Comment 33 Yuzo Fujishima 2010-04-27 04:19:45 PDT
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
...
Comment 34 Shinichiro Hamaji 2010-04-27 05:33:35 PDT
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
Comment 35 Yuzo Fujishima 2010-04-27 20:42:32 PDT
Committed r58374: <http://trac.webkit.org/changeset/58374>
Comment 36 Simon Fraser (smfr) 2010-04-28 10:53:02 PDT
This caused bug 38272.
Comment 37 Alexey Proskuryakov 2010-05-06 15:15:54 PDT
This also caused bug 38697.