[css-tables] Clean up code that detects significant border changes

De-duplicate; narrow a condition; fix a method that did the opposite of
its name; add comments, etc.

No new tests because there _should_ be no behavior changes.

BUG=613728

Review-Url: https://codereview.chromium.org/2153283002
Cr-Commit-Position: refs/heads/master@{#406051}

third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp

@@ -4,6 +4,7 @@
 
 #include "core/layout/LayoutTableBoxComponent.h"
 
+#include "core/layout/LayoutTable.h"
 #include "core/style/ComputedStyle.h"
 
 namespace blink {
@@ -25,4 +26,13 @@ void LayoutTableBoxComponent::imageChanged(WrappedImagePtr, const IntRect*)
     m_backgroundChangedSinceLastPaintInvalidation = true;
 }
 
+bool LayoutTableBoxComponent::doCellsHaveDirtyWidth(const LayoutObject& tablePart, const LayoutTable& table, const StyleDifference& diff, const ComputedStyle& oldStyle)
+{
+    // ComputedStyle::diffNeedsFullLayoutAndPaintInvalidation sets needsFullLayout when border sizes
+    // change: checking diff.needsFullLayout() is an optimization, not required for correctness.
+    // TODO(dgrogan): Remove tablePart.needsLayout()? Perhaps it was an old optimization but now it
+    // seems that diff.needsFullLayout() implies tablePart.needsLayout().
+    return diff.needsFullLayout() && tablePart.needsLayout() && table.collapseBorders() && !oldStyle.border().sizeEquals(tablePart.style()->border());
+}
+
 } // namespace blink

third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.h

