Combine and simplify LayoutTableSection::DirtiedRows() and DirtiedEffectiveColumns()

Combine and simplify LayoutTableSection::DirtiedRows() and DirtiedEffectiveColumns()

As both need to get covered rows and covered columns, combining them can
reduce code lines and cost.

Also change:

- With collapsed borders, just expand the rows and columns by 1 cell in
  each side. This removes dependency to LayoutTable::OuterBorderXXX().
  (This is the initial purpose of this CL, for https://codereview.chromium.org/2890543002
   which will remove LayoutTable::OuterBorderXXX()).

- Besides primary cells, also expand rows and columns to cover all
  overlapping cells. This avoids the extra logic in TableSectionPainter
  handling overlapping cells.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2884763003
Cr-Commit-Position: refs/heads/master@{#472660}
Committed: https://chromium.googlesource.com/chromium/src/+/220cde510f9c973d9c2b546a8d8b3c9196cfe7ef

[Xianzhu, 2017-05-16 03:00:13]

Description was changed from

==========
Combine and simplify LayoutTableSection::DirtiedRows() and
DirtiedEffectiveColumns()

As both need to get covered rows and covered columns, combining them can
reduce code lines and cost.

Also change:
- With collapsed borders, just expand the rows and columns by 1 cell in
  each side. This removes dependency to LayoutTable::OuterBorderXXX().
- Besides primary cells, also expand rows and columns to cover all
  overlapping cells. This avoids the extra logic in TableSectionPainter
  handling overlapping cells.
==========

to

==========
Combine and simplify LayoutTableSection::DirtiedRows() and
DirtiedEffectiveColumns()

As both need to get covered rows and covered columns, combining them can
reduce code lines and cost.

Also change:
- With collapsed borders, just expand the rows and columns by 1 cell in
  each side. This removes dependency to LayoutTable::OuterBorderXXX().
- Besides primary cells, also expand rows and columns to cover all
  overlapping cells. This avoids the extra logic in TableSectionPainter
  handling overlapping cells.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-05-16 17:15:51]

Description was changed from

==========
Combine and simplify LayoutTableSection::DirtiedRows() and
DirtiedEffectiveColumns()

As both need to get covered rows and covered columns, combining them can
reduce code lines and cost.

Also change:
- With collapsed borders, just expand the rows and columns by 1 cell in
  each side. This removes dependency to LayoutTable::OuterBorderXXX().
- Besides primary cells, also expand rows and columns to cover all
  overlapping cells. This avoids the extra logic in TableSectionPainter
  handling overlapping cells.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Combine and simplify LayoutTableSection::DirtiedRows() and
DirtiedEffectiveColumns()

As both need to get covered rows and covered columns, combining them can
reduce code lines and cost.

Also change:

- With collapsed borders, just expand the rows and columns by 1 cell in
  each side. This removes dependency to LayoutTable::OuterBorderXXX().
  (This is the initial purpose of this CL, for
https://codereview.chromium.org/2890543002
   which will remove LayoutTable::OuterBorderXXX()).

- Besides primary cells, also expand rows and columns to cover all
  overlapping cells. This avoids the extra logic in TableSectionPainter
  handling overlapping cells.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-05-16 17:18:13]

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

[Xianzhu, 2017-05-16 17:18:13]

https://codereview.chromium.org/2884763003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2884763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:265:
cells.push_back(cell);
This is simplified because dirtied_columns and dirtied_rows now cover
originating cells intersecting the dirty rect.

https://codereview.chromium.org/2884763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:271:
std::sort(cells.begin(), cells.end(), LayoutTableCell::CompareInDOMOrder);
Now only one sort method left because we enter this path only if there are
overflowing cells.

[Xianzhu, 2017-05-16 17:18:58]

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

[commit-bot: I haz the power, 2017-05-16 17:19:58]

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-16 18:43:00]

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

[commit-bot: I haz the power, 2017-05-16 18:43:01]

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_...)

[wkorman, 2017-05-16 20:30:11]

lgtm

https://codereview.chromium.org/2884763003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableRowPainter.cpp (right):

https://codereview.chromium.org/2884763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableRowPainter.cpp:38: dirtied_rows,
dirtied_columns);
If we passed pointers rather than refs, could pass nullptr for dirtied_rows and
then skip computation. Assume computation is pretty cheap but maybe for huge
tables could be useful. Just a thought while reading.

