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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 98432: Patch
https://bugs.webkit.org/attachment.cgi?id=98432&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=98432&action=review

review- because of the failing test in the test results in the patch

> LayoutTests/fast/dom/attribute-legacy-colors-expected.txt:1
> +This test ensures that legacy color attributes are parsed properly.

I don’t think there are enough tests here to test the algorithm and edge cases
given my reading of the code. I don’t see any test cases that get up to 128
characters, or pass 128 characters. I don’t see tests that cover the behavior
of skipping up to 8 zeroes but not 9 in the individual components.

> LayoutTests/fast/dom/attribute-legacy-colors-expected.txt:8
> +FAIL document.body.bgColor=' transparent
';getComputedStyle(document.body).backgroundColor; should be rgba(0, 0, 0, 0).
Was rgb(0, 0, 0).

Failing test!?

> Source/WebCore/dom/StyledElement.cpp:321
> +static String parseInvalidColorAttributeValue(const String attrValue)

This should be const String& attributeValue, not const String attrValue.

> Source/WebCore/dom/StyledElement.cpp:323
> +    const size_t maxColorLength = 128;

It would be good to say where this 128 comes from. I assume it’s from the HTML
specification’s invalid color parsing rules.

> Source/WebCore/dom/StyledElement.cpp:325
> +    // We'll pad the buffer to a multiple of 3, so we need to reserve a
little more for safety
> +    Vector<char, maxColorLength+3> chars;

A vector will automatically start using the heap instead of the stack once you
go past the inline buffer, so there's no need to have the inline buffer size be
large enough for the worst case. It just needs to be big enough to cover the
normal case. I suppose this is OK as is, but this pays the cost of using the
vector class without getting any benefit from it.

Since this buffer holds only hex digits, I'd suggest naming it digitBuffer or
digits, rather than naming it characters.

Given the comment the code should say "+ 2" rather than "+ 3". Saying "a little
more for safety" is unnecessarily vague; I think you’re saying that because
there’s no easy way to write "round up to multiple of 3", so we are using "+ 2"
to approximate the ceiling of that.

> Source/WebCore/dom/StyledElement.cpp:327
> +    size_t colorPos = 0;

I’d rather see this be named something like "i". It’s not the position of a
color, so I don’t like the name “color position” for it, and I also do not care
for abbreviations like “pos”.

> Source/WebCore/dom/StyledElement.cpp:333
> +    // Grab the first 128 chars, replacing non-hex chars with 0
> +    // Non-BMP chars are replaced with "00" due to them appearing as two
UChars in the String

We put periods on the end of sentences or even sentence-like fragments in our
comments in WebKit.

Also, I would say characters, not chars.

> Source/WebCore/dom/StyledElement.cpp:340
> +    // Pad the buffer to a multiple of 3

Again, period needed.

> Source/WebCore/dom/StyledElement.cpp:346
> +    if (chars.size() % 3 == 2)
> +	   chars.append('0');
> +    else if (chars.size() % 3 == 1) {
> +	   chars.append('0');
> +	   chars.append('0');
> +    }

No real need to check the size here. We could just unconditionally append two
"0" characters instead after the size zero check.

> Source/WebCore/dom/StyledElement.cpp:351
> +    if (chars.size() == 3)

If we unconditionally added two "0" characters, then this would be "< 6" rather
than "== 3", but otherwise the code could be the same.

> Source/WebCore/dom/StyledElement.cpp:361
> +    // Skip digits until one of them is non-zero, or you've only got two
left in the component.

We’d normally say “we’ve”.

> Source/WebCore/dom/StyledElement.cpp:373
> +    return String::format("#%c%c%c%c%c%c", chars[redIndex],
chars[redIndex+1], chars[greenIndex], chars[greenIndex+1], chars[blueIndex],
chars[blueIndex+1]);   

We normally would put spaces around the "+ 1". in this expression.

> Source/WebCore/dom/StyledElement.cpp:376
> +// color parsing that matches HTML's "rules for parsing a legacy color
value"

WebKit style would be to capitalize and put a period at the end.

> Source/WebCore/dom/StyledElement.cpp:377
> +void StyledElement::addCSSColor(Attribute* attr, int id, const String&
attrValue)

I think these should have the names attribute and value or attributeValue
rather than attr and attrValue.

> Source/WebCore/dom/StyledElement.cpp:383
> +    const String color = attrValue.stripWhiteSpace();

Not our WebKit style to use const in a case like this, although there is at
least one person debating this style point on webkit-dev for the future.

> Source/WebCore/dom/StyledElement.cpp:385
> +    // "transparent" doesn't apply a color either.

Period at the end here.

> Source/WebCore/dom/StyledElement.cpp:402
> +    // If the string is a 3 or 6-digit hex color, use that color.
> +    if (color[0] == '#' && (color.length() == 4 || color.length() == 7) &&
attr->decl()->setProperty(id, color, false))
> +	   return;

The comment says this runs if the string is a "hex color", but there is nothing
here that checks if the characters are hex digits. So either the comment or the
code is wrong.

> Source/WebCore/dom/StyledElement.cpp:404
> +    // Otherwise, use the crazy legacy parsing rules.

Not sure this comment helps. If the right name for these rules are "crazy
legacy parsing rules" then the function should just be named that so we don’t
need the comment.