[webkit-reviews] review denied: [Bug 54412] flex/bison tokens and grammar for CSS calc : [Attachment 82362] Patch

Thu Mar 17 22:47:45 PDT 2011

Ojan Vafai <ojan at chromium.org> has denied Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 54412: flex/bison tokens and grammar for CSS calc
https://bugs.webkit.org/show_bug.cgi?id=54412

Attachment 82362: Patch
https://bugs.webkit.org/attachment.cgi?id=82362&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.