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

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted 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 86407: Patch
https://bugs.webkit.org/attachment.cgi?id=86407&action=review

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