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

[bugzilla-daemon at webkit.org]

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

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

This change looks OK, but the same logic could be done with no memory
allocation except for the allocation of the strings for the property values.
I’d prefer to see that.

> Source/WebCore/dom/StyledElement.cpp:354
> +	       color.replace(pos, 1, "0");

Calling replace on a string is an extremely inefficient way to modify a
character at a time, and involves multiple allocations per call. I suggest
using a UChar array on the stack instead.

> Source/WebCore/dom/StyledElement.cpp:357
> +	   color += "0";

Appending a string like this is inefficient, requiring a memory allocation each
time.

> Source/WebCore/dom/StyledElement.cpp:359
> +	   color += "00";

Ditto.

> Source/WebCore/dom/StyledElement.cpp:361
> +	   color = "000";

Ditto.