Bug 12078 - Clean up RenderTable*
Summary: Clean up RenderTable*
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-02 11:56 PST by mitz
Modified: 2007-01-04 22:12 PST (History)
0 users

See Also:


Attachments
RenderTable* cleanup and reorg (115.48 KB, patch)
2007-01-02 11:58 PST, mitz
sam: review-
Details | Formatted Diff | Diff
RenderTable* cleanup and reorg (121.39 KB, patch)
2007-01-04 04:35 PST, mitz
sam: review-
Details | Formatted Diff | Diff
RenderTable* cleanup and reorg (121.90 KB, patch)
2007-01-04 07:03 PST, mitz
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-01-02 11:56:27 PST
Going to attach a patch with some cleanup and reorganization.
Comment 1 mitz 2007-01-02 11:58:50 PST
Created attachment 12169 [details]
RenderTable* cleanup and reorg

No layout test regressions.
Comment 2 Sam Weinig 2007-01-02 13:59:54 PST
Comment on attachment 12169 [details]
RenderTable* cleanup and reorg

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;
Comment 3 mitz 2007-01-04 04:35:33 PST
Created attachment 12213 [details]
RenderTable* cleanup and reorg

Did as Sam asked, except for removing qdebug()s from the table layout classes, since it's outside the scope of this bug. Did another change that Sam suggested on IRC, which is the addition of recalcCellsIfNeeded().
Comment 4 Sam Weinig 2007-01-04 06:30:58 PST
Comment on attachment 12213 [details]
RenderTable* cleanup and reorg

A couple more nits

In FixedTableLayout.cpp:

put spaces around the infix '+'

m_width[cCol+i].setRawValue(w.type(), w.rawValue() * eSpan);

and here

if (m_width[cCol+i].isAuto() && w.type() != Auto) {
    m_width[cCol+i].setRawValue(w.type(), w.rawValue() * eSpan);

and here around the infix '-'

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


In RenderTableSection.cpp:

Convert to use max()

if (totalPercent > 100 * percentScaleFactor)
    totalPercent = 100 * percentScaleFactor;
Comment 5 mitz 2007-01-04 07:03:59 PST
Created attachment 12219 [details]
RenderTable* cleanup and reorg

Discussed it with Sam on IRC and made a few changes.
Comment 6 Sam Weinig 2007-01-04 07:06:11 PST
Comment on attachment 12219 [details]
RenderTable* cleanup and reorg

r=me!
Comment 7 David Kilzer (:ddkilzer) 2007-01-04 22:12:37 PST
Committed revision 18608.