Fix LayoutTableRow::AddOverflowFromCell()

Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106, 603993, 646015
R=wkorman@chromium.org

Review-Url: https://codereview.chromium.org/2861653006 .
Cr-Commit-Position: refs/heads/master@{#470166}
Committed: https://chromium.googlesource.com/chromium/src/+/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb

[Xianzhu, 2017-05-06 00:08:40]

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

[commit-bot: I haz the power, 2017-05-06 00:09:14]

Dry run: CQ is trying da patch.

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

[Xianzhu, 2017-05-06 00:12:47]

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

[Xianzhu, 2017-05-06 00:12:47]

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

https://codereview.chromium.org/2861653006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:309:
cell_visual_overflow_rect.MoveBy(-Location());
cell_visual_overflow_rect is in cell's coordinates. To convert to row
coordinates, we should move by the difference of locations of the row and the
cell.

This was changed in
https://codereview.chromium.org/2786463004/diff/120001/third_party/WebKit/Sou...,
but the old version was also incorrect:

 LayoutUnit cellOffsetLogicalTopDifference =
    cell->location().y() - location().y();
 cellVisualOverflowRect.move(LayoutUnit(), cellOffsetLogicalTopDifference);	
 addContentsVisualOverflow(cellVisualOverflowRect);

Actually the above code should adjust by difference of x instead of y. The row
and the cell should have the same y, but different x.

[commit-bot: I haz the power, 2017-05-06 01:16:14]

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

[commit-bot: I haz the power, 2017-05-06 01:16:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2017-05-06 04:31:39]

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

[commit-bot: I haz the power, 2017-05-06 04:32:19]

Dry run: 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, 2017-05-06 05:58:52]

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

[commit-bot: I haz the power, 2017-05-06 05:58:53]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-05-07 20:34:54]

Description was changed from

==========
Fix table row ContentsVisualOverflow

BUG=719106
==========

to

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes 3 problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.

BUG=719106,603993
==========

[Xianzhu, 2017-05-07 21:12:05]

Description was changed from

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes 3 problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.

BUG=719106,603993
==========

to

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106,603993
==========

[Xianzhu, 2017-05-07 21:12:11]

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

[commit-bot: I haz the power, 2017-05-07 21:12:15]

Dry run: 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, 2017-05-07 22:27:35]

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

[commit-bot: I haz the power, 2017-05-07 22:27:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2017-05-08 01:50:01]

Description was changed from

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106,603993
==========

to

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106,603993,646015
==========

[Xianzhu, 2017-05-08 01:50:35]

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

[commit-bot: I haz the power, 2017-05-08 01:50:54]

Dry run: 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, 2017-05-08 03:06:02]

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

[commit-bot: I haz the power, 2017-05-08 03:06:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2017-05-08 15:56:42]

Description was changed from

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106,603993,646015
==========

to

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106,603993,646015
==========

[Xianzhu, 2017-05-08 16:07:00]

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

[Xianzhu, 2017-05-08 16:07:01]

Now this patch fixes more issues of LayoutTableRow::AddOverflowFromCell().

https://codereview.chromium.org/2861653006/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2861653006/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:75: crbug.com/646015
virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table.html
[ NeedsRebaseline ]
Rebaseline-cl didn't work because of failures on android.

[Xianzhu, 2017-05-08 16:07:18]

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

[commit-bot: I haz the power, 2017-05-08 16:08:03]

Dry run: 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, 2017-05-08 16:26:22]

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

[commit-bot: I haz the power, 2017-05-08 16:26:23]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[wkorman, 2017-05-08 17:32:14]

lgtm

https://codereview.chromium.org/2861653006/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2861653006/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:75: crbug.com/646015
virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table.html
[ NeedsRebaseline ]
On 2017/05/08 16:07:00, Xianzhu wrote:
> Rebaseline-cl didn't work because of failures on android.

For future you may be able to pass --fill-missing when that happens.

[Xianzhu, 2017-05-08 17:45:53]

Patchset #6 (id:100001) has been deleted

[Xianzhu, 2017-05-08 17:56:48]

On 2017/05/08 17:32:14, wkorman wrote:
> lgtm
> 
>
https://codereview.chromium.org/2861653006/diff/100001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2861653006/diff/100001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:75: crbug.com/646015
>
virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table.html
> [ NeedsRebaseline ]
> On 2017/05/08 16:07:00, Xianzhu wrote:
> > Rebaseline-cl didn't work because of failures on android.
> 
> For future you may be able to pass --fill-missing when that happens.

Done.

[Xianzhu, 2017-05-08 17:57:27]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-08 17:57:27]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2861653006/#ps120001
(title: "Rebaseline")

[commit-bot: I haz the power, 2017-05-08 17:58:12]

CQ is trying da patch.

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

[Xianzhu, 2017-05-08 20:46:53]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-08 20:46:54]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2861653006/#ps140001
(title: "Deflake the test")

[commit-bot: I haz the power, 2017-05-08 20:47:46]

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, 2017-05-09 02:21:27]

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

[commit-bot: I haz the power, 2017-05-09 02:21:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2017-05-09 03:47:26]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-09 03:47:50]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-09 03:49:39]

Description was changed from

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106,603993,646015
==========

to

==========
Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG=719106,603993,646015
R=wkorman@chromium.org

Review-Url: https://codereview.chromium.org/2861653006 .
Cr-Commit-Position: refs/heads/master@{#470166}
Committed:
https://chromium.googlesource.com/chromium/src/+/0391059128167dc8dc8fdbcbc7d3...
==========

[Xianzhu, 2017-05-09 03:49:40]

Committed patchset #7 (id:140001) manually as
0391059128167dc8dc8fdbcbc7d3e3b81b7edefb (presubmit successful).