54412 – flex/bison tokens and grammar for CSS calc

RESOLVED FIXED Bug 54412

flex/bison tokens and grammar for CSS calc

https://bugs.webkit.org/show_bug.cgi?id=54412

Summary flex/bison tokens and grammar for CSS calc

Mike Lawther

Reported 2011-02-14 14:13:47 PST

flex/bison tokens and grammar for CSS calc

Attachments
Patch (26.72 KB, patch) 2011-02-14 14:19 PST, Mike Lawther	no flags	Details Formatted Diff Diff
Patch (40.94 KB, patch) 2011-03-21 19:17 PDT, Mike Lawther	no flags	Details Formatted Diff Diff
Patch (40.47 KB, patch) 2011-03-23 18:00 PDT, Mike Lawther	no flags	Details Formatted Diff Diff
Patch (44.38 KB, patch) 2011-04-10 16:10 PDT, Mike Lawther	no flags	Details Formatted Diff Diff
Patch (44.35 KB, patch) 2011-04-10 19:00 PDT, Mike Lawther	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Mike Lawther

Comment 1 2011-02-14 14:19:34 PST

Created attachment 82362 [details] Patch

Mike Lawther

Comment 2 2011-02-14 14:23:07 PST

This patch is the first in a series to implement CSS calc. Only the lexing and grammar stage is included here. Net effect is no change to WebKit's user visible behaviour, other than pages including -webkit-calc(), -webkit-min() and -webkit-max() will be correctly lexed - but nothing will happen with that information.

Mike Lawther

Comment 3 2011-02-14 14:23:42 PST

Added block on master CSS calc bug

Mike Lawther

Comment 4 2011-02-14 14:39:02 PST

I should also say that Tab Atkins from the CSS WG pointed out http://www.w3.org/Style/CSS/Tracker/issues/149 to me - which says that the + and - operators shall require spaces around them. So this patch implements this. See the included test files for what is intended to work and not work. That said - it is pretty simple to not require the whitespace - all we need to do is disallow a trailing '-' on a number's dimension.

Mike Lawther

Comment 5 2011-02-14 18:27:22 PST

Sorry - of course I meant 'disallow '-' anywhere in a number's dimension'.

Ojan Vafai

Comment 6 2011-03-17 22:47:45 PDT

