Bug 94772 - Fix CSSParserValue::createCSSValue() for viewport based units.
Summary: Fix CSSParserValue::createCSSValue() for viewport based units.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-22 21:00 PDT by Luke Macpherson
Modified: 2012-08-27 17:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.55 KB, patch)
2012-08-22 21:03 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (7.55 KB, patch)
2012-08-26 23:26 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (7.58 KB, patch)
2012-08-27 17:13 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-08-22 21:00:29 PDT
Fix CSSParserValue::createCSSValue() for viewport based units.
Comment 1 Luke Macpherson 2012-08-22 21:03:12 PDT
Created attachment 160080 [details]
Patch
Comment 2 Tony Chang 2012-08-23 16:55:03 PDT
Comment on attachment 160080 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Add test that uses variables, calc and viewport units together.

Does the test have to use variables to reproduce?  It would be nice if we could have a test case that runs on all ports.
Comment 3 Luke Macpherson 2012-08-23 19:05:41 PDT
(In reply to comment #2)
> (From update of attachment 160080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160080&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Add test that uses variables, calc and viewport units together.
> 
> Does the test have to use variables to reproduce?  It would be nice if we could have a test case that runs on all ports.

For the test case (found by fuzz testing) without the variable definition you don't get the call to createCSSValue from the parser. There could be other code paths that can make it here too, but if so it's surprising that fuzz testing hasn't hit them yet.
Comment 4 Tony Chang 2012-08-24 12:02:11 PDT
Comment on attachment 160080 [details]
Patch

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

>>> LayoutTests/ChangeLog:8
>>> +        Add test that uses variables, calc and viewport units together.
>> 
>> Does the test have to use variables to reproduce?  It would be nice if we could have a test case that runs on all ports.
> 
> For the test case (found by fuzz testing) without the variable definition you don't get the call to createCSSValue from the parser. There could be other code paths that can make it here too, but if so it's surprising that fuzz testing hasn't hit them yet.

To answer my own question: Yes, this requires variables (I checked all the callers).

I think we should change createCSSValue to use a switch statement so the compiler can tell us if we forgot something. It'll also make it easier to add values in the future.  We use switch statements in a few other places too.
Comment 5 Luke Macpherson 2012-08-26 23:26:32 PDT
Created attachment 160649 [details]
Patch
Comment 6 Tony Chang 2012-08-27 10:50:20 PDT
Comment on attachment 160649 [details]
Patch

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

> LayoutTests/fast/css/variables/calc-vw-crash.html:2
> +<script>
> +if (window.testRunner) {

Nit: <!DOCTYPE html> and <html> tag are missing.
Comment 7 Luke Macpherson 2012-08-27 17:13:26 PDT
Created attachment 160863 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-08-27 17:45:21 PDT
Comment on attachment 160863 [details]
Patch for landing

Clearing flags on attachment: 160863

Committed r126828: <http://trac.webkit.org/changeset/126828>
Comment 9 WebKit Review Bot 2012-08-27 17:45:24 PDT
All reviewed patches have been landed.  Closing bug.