Changeset 117988 in webkit

Timestamp:

May 22, 2012 9:43:48 AM (12 years ago)

Author:

jchaffraix@webkit.org

Message:

Centralize and clean-up table column iteration
​https://bugs.webkit.org/show_bug.cgi?id=87051

Reviewed by Eric Seidel.

Source/WebCore:

Tests: fast/table/caption-between-column-and-column-group.html

fast/table/caption-between-column-group-and-column.html
fast/table/caption-between-columns.html

The existing code had duplications between different classes and some
of the iterating functions could use a better place. That's what this
change solves, along with several renamings.

• rendering/RenderTableCol.h:

(WebCore::RenderTableCol::isTableColumnGroupWithColumnChildren):
Renamed isTableColGroup to this to better reflect what it checks.
Also added a new function: nextColumn.

• rendering/FixedTableLayout.cpp:

(WebCore::FixedTableLayout::calcWidthArray):
Updated after isTableColGroup renaming.

• rendering/RenderTable.cpp:

(WebCore::RenderTable::firstColumn):
Added this new function to get the first column (or column group).

(WebCore::RenderTable::colElement):
Cleaned up this function: switched the loop to a 'for' now that the
helper functions make it easy. Cleaned up the ordering and the naming.

• rendering/RenderTable.h:

Added firstColumn.

• rendering/RenderTableCell.cpp:

(WebCore::RenderTableCell::styleOrColLogicalWidth):
Updated to use nextColumn().

• rendering/RenderTableCol.cpp:

(WebCore::RenderTableCol::nextColumn):
Added this new helper function to centralize the code to iterate over columns.

LayoutTests:

Those tests just validate and enforce what we were already doing that were
not covered by any existing tests.

• fast/table/caption-between-column-and-column-group-expected.html: Added.
• fast/table/caption-between-column-and-column-group.html: Added.
• fast/table/caption-between-column-group-and-column-expected.html: Added.
• fast/table/caption-between-column-group-and-column.html: Added.
• fast/table/caption-between-columns-expected.html: Added.
• fast/table/caption-between-columns.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/table/caption-between-column-and-column-group-expected.html (added)
• LayoutTests/fast/table/caption-between-column-and-column-group.html (added)
• LayoutTests/fast/table/caption-between-column-group-and-column-expected.html (added)
• LayoutTests/fast/table/caption-between-column-group-and-column.html (added)
• LayoutTests/fast/table/caption-between-columns-expected.html (added)
• LayoutTests/fast/table/caption-between-columns.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/FixedTableLayout.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderTable.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderTable.h (modified) (1 diff)
• Source/WebCore/rendering/RenderTableCell.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderTableCol.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderTableCol.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-05-22  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Centralize and clean-up table column iteration
+        https://bugs.webkit.org/show_bug.cgi?id=87051
+
+        Reviewed by Eric Seidel.
+
+        Those tests just validate and enforce what we were already doing that were
+        not covered by any existing tests.
+
+        * fast/table/caption-between-column-and-column-group-expected.html: Added.
+        * fast/table/caption-between-column-and-column-group.html: Added.
+        * fast/table/caption-between-column-group-and-column-expected.html: Added.
+        * fast/table/caption-between-column-group-and-column.html: Added.
+        * fast/table/caption-between-columns-expected.html: Added.
+        * fast/table/caption-between-columns.html: Added.
+
 2012-05-22  Sudarsana Nagineni  <sudarsana.nagineni@linux.intel.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-05-22  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Centralize and clean-up table column iteration
+        https://bugs.webkit.org/show_bug.cgi?id=87051
+
+        Reviewed by Eric Seidel.
+
+        Tests: fast/table/caption-between-column-and-column-group.html
+               fast/table/caption-between-column-group-and-column.html
+               fast/table/caption-between-columns.html
+
+        The existing code had duplications between different classes and some
+        of the iterating functions could use a better place. That's what this
+        change solves, along with several renamings.
+
+        * rendering/RenderTableCol.h:
+        (WebCore::RenderTableCol::isTableColumnGroupWithColumnChildren):
+        Renamed isTableColGroup to this to better reflect what it checks.
+        Also added a new function: nextColumn.
+
+        * rendering/FixedTableLayout.cpp:
+        (WebCore::FixedTableLayout::calcWidthArray):
+        Updated after isTableColGroup renaming.
+
+        * rendering/RenderTable.cpp:
+        (WebCore::RenderTable::firstColumn):
+        Added this new function to get the first column (or column group).
+
+        (WebCore::RenderTable::colElement):
+        Cleaned up this function: switched the loop to a 'for' now that the
+        helper functions make it easy. Cleaned up the ordering and the naming.
+
+        * rendering/RenderTable.h:
+        Added firstColumn.
+
+        * rendering/RenderTableCell.cpp:
+        (WebCore::RenderTableCell::styleOrColLogicalWidth):
+        Updated to use nextColumn().
+
+        * rendering/RenderTableCol.cpp:
+        (WebCore::RenderTableCol::nextColumn):
+        Added this new helper function to centralize the code to iterate over columns.
+
 2012-05-22  Hao Zheng  <zhenghao@chromium.org>
 

trunk/Source/WebCore/rendering/FixedTableLayout.cpp

@@ -78 +78 @@
 }
 
