[webkit-reviews] review denied: [Bug 134661] CSS color parsing accepts invalid color identifiers : [Attachment 234454] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied  review:
Bug 134661: CSS color parsing accepts invalid color identifiers
https://bugs.webkit.org/show_bug.cgi?id=134661

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=234454&action=review


Hilarious bug, that's a good catch.

I disagree with your fix though. IMHO, we should never pass an invalid ID to
systemColor(). You could check that the input ID is in the range
alpha->-webkit-text.

I would also like the same test for CSS style resolution in addition to canvas.
I know CSSParser::parseSystemColor() is not used for CSS parsing, but it is
better to have the test to be on the safe side if the code changes.

Long term, someone should investigate if it would be better to split
CSSValueKeywords into tiny perfect hash tables. That's completely out of scope
here though :)

> Source/WebCore/ChangeLog:9
> +	   that if a valid cssValueKeywordID is got from the color string

"is got".

> Source/WebCore/css/CSSParser.cpp:1362
> +    color = parsedColor.rgb();

Changing the color when parsedColor.isValid() is false does not seem right.