Optimize collapsed border calculation (step 2)

Optimize collapsed border calculation (step 2)

Invalidate cell collapsed borders for affected cells only when border
style changes.

BUG=626748, 663208

[Xianzhu, 2017-04-07 00:48:19]

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

[commit-bot: I haz the power, 2017-04-07 00:48:44]

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-04-07 01:09:34]

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

[commit-bot: I haz the power, 2017-04-07 01:09:35]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Xianzhu, 2017-04-24 17:58:14]

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

[commit-bot: I haz the power, 2017-04-24 17:58:59]

Dry run: CQ is trying da patch.

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

[Xianzhu, 2017-04-24 18:09:57]

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

[commit-bot: I haz the power, 2017-04-24 18:10:21]

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-04-24 20:15:17]

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

[commit-bot: I haz the power, 2017-04-24 20:15:17]

Dry run: 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-04-26 15:56:54]

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

[commit-bot: I haz the power, 2017-04-26 15:57: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-04-26 18:10:50]

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

[commit-bot: I haz the power, 2017-04-26 18:10:51]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-04-26 22:55:55]

Description was changed from

==========
Optimize collapsed border calculation
==========

to

==========
Optimize collapsed border calculation (step 2)

Invalidate cell collapsed borders for affected cells only when border
style changes.

BUG=626748, 663208
==========

[Xianzhu, 2017-04-26 22:57:12]

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

[commit-bot: I haz the power, 2017-04-26 22:58:21]

Dry run: CQ is trying da patch.

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

[Xianzhu, 2017-04-26 23:14:33]

Description was changed from

==========
Optimize collapsed border calculation (step 2)

Invalidate cell collapsed borders for affected cells only when border
style changes.

BUG=626748, 663208
==========

to

==========
Optimize collapsed border calculation (step 2)

Invalidate cell collapsed borders for affected cells only when border
style changes.

BUG=626748, 663208
==========

[commit-bot: I haz the power, 2017-04-27 01:00:53]

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

[commit-bot: I haz the power, 2017-04-27 01:00:54]

Dry run: 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-04-27 16:59:20]

wangxianzhu@chromium.org changed reviewers:
+ dgrogan@chromium.org, robhogan@gmail.com, wkorman@chromium.org

[dgrogan, 2017-04-27 20:43:25]

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:509: //
TODO(wangxianzhu,dgrogan): The following seems incorrect if the cell has
Could you expound on this a little? When can a cell have adjacent cells other
than PreviousCell and NextCell? Do you mean above and below? Or something with
spans? Or something else entirely?

[Xianzhu, 2017-04-27 21:18:31]

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:509: //
TODO(wangxianzhu,dgrogan): The following seems incorrect if the cell has
On 2017/04/27 20:43:25, dgrogan wrote:
> Could you expound on this a little? When can a cell have adjacent cells other
> than PreviousCell and NextCell? Do you mean above and below? Or something with
> spans? Or something else entirely?

Actually I'm not very sure if the TODO is valid. I will remove it if it's
invalid, otherwise will make it clearer.

For example:
<tr>
  <td rowspan="2">A</td><td>B</td>
</tr>
<tr>
  <td>C</td>
</tr>

It seems that A's border width may affect preferred width of both B and C.

(However, I'm not sure if C's border width should affect A. A's right border
actually has two segments which may differ in width. Currently LayoutTableCell
supports only one CollapsedBorderValue in each side so for layout it seems that
we just consider the first segment for now. @robhogan's
https://codereview.chromium.org/2791433003/ will support multiple segments
though.)

[wkorman, 2017-04-27 22:05:36]

There are large chunks of new/revised code here and no unit test changes. Useful
to add some for LayoutTable, LayoutTableCell, LayoutTableRow,
LayoutTableSection?

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (left):

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:1499: // Find the section
and row to look in
Is the simplified logic here and in CellBelow beneath essentially identical, and
we're just using the new helper methods so as to share logic?

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.h (right):

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.h:21: static bool
NeedsInvalidateCollapsedBordersOnStyleChange(
Document this method in particular what it returns.

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right):

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableRow.h:178: void
InvalidateCollapsedBordersOfAffectedCells();
Affected by what? Document method, consider rename to either clarify
"AffectedCells" or just remove "OfAffectedCells" since it seems implicit.

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2805103003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:465: void
InvalidateCollapsedBordersOfAffectedCells();
Same as previous comment re: "OfAffectedCells"

[Xianzhu, 2017-04-28 18:37:06]

Eventually we need to switch to calculating collapsed borders based on grids
instead of cells to correctly handle layout and painting of spanning cells. I
think we'd better switch to grids before this CL because many changes in this CL
would be changed again if we switch to grids later.

I withdraw review request of this CL.

[Xianzhu, 2017-04-28 18:39:57]

wangxianzhu@chromium.org changed reviewers:
- dgrogan@chromium.org, robhogan@gmail.com, wkorman@chromium.org