Fix Border collapsing with colpsan / rowspan cells

Move border-collapsing to the paint phase and collapse rowspan/colspans correctly.

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

[rhogan, 2017-03-30 18:00:39]

Description was changed from

==========
bug 2902

BUG=2902
==========

to

==========
bug 2902

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

[rhogan, 2017-04-04 19:58:58]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 19:59:39]

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-04 20:05:53]

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

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

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-04-04 20:35:06]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 20:36:57]

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

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

[commit-bot: I haz the power, 2017-04-04 20:56:18]

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...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[rhogan, 2017-04-05 19:49:16]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 19:49: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-04-05 19:56:41]

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

[commit-bot: I haz the power, 2017-04-05 19:56:43]

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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-04-13 19:34:13]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 19:34:52]

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-13 19:47:50]

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

[commit-bot: I haz the power, 2017-04-13 19:47:51]

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...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-04-16 10:45:32]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-16 10:45:39]

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-16 10:50:24]

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

[commit-bot: I haz the power, 2017-04-16 10:50:26]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sketch420stoner, 2017-04-16 11:06:38]

sketch420stoner@gmail.com changed reviewers:
+ sketch420stoner@gmail.com

[sketch420stoner, 2017-04-16 11:06:39]

lgtm

[rhogan, 2017-04-16 11:21:01]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-16 11:21:10]

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-16 11:40:01]

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

[commit-bot: I haz the power, 2017-04-16 11:40:02]

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

[Stephen Chennney, 2017-04-17 13:08:42]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org, wangxianzhu@chromium.org, wkorman@chromium.org
- sketch420stoner@gmail.com

[Stephen Chennney, 2017-04-17 13:08:43]

Could you fix the change description? I'm guessing this is WIP at this point?

Let me know if you're ready for review.

[Stephen Chennney, 2017-04-17 13:09:14]

Description was changed from

==========
bug 2902

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

to

==========
bug 2902

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

[rhogan, 2017-04-23 15:07:03]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-23 15:07:09]

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-23 15:33:27]

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

[commit-bot: I haz the power, 2017-04-23 15:33:29]

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

[rhogan, 2017-04-27 19:08:56]

Description was changed from

==========
bug 2902

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

to

==========
Move border-collapsing to the paint phase and collapse rowspan/colspans
correctly.

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

[rhogan, 2017-04-27 19:08:57]

robhogan@gmail.com changed reviewers:
+ atotic@chromium.org

[rhogan, 2017-04-27 19:11:10]

robhogan@gmail.com changed reviewers:
+ sketch420stoner@gmail.com

[rhogan, 2017-04-27 19:11:12]

Still a bit away from review.

@atotic - one of the things puzzling me is your attention to the direction
property. I can't see any reason why we would want to paint the borders in the
row's or table's direction - it looks like border-collapsing should always be
done in DOM order. What prompted you to do that?

[Xianzhu, 2017-04-27 19:59:19]

On 2017/04/27 19:11:12, rhogan wrote:
> Still a bit away from review.
> 
> @atotic - one of the things puzzling me is your attention to the direction
> property. I can't see any reason why we would want to paint the borders in the
> row's or table's direction - it looks like border-collapsing should always be
> done in DOM order. What prompted you to do that?

I'm not sure if @atotic still remembers that. Can you point me the related code
in @atotic's original CL?


About the CL:

Thanks for working on this! The new pixel results look great! 

I found you let the section paint the collapsed borders. There is a related task
bug crbug.com/663208. I tried painting all collapsed borders of a table in one
display item (https://codereview.chromium.org/2551963003), but reverted it
because of performance concerns. The collapsed border display item for a table
(or a section) might be huge and is invalidated on any change of border style or
layout in the table/section. During rasterization, replaying the huge display
item may be expensive. Also the expensive operation needs to be done on every
tile intersecting the display item. I considered painting collapsed borders by
rows. It'll be great if you will implement that in this CL.

My current work in progress on collapsed borders is mainly for paint
invalidation issues (e.g. crbug.com/626748) which are basically orthogonal to
crbug.com/2902, but as we are changing the same code, there might be conflicts
and depencencies.

[atotic, 2017-04-27 20:26:26]

On 2017/04/27 at 19:11:12, robhogan wrote:
> Still a bit away from review.
> 
> @atotic - one of the things puzzling me is your attention to the direction
property. I can't see any reason why we would want to paint the borders in the
row's or table's direction - it looks like border-collapsing should always be
done in DOM order. What prompted you to do that?

Super glad that you are taking a look at this. It has been a while since I've
been in this code. Can you be more specific about what do you mean by direction,
lots of directions in DOM? What file?

Just FYI: there was still work left to get integer rounding of border positions
correct.

[Xianzhu, 2017-04-28 16:27:25]

@rhogan can you run perf tests blink_perf.paint to see the performance impact?
You can run it locally with and without patch:
  tools/perf/run_benchmark run --browser=release blink_perf.paint
or on trybot:
  git checkout <the-branch-for-this-cl>
  tools/perf/run_benchmark trybot linux blink_perf.paint
  then you'll see the result comparing performance with patch and without patch,
in the trybot result section of the latest patch set of this CL.

I asked because I feel we need to cooperate for this CL and my
https://codereview.chromium.org/2846563002/ and
https://codereview.chromium.org/2846563002/. My CLs try to cache collapsed
borders per cell, but for the purpose of this CL, we may need to cache computed
collapsed borders per grid (in LayoutTableSection::CellStruct). We need to
determine the order in which we will land the CLs based on the performance
impact of this CL. If this CL won't cause performance regressions, we can land
it before mine, and then I modify my CLs to implement per-grid caching.
Otherwise we'll need to implement caching first.

[rhogan, 2017-04-28 16:39:44]

On 2017/04/28 at 16:27:25, wangxianzhu wrote:
> @rhogan can you run perf tests blink_perf.paint to see the performance impact?
> You can run it locally with and without patch:
>   tools/perf/run_benchmark run --browser=release blink_perf.paint
> or on trybot:
>   git checkout <the-branch-for-this-cl>
>   tools/perf/run_benchmark trybot linux blink_perf.paint
>   then you'll see the result comparing performance with patch and without
patch, in the trybot result section of the latest patch set of this CL.
> 
> I asked because I feel we need to cooperate for this CL and my
https://codereview.chromium.org/2846563002/ and
https://codereview.chromium.org/2846563002/. My CLs try to cache collapsed
borders per cell, but for the purpose of this CL, we may need to cache computed
collapsed borders per grid (in LayoutTableSection::CellStruct). We need to
determine the order in which we will land the CLs based on the performance
impact of this CL. If this CL won't cause performance regressions, we can land
it before mine, and then I modify my CLs to implement per-grid caching.
Otherwise we'll need to implement caching first.

@wangxianzhu - just go ahead with your CL. This one is a few weeks away from
being ready!

[dgrogan, 2017-04-28 17:53:10]

Description was changed from

==========
Move border-collapsing to the paint phase and collapse rowspan/colspans
correctly.

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

to

==========
Move border-collapsing to the paint phase and collapse rowspan/colspans
correctly.

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