Bug 77960 - CSS3 calc() - simple parse time evaluation
Summary: CSS3 calc() - simple parse time evaluation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Lawther
URL:
Keywords:
Depends on:
Blocks: 16662
  Show dependency treegraph
 
Reported: 2012-02-07 03:45 PST by Mike Lawther
Modified: 2012-02-07 19:40 PST (History)
3 users (show)

See Also:


Attachments
Patch (7.55 KB, patch)
2012-02-07 03:49 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch for landing (7.44 KB, patch)
2012-02-07 16:50 PST, 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 2012-02-07 03:45:24 PST
CSS3 calc() - simple parse time evaluation
Comment 1 Mike Lawther 2012-02-07 03:49:28 PST
Created attachment 125806 [details]
Patch
Comment 2 Ojan Vafai 2012-02-07 10:07:58 PST
Comment on attachment 125806 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125806&action=review

I know it's a pain to post these smaller patches, but these are 1000x easier to review. :)

> Source/WebCore/css/CSSCalculationValue.cpp:117
> +
> +        ASSERT_NOT_REACHED();
> +        return 0;

Does the compiler complain if you leave this out? If not, I'd just delete this since you will get a warning if a new Calc type is added since your switch above doesn't have a "default" clause (== a good thing!).

> Source/WebCore/css/CSSCalculationValue.cpp:206
> +        case CalcMod:
> +            if (rightValue)
> +                return static_cast<int>(leftValue) % static_cast<int>(rightValue);
> +            return std::numeric_limits<double>::quiet_NaN();

Wasn't mod dropped from the spec?

> Source/WebCore/css/CSSCalculationValue.cpp:209
> +        ASSERT_NOT_REACHED();
> +        return 0;

Ditto re: deleting the unreachable code if the compiler doesn't complain.
Comment 3 Mike Lawther 2012-02-07 16:49:46 PST
Comment on attachment 125806 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125806&action=review

>> Source/WebCore/css/CSSCalculationValue.cpp:117
>> +        return 0;
> 
> Does the compiler complain if you leave this out? If not, I'd just delete this since you will get a warning if a new Calc type is added since your switch above doesn't have a "default" clause (== a good thing!).

Nope, it doesn't. Removed.

>> Source/WebCore/css/CSSCalculationValue.cpp:206
>> +            return std::numeric_limits<double>::quiet_NaN();
> 
> Wasn't mod dropped from the spec?

Indeed it has been. I've removed the evaluation code, and I'll get rid of the enum value in another patch.

>> Source/WebCore/css/CSSCalculationValue.cpp:209
>> +        return 0;
> 
> Ditto re: deleting the unreachable code if the compiler doesn't complain.

Removed.
Comment 4 Mike Lawther 2012-02-07 16:50:31 PST
Created attachment 125956 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-02-07 19:40:49 PST
Comment on attachment 125956 [details]
Patch for landing

Clearing flags on attachment: 125956

Committed r107030: <http://trac.webkit.org/changeset/107030>
Comment 6 WebKit Review Bot 2012-02-07 19:40:53 PST
All reviewed patches have been landed.  Closing bug.