[css-tables] Mark sibling cells dirty when a cell is added

[css-tables] Mark sibling cells dirty when a cell is added

Cells' border boxes were not getting resized when their collapsed
border changed when a sibling with a dominant border was added.

BUG=613728
Committed: https://crrev.com/4d064b54d8a747c1400f88db62a0ac5df90b1cb4
Cr-Commit-Position: refs/heads/master@{#415796}

[dgrogan, 2016-08-27 03:38:58]

dgrogan@chromium.org changed reviewers:
+ mstensho@opera.com

[dgrogan, 2016-08-27 03:38:58]

Looks like I need to re-rebaseline the paint test, but could you take a look
assuming that's trivial?

Thanks!

[dgrogan, 2016-08-27 03:39:42]

Description was changed from

==========
[css-tables] Mark sibling cells dirty when a cell is added

Cells's border boxes were not getting resized when their collapsed
border changed because a sibling with a dominant border was added.

BUG=613728
==========

to

==========
[css-tables] Mark sibling cells dirty when a cell is added

Cells's border boxes were not getting resized when their collapsed
border changed when a sibling with a dominant border was added.

BUG=613728
==========

[mstensho (USE GERRIT), 2016-08-29 09:07:17]

lgtm with nits. There's also a typo in the commit comment. "Cells's" -> "Cells'"

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/table/add-cell-with-large-border.html
(right):

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/add-cell-with-large-border.html:14:
var cellToAdd = document.createElement("td");
I'm pretty sure making DOM tree structure changes isn't necessary here. You
could just put this TD in the markup with display:none, and then
display:table-cell it here. But I'm fine either way. Just a comment.

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:161: if
(cell->previousCell()) {
Might as well write "if (LayoutTableCell* previousCell = cell->previousCell())"
here, to avoid repeated calls to cell->previousCell().

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:162:
cell->previousCell()->setNeedsLayout(LayoutInvalidationReason::TableChanged);
Can use setNeedsLayoutAndPrefWidthsRecalc() instead of these two calls.

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:165: if
(cell->nextCell()) {
Ditto 1.

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:166:
cell->nextCell()->setNeedsLayout(LayoutInvalidationReason::TableChanged);
Ditto 2.

[dgrogan, 2016-08-31 20:53:15]

Description was changed from

==========
[css-tables] Mark sibling cells dirty when a cell is added

Cells's border boxes were not getting resized when their collapsed
border changed when a sibling with a dominant border was added.

BUG=613728
==========

to

==========
[css-tables] Mark sibling cells dirty when a cell is added

Cells' border boxes were not getting resized when their collapsed
border changed when a sibling with a dominant border was added.

BUG=613728
==========

[dgrogan, 2016-08-31 21:42:19]

Thanks for the review!

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/table/add-cell-with-large-border.html
(right):

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/add-cell-with-large-border.html:14:
var cellToAdd = document.createElement("td");
On 2016/08/29 09:07:17, mstensho wrote:
> I'm pretty sure making DOM tree structure changes isn't necessary here. You
> could just put this TD in the markup with display:none, and then
> display:table-cell it here. But I'm fine either way. Just a comment.

Ah, thanks for the tip.

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:161: if
(cell->previousCell()) {
On 2016/08/29 09:07:17, mstensho wrote:
> Might as well write "if (LayoutTableCell* previousCell =
cell->previousCell())"
> here, to avoid repeated calls to cell->previousCell().

Done.

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:162:
cell->previousCell()->setNeedsLayout(LayoutInvalidationReason::TableChanged);
On 2016/08/29 09:07:17, mstensho wrote:
> Can use setNeedsLayoutAndPrefWidthsRecalc() instead of these two calls.

Done.

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:165: if
(cell->nextCell()) {
On 2016/08/29 09:07:17, mstensho wrote:
> Ditto 1.

Done.

https://codereview.chromium.org/2281343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:166:
cell->nextCell()->setNeedsLayout(LayoutInvalidationReason::TableChanged);
On 2016/08/29 09:07:17, mstensho wrote:
> Ditto 2.

Done.

[dgrogan, 2016-08-31 21:42:50]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2016-08-31 21:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2281343002/#ps20001
(title: "respond to comments; fix test")

[commit-bot: I haz the power, 2016-08-31 21:43:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 23:14:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-31 23:17:25]

Description was changed from

==========
[css-tables] Mark sibling cells dirty when a cell is added

Cells' border boxes were not getting resized when their collapsed
border changed when a sibling with a dominant border was added.

BUG=613728
==========

to

==========
[css-tables] Mark sibling cells dirty when a cell is added

Cells' border boxes were not getting resized when their collapsed
border changed when a sibling with a dominant border was added.

BUG=613728

Committed: https://crrev.com/4d064b54d8a747c1400f88db62a0ac5df90b1cb4
Cr-Commit-Position: refs/heads/master@{#415796}
==========

[commit-bot: I haz the power, 2016-08-31 23:17:26]

Patchset 2 (id:??) landed as
https://crrev.com/4d064b54d8a747c1400f88db62a0ac5df90b1cb4
Cr-Commit-Position: refs/heads/master@{#415796}