Changeset 122282 in webkit

Timestamp:

Jul 10, 2012 5:41:48 PM (12 years ago)

Author:

commit-queue@webkit.org

Message:

Re-factoring recalcColumn in AutoTableLayout.cpp for readability
​https://bugs.webkit.org/show_bug.cgi?id=89636

Patch by Pravin D <​pravind.2k4@gmail.com> on 2012-07-10
Reviewed by Julien Chaffraix.

No test case required. Code re-factoring.

• rendering/AutoTableLayout.cpp:

Added a const integer place holder for 32760.

(WebCore::AutoTableLayout::recalcColumn):

Changes :
1) Moved the continue statement above the bool cellHasContent for an early return.
2) Replaced the constant 32760 by a placeholder.
3) Initialization of columnLayout max and min logical widths is made common for both cells having col span == 1 and span > 1.
4) Removed redundant check for cell logical width type.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/AutoTableLayout.cpp (modified) (5 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-07-10  Pravin D  <pravind.2k4@gmail.com>
+
+        Re-factoring recalcColumn in AutoTableLayout.cpp for readability
+        https://bugs.webkit.org/show_bug.cgi?id=89636
+
+        Reviewed by Julien Chaffraix.
+
+        No test case required. Code re-factoring.
+
+        * rendering/AutoTableLayout.cpp:
+        Added a const integer place holder for 32760.
+
+        (WebCore::AutoTableLayout::recalcColumn):
+         Changes :
+         1) Moved the continue statement above the bool cellHasContent for an early return.
+         2) Replaced the constant 32760 by a placeholder.
+         3) Initialization of columnLayout max and min logical widths is made common for both cells having col span == 1 and span > 1.
+         4) Removed redundant check for cell logical width type. 
+
 2012-07-10  Adam Barth  <abarth@webkit.org>
 

trunk/Source/WebCore/rendering/AutoTableLayout.cpp

@@ -60 +60 @@
                 RenderTableCell* cell = current.primaryCell();
                 
-                bool cellHasContent = cell && !current.inColSpan && (cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding());
+                if (current.inColSpan || !cell)
+                    continue;
+
+                bool cellHasContent = cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding();
                 if (cellHasContent)
                     columnLayout.emptyCellsOnly = false;
-                    
-                if (current.inColSpan || !cell)
-                    continue;
+
+                // A cell originates in this column. Ensure we have
+                // a min/max width of at least 1px for this column now.
+                columnLayout.minLogicalWidth = max<int>(columnLayout.minLogicalWidth, cellHasContent ? 1 : 0);
+                columnLayout.maxLogicalWidth = max<int>(columnLayout.maxLogicalWidth, 1);
 
                 if (cell->colSpan() == 1) {
-                    // A cell originates in this column.  Ensure we have
-                    // a min/max width of at least 1px for this column now.
-                    columnLayout.minLogicalWidth = max<int>(columnLayout.minLogicalWidth, cellHasContent ? 1 : 0);
-                    columnLayout.maxLogicalWidth = max<int>(columnLayout.maxLogicalWidth, 1);
                     if (cell->preferredLogicalWidthsDirty())
                         cell->computePreferredLogicalWidths();
@@ -80 +81 @@
                     }
 
+                    // All browsers implement a size limit on the cell's max width. 
+                    // Our limit is based on KHTML's representation that used 16 bits widths.
+                    // FIXME: Other browsers have a lower limit for the cell's max width. 
+                    const int cCellMaxWidth = 32760;
                     Length cellLogicalWidth = cell->styleOrColLogicalWidth();
-                    // FIXME: What is this arbitrary value?
-                    if (cellLogicalWidth.value() > 32760)
-                        cellLogicalWidth.setValue(32760);
+                    if (cellLogicalWidth.value() > cCellMaxWidth)
+                        cellLogicalWidth.setValue(cCellMaxWidth);
                     if (cellLogicalWidth.isNegative())
                         cellLogicalWidth.setValue(0);
@@ -89 +93 @@
                     case Fixed:
                         // ignore width=0
-                        if (cellLogicalWidth.value() > 0 && columnLayout.logicalWidth.type() != Percent) {
+                        if (cellLogicalWidth.isPositive() && !columnLayout.logicalWidth.isPercent()) {
                             int logicalWidth = cell->computeBorderBoxLogicalWidth(cellLogicalWidth.value());
                             if (columnLayout.logicalWidth.isFixed()) {
                                 // Nav/IE weirdness
-                                if ((logicalWidth > columnLayout.logicalWidth.value()) ||
-                                    ((columnLayout.logicalWidth.value() == logicalWidth) && (maxContributor == cell))) {
-                                    columnLayout.logicalWidth.setValue(logicalWidth);
+                                if ((logicalWidth > columnLayout.logicalWidth.value()) 
+                                    || ((columnLayout.logicalWidth.value() == logicalWidth) && (maxContributor == cell))) {
+                                    columnLayout.logicalWidth.setValue(Fixed, logicalWidth);
                                     fixedContributor = cell;
                                 }
@@ -112 +116 @@
                         // FIXME: Need to understand this case and whether it makes sense to compare values
                         // which are not necessarily of the same type.
-                        if (cellLogicalWidth.isAuto() || (cellLogicalWidth.isRelative() && cellLogicalWidth.value() > columnLayout.logicalWidth.value()))
+                        if (cellLogicalWidth.value() > columnLayout.logicalWidth.value())
                             columnLayout.logicalWidth = cellLogicalWidth;
                     default:
@@ -118 +122 @@
                     }
                 } else if (!effCol || section->primaryCellAt(i, effCol - 1) != cell) {
-                    // This spanning cell originates in this column.  Ensure we have
-                    // a min/max width of at least 1px for this column now.
-                    columnLayout.minLogicalWidth = max<int>(columnLayout.minLogicalWidth, cellHasContent ? 1 : 0);
-                    columnLayout.maxLogicalWidth = max<int>(columnLayout.maxLogicalWidth, 1);
+                    // This spanning cell originates in this column. Insert the cell into spanning cells list.
                     insertSpanCell(cell);
                 }