Add a runtime flag for distributing extra logical height to rowspans

Source/core/rendering/RenderTableSection.cpp

Index: Source/core/rendering/RenderTableSection.cpp
diff --git a/Source/core/rendering/RenderTableSection.cpp b/Source/core/rendering/RenderTableSection.cpp
index 3bcff2dbf6d5d9c45795c5cf47efbd7cf47e71d3..6f951d4cfb1f89d89c5765d578eef2d6b48b3442 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -24,6 +24,7 @@
  */
 
 #include "config.h"
+

[Julien - ping for review, 2013/07/02 17:39:28]

#include "core/rendering/RenderTableSection.h" should be here per our coding
style.

[a.suchit, 2013/07/02 18:33:35]

It is giving me presubmit failure.

[Julien - ping for review, 2013/07/02 19:08:58]

#include "config.h"
#include "core/rendering/RenderTableSection.h"

# include <limits>
...

doesn't (and shouldn't) give any error.

[a.suchit, 2013/07/03 06:33:40]

Done.

 #include <limits>
 #include "core/rendering/HitTestResult.h"
 #include "core/rendering/PaintInfo.h"
@@ -31,6 +32,7 @@
 #include "core/rendering/RenderTableCol.h"
 #include "core/rendering/RenderTableRow.h"
 #include "core/rendering/RenderTableSection.h"
+#include "RuntimeEnabledFeatures.h"

[Julien - ping for review, 2013/07/02 17:39:28]

We are not consistent in how we sort the #includes but it
RuntimeEnabledFeatures.h shouldn't be among core/ #includes. I would have put it
just before wtf/ #includes (to keep everything alphabetical).

[a.suchit, 2013/07/02 18:33:35]

It gives me presubmit failure.

[Julien - ping for review, 2013/07/02 19:08:58]

The way you went triggers a style violation for me. The way to go is #include
"RuntimeEnabledFeatures.h" just after <limits>.

[a.suchit, 2013/07/03 06:33:40]

Done.

 #include "core/rendering/RenderView.h"
 #include <wtf/HashSet.h>
 #include <wtf/MemoryInstrumentationHashMap.h>
@@ -360,18 +362,25 @@ int RenderTableSection::calcRowLogicalHeight()
                 if (current.inColSpan && cell->rowSpan() == 1)
                     continue;
 
-                if (cell->rowSpan() > 1) {
-                    // For row spanning cells, we only handle them for the first row they span. This ensures we take their baseline into account.
-                    if (cell->rowIndex() == r) {
-                        rowSpanCells.append(cell);
+                if (RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled()) {
+                    if (cell->rowSpan() > 1) {
+                        // For row spanning cells, we only handle them for the first row they span. This ensures we take their baseline into account.
+                        if (cell->rowIndex() == r) {
+                            rowSpanCells.append(cell);
 
-                        // Find out the baseline. The baseline is set on the first row in a rowSpan.
-                        updateBaselineForCell(cell, r, baselineDescent);
+                            // Find out the baseline. The baseline is set on the first row in a rowSpan.
+                            updateBaselineForCell(cell, r, baselineDescent);
+                        }
+                        continue;
                     }
-                    continue;
-                }
 
-                ASSERT(cell->rowSpan() == 1);
+                    ASSERT(cell->rowSpan() == 1);
+                } else {
+                    // FIXME: We are always adding the height of a rowspan to the last rows which doesn't match

[Julien - ping for review, 2013/07/02 17:39:28]

We probably want to remove this FIXME but have it mention the crbug and saying
that it's a work-in-progress.

[a.suchit, 2013/07/02 18:33:35]

Done.

+                    // other browsers. See webkit.org/b/52185 for example.
+                    if ((cell->rowIndex() + cell->rowSpan() - 1) != r)
+                        continue;
+                }
 
                 if (cell->hasOverrideHeight()) {
                     if (!statePusher.didPush()) {
@@ -385,10 +394,20 @@ int RenderTableSection::calcRowLogicalHeight()
                     cell->layoutIfNeeded();
                 }
 
-                m_rowPos[r + 1] = max(m_rowPos[r + 1], m_rowPos[r] + cell->logicalHeightForRowSizing());
+                if (RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled()) {
+                    m_rowPos[r + 1] = max(m_rowPos[r + 1], m_rowPos[r] + cell->logicalHeightForRowSizing());
+
+                    // Find out the baseline.
+                    updateBaselineForCell(cell, r, baselineDescent);
+                } else {
+                    // For row spanning cells, |r| is the last row in the span.
+                    unsigned cellStartRow = cell->rowIndex();
+
+                    m_rowPos[r + 1] = max(m_rowPos[r + 1], m_rowPos[cellStartRow] + cell->logicalHeightForRowSizing());
 
-                // Find out the baseline.
-                updateBaselineForCell(cell, r, baselineDescent);
+                    // Find out the baseline.
+                    updateBaselineForCell(cell, cellStartRow, baselineDescent);
+                }
             }
         }
 
@@ -397,8 +416,10 @@ int RenderTableSection::calcRowLogicalHeight()
         m_rowPos[r + 1] = max(m_rowPos[r + 1], m_rowPos[r]);
     }
 
-    if (!rowSpanCells.isEmpty())
-        distributeRowSpanHeightToRows(rowSpanCells);
+    if (RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled()) {
+        if (!rowSpanCells.isEmpty())

[Julien - ping for review, 2013/07/02 17:39:28]

rowSpanCells will be empty if the feature is disabled AFAICT so I would rather
have an ASSERT instead.

[a.suchit, 2013/07/02 18:33:35]

do you want me to use both
if(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled()) and
ASSERT(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled())?

There is no use of assert if 'if condition' is present.

[Julien - ping for review, 2013/07/02 19:08:58]

That's what I meant as you only collect rowSpanCells if
RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled():

if (!rowSpanCells.isEmpty()) {
    ASSERT(RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled());
    distributeRowSpanHeightToRows(rowSpanCells);
}

[a.suchit, 2013/07/03 06:33:40]

Done.

+            distributeRowSpanHeightToRows(rowSpanCells);
+    }
 
     ASSERT(!needsLayout());