[webkit-reviews] review denied: [Bug 52185] Cell heights are disproportional when a row-spanning cell contains a block element that determines the height of the rows : [Attachment 200387] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Suchit Agrawal <a.suchit at samsung.com>'s
request for review:
Bug 52185: Cell heights are disproportional when a row-spanning cell contains a
block element that determines the height of the rows
https://bugs.webkit.org/show_bug.cgi?id=52185

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=200387&action=review


Looks pretty good. A couple of comments.

> Source/WebCore/rendering/RenderTableSection.cpp:279
> +    Vector<RenderTableCell*> rowspanCells;

This should be rowSpanCells (capitalize the S).

I imagine this is empty or small most of the time, so maybe give it a small
inline capacity, e.g.,

Vector<RenderTableCell*, 2> rowSpanCells;

> Source/WebCore/rendering/RenderTableSection.cpp:346
> +    if (rowspanCells.size() > 0) {

We should pull all this code into a helper function rather than making
calcRowLogicalHeight so much bigger and just call the helper function if
rowspanCells is non-empty.

if (!rowSpanCells.isEmpty())
    callHelperFunction(rowSpanCells);