[webkit-reviews] review denied: [Bug 94772] Fix CSSParserValue::createCSSValue() for viewport based units. : [Attachment 160080] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 94772: Fix CSSParserValue::createCSSValue() for viewport based units.
https://bugs.webkit.org/show_bug.cgi?id=94772

Attachment 160080: Patch
https://bugs.webkit.org/attachment.cgi?id=160080&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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.