[webkit-reviews] review denied: [Bug 75523] Unify and modernize fast/css/{outline, background}-currentcolor.html : [Attachment 135027] 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 75523: Unify and modernize fast/css/{outline,background}-currentcolor.html
https://bugs.webkit.org/show_bug.cgi?id=75523

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

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


> LayoutTests/ChangeLog:7
> +	   Unify and modernize fast/css/{outline,background}-currentcolor.html
> +	   https://bugs.webkit.org/show_bug.cgi?id=75523
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

This bug title isn't sufficiently descriptive of this change. Can you elaborate
on how your proposed change unifies and modernizes these tests?

Maybe adding a description can help clarify the bug title and explain the
motivation behind making these changes. You may want to say something like:

Use js-test-pre utility functions instead of hardcoded testing logic in
fast/css/{outline, background}-currentcolor.html so as to simplify the test
code and make the test more closely conform to the visual appearance of other
PASS/FAIL tests.

> LayoutTests/fast/css/background-currentcolor.html:3
> +#test div { height: 5em; width: 10em; margin-bottom: 1em; }

This is OK as-as. That being said, we're underutilizing the class="test" in the
markup (below). We can a class selector here instead of a descendent selector
to match the same set of elements. If you choose to use a descendent selector
then we should remove the 'class="test"' from the markup since it isn't being
used.

> LayoutTests/fast/css/background-currentcolor.html:6
> +<div id="test">

Can we come up with a better name for this? Maybe, test-container?

> LayoutTests/fast/css/background-currentcolor.html:9
> +<div class="test" id="one" style="color:green; background: currentColor"
></div>
> +<div class="test" id="two" style="color:red; background: currentColor"
></div>
> +<div class="test" style="color:green">

We're underutilizing the class "test" here. Currently we match these elements
using the descendent selector, #test div. We should either match these element
using a class selector or remove the class attribute. See my remark on #test
div (above) for more details.

> LayoutTests/fast/css/background-currentcolor.html:13
> +<p id="description">Test that background-color is non-inherit and
currentColor is handled correctly. The test passes if 3 boxes with green
backgrounds are displayed.</p>

As can be seen in the expected results,
LayoutTests/fast/css/background-currentcolor-expected.txt, the text "TEST
COMPLETE" is above the test description because there isn't an explicit <div
id="console"></div> in the markup; => js-test-pre.js will insert such an
element above <div id="test">. This makes both the test case and its expected
results look weird and inconsistent with the output of other PASS/FAIL tests. I
suggest that either we use description() (*) or explicitly declare <div
id="console"></div> after <div id="test"> in the markup such that the text
"TEST COMPLETE" is the last line of text in the expected results file.

(*)
<http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/js-test-pre
.js?rev=110650#L28>

> LayoutTests/fast/css/outline-currentcolor.html:13
> +<p id="description">Test that outline-color is non-inherit and currentColor
is handled correctly. The test passes if 3 boxes with green outlines are
displayed.</p>

As can be seen in the expected results,
LayoutTests/fast/css/outline-currentcolor-expected.txt, the text "TEST
COMPLETE" is above the test description. This looks weird. See my remark for
the descriptive text in file background-currentcolor.html for more details.

> LayoutTests/fast/css/outline-currentcolor.html:21
> +var two = document.getElementById("two");
> +var three = document.getElementById("three");
> +two.style.color = "green";
> +shouldBeEqualToString('window.getComputedStyle(two)["' + property + '"]',
'rgb(0, 128, 0)');
> +shouldBeEqualToString('window.getComputedStyle(three).color', 'rgb(0, 128,
0)');

This code is identical to code in file
LayoutTests/fast/css/background-currentcolor.html. Maybe we should consider
extracting this code into a common