[webkit-reviews] review denied: [Bug 73180] CSS3 currentColor on outline-color gets treated as inherit : [Attachment 121055] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied David Barr
<davidbarr at chromium.org>'s request for review:
Bug 73180: CSS3 currentColor on outline-color gets treated as inherit
https://bugs.webkit.org/show_bug.cgi?id=73180

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121055&action=review


The test case looks better. Thanks for abstracting the CSS property logic in
runTest() (*). That being said, putting the test description in the HTML Title
element is insufficient. Please put a test description in the HTML markup to
make it straight forward to both understand what the test when testing by hand
as well as to help someone interpret the meaning of the message "PASSED" in the
expected results file, outline-currentcolor-expected.txt. One example of
putting a description in the HTML markup is
<http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/text-align-webkit-ma
tch-parent-parse.html>.

Also, we should extract the inline JavaScript script in this file into an
external JavaScript file, say
LayoutTests/fast/css/resources/computeCSSColorProperty.js, and then reference
this external script in both this file and
<http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/background-currentco
lor.html>.

> LayoutTests/fast/css/outline-currentcolor.html:11
> +    // FIXME: Re-write this and background-currentcolor.html to use
js-test-pre.js

This is fine as-is. That being said, I suggest we file a bug for this and
reference the bug number in this FIXME so as to make this FIXME actionable.