[Xianzhu, 2017-05-17 00:31:29]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-17 00:31:29]

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/2884763003/#ps40001
(title: "Rebase on origin/master")

[Xianzhu, 2017-05-17 00:33:22]

https://codereview.chromium.org/2884763003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableRowPainter.cpp (right):

https://codereview.chromium.org/2884763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableRowPainter.cpp:38: dirtied_rows,
dirtied_columns);
On 2017/05/16 20:30:11, wkorman wrote:
> If we passed pointers rather than refs, could pass nullptr for dirtied_rows
and
> then skip computation. Assume computation is pretty cheap but maybe for huge
> tables could be useful. Just a thought while reading.

This is the only case that only one output is needed, and the case is rare
(table row has self-painting layer). Even if the caller only wants one, the
function itself still needs to compute both in some cases.

[commit-bot: I haz the power, 2017-05-17 00:34:52]

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-17 02:25:19]

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

[commit-bot: I haz the power, 2017-05-17 02:25:20]

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

[Xianzhu, 2017-05-17 02:34:27]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-05-17 02:35:41]

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-17 04:06:10]

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

[commit-bot: I haz the power, 2017-05-17 04:06:11]

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

[Xianzhu, 2017-05-17 15:50:42]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-05-17 15:51:09]

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-17 17:34:50]

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

[commit-bot: I haz the power, 2017-05-17 17:34:51]

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

[Xianzhu, 2017-05-17 18:04:19]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-05-17 18:06:13]

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-17 20:35:11]

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

[commit-bot: I haz the power, 2017-05-17 20:35:13]

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

[Xianzhu, 2017-05-18 02:12:31]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-05-18 02:13:45]

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-18 04:26:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495073551493770,
"parent_rev": "fefc7cd8a1677036dca0c7ed55fc1c72f683c16d", "commit_rev":
"220cde510f9c973d9c2b546a8d8b3c9196cfe7ef"}

[commit-bot: I haz the power, 2017-05-18 04:26:55]

Description was changed from

==========
Combine and simplify LayoutTableSection::DirtiedRows() and
DirtiedEffectiveColumns()

As both need to get covered rows and covered columns, combining them can
reduce code lines and cost.

Also change:

- With collapsed borders, just expand the rows and columns by 1 cell in
  each side. This removes dependency to LayoutTable::OuterBorderXXX().
  (This is the initial purpose of this CL, for
https://codereview.chromium.org/2890543002
   which will remove LayoutTable::OuterBorderXXX()).

- Besides primary cells, also expand rows and columns to cover all
  overlapping cells. This avoids the extra logic in TableSectionPainter
  handling overlapping cells.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Combine and simplify LayoutTableSection::DirtiedRows() and
DirtiedEffectiveColumns()

As both need to get covered rows and covered columns, combining them can
reduce code lines and cost.

Also change:

- With collapsed borders, just expand the rows and columns by 1 cell in
  each side. This removes dependency to LayoutTable::OuterBorderXXX().
  (This is the initial purpose of this CL, for
https://codereview.chromium.org/2890543002
   which will remove LayoutTable::OuterBorderXXX()).

- Besides primary cells, also expand rows and columns to cover all
  overlapping cells. This avoids the extra logic in TableSectionPainter
  handling overlapping cells.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2884763003
Cr-Commit-Position: refs/heads/master@{#472660}
Committed:
https://chromium.googlesource.com/chromium/src/+/220cde510f9c973d9c2b546a8d8b...
==========

[commit-bot: I haz the power, 2017-05-18 04:26:57]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/220cde510f9c973d9c2b546a8d8b...

[dgrogan, 2017-08-10 02:30:02]

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

[dgrogan, 2017-08-10 02:30:03]

https://codereview.chromium.org/2884763003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2884763003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1531: columns =
CellSpan();
wkorman, could you take another look at this? Seems like a return is missing? I
blindly added it to try to fix https://crbug.com/740403 but then
LayoutTableSectionTest.EmptySectionDirtiedRowsAndEffeciveColumns fails, so I'm a
little confused.

If this (no return) is correct, what's the purpose of this clause?