Enhance CSS parser for Paged Media (Iteration 1)

• Component: CSS
• Status: RESOLVED FIXED

[Yuzo Fujishima]

Enhance CSS parser for Paged Media.

First, extend the grammar and define skeletal methods for creating page rules and margin at-rules.

[Yuzo Fujishima]

Created attachment 49359 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Shinichiro Hamaji]

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
> +        [e-mail].

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

[Yuzo Fujishima]

Created attachment 49365 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[WebKit Review Bot]

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.

[Yuzo Fujishima]

Created attachment 49367 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[WebKit Review Bot]

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.

[Yuzo Fujishima]

Created attachment 49368 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

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
> > +        [e-mail].
> 
> 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

[Yuzo Fujishima]

Created attachment 49713 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

Changed not to use unnamed namespace.

[Yuzo Fujishima]

Created attachment 49716 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

Changed to include limits.h instead of climits.

[Yuzo Fujishima]

Created attachment 49781 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

Simplified the grammar.
Fixed indentation.

[Yuzo Fujishima]

Created attachment 49970 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

1-line fix in the patch. Sunk floating selector.

[Yuzo Fujishima]

Created attachment 50004 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

1-line fix: use nullAtom for selector local name for no identifier no pseudo-class case.

[Yuzo Fujishima]

Created attachment 50382 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

Ping?

[Eric Seidel (no email)]

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. :)

[Yuzo Fujishima]

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

[Yuzo Fujishima]

Darin,

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

Yuzo

[Shinichiro Hamaji]

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)?

[Yuzo Fujishima]

Created attachment 52820 [details]
Enhance CSS parser for Paged Media (Iteration 1)

[Yuzo Fujishima]

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

[Darin Adler]

Hyatt, could you look at this patch please?

[Dave Hyatt]

Comment on attachment 52820 [details]
Enhance CSS parser for Paged Media (Iteration 1)

r=me

[Yuzo Fujishima]

Committed r58299: <http://trac.webkit.org/changeset/58299>

[Yuzo Fujishima]

Committed r58301: <http://trac.webkit.org/changeset/58301>

[Yuzo Fujishima]

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

[Yuzo Fujishima]

The Tiger compilation error needs to be fixed.

[Yuzo Fujishima]

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
...

[Shinichiro Hamaji]

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

[Yuzo Fujishima]

Committed r58374: <http://trac.webkit.org/changeset/58374>

[Simon Fraser (smfr)]

This caused bug 38272.

[Alexey Proskuryakov]

This also caused bug 38697.