Fix LayoutBox::topLeftLocation for table rows and table cells

Fix LayoutBox::topLeftLocation for table rows and table cells

Previously we used containingBlock() as the default flipped blocks
container in LayoutBox::topLeftLocation(). This was incorrect for
table rows and table cells whose locations and topLeftLocations
should be relative to the containing section.

This fixes layout tests about table row/cells in vertical writing
mode for slimmingPaintInvalidation.

BUG=652392
Committed: https://crrev.com/9c820804326c7a2beac237fd826f73e237a05c46
Cr-Commit-Position: refs/heads/master@{#422917}

[Xianzhu, 2016-10-03 23:02:29]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 23:03:07]

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

[Xianzhu, 2016-10-03 23:38:37]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 23:39:27]

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

[Xianzhu, 2016-10-03 23:39:44]

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

[commit-bot: I haz the power, 2016-10-04 01:13:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 01:13:53]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-10-04 09:34:17]

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

[mstensho (USE GERRIT), 2016-10-04 09:34:17]

lgtm. One typo in the commit comment: "containinBlock()".

I wonder if LayoutBox::topLeftLocation() is the only place where we need to do
this. LayoutTableCell::offsetFromContainer(), for instance, has a "hack" that
subtracts the table row's location, since offsetFromContainer() is supposed to
be called when walking the ancestry using container().

But if we find out that others need this, we can always move the code you just
added to topLeftLocation() to a LayoutObject::locationContainer() or something. 

Or: fix the table code to store its cell locations relatively to the container()
(i.e the row). I don't know why it is like it is in there. Beacuse of rowspan,
perhaps?

Anyway, to repeat myself: CL lgtm :)

[Xianzhu, 2016-10-04 15:41:17]

Description was changed from

==========
Fix LayoutBox::topLeftLocation for table rows and table cells

Previously we used containinBlock() as the default flipped blocks
container in LayoutBox::topLeftLocation(). This was incorrect for
table rows and table cells whose locations and topLeftLocations
should be relative to the containing section.

This fixes layout tests about table row/cells in vertical writing
mode for slimmingPaintInvalidation.

BUG=652392
==========

to

==========
Fix LayoutBox::topLeftLocation for table rows and table cells

Previously we used containingBlock() as the default flipped blocks
container in LayoutBox::topLeftLocation(). This was incorrect for
table rows and table cells whose locations and topLeftLocations
should be relative to the containing section.

This fixes layout tests about table row/cells in vertical writing
mode for slimmingPaintInvalidation.

BUG=652392
==========

[eae, 2016-10-04 15:52:05]

LGTM

[Xianzhu, 2016-10-04 16:20:59]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[Xianzhu, 2016-10-04 16:21:00]

On 2016/10/04 09:34:17, mstensho wrote:
> lgtm. One typo in the commit comment: "containinBlock()".
> 
> I wonder if LayoutBox::topLeftLocation() is the only place where we need to do
> this. LayoutTableCell::offsetFromContainer(), for instance, has a "hack" that
> subtracts the table row's location, since offsetFromContainer() is supposed to
> be called when walking the ancestry using container().
> 
> But if we find out that others need this, we can always move the code you just
> added to topLeftLocation() to a LayoutObject::locationContainer() or
something. 
> 

I extracted LayoutBox::locationContainer() which can improve readability.

However, it seems not easy use the new function at other places where we have
the hack but don't want to skip the real container. We may have to keep the
hacks until we do the ultimate fix you suggested below.

> Or: fix the table code to store its cell locations relatively to the
container()
> (i.e the row). I don't know why it is like it is in there. Beacuse of rowspan,
> perhaps?
> 

I also don't know the reason but making cell location relative to row sounds
good.
Is this planned in LayoutNG?

> Anyway, to repeat myself: CL lgtm :)

[commit-bot: I haz the power, 2016-10-04 16:21:22]

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

[Xianzhu, 2016-10-04 17:27:10]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-10-04 17:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2392503003/#ps40001
(title: "locationContainer")

[commit-bot: I haz the power, 2016-10-04 17:27:26]

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

[Xianzhu, 2016-10-04 17:28:01]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-10-04 17:28:02]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2392503003/#ps60001
(title: "virtual")

[commit-bot: I haz the power, 2016-10-04 17:28:09]

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

[chrishtr, 2016-10-04 17:32:14]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2016-10-04 17:32:17]

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2288:
DCHECK(container->isTableRow() && parentBox() == container);
I changed the code to remove the DCHECK just last week due to the possibility of
elements having display: table-cell. You will need to change the code back.

https://blink.lc/chromium/commit/?id=c92b29db029d52d6423835f13741112867a3c00d

[Xianzhu, 2016-10-04 17:48:02]

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2288:
DCHECK(container->isTableRow() && parentBox() == container);
On 2016/10/04 17:32:17, chrishtr wrote:
> I changed the code to remove the DCHECK just last week due to the possibility
of
> elements having display: table-cell. You will need to change the code back.
> 
> https://blink.lc/chromium/commit/?id=c92b29db029d52d6423835f13741112867a3c00d

