[webkit-reviews] review denied: [Bug 19681] borderColor on table elements should be ignored : [Attachment 125623] Fix to ignore borderColor attribute of table when its border attribute is not specified.

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied KishoreGanesh
<kbolisetty at innominds.com>'s request for review:
Bug 19681: borderColor on table elements should be ignored
https://bugs.webkit.org/show_bug.cgi?id=19681

Attachment 125623: Fix to ignore borderColor attribute of table when its border
attribute is not specified.
https://bugs.webkit.org/attachment.cgi?id=125623&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125623&action=review


I would have a question: do we know of any site that are failing now due to
this bug? Is borderColor that important in the wild that we should support it?
(bugzilla reports 2 bugs that are pretty old on 'borderColor')

I don't know what you used to generate the patch but it is confusing our review
tool (we can't see the image you are adding) and our EWS (they can't apply the
patch). Please generate your patch with webkit-patch as it guarantees that our
tools will be able to process it. r- mostly because no automated test were run.


> Source/WebCore/ChangeLog:8
> +	   borderColor is not a standard table attribute,

I don't know what is a "standard table attribute".

> Source/WebCore/ChangeLog:9
> +	   hence borderColor should be ignored atleast when border is not
specified.

You should put in your ChangeLog the comparison you did with FF and IE and say
that it matches them. It's also an IE-only attribute that is not standardized
anywhere so this makes sense.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:3
> +    <title>Test to ignore borderColor attribute when border is not present.
19681</title>

The bug number is kind of meaningless here. It would be more readable to state
"bug 19681" without abbreviating. Usually you want your bug number to be
visible in your test output (if you decide to include some text of course)
which won't be the case here.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:6
> +	   if (window.layoutTestController)
> +	       layoutTestController.waitUntilDone();

Not calling layoutTestController.notifyDone() make your test fail! (see the
first line of your expected.txt file)

You don't need waitUntilDone as by default DRT waits for the end of the 'load'
event handler.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:17
> +		   <td >Some Text</td> 

This text doesn't add anything and worse: it makes your output
platform-dependent. See
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Pix
eltesttips

Here you could be just setting height: 100px; width: 100px to all table cells
without this bad effect.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:21
> +	   <table borderColor="red"> 

My take on using colors is that red should be for failures. Other reviewers
have other options on that though.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:39
> +</html>

This test is a good example where:
* you don't really need a DRT dumps and the associated image as you just want
to know if the style was applied so using getComputedStyle() should be enough.
* if I am mistaking, this should be a ref-test as pointed out by Robert.