Don't add artifical 1 pixel width to empty tables

Source/core/layout/TableLayoutAlgorithmAuto.cpp

Index: Source/core/layout/TableLayoutAlgorithmAuto.cpp
diff --git a/Source/core/layout/TableLayoutAlgorithmAuto.cpp b/Source/core/layout/TableLayoutAlgorithmAuto.cpp
index b0a11f06aa0570e29c76d901469ac9a9c1098d14..70e795d9d673ceb5fc2ada25ee45aa5f1ade5ba8 100644
--- a/Source/core/layout/TableLayoutAlgorithmAuto.cpp
+++ b/Source/core/layout/TableLayoutAlgorithmAuto.cpp
@@ -26,6 +26,7 @@
 #include "core/layout/LayoutTableCell.h"
 #include "core/layout/LayoutTableCol.h"
 #include "core/layout/LayoutTableSection.h"
+#include "core/layout/LayoutText.h"
 #include "core/layout/TextAutosizer.h"
 
 namespace blink {
@@ -41,6 +42,18 @@ TableLayoutAlgorithmAuto::~TableLayoutAlgorithmAuto()
 {
 }
 
+static bool isEmptyCell(LayoutObject* object)

[mstensho (USE GERRIT), 2015/05/18 08:45:38]

I don't get this.

<p>Should be 50% lime and 50% cyan.</p>
<table cellspacing="0" cellpadding="0" style="width:100%; height:50px;">
    <tr>
        <td style="background:lime;"></td>
        <td style="background:cyan;"><div></div></td>
    </tr>
</table>

With your changes, the two columns end up in different "spread categories", so
that all extra space is given to the second one that "has content". No other
browser does this. I think the "has content" distinction should go away
completely.

[rhogan, 2015/05/19 19:45:42]

The second one should definitely be treated as empty. In fact, I realized while
working through your review that we should be allocating initial width to
columns that have only empty cells. Doing this allows us to match FF on
empty-cells.html and is just saner in general. I think the reason it wasn't done
before is that columns with empty cells were sometimes shorthand for columns
that had no cells of their own and just fleshed out a colspan so it wasn't safe,
without recognizing that's what they were, to go ahead
and allocate width.

+{
+    for (LayoutObject* curr = object; curr; curr = curr->nextSibling()) {
+        if (curr->isText() && toLayoutText(curr)->isAllCollapsibleWhitespace())
+            continue;
+        if (curr->slowFirstChild() && isEmptyCell(curr->slowFirstChild()))
+            continue;
+        return false;
+    }
+    return true;
+}
+
 void TableLayoutAlgorithmAuto::recalcColumn(unsigned effCol)
 {
     Layout& columnLayout = m_layoutStruct[effCol];
@@ -64,14 +77,13 @@ void TableLayoutAlgorithmAuto::recalcColumn(unsigned effCol)
                 if (current.inColSpan || !cell)
                     continue;
 
-                bool cellHasContent = cell->children()->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding() || cell->style()->hasBackground();
+                bool cellHasContent = cell->style()->hasBorder() || cell->style()->hasPadding() || !isEmptyCell(cell->firstChild());

[mstensho (USE GERRIT), 2015/05/18 08:45:38]

This certainly looks like a step in the right direction, but, as I wrote above,
I think the "has content" thing should go away (maybe except for using it to set
columnLayout.emptyCellsOnly; not sure how that one works). Why even bother to
check for border of padding. If such things are present, min logical width
should already be larger than 0, so there should be no need to force it to 1.

[rhogan, 2015/05/19 19:45:42]

Good point - and in your example above (which I've added as a test) both should
be treated as empty cells. So I've adapted the check to look for minimum
preferred width and failing that, a div element with non-empty children.

                 if (cellHasContent)
                     columnLayout.emptyCellsOnly = false;
 
                 // A cell originates in this column. Ensure we have
                 // a min/max width of at least 1px for this column now.
                 columnLayout.minLogicalWidth = std::max<int>(columnLayout.minLogicalWidth, cellHasContent ? 1 : 0);
-                columnLayout.maxLogicalWidth = std::max<int>(columnLayout.maxLogicalWidth, 1);
 
                 if (cell->colSpan() == 1) {
                     columnLayout.minLogicalWidth = std::max<int>(cell->minPreferredLogicalWidth(), columnLayout.minLogicalWidth);
@@ -373,13 +385,13 @@ int TableLayoutAlgorithmAuto::calcEffectiveLogicalWidth()
                 int totalWidth = 0;
                 for (unsigned pos = effCol; pos < lastCol; ++pos) {
                     if (!m_layoutStruct[pos].effectiveLogicalWidth.hasPercent())
-                        totalWidth += m_layoutStruct[pos].effectiveMaxLogicalWidth;
+                        totalWidth += m_layoutStruct[pos].clampedEffectiveMaxLogicalWidth();
                 }
 
                 for (unsigned pos = effCol; pos < lastCol && totalWidth > 0; ++pos) {
                     if (!m_layoutStruct[pos].effectiveLogicalWidth.hasPercent()) {
                         float percent = percentMissing * static_cast<float>(m_layoutStruct[pos].effectiveMaxLogicalWidth) / totalWidth;
-                        totalWidth -= m_layoutStruct[pos].effectiveMaxLogicalWidth;
+                        totalWidth -= m_layoutStruct[pos].clampedEffectiveMaxLogicalWidth();
                         percentMissing -= percent;
                         if (percent > 0)
                             m_layoutStruct[pos].effectiveLogicalWidth.setValue(Percent, percent);
@@ -538,7 +550,7 @@ void TableLayoutAlgorithmAuto::layout()
             break;
         case Fixed:
             numFixed++;
-            totalFixed += m_layoutStruct[i].effectiveMaxLogicalWidth;
+            totalFixed += m_layoutStruct[i].clampedEffectiveMaxLogicalWidth();
             // fall through
             break;
         case Auto:
@@ -546,7 +558,7 @@ void TableLayoutAlgorithmAuto::layout()
                 numAutoEmptyCellsOnly++;
             } else {
                 numAuto++;
-                totalAuto += m_layoutStruct[i].effectiveMaxLogicalWidth;
+                totalAuto += m_layoutStruct[i].clampedEffectiveMaxLogicalWidth();
                 allocAuto += cellLogicalWidth;
             }
             break;
@@ -613,6 +625,12 @@ void TableLayoutAlgorithmAuto::layout()
         distributeWidthToColumns<unsigned, Auto, NonEmptyCells, LeftoverWidth, EndToStart>(available, total);
     }
 
+    // spread over the empty cells
+    if (available > 0) {

[mstensho (USE GERRIT), 2015/05/20 09:34:22]

Why not remove this code block entirely? You now spread over empty cells before
distributing excess space, so I don't see how we would ever get here.

+        unsigned total = numAutoEmptyCellsOnly;
+        distributeWidthToColumns<unsigned, Auto, EmptyCells, LeftoverWidth, EndToStart>(available, total);

[mstensho (USE GERRIT), 2015/05/18 08:45:39]

Is it possible to over-allocate here? If so, don't you need to add a
corresponding shrink step?

If it's not possible to over-allocate, you should ASSERT on that after having
distributed.

[rhogan, 2015/05/19 19:45:42]

No, the shrink step is keyed to Length type only - it's agnostic to empty cells.

+    }
+
     // If we have overallocated, reduce every cell according to the difference between desired width and minwidth
     // this seems to produce to the pixel exact results with IE. Wonder is some of this also holds for width distributing.
     // This is basically the reverse of how we grew the cells.
@@ -641,6 +659,8 @@ void TableLayoutAlgorithmAuto::distributeWidthToColumns(int& available, Total to
         const Length& logicalWidth = m_layoutStruct[i].effectiveLogicalWidth;
         if (cellsToProcess == NonEmptyCells && logicalWidth.isAuto() && m_layoutStruct[i].emptyCellsOnly)
             continue;
+        if (cellsToProcess == EmptyCells && logicalWidth.isAuto() && !m_layoutStruct[i].emptyCellsOnly)
+            continue;
         if (distributionMode != LeftoverWidth && logicalWidth.type() != lengthType)
             continue;
 
@@ -649,7 +669,7 @@ void TableLayoutAlgorithmAuto::distributeWidthToColumns(int& available, Total to
             if (lengthType == Percent)
                 factor = logicalWidth.percent();
             else if (lengthType == Auto || lengthType == Fixed)
-                factor = m_layoutStruct[i].effectiveMaxLogicalWidth;
+                factor = m_layoutStruct[i].clampedEffectiveMaxLogicalWidth();
         }
 
         int newWidth = available * factor / total;