Bug 54412 - flex/bison tokens and grammar for CSS calc
Summary: flex/bison tokens and grammar for CSS calc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Mike Lawther
URL:
Keywords:
Depends on: 57588
Blocks: 16662
  Show dependency treegraph
 
Reported: 2011-02-14 14:13 PST by Mike Lawther
Modified: 2011-04-10 19:32 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2011-02-14 14:13:47 PST
flex/bison tokens and grammar for CSS calc
Comment 1 Mike Lawther 2011-02-14 14:19:34 PST
Created attachment 82362 [details]
Patch
Comment 2 Mike Lawther 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.
Comment 3 Mike Lawther 2011-02-14 14:23:42 PST
Added block on master CSS calc bug
Comment 4 Mike Lawther 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.
Comment 5 Mike Lawther 2011-02-14 18:27:22 PST
Sorry - of course I meant 'disallow '-' anywhere in a number's dimension'.
Comment 6 Ojan Vafai 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.
Comment 7 Mike Lawther 2011-03-21 19:17:35 PDT
Created attachment 86407 [details]
Patch
Comment 8 Mike Lawther 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.
Comment 9 Mike Lawther 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.
Comment 10 Mike Lawther 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!
Comment 11 Ojan Vafai 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
Comment 12 Mike Lawther 2011-03-23 18:00:53 PDT
Created attachment 86729 [details]
Patch
Comment 13 Mike Lawther 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 :)
Comment 14 Mike Lawther 2011-03-23 18:07:38 PDT
Thanks for the r+ - yay!
Comment 15 WebKit Commit Bot 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>
Comment 16 WebKit Commit Bot 2011-03-23 20:19:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Mike Lawther 2011-04-10 16:00:05 PDT
Reopening as patch was rolled out due to https://bugs.webkit.org/show_bug.cgi?id=57588.
Comment 18 Mike Lawther 2011-04-10 16:10:34 PDT
Created attachment 88960 [details]
Patch
Comment 19 Mike Lawther 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.
Comment 20 Mike Lawther 2011-04-10 19:00:51 PDT
Created attachment 88964 [details]
Patch
Comment 21 WebKit Commit Bot 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>
Comment 22 WebKit Commit Bot 2011-04-10 19:32:12 PDT
All reviewed patches have been landed.  Closing bug.