Based on my test, we create anonymous LayoutTable, LayoutTableSection and
LayoutTableRow for display: table-cell. Have you encountered exception of this?

[chrishtr, 2016-10-04 17:49:28]

Does paint/invalidation/display-table-row-crash.html crash?

[Xianzhu, 2016-10-04 18:01:32]

On 2016/10/04 17:49:28, chrishtr wrote:
> Does paint/invalidation/display-table-row-crash.html crash?

It doesn't crash in either normal mode or slimmingPaintInvalidation mode, with
this patch.

[chrishtr, 2016-10-04 18:17:30]

On 2016/10/04 at 18:01:32, wangxianzhu wrote:
> On 2016/10/04 17:49:28, chrishtr wrote:
> > Does paint/invalidation/display-table-row-crash.html crash?
> 
> It doesn't crash in either normal mode or slimmingPaintInvalidation mode, with
this patch.

Just tested locally with an example that uses display: table-cell. Layout
generates an anonymous LayoutTableRow that contains the cell. Interesting.

[chrishtr, 2016-10-04 18:17:34]

lgtm

[chrishtr, 2016-10-04 18:17:36]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2016-10-04 18:18:10]

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

[chrishtr, 2016-10-04 18:20:07]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2016-10-04 18:25:59]

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:5049: LayoutBox*
LayoutBox::locationContainer() const {
Please add a unittest that tests this method directly.

[Xianzhu, 2016-10-04 19:20:22]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-10-04 19:20:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com,
chrishtr@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2392503003/#ps80001
(title: "Add LayoutBoxTest.LocationContainer")

[Xianzhu, 2016-10-04 19:20:30]

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2392503003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:5049: LayoutBox*
LayoutBox::locationContainer() const {
On 2016/10/04 18:25:59, chrishtr wrote:
> Please add a unittest that tests this method directly.

Done.

[commit-bot: I haz the power, 2016-10-04 19:20:46]

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

[mstensho (USE GERRIT), 2016-10-04 20:20:25]

On 2016/10/04 16:21:00, Xianzhu wrote:
> On 2016/10/04 09:34:17, mstensho wrote:
> > Or: fix the table code to store its cell locations relatively to the
> container()
> > (i.e the row). I don't know why it is like it is in there. Beacuse of
rowspan,
> > perhaps?
> > 
> 
> I also don't know the reason but making cell location relative to row sounds
> good.
> Is this planned in LayoutNG?

I don't know how paint invalidation is going to work in LayoutNG, or how we're
going to walk the ancestry to calculate offsets, but I'm pretty sure we're not
going to port this weirdness over to LayoutNG. :)

[commit-bot: I haz the power, 2016-10-04 20:40:24]

Description was changed from

==========
Fix LayoutBox::topLeftLocation for table rows and table cells

Previously we used containingBlock() as the default flipped blocks
container in LayoutBox::topLeftLocation(). This was incorrect for
table rows and table cells whose locations and topLeftLocations
should be relative to the containing section.

This fixes layout tests about table row/cells in vertical writing
mode for slimmingPaintInvalidation.

BUG=652392
==========

to

==========
Fix LayoutBox::topLeftLocation for table rows and table cells

Previously we used containingBlock() as the default flipped blocks
container in LayoutBox::topLeftLocation(). This was incorrect for
table rows and table cells whose locations and topLeftLocations
should be relative to the containing section.

This fixes layout tests about table row/cells in vertical writing
mode for slimmingPaintInvalidation.

BUG=652392
==========

[commit-bot: I haz the power, 2016-10-04 20:40:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-04 20:43:57]

Description was changed from

==========
Fix LayoutBox::topLeftLocation for table rows and table cells

Previously we used containingBlock() as the default flipped blocks
container in LayoutBox::topLeftLocation(). This was incorrect for
table rows and table cells whose locations and topLeftLocations
should be relative to the containing section.

This fixes layout tests about table row/cells in vertical writing
mode for slimmingPaintInvalidation.

BUG=652392
==========

to

==========
Fix LayoutBox::topLeftLocation for table rows and table cells

Previously we used containingBlock() as the default flipped blocks
container in LayoutBox::topLeftLocation(). This was incorrect for
table rows and table cells whose locations and topLeftLocations
should be relative to the containing section.

This fixes layout tests about table row/cells in vertical writing
mode for slimmingPaintInvalidation.

BUG=652392

Committed: https://crrev.com/9c820804326c7a2beac237fd826f73e237a05c46
Cr-Commit-Position: refs/heads/master@{#422917}
==========

[commit-bot: I haz the power, 2016-10-04 20:43:58]

Patchset 5 (id:??) landed as
https://crrev.com/9c820804326c7a2beac237fd826f73e237a05c46
Cr-Commit-Position: refs/heads/master@{#422917}