RESOLVED FIXED 94772
Fix CSSParserValue::createCSSValue() for viewport based units. 
https://bugs.webkit.org/show_bug.cgi?id=94772
Summary Fix CSSParserValue::createCSSValue() for viewport based units.
Luke Macpherson
Reported 2012-08-22 21:00:29 PDT
Fix CSSParserValue::createCSSValue() for viewport based units.
Attachments
Patch (3.55 KB, patch)
2012-08-22 21:03 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch (7.55 KB, patch)
2012-08-26 23:26 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.58 KB, patch)
2012-08-27 17:13 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Luke Macpherson
Comment 1 2012-08-22 21:03:12 PDT
Created attachment 160080 [details] Patch
Tony Chang
Comment 2 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.
Luke Macpherson
Comment 3 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.
Tony Chang
Comment 4 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.
Luke Macpherson
Comment 5 2012-08-26 23:26:32 PDT
Created attachment 160649 [details] Patch
Tony Chang
Comment 6 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.
Luke Macpherson
Comment 7 2012-08-27 17:13:26 PDT
Created attachment 160863 [details] Patch for landing
WebKit Review Bot
Comment 8 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>
WebKit Review Bot
Comment 9 2012-08-27 17:45:24 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.