Use LocalStyleChange when changing TD presentation attributes.

Source/core/html/HTMLTableElement.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, 2010, 2011 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 16 matching lines @@
 
 #include "CSSPropertyNames.h"
 #include "CSSValueKeywords.h"
 #include "HTMLNames.h"
 #include "bindings/v8/ExceptionState.h"
 #include "bindings/v8/ExceptionStatePlaceholder.h"
 #include "core/css/CSSImageValue.h"
 #include "core/css/CSSValuePool.h"
 #include "core/css/StylePropertySet.h"
 #include "core/dom/Attribute.h"
+#include "core/dom/ElementTraversal.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/html/HTMLTableCaptionElement.h"
 #include "core/html/HTMLTableRowElement.h"
 #include "core/html/HTMLTableRowsCollection.h"
 #include "core/html/HTMLTableSectionElement.h"
 #include "core/html/parser/HTMLParserIdioms.h"
 #include "core/rendering/RenderTable.h"
 #include "wtf/StdLibExtras.h"
 
 namespace WebCore {
@@ skipping 197 matching lines @@
                 break;
         }
     }
     if (!row) {
         exceptionState.throwDOMException(IndexSizeError, "The index provided (" + String::number(index) + ") is greater than the number of rows in the table (" + String::number(i) + ").");
         return;
     }
     row->remove(exceptionState);
 }
 
-static inline bool isTableCellAncestor(Node* n)
+static inline bool isTableCellAncestor(Element* e)
 {
-    return n->hasTagName(theadTag) || n->hasTagName(tbodyTag) ||
-           n->hasTagName(tfootTag) || n->hasTagName(trTag) ||
-           n->hasTagName(thTag);
+    return e->hasTagName(theadTag) || e->hasTagName(tbodyTag) || e->hasTagName(tfootTag)
+        || e->hasTagName(trTag) || e->hasTagName(thTag);

[mstensho (USE GERRIT), 2014/02/10 10:02:06]

This looks wrong (both before and after the change), regarding TH elements. A TH
element typically IS a table cell, not an ancestor. It's more important to do
this right now that no element will be marked for style recalc by accident
anymore.

[rune, 2014/02/18 10:53:06]

Yes, indeed. It was saved by enough SubtreeStyleChange before.

 }
 
-static bool setTableCellsChanged(Node* n)
+void HTMLTableElement::setCellsNeedStyleRecalc() const
 {
-    ASSERT(n);
-    bool cellChanged = false;
-
-    if (n->hasTagName(tdTag))
-        cellChanged = true;
-    else if (isTableCellAncestor(n)) {
-        for (Node* child = n->firstChild(); child; child = child->nextSibling())
-            cellChanged |= setTableCellsChanged(child);
+    Element* element = ElementTraversal::next(*this, this);
+    while (element) {
+        if (element->hasTagName(tdTag)) {

[mstensho (USE GERRIT), 2014/02/10 10:02:06]

Not that it was explicitly taken care of before your change, but, what about TH
elements? It's probably more important now that the table doesn't get its
subtree marked for style recalc as long as there is at least one TD element
somewhere.

[rune, 2014/02/18 10:53:06]

Done.

+            element->setNeedsStyleRecalc(LocalStyleChange);
+        } else if (isTableCellAncestor(element)) {
+            element = ElementTraversal::next(*element, this);
+            continue;
+        }
+        element = ElementTraversal::nextSkippingChildren(*element, this);
     }
-
-    if (cellChanged)
-        n->setNeedsStyleRecalc(SubtreeStyleChange);
-
-    return cellChanged;
 }
 
 static bool getBordersFromFrameAttributeValue(const AtomicString& value, bool& borderTop, bool& borderRight, bool& borderBottom, bool& borderLeft)
 {
     borderTop = false;
     borderRight = false;
     borderBottom = false;
     borderLeft = false;
 
     if (equalIgnoringCase(value, "above"))
@@ skipping 113 matching lines @@
             m_padding = max(0, value.toInt());
         else
             m_padding = 1;
     } else if (name == colsAttr) {
         // ###
     } else
         HTMLElement::parseAttribute(name, value);
 
     if (bordersBefore != cellBorders() || oldPadding != m_padding) {
         m_sharedCellStyle = 0;
-        bool cellChanged = false;
-        for (Node* child = firstChild(); child; child = child->nextSibling())
-            cellChanged |= setTableCellsChanged(child);
-        if (cellChanged)
-            setNeedsStyleRecalc(SubtreeStyleChange);
+        setCellsNeedStyleRecalc();

[mstensho (USE GERRIT), 2014/02/10 10:02:06]

It looks like table columns, column groups and row/header/footer groups also may
need their style recalculated, if table border changes, at least if border
collapsing is enabled. I suppose this could have worked by accident before, but
it won't anymore.

I wonder if you should just recalculate style on all TD, TH, TR, TBODY, THEAD,
TFOOT, COL and COLGROUP elements in the table. The current solution is to
recalculate style on all TD elements, anyway, which I guess typically comprise a
lot more than 50% of all elements in question.

[rune, 2014/02/18 10:53:06]

Done.

     }
 }
 
 static PassRefPtr<StylePropertySet> createBorderStyle(CSSValueID value)
 {
     RefPtr<MutableStylePropertySet> style = MutableStylePropertySet::create();
     style->setProperty(CSSPropertyBorderTopStyle, value);
     style->setProperty(CSSPropertyBorderBottomStyle, value);
     style->setProperty(CSSPropertyBorderLeftStyle, value);
     style->setProperty(CSSPropertyBorderRightStyle, value);
@@ skipping 142 matching lines @@
 {
     return getAttribute(rulesAttr);
 }
 
 const AtomicString& HTMLTableElement::summary() const
 {
     return getAttribute(summaryAttr);
 }
 
 }