Add a runtime flag for distributing extra logical height to rowspans

Source/core/rendering/RenderTableSection.cpp

 /*
  * Copyright (C) 1997 Martin Jones (mjones@kde.org)
  *           (C) 1997 Torben Weis (weis@kde.org)
  *           (C) 1998 Waldo Bastian (bastian@kde.org)
  *           (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
 
 #include "config.h"
+#include "core/rendering/RenderTableSection.h"
+
+// FIXME: Remove 'RuntimeEnabledFeatures.h' when the master bug #78724 is closed.

[Julien - ping for review, 2013/07/08 16:26:13]

Better still: when http://crbug.com/78724 is closed.
(this makes the bug number clickable if you are using a smart enough editor)

+#include "RuntimeEnabledFeatures.h"
 #include <limits>
 #include "core/rendering/HitTestResult.h"
 #include "core/rendering/PaintInfo.h"
 #include "core/rendering/RenderTableCell.h"
 #include "core/rendering/RenderTableCol.h"
 #include "core/rendering/RenderTableRow.h"
-#include "core/rendering/RenderTableSection.h"
 #include "core/rendering/RenderView.h"
 #include <wtf/HashSet.h>
 #include <wtf/MemoryInstrumentationHashMap.h>
 #include <wtf/MemoryInstrumentationHashSet.h>
 #include <wtf/MemoryInstrumentationVector.h>
 #include <wtf/Vector.h>
 
 using namespace std;
 
 namespace WebCore {
@@ skipping 309 matching lines @@
         Row& row = m_grid[r].row;
         unsigned totalCols = row.size();
 
         for (unsigned c = 0; c < totalCols; c++) {
             CellStruct& current = cellAt(r, c);
             for (unsigned i = 0; i < current.cells.size(); i++) {
                 cell = current.cells[i];
                 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);
+                } else {
+                    // Below issue, Fix is in progress under RuntimeEnabledFeature flag
+                    // 'RowSpanLogicalHeightSpreading'. crbug.com/78724
+                    // Issue : We are always adding the height of a rowspan to the last rows which doesn't match
+                    // other browsers. See crbug.com/78724 for example.

[Julien - ping for review, 2013/07/08 16:26:13]

Not sure I understand your comment. First you repeat crbug.com/78724 which seems
useless but mostly, you are (again) repeating what the code already says in a
more concise, clearer way.

If you want to put something, here would be my take:

// FIXME: We add all the logical row of a rowspan to the last rows
// until crbug.com/78724 is fixed and the runtime flag removed.
// This avoids propagating temporary regressions while we fix the bug.

+                    if ((cell->rowIndex() + cell->rowSpan() - 1) != r)
+                        continue;
                 }
 
-                ASSERT(cell->rowSpan() == 1);
-
                 if (cell->hasOverrideHeight()) {
                     if (!statePusher.didPush()) {
                         // Technically, we should also push state for the row, but since
                         // rows don't push a coordinate transform, that's not necessary.
                         statePusher.push(this, locationOffset());
                     }
                     cell->clearIntrinsicPadding();
                     cell->clearOverrideSize();
                     cell->setChildNeedsLayout(true, MarkOnlyThis);
                     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);
+                    // 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, cellStartRow, baselineDescent);
+                }
             }
         }
 
         // Add the border-spacing to our final position.
         m_rowPos[r + 1] += borderSpacingForRow(r);
         m_rowPos[r + 1] = max(m_rowPos[r + 1], m_rowPos[r]);
     }
 
-    if (!rowSpanCells.isEmpty())
+    if (!rowSpanCells.isEmpty()) {
+        ASSERT(RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled());
         distributeRowSpanHeightToRows(rowSpanCells);
+    }
 
     ASSERT(!needsLayout());
 
     statePusher.pop();
 
     return m_rowPos[m_grid.size()];
 }
 
 void RenderTableSection::layout()
 {
@@ skipping 1109 matching lines @@
     info.addMember(logicalHeight, "logicalHeight");
 }
 
 void RenderTableSection::CellStruct::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
 {
     MemoryClassInfo info(memoryObjectInfo, this, PlatformMemoryTypes::Rendering);
     info.addMember(cells, "cells");
 }
 
 } // namespace WebCore