[webkit-reviews] review denied: [Bug 87900] Add support for direction on table row group with collapsing borders : [Attachment 145402] Properly support direction on row group, naming nits / forgotten tests welcome. Admire the new hotness\!

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 87900: Add support for direction on table row group with collapsing borders
https://bugs.webkit.org/show_bug.cgi?id=87900

Attachment 145402: Properly support direction on row group, naming nits /
forgotten tests welcome. Admire the new hotness\!
https://bugs.webkit.org/attachment.cgi?id=145402&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145402&action=review


R- just for the naming issues. Nice use of reftests!

> Source/WebCore/rendering/RenderTableSection.cpp:1426
> +void RenderTableSection::determineLogicalPositionForCell(RenderTableCell*
cell, unsigned effectiveColumn) const

Maybe name this setLogicalPositionForCell? I'd expect "determine" to do some
computation and then return a value.

> Source/WebCore/rendering/RenderTableSection.h:117
> +    bool hasMixedDirection() const

I don't think this method name is clear enough. It's not obvious that it
compares against the table.

How about inverting the logical and calling this hasSameDirectionAsParentTable?
or just hasSameDirectionAsTable?

> Source/WebCore/rendering/RenderTableSection.h:120
> +	   TextDirection tableDirection = table()->style()->direction();
> +	   return tableDirection != style()->direction();

Nit: I'd inline tableDirection here.

> LayoutTests/fast/table/table-rtl-section-ltr.html:21
> +    <p>Test for bug <a
href="https://bugs.webkit.org/show_bug.cgi?id=87900">87900</a>: Add support for
direction on table row group with collapsing borders</p>

Nit: I prefer the description to say what the test is actually testing (e.g.
Test that borders are properly collapsed when a tbody has a different direction
than it's parent table.).

I don't think in these cases that linking to the bug gives useful information
beyond that description since it's not a tricky edge case for which you'd need
to read the bug in order to understand what's going on.

Also, you can always find the bug number by looking at the commit history.

Finally, if we were ever to upstream these tests to the W3C, we'd have to strip
all these.