[webkit-reviews] review granted: [Bug 85229] Move RenderTableCell's row index to RenderTableRow : [Attachment 139525] Proposed refactoring 1: move the code to RenderTableRow and rename row() to rowIndex().

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 85229: Move RenderTableCell's row index to RenderTableRow
https://bugs.webkit.org/show_bug.cgi?id=85229

Attachment 139525: Proposed refactoring 1: move the code to RenderTableRow and
rename row() to rowIndex().
https://bugs.webkit.org/attachment.cgi?id=139525&action=review

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


Certainly looks like a big improvement. Please address comments before
committing.

> Source/WebCore/rendering/RenderTableCell.cpp:333
> +    if (parent() && section() && oldStyle && style()->height() !=
oldStyle->height())

Shouldn't you continue to check rowWasSet here? Seems like the same applies.
rowIndex could be unsetRowIndex, no? Actually, now that I look at
RenderTableSection::addChild, it looks like it might not be possible. If you
think it's not possible, maybe add an ASSERT?

> Source/WebCore/rendering/RenderTableCell.h:185
>      unsigned m_column : 31;

If you s/31/30/ here you should be able to get better big packing here.

> Source/WebCore/rendering/RenderTableRow.h:93
> +    unsigned m_rowIndex : 31;

Don't think we need to make this a bitfield now that it doesn't save us any
memory.