Bug 54412

Summary: flex/bison tokens and grammar for CSS calc
Product: WebKit Reporter: Mike Lawther <mikelawther>
Component: CSSAssignee: Mike Lawther <mikelawther>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hamaji, nicolas.paton, noel.gordon, peter
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 57588    
Bug Blocks: 16662    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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
DetailsFormatted DiffDiff
Patch (40.94 KB, patch)
2011-03-21 19:17 PDT, Mike Lawther 		no flags
DetailsFormatted DiffDiff
Patch (40.47 KB, patch)
2011-03-23 18:00 PDT, Mike Lawther 		no flags
DetailsFormatted DiffDiff
Patch (44.38 KB, patch)
2011-04-10 16:10 PDT, Mike Lawther 		no flags
DetailsFormatted DiffDiff
Patch (44.35 KB, patch)
2011-04-10 19:00 PDT, Mike Lawther 		no flags
DetailsFormatted DiffDiff
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.