Improved parse error handling for CSSMQ.

Source/core/css/CSSGrammar.y.in

Index: Source/core/css/CSSGrammar.y.in
diff --git a/Source/core/css/CSSGrammar.y.in b/Source/core/css/CSSGrammar.y.in
index 3ecec22a56556c6e208da31aacfa957ea07de3f8..2b44dbcdf04ebc2014601b51d93360089820c979 100644
--- a/Source/core/css/CSSGrammar.y.in
+++ b/Source/core/css/CSSGrammar.y.in
@@ -87,7 +87,7 @@ static inline bool isCSSTokenAString(int yytype)
 
 %}
 
-%expect 11
+%expect 3
 
 %nonassoc LOWEST_PREC
 
@@ -251,6 +251,7 @@ static inline bool isCSSTokenAString(int yytype)
 %type <string> media_feature
 %type <mediaList> media_list
 %type <mediaList> maybe_media_list
+%type <mediaList> mq_list
 %type <mediaQuery> media_query
 %type <mediaQueryRestrictor> maybe_media_restrictor
 %type <valueList> maybe_media_value
@@ -364,8 +365,8 @@ internal_value:
 ;
 
 webkit_mediaquery:
-     WEBKIT_MEDIAQUERY_SYM WHITESPACE maybe_space media_query '}' {
-         parser->m_mediaQuery = parser->sinkFloatingMediaQuery($4);
+     WEBKIT_MEDIAQUERY_SYM maybe_space media_query '}' {
+         parser->m_mediaQuery = parser->sinkFloatingMediaQuery($3);
      }
 ;
 
@@ -575,7 +576,7 @@ STRING
 ;
 
 media_feature:

[SeRya, 2013/06/01 06:28:03]

May be use just IDENT rather than media_feature?

[rune, 2013/06/03 09:08:11]

Fixed in next patch set.

-    IDENT maybe_space {
+    IDENT {
         $$ = $1;
     }
     ;
@@ -590,29 +591,30 @@ maybe_media_value:
     ;
 
 media_query_exp:
-    maybe_media_restrictor maybe_space '(' maybe_space media_feature maybe_space maybe_media_value ')' maybe_space {
-        // If restrictor is specified, media query expression is invalid.
-        // Create empty media query expression and continue parsing media query.
-        if ($1 != MediaQuery::None)
-            $$ = parser->createFloatingMediaQueryExp("", 0);
-        else {
-            parser->tokenToLowerCase($5);
-            $$ = parser->createFloatingMediaQueryExp($5, $7);
-        }
+    '(' maybe_space media_feature maybe_space maybe_media_value closing_parenthesis maybe_space {
+        parser->tokenToLowerCase($3);
+        $$ = parser->createFloatingMediaQueryExp($3, $5);
     }
-    | maybe_media_restrictor maybe_space '(' error error_recovery ')' {
-        YYERROR;
+    | '(' error error_recovery closing_parenthesis {

[SeRya, 2013/06/01 06:28:03]

If you remove YYERROR you need to add maybe_space at the end.

However I'd suggest not remove YYERROR but handle errors on the hier levels. It
seems that media_list/mq_list know that to do with errors. I think it is better
place.

[rune, 2013/06/03 09:08:11]

Fixed in next patch set.

+        $$ = 0;
     }
     ;
 
 media_query_exp_list:
     media_query_exp {
-        $$ = parser->createFloatingMediaQueryExpList();
-        $$->append(parser->sinkFloatingMediaQueryExp($1));
+        $$ = 0;
+        if ($1) {
+            $$ = parser->createFloatingMediaQueryExpList();
+            $$->append(parser->sinkFloatingMediaQueryExp($1));
+        }
     }
-    | media_query_exp_list maybe_space MEDIA_AND maybe_space media_query_exp {
-        $$ = $1;
-        $$->append(parser->sinkFloatingMediaQueryExp($5));
+    | media_query_exp_list MEDIA_AND maybe_space media_query_exp {
+        if ($1 && $4) {

[SeRya, 2013/06/01 06:28:03]

If you handle errors on higher level you likely can relay $1 & $4 are never 0
(unless createFloatingMediaQueryExpList can return it).

[rune, 2013/06/03 09:08:11]

Fixed in next patch set.

+            $$ = $1;
+            $$->append(parser->sinkFloatingMediaQueryExp($4));
+        }
+        else
+            $$ = 0;
     }
     ;
 
@@ -629,47 +631,78 @@ maybe_media_restrictor:
     /*empty*/ {
         $$ = MediaQuery::None;
     }
-    | MEDIA_ONLY {
+    | MEDIA_ONLY maybe_space {
         $$ = MediaQuery::Only;
     }
-    | MEDIA_NOT {
+    | MEDIA_NOT maybe_space {
         $$ = MediaQuery::Not;
     }
     ;
 
 media_query:
     media_query_exp_list {
-        $$ = parser->createFloatingMediaQuery(parser->sinkFloatingMediaQueryExpList($1));
+        if ($1)
+            $$ = parser->createFloatingMediaQuery(parser->sinkFloatingMediaQueryExpList($1));
+        else
+            $$ = 0;
     }
-    |
-    maybe_media_restrictor maybe_space medium maybe_and_media_query_exp_list {
-        parser->tokenToLowerCase($3);
-        $$ = parser->createFloatingMediaQuery($1, $3, parser->sinkFloatingMediaQueryExpList($4));
+    | maybe_media_restrictor medium maybe_and_media_query_exp_list {
+        parser->tokenToLowerCase($2);
+        if ($3)
+            $$ = parser->createFloatingMediaQuery($1, $2, parser->sinkFloatingMediaQueryExpList($3));
+        else
+            $$ = 0;
+    }
+    | error rule_error_recovery {
+        $$ = 0;
     }
     ;
 
 maybe_media_list:
-     /* empty */ {
+    /* empty */ {
         $$ = parser->createMediaQuerySet();
-     }
-     | media_list
-     ;
+    }
+    | media_list
+    ;
 
 media_list:
     media_query {
         $$ = parser->createMediaQuerySet();
-        $$->addMediaQuery(parser->sinkFloatingMediaQuery($1));
+        MediaQuery* query = $1;
+        if (!query)
+            query = parser->createFloatingNotAllQuery();
+        $$->addMediaQuery(parser->sinkFloatingMediaQuery(query));
         parser->updateLastMediaLine($$);
     }
-    | media_list ',' maybe_space media_query {
+    | mq_list media_query {
         $$ = $1;
-        if ($$) {
-            $$->addMediaQuery(parser->sinkFloatingMediaQuery($4));
-            parser->updateLastMediaLine($$);
-        }
+        MediaQuery* query = $2;
+        if (!query)
+            query = parser->createFloatingNotAllQuery();
+        $$->addMediaQuery(parser->sinkFloatingMediaQuery($2));
+        parser->updateLastMediaLine($$);
     }
-    | media_list error {
-        $$ = 0;
+    | mq_list {
+        $$ = $1;
+        $$->addMediaQuery(parser->sinkFloatingMediaQuery(parser->createFloatingNotAllQuery()));
+        parser->updateLastMediaLine($$);
+    }
+    ;
+
+mq_list:
+    media_query ',' maybe_space {
+        $$ = parser->createMediaQuerySet();
+        MediaQuery* query = $1;
+        if (!query)
+            query = parser->createFloatingNotAllQuery();
+        $$->addMediaQuery(parser->sinkFloatingMediaQuery(query));
+    }
+    | mq_list media_query ',' maybe_space {
+        $$ = $1;
+        MediaQuery* query = $2;
+        if (!query)
+            query = parser->createFloatingNotAllQuery();
+        $$->addMediaQuery(parser->sinkFloatingMediaQuery(query));
     }
     ;
 
@@ -698,6 +731,10 @@ media:
     | before_media_rule MEDIA_SYM at_rule_header_end_maybe_space '{' at_rule_body_start maybe_space block_rule_body closing_brace {
         $$ = parser->createMediaRule(0, $7);
     }
+    | before_media_rule MEDIA_SYM maybe_space media_list ';' {

[SeRya, 2013/06/01 06:28:03]

It shouldn't be needed. "before_media_rule MEDIA_SYM at_rule_recovery" handles
this case.

grammar should push "error" on ";". Then drop media_list, then drop maybe_space,
then "error ';'" should be reduced to at_rule_recovery. Have you checked if
grammar works without this rule?

[rune, 2013/06/03 09:08:11]

Media query error #4 (parsing of "@media all; @media tv {}") in
fast/media/media-query-list-syntax.html fails without this rule. I guess that's
because without it, the error recovery in media_query will look for either a ','
or a '{'.

[SeRya, 2013/06/03 11:31:27]

I see why it needed. Because "all;" reduces to media_query (in error recovery
part). So it looks inevitable.

+        $$ = 0;
+        parser->endRuleBody(true);
+    }
     | before_media_rule MEDIA_SYM at_rule_recovery {
         $$ = 0;
         parser->endRuleBody(true);