Comment on attachment 82362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82362&action=review Just some nits on the test cases and a few concerns about error cases. > LayoutTests/css3/calc/calc-errors.html:3 > +<html> > +<head> > + For this and the other new tests, you should put a doctype to make sure we're testing standards mode by default: <!DOCTYPE HTML> Also, it's not a big deal, but you don't really need the html/head/body elements. > LayoutTests/css3/calc/calc-errors.html:38 > +<body onload="test()"> If you just move the script element at the end of the test, then you won't need to do this onload. You can just execute the script directly. > Source/WebCore/css/CSSGrammar.y:1554 > + if ($2) { > + CSSParserValue v; > + v.id = 0; > + v.unit = CSSParserValue::Operator; > + v.iValue = $2; > + $$->addValue(v); > + } I'm not sure this is right. I don't think you need to null check $2. In either case, please add a test for this case. > Source/WebCore/css/CSSGrammar.y:1569 > + if ($2) { > + CSSParserValue v; > + v.id = 0; > + v.unit = CSSParserValue::Operator; > + v.iValue = $2; > + $$->addValue(v); > + } > + if ($3) > + $$->append(*($3)); Ditto above. Please add tests for the cases where !$2 and !$3. > Source/WebCore/css/CSSGrammar.y:1582 > + | calc_func_expr_list ',' maybe_space calc_func_expr { Make sure to test the case of having a space before the comma. I believe that you have the logic correct, but it would be good to have tests for all these cases. > Source/WebCore/css/CSSGrammar.y:1612 > + | CALCFUNCTION maybe_space error { > + CSSParser* p = static_cast<CSSParser*>(parser); > + CSSParserFunction* f = p->createFloatingFunction(); > + f->name = $1; > + f->args = 0; > + $$.id = 0; > + $$.unit = CSSParserValue::Function; > + $$.function = f; I think in the error case you just want to set $$=0, no? > Source/WebCore/css/CSSGrammar.y:1643 > + | min_or_max maybe_space error { > + CSSParser* p = static_cast<CSSParser*>(parser); > + CSSParserFunction* f = p->createFloatingFunction(); > + f->name = $1; > + f->args = 0; > + $$.id = 0; > + $$.unit = CSSParserValue::Function; > + $$.function = f; ditto > Source/WebCore/css/CSSParserValues.cpp:57 > +void CSSParserValueList::append(CSSParserValueList& right) nit: i'd s/right/valueList/. append implies going on the right. :) Although, i think calling this extend would be more clear. FWIW, it matches python's list class. :) > Source/WebCore/css/CSSParserValues.h:76 > + void append(CSSParserValueList& right); Shouldn't include variable names in header files unless they add to readability.

Mike Lawther

Comment 7 2011-03-21 19:17:35 PDT

Created attachment 86407 [details] Patch

Mike Lawther

Comment 8 2011-03-21 19:25:45 PDT

Comment on attachment 82362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82362&action=review >> LayoutTests/css3/calc/calc-errors.html:3 >> + > > For this and the other new tests, you should put a doctype to make sure we're testing standards mode by default: <!DOCTYPE HTML> > > Also, it's not a big deal, but you don't really need the html/head/body elements. Done. >> LayoutTests/css3/calc/calc-errors.html:38 >> +<body onload="test()"> > > If you just move the script element at the end of the test, then you won't need to do this onload. You can just execute the script directly. Good idea - done. >> Source/WebCore/css/CSSGrammar.y:1554 >> + } > > I'm not sure this is right. I don't think you need to null check $2. In either case, please add a test for this case. Test added (see 'invalid operator 'flim'' in calc-errors.html). calc_func_operator has been changed to $$=0 in the error case, so I can check for it here. >> Source/WebCore/css/CSSGrammar.y:1582 >> + | calc_func_expr_list ',' maybe_space calc_func_expr { > > Make sure to test the case of having a space before the comma. I believe that you have the logic correct, but it would be good to have tests for all these cases. Test added to simple-minmax,html (see min( 150px , 100px ,200px)). >> Source/WebCore/css/CSSGrammar.y:1612 >> + $$.function = f; > > I think in the error case you just want to set $$=0, no? In this case, since calc_function is a value, $$ can't be set to 0. But you're right in that what I was doing was wrong, so I just YYERRROR here instead. >> Source/WebCore/css/CSSParserValues.cpp:57 >> +void CSSParserValueList::append(CSSParserValueList& right) > > nit: i'd s/right/valueList/. append implies going on the right. :) > > Although, i think calling this extend would be more clear. FWIW, it matches python's list class. :) Done. >> Source/WebCore/css/CSSParserValues.h:76 >> + void append(CSSParserValueList& right); > > Shouldn't include variable names in header files unless they add to readability. Done.

Mike Lawther

Comment 9 2011-03-21 19:29:25 PDT

Comment on attachment 86407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86407&action=review > Source/WebCore/css/CSSGrammar.y:1442 > +calc_func_term: > + unary_term { $$ = $1; } > + | unary_operator unary_term { $$ = $2; $$.fValue *= $1; } > + ; I reduced the matching of calc_func_term to just these. Previously it was a synonym for 'term', which incorrectly allowed eg -webkit-calc() embedded inside a -webkit-calc(), or a color, or a URI etc.

Mike Lawther

Comment 10 2011-03-21 19:31:05 PDT

I was so caught up in the niceness of inline comments in the new review tool that I forgot to say thanks for the review Ojan!

Ojan Vafai

Comment 11 2011-03-22 22:27:14 PDT

Comment on attachment 86407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86407&action=review Please address the comments and repost the patch for committing. > LayoutTests/css3/calc/calc-errors.html:2 > +<style type="text/css"> Nit: don't need the type on the style element. css is assumed. Ditto for the other tests. > LayoutTests/css3/calc/calc-errors.html:106 > +function test() > +{ > + var test = document.getElementById("test"); > + for (var element = test.firstChild; element; element = element.nextSibling) { > + var width = element.offsetWidth; > + var error = []; > + if (width != 100) > + error.push("expected width of 100, but was " + width); > + var height = element.offsetHeight; > + if (height != 100) > + error.push("expected height of 100, but was " + width); > + > + if (error == "") { > + element.style.backgroundColor = "green"; > + element.innerHTML += " => PASS"; > + } else { > + element.innerHTML += " => FAIL: " + error.join(", "); > + } > + } > +} Nit: no need to create the function. Just call this code directly. Ditto for the other tests. > Source/WebCore/css/CSSGrammar.y:1461 > + '+' WHITESPACE { > + $$ = '+'; > + } > + | '-' WHITESPACE { > + $$ = '-'; > + } > + | '*' maybe_space { > + $$ = '*'; > + } > + | '/' maybe_space { > + $$ = '/'; > + } > + | IDENT maybe_space { > + if (equalIgnoringCase("mod", $1.characters, $1.length)) > + $$ = '%'; > + else > + $$ = 0; The indentation here and in some of the places below is a bit funky. I know this file isn't very consistent, but I don't see that this matches the other various indentation styles in the file. :) > Source/WebCore/css/CSSGrammar.y:1487 > + if ($1 && $2) { Do you not need to null check $3? > Source/WebCore/css/CSSGrammar.y:1523 > + $$ = $1; > + if ($$ && $4) { This is fine. I'd find it easier to read and more consistent with the other code if it was: if ($1 && $4) { $$ = $1; ... > Source/WebCore/css/CSSGrammar.y:1546 > + YYERROR; Why use YYERROR here instead of $$ = 0? > Source/WebCore/css/CSSGrammar.y:1571 > + YYERROR; ditto

Mike Lawther

Comment 12 2011-03-23 18:00:53 PDT

Created attachment 86729 [details] Patch

Mike Lawther

Comment 13 2011-03-23 18:07:03 PDT

Comment on attachment 86407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86407&action=review >> LayoutTests/css3/calc/calc-errors.html:2 >> +<style type="text/css"> > > Nit: don't need the type on the style element. css is assumed. > > Ditto for the other tests. Done. >> LayoutTests/css3/calc/calc-errors.html:106 >> +} > > Nit: no need to create the function. Just call this code directly. > > Ditto for the other tests. Done. >> Source/WebCore/css/CSSGrammar.y:1461 >> + $$ = 0; > > The indentation here and in some of the places below is a bit funky. I know this file isn't very consistent, but I don't see that this matches the other various indentation styles in the file. :) Fixed. >> Source/WebCore/css/CSSGrammar.y:1487 >> + if ($1 && $2) { > > Do you not need to null check $3? No, because calc_func_term is a value (see definition on line 277), so it can't be null. Similarly I don't check $1 in the clause above. >> Source/WebCore/css/CSSGrammar.y:1523 >> + if ($$ && $4) { > > This is fine. I'd find it easier to read and more consistent with the other code if it was: > if ($1 && $4) { > $$ = $1; > ... Done. >> Source/WebCore/css/CSSGrammar.y:1546 >> + YYERROR; > > Why use YYERROR here instead of $$ = 0? Because $$ in this case is defined as a value (which is a struct, not a pointer), so it can't be set to 0. YYERROR seems the right thing to do here. >> Source/WebCore/css/CSSGrammar.y:1571 >> + YYERROR; > > ditto ditto :)

Mike Lawther

Comment 14 2011-03-23 18:07:38 PDT

Thanks for the r+ - yay!

WebKit Commit Bot

Comment 15 2011-03-23 20:18:58 PDT

Comment on attachment 86729 [details] Patch Clearing flags on attachment: 86729 Committed r81849: <http://trac.webkit.org/changeset/81849>

WebKit Commit Bot

Comment 16 2011-03-23 20:19:03 PDT

All reviewed patches have been landed. Closing bug.

Mike Lawther

Comment 17 2011-04-10 16:00:05 PDT

Reopening as patch was rolled out due to https://bugs.webkit.org/show_bug.cgi?id=57588.

Mike Lawther

Comment 18 2011-04-10 16:10:34 PDT

Created attachment 88960 [details] Patch

Mike Lawther

Comment 19 2011-04-10 16:15:39 PDT

Bugfix was adding the else clause on line 1523 of CSSGrammar.y. Three regression tests from https://bugs.webkit.org/show_bug.cgi?id=57581 were added as well.

Mike Lawther

Comment 20 2011-04-10 19:00:51 PDT

Created attachment 88964 [details] Patch

WebKit Commit Bot

Comment 21 2011-04-10 19:32:04 PDT

Comment on attachment 88964 [details] Patch Clearing flags on attachment: 88964 Committed r83415: <http://trac.webkit.org/changeset/83415>

WebKit Commit Bot

Comment 22 2011-04-10 19:32:12 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Other

OS OS X 10.5

Product WebKit

Component CSS

Assignee

Mike Lawther

Reported

2011-02-14 14:13 PST

Modified

2011-04-10 19:32 PDT History

CC List

5 users Show

URL

Keywords

Depends on

57588

Blocks

16662

Dependancies

tree graph