Changeset 129136 in webkit

Timestamp:

Sep 20, 2012, 8:05:30 AM (13 years ago)

Author:

jchaffraix@webkit.org

Message:

Remove isStartColumn in the border collapsing code
​https://bugs.webkit.org/show_bug.cgi?id=97024

Reviewed by Abhishek Arya.

isStartColumn is embedding the same information as prevCell. As we need to compute it
in most of the cases, we may as well just reuse them.

While touching this code, I cleaned up the code by removing some unneeded checks and renaming
some variables in preparation for bug 79272.

Refactoring covered by existing collapsing borders tests.

• rendering/RenderTableCell.cpp:

(WebCore::RenderTableCell::computeCollapsedStartBorder):
Removed |isStartColumn|.

(WebCore::RenderTableCell::computeCollapsedEndBorder):
Added a comment about why |isEndColumn| is needed. Also cleaned up this code to be
consistent with computeCollapsedStartBorder.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RenderTableCell.cpp (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-09-20  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Remove isStartColumn in the border collapsing code
+        https://bugs.webkit.org/show_bug.cgi?id=97024
+
+        Reviewed by Abhishek Arya.
+
+        isStartColumn is embedding the same information as prevCell. As we need to compute it
+        in most of the cases, we may as well just reuse them.
+
+        While touching this code, I cleaned up the code by removing some unneeded checks and renaming
+        some variables in preparation for bug 79272.
+
+        Refactoring covered by existing collapsing borders tests.
+
+        * rendering/RenderTableCell.cpp:
+        (WebCore::RenderTableCell::computeCollapsedStartBorder):
+        Removed |isStartColumn|.
+
+        (WebCore::RenderTableCell::computeCollapsedEndBorder):
+        Added a comment about why |isEndColumn| is needed. Also cleaned up this code to be
+        consistent with computeCollapsedStartBorder.
+
 2012-09-20  Andrey Adaikin  <aandrey@chromium.org>
 

trunk/Source/WebCore/rendering/RenderTableCell.cpp

@@ -413 +413 @@
 {
     RenderTable* table = this->table();
-    bool isStartColumn = col() == 0;
 
     // For the start border, we need to check, in order of precedence:
@@ -422 +421 @@
 
     // (2) The end border of the preceding cell.
-    if (RenderTableCell* prevCell = table->cellBefore(this)) {
-        CollapsedBorderValue prevCellBorder = CollapsedBorderValue(prevCell->borderAdjoiningCellAfter(this), includeColor ? prevCell->style()->visitedDependentColor(endColorProperty) : Color(), BCELL);
-        result = chooseBorder(prevCellBorder, result);
-        if (!result.exists())
-            return result;
-    } else if (isStartColumn) {
+    RenderTableCell* cellBefore = table->cellBefore(this);
+    if (cellBefore) {
+        CollapsedBorderValue cellBeforeAdjoiningBorder = CollapsedBorderValue(cellBefore->borderAdjoiningCellAfter(this), includeColor ? cellBefore->style()->visitedDependentColor(endColorProperty) : Color(), BCELL);
+        // |result| should be the 2nd argument as |cellBefore| should win in case of equality per CSS 2.1 (Border conflict resolution, point 4).
+        result = chooseBorder(cellBeforeAdjoiningBorder, result);
+        if (!result.exists())
+            return result;
+    } else {
         // (3) Our row's start border.
         result = chooseBorder(result, CollapsedBorderValue(row()->borderAdjoiningStartCell(this), includeColor ? parent()->style()->visitedDependentColor(startColorProperty) : Color(), BROW));
@@ -455 +456 @@
     
     // (6) The end border of the preceding column.
-    if (!isStartColumn) {
+    if (cellBefore) {
         colElt = table->colElement(col() -1, &startColEdge, &endColEdge);
         if (colElt && endColEdge) {
-            CollapsedBorderValue endBorder = CollapsedBorderValue(colElt->borderAdjoiningCellAfter(this), includeColor ? colElt->style()->visitedDependentColor(endColorProperty) : Color(), BCOL);
-            result = chooseBorder(endBorder, result);
+            CollapsedBorderValue columnBeforeAdjoiningBorder = CollapsedBorderValue(colElt->borderAdjoiningCellAfter(this), includeColor ? colElt->style()->visitedDependentColor(endColorProperty) : Color(), BCOL);
+            result = chooseBorder(columnBeforeAdjoiningBorder, result);
             if (!result.exists())
                 return result;
@@ -484 +485 @@
 {
     RenderTable* table = this->table();
+    // Note: We have to use the effective column information instead of whether we have a cell after as a table doesn't
+    // have to be regular (any row can have less cells than the total cell count).
     bool isEndColumn = table->colToEffCol(col() + colSpan() - 1) == table->numEffCols() - 1;
 
@@ -494 +497 @@
     // (2) The start border of the following cell.
     if (!isEndColumn) {
-        RenderTableCell* nextCell = table->cellAfter(this);
-        if (nextCell && nextCell->style()) {
-            CollapsedBorderValue startBorder = CollapsedBorderValue(nextCell->borderAdjoiningCellBefore(this), includeColor ? nextCell->style()->visitedDependentColor(startColorProperty) : Color(), BCELL);
-            result = chooseBorder(result, startBorder);
+        if (RenderTableCell* cellAfter = table->cellAfter(this)) {
+            CollapsedBorderValue cellAfterAdjoiningBorder = CollapsedBorderValue(cellAfter->borderAdjoiningCellBefore(this), includeColor ? cellAfter->style()->visitedDependentColor(startColorProperty) : Color(), BCELL);
+            result = chooseBorder(result, cellAfterAdjoiningBorder);
             if (!result.exists())
                 return result;
@@ -532 +534 @@
         colElt = table->colElement(col() + colSpan(), &startColEdge, &endColEdge);
         if (colElt && startColEdge) {
-            CollapsedBorderValue startBorder = CollapsedBorderValue(colElt->borderAdjoiningCellBefore(this), includeColor ? colElt->style()->visitedDependentColor(startColorProperty) : Color(), BCOL);
-            result = chooseBorder(result, startBorder);
+            CollapsedBorderValue columnAfterAdjoiningBorder = CollapsedBorderValue(colElt->borderAdjoiningCellBefore(this), includeColor ? colElt->style()->visitedDependentColor(startColorProperty) : Color(), BCOL);
+            result = chooseBorder(result, columnAfterAdjoiningBorder);
             if (!result.exists())
                 return result;