[webkit-reviews] review denied: [Bug 12078] Clean up RenderTable* : [Attachment 12169] RenderTable* cleanup and reorg

[bugzilla-request-daemon at macosforge.org]

Sam Weinig <sam at webkit.org> has denied Sam Weinig <sam at webkit.org>'s request
for review:
Bug 12078: Clean up RenderTable*
http://bugs.webkit.org/show_bug.cgi?id=12078

Attachment 12169: RenderTable* cleanup and reorg
http://bugs.webkit.org/attachment.cgi?id=12169&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
Looks great!! But r- for now.


In AutoTableLayout.cpp and FixedTableLayout.cpp:

Put spaces around infix operators.

+    m_table->columnPositions()[m_table->columnPositions().size()-1] = pos;

It should be alright to remove the remaining qDebug() calls from these files as
well.


In RenderTable.cpp:

Resize and fill can be done with one call to Vector::fill(value, newSize)

+    m_columnPos.resize(2);
+    m_columnPos.fill(0);
+    m_columns.resize(1);
+    m_columns.fill(ColumnStruct());

Put spaces around '=='

+    if (m_caption && m_caption->style()->captionSide()==CAPBOTTOM) {

Missing spaces around infix '-'

+    memmove(m_columns.data() + pos + 1, m_columns.data() + pos, (oldSize-pos)
* sizeof(ColumnStruct));

Use 'unsigned' instead of 'unsigned int'

+    for (unsigned int i = 0; i < m_columns.size(); i++)


In RenderTableCell.cpp:

in function updateFromElement() there is no reason to declare the variables
'oldRSpan' and 'oldCSpan' outside of the if-statement, since that is the only
place they are used.


In RenderTableCell.h:

No reason to be inconsistent here, choose either setFoo(int foo) or setFoo(int
f)

+    int col() const { return m_column; }
+    void setCol(int col) { m_column = col; }
+    int row() const { return m_row; }
+    void setRow(int r) { m_row = r; }


In RenderTableSection.cpp:

Convert to use max(a,b)

+	     if (pos > m_rowPos[r + 1])
+		 m_rowPos[r + 1] = pos;

same here

+	     if (m_rowPos[r + 1] < bRowPos)
+		 m_rowPos[r + 1] = bRowPos;

and here

+	 if (m_rowPos[r + 1] < m_rowPos[r])
+	     m_rowPos[r + 1] = m_rowPos[r];

Put spaces around infix '+'

+		 cell->setPos(table()->columnPositions()[nEffCols] -
table()->columnPositions()[table()->colToEffCol(cell->col()+cell->colSpan())] +
hspacing, m_rowPos[rindx]);

Use '!foo' instead of 'foo == 0' (in a couple of identical places)

+    if (m_gridRows == 0 || totalCols == 0)

also here

+	 if (endcol == 0 && tx + table()->columnPositions()[0] -
table()->outerBorderLeft() <= y + w + os)


In RenderTableSection.h:

* on the wrong side

+    virtual const char *renderName() const { return "RenderTableSection"; }

and here

virtual void addChild(RenderObject *child, RenderObject *beforeChild = 0);

and here

+    virtual void dump(TextStream *, DeprecatedString ind = "") const;