Use LocalStyleChange when changing TD presentation attributes.

Use LocalStyleChange when changing TD presentation attributes.

Border and cellpadding attributes on TABLE affects computed styles of TD
elements, but LocalStyleChange on the TD elements should be sufficient to
have changes applied.

The setNeedsStyleRecalc call on the TABLE element is removed since that is
handled via HTMLTableElement::isPresentationAttribute called from
Element::attributeChanged.

Unwind the recursion looking for TD elements using ElementTraversal.

BUG=341987
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167568

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

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
File Source/core/html/HTMLTableElement.cpp (right):

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
Source/core/html/HTMLTableElement.cpp:258: || e->hasTagName(trTag) ||
e->hasTagName(thTag);
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.

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
Source/core/html/HTMLTableElement.cpp:265: if (element->hasTagName(tdTag)) {
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.

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
Source/core/html/HTMLTableElement.cpp:406: setCellsNeedStyleRecalc();
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]

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
File Source/core/html/HTMLTableElement.cpp (right):

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
Source/core/html/HTMLTableElement.cpp:258: || e->hasTagName(trTag) ||
e->hasTagName(thTag);
On 2014/02/10 10:02:06, Morten Stenshorne wrote:
> 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.

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

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
Source/core/html/HTMLTableElement.cpp:265: if (element->hasTagName(tdTag)) {
On 2014/02/10 10:02:06, Morten Stenshorne wrote:
> 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.

Done.

https://codereview.chromium.org/157383004/diff/1/Source/core/html/HTMLTableEl...
Source/core/html/HTMLTableElement.cpp:406: setCellsNeedStyleRecalc();
On 2014/02/10 10:02:06, Morten Stenshorne wrote:
> 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.

Done.

[mstensho (USE GERRIT), 2014-02-18 13:48:46]

lgtm (not an owner).

Feel free to ignore the issue I raised. This patch generally improves things,
and failing to skip some corner-case subtrees is certainly not a regression. :)

https://codereview.chromium.org/157383004/diff/60001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableElement.cpp (right):

https://codereview.chromium.org/157383004/diff/60001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableElement.cpp:263: element =
ElementTraversal::next(*element, this);
I wonder what happens here if a TR has a DIV table-cell child (inserted by a
script after parsing, for instance). You'd descend into the DIV subtree for no
reason. Also an issue for CAPTION elements.

Looking for which element types to descend into rather than looking for those to
skip could be a better option.

[ojan, 2014-02-21 03:28:55]

lgtm

[commit-bot: I haz the power, 2014-02-21 03:29:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/157383004/60001

[commit-bot: I haz the power, 2014-02-21 05:32:12]

Change committed as 167568