[webkit-reviews] review canceled: [Bug 83978] CSS 2.1 failure: table-columns-example-001 fails : [Attachment 137604] Patch

Wed Apr 18 09:14:06 PDT 2012

Julien Chaffraix <jchaffraix at webkit.org> has canceled Robert Hogan
<robert at webkit.org>'s request for review:
Bug 83978: CSS 2.1 failure: table-columns-example-001 fails
https://bugs.webkit.org/show_bug.cgi?id=83978

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

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

It looks really much better, thanks!

> Source/WebCore/html/HTMLTableElement.cpp:429
> +    if ((!m_borderAttr && !m_borderColorAttr) || m_frameAttr) {

I think the m_frameAttr case should be singled out as you are basically doing:

if (m_frameAttr)
   return 0;

> Source/WebCore/html/HTMLTableElement.cpp:430
> +	   if (!m_frameAttr && (m_rulesAttr == ColsRules || m_rulesAttr ==
RowsRules || m_rulesAttr == AllRules)) {

Shouldn't you also check for m_rulesAttr == GroupsRules? I don't see this
tested and would bet our testing is lacking...

If I am right here, I think the check should be moved to an helper methods:
like rulesIsSetAndNotNone().

> LayoutTests/ChangeLog:18
> +	   * fast/css/table-rules-attribute-with-frame1-expected.png: Added.
> +	   * fast/css/table-rules-attribute-with-frame1-expected.txt: Added.
> +	   * fast/css/table-rules-attribute-with-frame1.html: Added.
> +	   * fast/css/table-rules-attribute-with-frame2-expected.png: Added.
> +	   * fast/css/table-rules-attribute-with-frame2-expected.txt: Added.
> +	   * fast/css/table-rules-attribute-with-frame2.html: Added.

I really looks like those could be ref-tests as we don't care about the DRT
dump. Could it be possible to compare to the equivalent border-collapse:
separate case?

> LayoutTests/css2.1/20110323/table-columns-example-001.htm:9
> +	   <meta name="assert" content="CSS can be used to emmulate the HTML
'rules = cols' attribute (example from section 17.3).">

Typo 'emmulate', was that in the original test case?

> LayoutTests/fast/css/table-rules-attribute-with-frame1.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">

We usually prefer the HTML5 doctype for new tests:

<!DOCTYPE html>

> LayoutTests/fast/css/table-rules-attribute-with-frame1.html:10
> +	   <meta name="assert" content="The first table has four vertical lines
capped and bottomed by two horizontal lines.">
> +	   <meta name="assert" content="The second table has 2 vertical
lines.">
> +	   <meta name="assert" content="The third table has 2 vertical lines
capped by a horizontal line.">
> +	   <meta name="assert" content="The fourth table has 2 vertical lines
bottomed by a horizontal line.">
> +	   <meta name="assert" content="The fifth table has 3 vertical lines.">

> +	   <meta name="assert" content="The sixth table has 3 vertical lines.">

That's quite a neat way to embed the expected result.