Extract LayoutTableSection::CellStruct to a standalone class TableGridCell

Extract LayoutTableSection::CellStruct to a standalone class TableGridCell

This is a refactory before implmentation of grid based collapsed
borders.

BUG=716507, 2902
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2884533002
Cr-Commit-Position: refs/heads/master@{#471936}
Committed: https://chromium.googlesource.com/chromium/src/+/f8c4033754c54add7fbc1ce521116d88cda5c410

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

Description was changed from

==========
Make LayoutTableSection::CellStruct standalone class TableGridCell

This is a refactory before implmentation of grid based collapsed
borders.

BUG=716507
==========

to

==========
Make LayoutTableSection::CellStruct standalone class TableGridCell

This is a refactory before implmentation of grid based collapsed
borders.

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

[Xianzhu, 2017-05-14 17:45:45]

Description was changed from

==========
Make LayoutTableSection::CellStruct standalone class TableGridCell

This is a refactory before implmentation of grid based collapsed
borders.

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

to

==========
Extract LayoutTableSection::CellStruct to a standalone class TableGridCell

This is a refactory before implmentation of grid based collapsed
borders.

BUG=716507,2902
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-05-14 19:03:15]

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

[commit-bot: I haz the power, 2017-05-14 19:03:22]

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-14 19:07:31]

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

[commit-bot: I haz the power, 2017-05-14 19:07:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Xianzhu, 2017-05-15 16:18:56]

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

[commit-bot: I haz the power, 2017-05-15 16:19:35]

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-15 16:33:21]

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

[commit-bot: I haz the power, 2017-05-15 16:33:22]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-05-15 18:07:46]

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

[commit-bot: I haz the power, 2017-05-15 18:08:35]

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-15 18:35:53]

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

[eae, 2017-05-15 18:38:41]

LGTM but please allow atotic or dgrogan to comment before landing.

[dgrogan, 2017-05-15 19:53:38]

lgtm

Some pure bikeshedding awaits below.

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left):

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:556: if
(!row_span_cell.cells.size())
Thanks for changing these continue to DCHECK, I wouldn't have known they weren't
possible.

Actually, how sure are you that they are not possible? Obviously they don't
trigger in our tests because the try bots are green, but we may be missing test
coverage for some weird case. Worth it to change them to CHECK so that we get a
clear signal if pages in the wild trigger them?

If you've carefully traced the code and are convinced that it can't happen then
ignore the CHECK suggestion; I'm just channeling atotic@, who likes to say that
some of this code is "unknowable".

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

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:281: DCHECK(cell);
Optional cleanup while you're here? I think this DCHECK can/should be moved to
line 236? If I'm wrong would you mind pointing out what I missed?

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:394: struct GridRow {
TableGridRow for consistency with the new class?

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:402:
Vector<TableGridCell> grid_cells;
This is so much clearer than the old "typedef Vector<blah> row".

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:403: LayoutTableRow*
row = nullptr;
I know it's longer, but wdyt about s/row/layout_table_row/ or reverting back to
row_layout_object? This CL does a great job disambiguating layout objects from
grid objects except here, where it's not clear from the member name which it is.

Eh, after reading the whole CL, it's not that hard to remember that row means
LayoutTableRow. Up to you.

[Xianzhu, 2017-05-15 20:28:26]

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

[Xianzhu, 2017-05-15 20:28:38]

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (left):

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:556: if
(!row_span_cell.cells.size())
On 2017/05/15 19:53:38, dgrogan wrote:
> Thanks for changing these continue to DCHECK, I wouldn't have known they
weren't
> possible.
> 
> Actually, how sure are you that they are not possible? Obviously they don't
> trigger in our tests because the try bots are green, but we may be missing
test
> coverage for some weird case. Worth it to change them to CHECK so that we get
a
> clear signal if pages in the wild trigger them?
> 
> If you've carefully traced the code and are convinced that it can't happen
then
> ignore the CHECK suggestion; I'm just channeling atotic@, who likes to say
that
> some of this code is "unknowable".

The new DCHECKs are ensured by the DCHECK at the beginning of the function:
  DCHECK(RowHasOnlySpanningCells(row));

If any of the early continue conditions are true, RowHasOnlySpanningCells() is
false.

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

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:281: DCHECK(cell);
On 2017/05/15 19:53:38, dgrogan wrote:
> Optional cleanup while you're here? I think this DCHECK can/should be moved to
> line 236? If I'm wrong would you mind pointing out what I missed?

Done.

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:394: struct GridRow {
On 2017/05/15 19:53:38, dgrogan wrote:
> TableGridRow for consistency with the new class?

Done.

https://codereview.chromium.org/2884533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:403: LayoutTableRow*
row = nullptr;
On 2017/05/15 19:53:38, dgrogan wrote:
> I know it's longer, but wdyt about s/row/layout_table_row/ or reverting back
to
> row_layout_object? This CL does a great job disambiguating layout objects from
> grid objects except here, where it's not clear from the member name which it
is.
> 
> Eh, after reading the whole CL, it's not that hard to remember that row means
> LayoutTableRow. Up to you.

I prefer the shorter name :)

[commit-bot: I haz the power, 2017-05-15 20:29:04]

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-15 21:32:19]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-15 21:32:26]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-15 21:32:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgrogan@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2884533002/#ps100001
(title: "-")

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

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-15 22:56:00]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494883946501970,
"parent_rev": "6958e7f12dc01b1e7412d12c5e0b40ca94c3d663", "commit_rev":
"f8c4033754c54add7fbc1ce521116d88cda5c410"}

[commit-bot: I haz the power, 2017-05-15 22:56:13]

Description was changed from

==========
Extract LayoutTableSection::CellStruct to a standalone class TableGridCell

This is a refactory before implmentation of grid based collapsed
borders.

BUG=716507,2902
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Extract LayoutTableSection::CellStruct to a standalone class TableGridCell

This is a refactory before implmentation of grid based collapsed
borders.

BUG=716507,2902
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2884533002
Cr-Commit-Position: refs/heads/master@{#471936}
Committed:
https://chromium.googlesource.com/chromium/src/+/f8c4033754c54add7fbc1ce52111...
==========

[commit-bot: I haz the power, 2017-05-15 22:56:16]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f8c4033754c54add7fbc1ce52111...