@@ -10,12 +10,14 @@
 
 namespace blink {
 
+class LayoutTable;
+
 // Common super class for LayoutTableCol, LayoutTableSection and LayoutTableRow.
 class CORE_EXPORT LayoutTableBoxComponent : public LayoutBox {
 public:
     bool backgroundChangedSinceLastPaintInvalidation() const { return m_backgroundChangedSinceLastPaintInvalidation; }
     void clearBackgroundChangedSinceLastPaintInvalidation() { m_backgroundChangedSinceLastPaintInvalidation = false; }
-
+    static bool doCellsHaveDirtyWidth(const LayoutObject& tablePart, const LayoutTable&, const StyleDifference&, const ComputedStyle& oldStyle);
 protected:
     explicit LayoutTableBoxComponent(Element* element)
         : LayoutBox(element)

third_party/WebKit/Source/core/layout/LayoutTableCol.cpp

@@ -45,29 +45,31 @@ LayoutTableCol::LayoutTableCol(Element* element)
 
 void LayoutTableCol::styleDidChange(StyleDifference diff, const ComputedStyle* oldStyle)
 {
+    DCHECK(style()->display() == TABLE_COLUMN || style()->display() == TABLE_COLUMN_GROUP);
+
     LayoutTableBoxComponent::styleDidChange(diff, oldStyle);
 
-    if (LayoutTable* table = this->table()) {
-        if (!oldStyle)
-            return;
-
-        // TODO(dgrogan): Is the "else" necessary for correctness or just a brittle optimization? The optimization would be:
-        // if the first branch is taken then the next one can't be, so don't even check its condition.
-        if (!table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle->border() != style()->border()) {
-            // If border was changed, notify table.
-            table->invalidateCollapsedBorders();
-        } else if ((oldStyle->logicalWidth() != style()->logicalWidth()) || (diff.needsFullLayout() && needsLayout() && table->collapseBorders() && oldStyle->border().sizeEquals(style()->border()))) {
-            // TODO(dgrogan): Move second clause above to LayoutTableBoxComponent for re-use.
-            // TODO(dgrogan): Optimization opportunities:
-            // (1) Only mark cells which are affected by this col, not every cell in the table.
-            // (2) If only the col width changes and its border width doesn't, do the cells need to be marked as
-            //     needing layout or just given dirty widths?
-            for (LayoutObject* child = table->children()->firstChild(); child; child = child->nextSibling()) {
-                if (!child->isTableSection())
-                    continue;
-                LayoutTableSection* section = toLayoutTableSection(child);
-                section->markAllCellsWidthsDirtyAndOrNeedsLayout(LayoutTableSection::MarkDirtyAndNeedsLayout);
-            }
+    if (!oldStyle)
+        return;
+
+    LayoutTable* table = this->table();
+    if (!table)
+        return;
+
+    // TODO(dgrogan): Is the "else" necessary for correctness or just a brittle optimization? The optimization would be:
+    // if the first branch is taken then the next one can't be, so don't even check its condition.
+    if (!table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle->border() != style()->border()) {
+        table->invalidateCollapsedBorders();
+    } else if ((oldStyle->logicalWidth() != style()->logicalWidth()) || LayoutTableBoxComponent::doCellsHaveDirtyWidth(*this, *table, diff, *oldStyle)) {
+        // TODO(dgrogan): Optimization opportunities:
+        // (1) Only mark cells which are affected by this col, not every cell in the table.
+        // (2) If only the col width changes and its border width doesn't, do the cells need to be marked as
+        //     needing layout or just given dirty widths?
+        for (LayoutObject* child = table->children()->firstChild(); child; child = child->nextSibling()) {
+            if (!child->isTableSection())
+                continue;
+            LayoutTableSection* section = toLayoutTableSection(child);
+            section->markAllCellsWidthsDirtyAndOrNeedsLayout(LayoutTableSection::MarkDirtyAndNeedsLayout);
         }
     }
 }

third_party/WebKit/Source/core/layout/LayoutTableRow.cpp

@@ -60,34 +60,39 @@ void LayoutTableRow::styleDidChange(StyleDifference diff, const ComputedStyle* o
     LayoutTableBoxComponent::styleDidChange(diff, oldStyle);
     propagateStyleToAnonymousChildren();
 
-    if (section() && oldStyle && style()->logicalHeight() != oldStyle->logicalHeight())
+    if (!oldStyle)
+        return;
+
+    if (section() && style()->logicalHeight() != oldStyle->logicalHeight())
         section()->rowLogicalHeightChanged(this);
 
-    // If border was changed, notify table.
-    if (parent()) {
-        LayoutTable* table = this->table();
-        if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle && oldStyle->border() != style()->border())
-            table->invalidateCollapsedBorders();
-
-        if (table && oldStyle && diff.needsFullLayout() && needsLayout() && table->collapseBorders() && oldStyle->border().sizeEquals(style()->border())) {
-            // If the border width changes on a row, we need to make sure the cells in the row know to lay out again.
-            // This only happens when borders are collapsed, since they end up affecting the border sides of the cell
-            // itself.
-            for (LayoutBox* childBox = firstChildBox(); childBox; childBox = childBox->nextSiblingBox()) {
-                if (!childBox->isTableCell())
-                    continue;
-                // TODO(dgrogan) Add a layout test showing that setChildNeedsLayout is needed instead of setNeedsLayout.
-                childBox->setChildNeedsLayout();
-                childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis);
-            }
-            // Most table componenents can rely on LayoutObject::styleDidChange
-            // to mark the container chain dirty. But LayoutTableSection seems
-            // to never clear its dirty bit, which stops the propagation. So
-            // anything under LayoutTableSection has to restart the propagation
-            // at the table.
-            // TODO(dgrogan): Make LayoutTableSection clear its dirty bit.
-            table->setPreferredLogicalWidthsDirty();
+    if (!parent())
+        return;
+    LayoutTable* table = this->table();
+    if (!table)
+        return;
+
+    if (!table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle->border() != style()->border())
+        table->invalidateCollapsedBorders();
+
+    if (LayoutTableBoxComponent::doCellsHaveDirtyWidth(*this, *table, diff, *oldStyle)) {
+        // If the border width changes on a row, we need to make sure the cells in the row know to lay out again.
+        // This only happens when borders are collapsed, since they end up affecting the border sides of the cell
+        // itself.
+        for (LayoutBox* childBox = firstChildBox(); childBox; childBox = childBox->nextSiblingBox()) {
+            if (!childBox->isTableCell())
+                continue;
+            // TODO(dgrogan) Add a layout test showing that setChildNeedsLayout is needed instead of setNeedsLayout.
+            childBox->setChildNeedsLayout();
+            childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis);
         }
+        // Most table componenents can rely on LayoutObject::styleDidChange
+        // to mark the container chain dirty. But LayoutTableSection seems
+        // to never clear its dirty bit, which stops the propagation. So
+        // anything under LayoutTableSection has to restart the propagation
+        // at the table.
+        // TODO(dgrogan): Make LayoutTableSection clear its dirty bit.
+        table->setPreferredLogicalWidthsDirty();
     }
 }
 

third_party/WebKit/Source/core/layout/LayoutTableSection.cpp

@@ -119,17 +119,23 @@ LayoutTableSection::~LayoutTableSection()
 
 void LayoutTableSection::styleDidChange(StyleDifference diff, const ComputedStyle* oldStyle)
 {
+    DCHECK(style()->display() == TABLE_FOOTER_GROUP || style()->display() == TABLE_ROW_GROUP || style()->display() == TABLE_HEADER_GROUP);
+
     LayoutTableBoxComponent::styleDidChange(diff, oldStyle);
     propagateStyleToAnonymousChildren();
 
-    // If border was changed, notify table.
+    if (!oldStyle)
+        return;
+
     LayoutTable* table = this->table();
-    if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle && oldStyle->border() != style()->border())
+    if (!table)
+        return;
+
+    if (!table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle->border() != style()->border())
         table->invalidateCollapsedBorders();
 
-    if (table && oldStyle && diff.needsFullLayout() && needsLayout() && table->collapseBorders() && oldStyle->border() != style()->border()) {
+    if (LayoutTableBoxComponent::doCellsHaveDirtyWidth(*this, *table, diff, *oldStyle))
         markAllCellsWidthsDirtyAndOrNeedsLayout(MarkDirtyAndNeedsLayout);
-    }
 }
 
 void LayoutTableSection::willBeRemovedFromTree()

third_party/WebKit/Source/core/style/BorderData.h

@@ -126,10 +126,10 @@ public:
 
     bool sizeEquals(const BorderData& o) const
     {
-        return borderLeftWidth() != o.borderLeftWidth()
-            || borderTopWidth() != o.borderTopWidth()
-            || borderRightWidth() != o.borderRightWidth()
-            || borderBottomWidth() != o.borderBottomWidth();
+        return borderLeftWidth() == o.borderLeftWidth()
+            && borderTopWidth() == o.borderTopWidth()
+            && borderRightWidth() == o.borderRightWidth()
+            && borderBottomWidth() == o.borderBottomWidth();
     }
 
     const BorderValue& left() const { return m_left; }