[webkit-reviews] review denied: [Bug 86885] CSS 2.1 failure: border-conflict-element-021a : [Attachment 144341] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 86885: CSS 2.1 failure: border-conflict-element-021a
https://bugs.webkit.org/show_bug.cgi?id=86885

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

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


r- due to the multiple headers / footers issue.

> Source/WebCore/ChangeLog:3
> +	   CSS 2.1 failure: border-conflict-element-021a

Btw, I would expect to see this test added or unskipped as part of this change.


> Source/WebCore/rendering/RenderTable.cpp:597
> +		   static_cast<RenderObject*>(section)->paint(info,
childPoint);

No static_cast please, if you can't call paint because paint() is private, you
should make it public (don't forget to annotate the function with OVERRIDE as
you move it).

> Source/WebCore/rendering/RenderTable.cpp:1122
> +    if (m_foot)
> +	   return m_foot;
> +    if (m_firstBody) {
> +	   for (RenderObject* section = lastChild(); section; section =
section->previousSibling())
> +	       if (section->isTableSection() && section != m_head)
> +		   return toRenderTableSection(section);
> +    }
> +    return m_head;

m_foot, m_firstBody, m_head are the top (ie first) sections of this type. We
allow several footers and headers to be inserted in our tree so this means this
function will be totally broken in this case.

The fact that it is making no test fail means that our testing is lacking and
should be improved.

> LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:12

> +  .red { border-color: red; }

As said, I would rather reserve 'red' for failures. In this case, use another
color as this makes the output confusing for someone unfamiliar with the test.