[webkit-reviews] review denied: [Bug 17674] <hr> appears gray instead of green because of color attribute is defined followed by noshade attribute : [Attachment 219966] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied gur.trio at gmail.com's request for
review:
Bug 17674: <hr> appears gray instead of green because of color attribute is
defined followed by noshade attribute
https://bugs.webkit.org/show_bug.cgi?id=17674

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

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


This patch might be an OK start, but I need to see more test cases, because I
don’t think these are handled correctly:

1) Cases where color and noshade attribute are added in different sequence.
2) Invalid color attribute; present but not something that we can parse.
3) Color set by CSS rather than with a color attribute.

I’m not sure what the right concept is here. Is the color attribute special, or
is any color specified in either an attribute or in CSS special?

> Source/WebCore/html/HTMLHRElement.cpp:85
> +	   if (nullAtom == getAttribute(colorAttr)) {

The efficient way to write this is:

    if (!hasAttribute(colorAttr))

> Source/WebCore/html/HTMLHRElement.cpp:90
> +	       style.setProperty(CSSPropertyBorderColor, darkGrayValue);
> +	       style.setProperty(CSSPropertyBackgroundColor, darkGrayValue);

This seems OK if the noshade attribute is handled after the color attribute.
But what will remove these properties if someone later adds a color attribute
to the <hr> element?