Bug 35329 - Enhance CSS parser for Paged Media (Iteration 1)
: Enhance CSS parser for Paged Media (Iteration 1)
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 15548 35782
  Show dependency treegraph
 
Reported: 2010-02-23 22:26 PST by
Modified: 2013-12-12 03:39 PST (History)


Attachments
Enhance CSS parser for Paged Media (Iteration 1) (13.30 KB, patch)
2010-02-23 22:48 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (17.80 KB, patch)
2010-02-24 02:16 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.58 KB, patch)
2010-02-24 02:24 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.58 KB, patch)
2010-02-24 02:29 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.56 KB, patch)
2010-03-01 02:09 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.57 KB, patch)
2010-03-01 02:21 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.33 KB, patch)
2010-03-01 22:51 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.38 KB, patch)
2010-03-03 18:13 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.45 KB, patch)
2010-03-04 04:26 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.38 KB, patch)
2010-03-10 02:11 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Enhance CSS parser for Paged Media (Iteration 1) (16.66 KB, patch)
2010-04-07 19:22 PST, Yuzo Fujishima
hyatt: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


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.