[webkit-reviews] review granted: [Bug 73703] StyledElement: Simplify addCSSColor(). : [Attachment 117689] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 73703: StyledElement: Simplify addCSSColor().
https://bugs.webkit.org/show_bug.cgi?id=73703

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

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


> Source/WebCore/dom/StyledElement.cpp:374
> +    // An empty string doesn't apply a color. (Only whitespace does, which
is why this check occurs before trimming.)

This wording (and the old wording) is terribly confusing.

The phrase “only whitespace applies a color” is not what we mean here. What we
mean is “A string containing only whitespace applies a color”. So we need to
word it that way: “A string containing only”, “One containing only”, “A string
with only”, or “One with only”.

The term “trimming” is confusing, since the function called below is
stripWhiteSpace, and does not use the word “trim”.

> Source/WebCore/dom/StyledElement.cpp:388
> +    Color color(colorString);

I think I’d call this local variable parsedColor.