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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser 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 220102: Patch
https://bugs.webkit.org/attachment.cgi?id=220102&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220102&action=review


Code seems OK but please reduce the number of separate tests.

> Source/WebCore/ChangeLog:9
> +	   it shows a gray color. Incase there is color attribute the default
gray

Incase there is -> When there is

> Source/WebCore/ChangeLog:12
> +	   Firefox and IE show the same behaviour and webkit is
different.Making
> +	   the behaviour similiar to Firefox and IE.

Space after period. "Making the behaviour similiar to Firefox and IE" is not a
full sentence.

> Source/WebCore/ChangeLog:23
> +	   Incase color attribute is present that value is applied and the
default

Incase -> when the

> LayoutTests/ChangeLog:20
> +	   * fast/dom/HTMLHrElement: Added.
> +	   *
fast/dom/HTMLHrElement/header-color-noshade-attribute-expected.txt: Added.
> +	   *
fast/dom/HTMLHrElement/header-color-noshade-attribute-javascript-expected.txt:
Added.
> +	   *
fast/dom/HTMLHrElement/header-color-noshade-attribute-javascript.html: Added.
> +	   * fast/dom/HTMLHrElement/header-color-noshade-attribute.html: Added.

> +	   * fast/dom/HTMLHrElement/header-noshade-attribute-expected.txt:
Added.
> +	   * fast/dom/HTMLHrElement/header-noshade-attribute.html: Added.
> +	   *
fast/dom/HTMLHrElement/header-noshade-color-attribute-expected.txt: Added.
> +	   *
fast/dom/HTMLHrElement/header-noshade-color-attribute-javascript-expected.txt:
Added.
> +	   *
fast/dom/HTMLHrElement/header-noshade-color-attribute-javascript.html: Added.
> +	   * fast/dom/HTMLHrElement/header-noshade-color-attribute.html: Added.

> +	   *
fast/dom/HTMLHrElement/header-noshade-invalidcolor-attribute-expected.txt:
Added.
> +	   * fast/dom/HTMLHrElement/header-noshade-invalidcolor-attribute.html:
Added.

That's a lot of tests for such a simple change. I think should fold all these
into a single test (it can test multiple <hr> elements).