[webkit-reviews] review granted: [Bug 70369] RenderTableCell m_row and m_column should not be signed values : [Attachment 112702] Changelog update + minor edit.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 70369: RenderTableCell m_row and m_column should not be signed values
https://bugs.webkit.org/show_bug.cgi?id=70369

Attachment 112702: Changelog update + minor edit.
https://bugs.webkit.org/attachment.cgi?id=112702&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112702&action=review


> Source/WebCore/rendering/RenderTable.h:149
> +    void splitColumn(unsigned pos, int firstSpan);

When touching a line of code like this, it’s nice to remove abbreviations like
“pos”.

> Source/WebCore/rendering/RenderTable.h:154
> +    unsigned colToEffCol(unsigned col) const

And “col”.

> Source/WebCore/rendering/RenderTableCell.h:33
> +// FIXME: It is possible for these indexes to be reached for a table big
enough.
> +// We would need to enforce a maximal index on both rows and columns.

We should make a test case.

> Source/WebCore/rendering/RenderTableCell.h:51
> +    void setCol(unsigned col) { m_column = col; }

“column” instead of “col” at least for the local variable would be better

> Source/WebCore/rendering/RenderTableSection.h:156
> +    unsigned m_gridRows;

Not a great name for a data member that is a count. It sounds like this holds
actual rows, not a count of the number of rows.