Bug 35329

Summary: Enhance CSS parser for Paged Media (Iteration 1)
Product: WebKit Reporter: Yuzo Fujishima <yuzo@google.com>
Component: CSSAssignee: Yuzo Fujishima <yuzo@google.com>
Status: RESOLVED FIXED    
Severity: Normal CC: darin@apple.com, douglas@paradise.net.nz, hamaji@chromium.org, hayato@chromium.org, hyatt@apple.com, mitz@webkit.org, m.kurz+webkitbugs@irregular.at, peter.linss@hp.com, simon.fraser@apple.com, tkent@chromium.org, webkit.review.bot@gmail.com
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 From 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 From 2010-02-23 22:48:08 PST -------
Created an attachment (id=49359) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #2 From 2010-02-24 00:21:07 PST -------
(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.

> +        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 From 2010-02-24 02:16:49 PST -------
Created an attachment (id=49365) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #4 From 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 From 2010-02-24 02:24:00 PST -------
Created an attachment (id=49367) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #6 From 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 From 2010-02-24 02:29:00 PST -------
Created an attachment (id=49368) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #8 From 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] [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 From 2010-03-01 02:09:46 PST -------
Created an attachment (id=49713) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #10 From 2010-03-01 02:10:14 PST -------
Changed not to use unnamed namespace.
------- Comment #11 From 2010-03-01 02:21:55 PST -------
Created an attachment (id=49716) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #12 From 2010-03-01 02:25:28 PST -------
Changed to include limits.h instead of climits.
------- Comment #13 From 2010-03-01 22:51:09 PST -------
Created an attachment (id=49781) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #14 From 2010-03-01 22:51:59 PST -------
Simplified the grammar.
Fixed indentation.
------- Comment #15 From 2010-03-03 18:13:42 PST -------
Created an attachment (id=49970) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #16 From 2010-03-03 18:14:51 PST -------
1-line fix in the patch. Sunk floating selector.
------- Comment #17 From 2010-03-04 04:26:16 PST -------
Created an attachment (id=50004) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #18 From 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 From 2010-03-10 02:11:55 PST -------
Created an attachment (id=50382) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #20 From 2010-03-14 23:43:33 PST -------
Ping?
------- Comment #21 From 2010-03-15 15:56:07 PST -------
(From update of attachment 50382 [details])
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 From 2010-03-16 03:16:54 PST -------
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 From 2010-03-31 23:41:25 PST -------
Darin,

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

Yuzo
------- Comment #24 From 2010-04-07 01:12:41 PST -------
(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?

> +        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 From 2010-04-07 19:22:51 PST -------
Created an attachment (id=52820) [details]
Enhance CSS parser for Paged Media (Iteration 1)
------- Comment #26 From 2010-04-07 19:27:27 PST -------
Thank you for the review.

(In reply to comment #24)
> (From update of attachment 50382 [details] [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 From 2010-04-20 17:40:17 PST -------
Hyatt, could you look at this patch please?
------- Comment #28 From 2010-04-21 12:36:36 PST -------
(From update of attachment 52820 [details])
r=me
------- Comment #29 From 2010-04-27 02:35:35 PST -------
Committed r58299: <http://trac.webkit.org/changeset/58299>
------- Comment #30 From 2010-04-27 03:55:03 PST -------
Committed r58301: <http://trac.webkit.org/changeset/58301>
------- Comment #31 From 2010-04-27 03:59:04 PST -------
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 From 2010-04-27 04:00:03 PST -------
The Tiger compilation error needs to be fixed.
------- Comment #33 From 2010-04-27 04:19:45 PST -------
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 From 2010-04-27 05:33:35 PST -------
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 From 2010-04-27 20:42:32 PST -------
Committed r58374: <http://trac.webkit.org/changeset/58374>
------- Comment #36 From 2010-04-28 10:53:02 PST -------
This caused bug 38272.
------- Comment #37 From 2010-05-06 15:15:54 PST -------
This also caused bug 38697.