-static RenderObject* nextCol(RenderObject* child)
-{
-    // If child is a colgroup, the next col is the colgroup's first child col.
-    if (RenderObject* next = child->firstChild())
-        return next;
-    // Otherwise it's the next col along.
-    if (RenderObject* next = child->nextSibling())
-        return next;
-    // Failing that, the child is the last col in a colgroup, so the next col is the next col/colgroup after its colgroup.
-    if (child->parent()->isTableCol())
-        return child->parent()->nextSibling();
-    return 0;
-}
-
 int FixedTableLayout::calcWidthArray(int)
 {
@@ -102 +88 @@
 
     unsigned currentEffectiveColumn = 0;
-    for (RenderObject* child = m_table->firstChild();child && child->isTableCol(); child = nextCol(child)) {
-
-        // Width specified by column-groups does not affect column width in fixed layout tables
-        RenderTableCol* col = toRenderTableCol(child);
+    for (RenderTableCol* col = m_table->firstColumn(); col; col = col->nextColumn()) {
         col->computePreferredLogicalWidths();
 
-        if (col->isTableColGroup())
+        // Width specified by column-groups that have column child does not affect column width in fixed layout tables
+        if (col->isTableColumnGroupWithColumnChildren())
             continue;
 

trunk/Source/WebCore/rendering/RenderTable.cpp

@@ -727 +727 @@
 }
 
-RenderTableCol* RenderTable::nextColElement(RenderTableCol* current) const
-{
-    RenderObject* next = current->firstChild();
-    if (!next)
-        next = current->nextSibling();
-    if (!next && current->parent()->isTableCol())
-        next = current->parent()->nextSibling();
-
-    while (next) {
-        if (next->isTableCol())
-            return toRenderTableCol(next);
-        if (!m_captions.contains(next))
+RenderTableCol* RenderTable::firstColumn() const
+{
+    for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
+        if (child->isTableCol())
+            return toRenderTableCol(child);
+
+        // We allow only table-captions before columns or column-groups.
+        if (!child->isTableCaption())
             return 0;
-        next = next->nextSibling();
-    }
-    
+    }
+
     return 0;
 }
@@ -750 +745 @@
     if (!m_hasColElements)
         return 0;
-    RenderObject* child = firstChild();
-    unsigned cCol = 0;
-
-    while (child) {
-        if (child->isTableCol())
-            break;
-        if (!m_captions.contains(child))
-            return 0;
-        child = child->nextSibling();
-    }
-    if (!child)
-        return 0;
-
-    RenderTableCol* colElem = toRenderTableCol(child);
-    while (colElem) {
-        unsigned span = colElem->span();
-        if (!colElem->firstChild()) {
-            unsigned startCol = cCol;
-            ASSERT(span >= 1);
-            unsigned endCol = cCol + span - 1;
-            cCol += span;
-            if (cCol > col) {
-                if (startEdge)
-                    *startEdge = startCol == col;
-                if (endEdge)
-                    *endEdge = endCol == col;
-                return colElem;
-            }
-        }
-        colElem = nextColElement(colElem);
+
+    unsigned columnCount = 0;
+    for (RenderTableCol* columnRenderer = firstColumn(); columnRenderer; columnRenderer = columnRenderer->nextColumn()) {
+        if (columnRenderer->isTableColumnGroupWithColumnChildren())
+            continue;
+
+        unsigned span = columnRenderer->span();
+        unsigned startCol = columnCount;
+        ASSERT(span >= 1);
+        unsigned endCol = columnCount + span - 1;
+        columnCount += span;
+        if (columnCount > col) {
+            if (startEdge)
+                *startEdge = startCol == col;
+            if (endEdge)
+                *endEdge = endCol == col;
+            return columnRenderer;
+        }
     }
 

trunk/Source/WebCore/rendering/RenderTable.h

@@ -176 +176 @@
     }
 
+    // Return the first column or column-group.
+    RenderTableCol* firstColumn() const;
+
     RenderTableCol* colElement(unsigned col, bool* startEdge = 0, bool* endEdge = 0) const;
-    RenderTableCol* nextColElement(RenderTableCol* current) const;
 
     bool needsSectionRecalc() const { return m_needsSectionRecalc; }

trunk/Source/WebCore/rendering/RenderTableCell.cpp

@@ -133 +133 @@
             colWidthSum = Length(colWidthSum.value() + colWidth.value(), Fixed);
 
-            tableCol = table()->nextColElement(tableCol);
+            tableCol = tableCol->nextColumn();
             // If no next <col> tag found for the span we just return what we have for now.
             if (!tableCol)

trunk/Source/WebCore/rendering/RenderTableCol.cpp

@@ -117 +117 @@
 }
 
+RenderTableCol* RenderTableCol::nextColumn() const
+{
+    // If |this| is a column-group, the next column is the colgroup's first child column.
+    if (RenderObject* firstChild = this->firstChild())
+        return toRenderTableCol(firstChild);
+
+    // Otherwise it's the next column along.
+    RenderObject* next = nextSibling();
+
+    // Failing that, the child is the last column in a column-group, so the next column is the next column/column-group after its column-group.
+    if (!next && parent()->isTableCol())
+        next = parent()->nextSibling();
+
+    for (; next && !next->isTableCol(); next = next->nextSibling()) {
+        // We allow captions mixed with columns and column-groups.
+        if (next->isTableCaption())
+            continue;
+
+        return 0;
+    }
+
+    return toRenderTableCol(next);
 }
+
+}

trunk/Source/WebCore/rendering/RenderTableCol.h

@@ -44 +44 @@
     unsigned span() const { return m_span; }
     void setSpan(unsigned span) { m_span = span; }
-    bool isTableColGroup() { return firstChild() ? true : false; }
+
+    bool isTableColumnGroupWithColumnChildren() { return firstChild(); }
+
+    // Returns the next column or column-group.
+    RenderTableCol* nextColumn() const;
+
 private:
     virtual RenderObjectChildList* virtualChildren() { return children(); }