[webkit-reviews] review denied: [Bug 86885] CSS 2.1 failure: border-conflict-element-021a : [Attachment 144785] 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 144785: Patch
https://bugs.webkit.org/attachment.cgi?id=144785&action=review

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


Please add the requested testing, this would make me confident that you are not
breaking already working code. Without those tests, I don't think we should
accept the patch as it's too easy to break painting.

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

My take would be to add the test as part of this change. You should grab the
change from the official test suite, not one of the test author's website.
Robert knows where it is if you can't find it.

> Source/WebCore/rendering/RenderTable.cpp:1118
> +    if (m_foot)
> +	   bottomSection = m_foot;

This is still wrong. All the pointers are to the *first* section of its kind,
including m_foot. Note that sectionBelow seems to have the same logical flaw.

If you had added some testing as requested previous for the multiple sections
of the same type you would have seen an issue here.

> Source/WebCore/rendering/RenderTable.h:195
> +    RenderTableSection* bottomSection() const;

Note that bottom / top are in device coordinates and the naming don't play well
with writing-mode (which is supported on table). As we are not consistent in
our naming here, this shouldn't be a show-stopper.

> LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:6
> +<title>Bugzilla Bug 86885: CSS 2.1 failure: border-conflict-element-021a.
Expected result.</title>

Unneeded title.

> LayoutTests/fast/table/border-collapsing/adjacent-row-groups-expected.html:7
> +<style type="text/css">

Unneeded type attribute.

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

> +

Can you remove the unneeded space?