[css-tables] Set table and cell widths dirty when row border width changes

[css-tables] Set table and cell widths dirty when row border width changes

Otherwise the old table width is reused.

BUG=609674
Committed: https://crrev.com/0a7429662dd258bb3af4dd81ea5c7b96d2af9583
Cr-Commit-Position: refs/heads/master@{#394329}

[dgrogan, 2016-05-07 00:25:53]

dgrogan@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[dgrogan, 2016-05-07 00:25:54]

Xianzhu, could you review this?

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

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:89:
childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis);
This works but I'm out of my element here; do you have any intuition if this
will have a significant performance impact?

[Xianzhu, 2016-05-07 20:58:19]

I feel the CL is correct, but am not very sure because my limited knowledge
about layout.

eae@ can you review this CL?

[Xianzhu, 2016-05-07 20:58:41]

wangxianzhu@chromium.org changed reviewers:
+ eae@chromium.org

[Xianzhu, 2016-05-07 20:58:41]

I feel the CL is correct, but am not very sure because my limited knowledge
about layout.

eae@ can you review this CL?

[mstensho (USE GERRIT), 2016-05-09 08:41:53]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-05-09 08:41:53]

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html
(right):

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html:17:
window.testRunner && window.testRunner.notifyDone();
I wonder if this could be simplified if you instead use runAfterLayoutAndPaint()
from ../../resources/run-after-layout-and-paint.js

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html:22:
<table style="border-collapse:collapse" data-expected-width=58>
Add data-expected-height too?

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

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:84:
table->setPreferredLogicalWidthsDirty(MarkOnlyThis);
What about the table section? Isn't it affected by this too?

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:89:
childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis);
On 2016/05/07 00:25:54, dgrogan wrote:
> This works but I'm out of my element here; do you have any intuition if this
> will have a significant performance impact?

If you're worried about performance, I guess a border change on a row can only
affect the preferred widths of the first and the last cell. But I don't really
think we need to worry.

[eae, 2016-05-09 15:36:42]

On 2016/05/09 08:41:53, mstensho wrote:
>
https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
> File third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html
> (right):
> 
>
https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html:17:
> window.testRunner && window.testRunner.notifyDone();
> I wonder if this could be simplified if you instead use
runAfterLayoutAndPaint()
> from ../../resources/run-after-layout-and-paint.js
> 
>
https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html:22:
> <table style="border-collapse:collapse" data-expected-width=58>
> Add data-expected-height too?
> 
>
https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):
> 
>
https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:84:
> table->setPreferredLogicalWidthsDirty(MarkOnlyThis);
> What about the table section? Isn't it affected by this too?
> 
>
https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:89:
> childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis);
> On 2016/05/07 00:25:54, dgrogan wrote:
> > This works but I'm out of my element here; do you have any intuition if this
> > will have a significant performance impact?
> 
> If you're worried about performance, I guess a border change on a row can only
> affect the preferred widths of the first and the last cell. But I don't really
> think we need to worry.

Agreed.

[dgrogan, 2016-05-16 23:27:04]

Patchset #2 (id:20001) has been deleted

[dgrogan, 2016-05-16 23:34:17]

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html
(right):

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html:17:
window.testRunner && window.testRunner.notifyDone();
On 2016/05/09 08:41:53, mstensho wrote:
> I wonder if this could be simplified if you instead use
runAfterLayoutAndPaint()
> from ../../resources/run-after-layout-and-paint.js

Beautiful. Done.

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/change-row-border-width.html:22:
<table style="border-collapse:collapse" data-expected-width=58>
On 2016/05/09 08:41:53, mstensho wrote:
> Add data-expected-height too?

Done.

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

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:84:
table->setPreferredLogicalWidthsDirty(MarkOnlyThis);
On 2016/05/09 08:41:53, mstensho wrote:
> What about the table section? Isn't it affected by this too?

We don't need to call section->setPreferredLogicalWidthsDirty() because
LayoutTableSection doesn't override computePreferredLogicalWidths, it just uses
the LayoutBox implementation:
virtual void computePreferredLogicalWidths() {
clearPreferredLogicalWidthsDirty(); }

https://codereview.chromium.org/1952133006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:89:
childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis);
On 2016/05/09 08:41:53, mstensho wrote:
> On 2016/05/07 00:25:54, dgrogan wrote:
> > This works but I'm out of my element here; do you have any intuition if this
> > will have a significant performance impact?
> 
> If you're worried about performance, I guess a border change on a row can only
> affect the preferred widths of the first and the last cell. But I don't really
> think we need to worry.

Ok, great.

[dgrogan, 2016-05-17 21:09:26]

eae, would you mind reviewing this? I suspect mstensho is OOO. It wouldn't be a
huge deal to wait for his return though, the only thing it's blocking is a patch
that he's been reviewing.

[eae, 2016-05-17 21:31:07]

LGTM

[eae, 2016-05-17 21:31:21]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-05-17 21:32:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1952133006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1952133006/60001

[commit-bot: I haz the power, 2016-05-18 03:59:23]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-18 04:01:03]

Description was changed from

==========
[css-tables] Set table and cell widths dirty when row border width changes

Otherwise the old table width is reused.

BUG=609674
==========

to

==========
[css-tables] Set table and cell widths dirty when row border width changes

Otherwise the old table width is reused.

BUG=609674

Committed: https://crrev.com/0a7429662dd258bb3af4dd81ea5c7b96d2af9583
Cr-Commit-Position: refs/heads/master@{#394329}
==========

[commit-bot: I haz the power, 2016-05-18 04:01:06]

Patchset 3 (id:??) landed as
https://crrev.com/0a7429662dd258bb3af4dd81ea5c7b96d2af9583
Cr-Commit-Position: refs/heads/master@{#394329}