[webkit-reviews] review denied: [Bug 40775] Table Cell Layering : [Attachment 59037] Table Cell Layering Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Fady Samuel <fsamuel at chromium.org>'s
request for review:
Bug 40775: Table Cell Layering
https://bugs.webkit.org/show_bug.cgi?id=40775

Attachment 59037: Table Cell Layering Patch
https://bugs.webkit.org/attachment.cgi?id=59037&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
Some initial feedback:

(1) I don't think CellNode is necessary. You can just make CellStruct have a
Vector<RenderTableCell, 1>.  I'd give CellStruct a cell() accessor to get the
"primary" RenderTableCell for the code that is using that.
(2) m_layer is not a good name for a variable added to RenderTableCell, since
it shadows the m_layer in one of RenderTableCell's base classes.  Let's not
overload "layer" if we can help it, since that has a pretty clear meaning in
WebCore rendering code.  Something more like m_overlapLevel would be better.
(3) I'm not convinced that the overlap level member variable is needed.  Adding
4 bytes to all RenderTableCells for this seems problematic (same with the extra
boolean that was added).  Can't paint order be inferred from ordering in the
CellStruct? Maybe the answer is no, but think about it.  I'd consider using
hashes in the .cpp file though rather than bloating RenderTableCell for a rare
case (e.g., like we do with column members in RenderBlock).
(4) I don't like the name renderCellAtTop.  Someone reading that is going to
think "top" means "top of the table."  If we're talking about the notion of a
principal/primary cell that occupies that slot, then maybe primaryCell is good
enough.