[webkit-reviews] review granted: [Bug 63029] Fix legacy color attribute parsing to match HTML spec : [Attachment 97893] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Tab Atkins <tabatkins at google.com>'s
request for review:
Bug 63029: Fix legacy color attribute parsing to match HTML spec
https://bugs.webkit.org/show_bug.cgi?id=63029

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97893&action=review

> Source/WebCore/dom/StyledElement.cpp:321
> -/* color parsing that tries to match as close as possible IE 6. */
> +/* color parsing that matches HTML's "rules for parsing a legacy color
value" */

C++ style comments pls

> Source/WebCore/dom/StyledElement.cpp:324
> +    // These two cases don't apply any color

Comments should be complete sentences (that end with a "." ).

> Source/WebCore/dom/StyledElement.cpp:326
> +    if (!c.length() || equalIgnoringCase(c, "transparent"))
>	   return;

Can we use a more descriptive variable name than "c" ?

> Source/WebCore/dom/StyledElement.cpp:332
> +    color.stripWhiteSpace();

Does this actually modify the string or just return a new version with
whitespace stripped?  I never remember.

> Source/WebCore/dom/StyledElement.cpp:355
> +	       if (!isASCIIHexDigit(color[pos]))
> +		   color.replace(pos, 1, "0");

For realz?  Amazing.

> Source/WebCore/dom/StyledElement.cpp:359
> +	   if (color.length() % 3 == 2)
> +	       color += "0";
> +	   if (color.length() % 3 == 1)

else if, right?

> Source/WebCore/dom/StyledElement.cpp:373
> +	       int redIndex = max(0, componentLength - 8);

8?  Can we name this constant.	It's not obvious to me where it comes from.

> Source/WebCore/dom/StyledElement.cpp:384
> +	       colors[0] = toASCIIHexValue(color[redIndex]) * 16 +
toASCIIHexValue(color[redIndex+1]);
> +	       colors[1] = toASCIIHexValue(color[greenIndex]) * 16 +
toASCIIHexValue(color[greenIndex+1]);
> +	       colors[2] = toASCIIHexValue(color[blueIndex]) * 16 +
toASCIIHexValue(color[blueIndex+1]);

Can you add some ASSERTs before this block to make sure all the indicies remain
sane and in their proper bounds?

> Source/WebCore/dom/StyledElement.cpp:390
> +    color = String::format("#%02x%02x%02x", colors[0], colors[1],
colors[2]);
> +    if (attr->decl()->setProperty(id, color, false))
> +	   return;

How could this fail?  Should we ASSERT_NOT_REACHED?