Paint backgrounds of a table section/row in one display item

Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item. Section also paints column backgrounds into its background
display item.

Previous paint invalidation logic invalidating cells on container
background change is removed.

Perf try job 
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/7134
shows 5% to 20% performance improvement for
PerformanceTests/paint/large-table*.html.

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

Review-Url: https://codereview.chromium.org/2786463004
Cr-Commit-Position: refs/heads/master@{#461907}
Committed: https://chromium.googlesource.com/chromium/src/+/62ad1c5bc2e538f2a57c2e16e06327414a6fbf7a

[Xianzhu, 2017-03-29 21:02:34]

Description was changed from

==========
Paint backgrounds of a table section/row in one display item

BUG=663208
==========

to

==========
Paint backgrounds of a table section/row in one display item

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

[Xianzhu, 2017-03-30 19:51:47]

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

[Xianzhu, 2017-03-30 19:52:20]

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

[commit-bot: I haz the power, 2017-03-30 19:53:24]

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-03-30 20:20:41]

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

[commit-bot: I haz the power, 2017-03-30 20:20:42]

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

[wkorman, 2017-03-30 20:31:35]

Are there particular files or areas where changes were made that we should focus
on, vs. the previous patches?

[Xianzhu, 2017-03-30 20:37:18]

Description was changed from

==========
Paint backgrounds of a table section/row in one display item

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

to

==========
Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item.

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

[Xianzhu, 2017-03-30 20:44:58]

Description was changed from

==========
Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item.

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

to

==========
Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item. Section also paints column backgrounds into its background
display item.

Previous paint invalidation logic invalidating cells on container
background change is removed.

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

[Xianzhu, 2017-03-30 20:52:43]

Sorry, I just noticed the CL description is blank. Updated. (I had updated local
git description but forgot to sync the CL description.)

On 2017/03/30 20:31:35, wkorman wrote:
> Are there particular files or areas where changes were made that we should
focus
> on, vs. the previous patches?

Main changes to look at first:
- TableCellPainter.h/cpp: TableCellPainter::paintContainerBackgroundBehindCell()
no longer creates display items.
- TableRowPainter.h/cpp and TableSectionPainter.cpp:
TableRowPainter::paintBoxDecorationBackground() and
TableSectionPainter::paintBoxDecorationBackground() creates display items for
backgrounds.
- TablePaintInvalidator.cpp: Original paint invalidation logic invalidating
cells on container background change (in TablePaintInvalidator.cpp) is removed.
- LayoutTableCell.h/cpp: Original row background display item client in cells is
removed.

Auxiliary changes to make the whole thing correct:
- LayoutTableRow.cpp: now LayoutTableRow's visual overflow covers cells spanning
multiple rows.
- TablePaintInvalidator.cpp: New paint invalidation logic to invalidate section
on column background change because section paints column backgrounds.
- LayoutTableBoxComponent.h/cpp: Invalidate display item client if the the
background display item will be clipped by the paint rect differently.

[Xianzhu, 2017-03-30 21:09:22]

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

[commit-bot: I haz the power, 2017-03-30 21:10: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-03-30 23:29:11]

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

[commit-bot: I haz the power, 2017-03-30 23:29:14]

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-03-31 16:00:29]

Perf try job
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...
shows 5% to 20% performance improvement for
PerformanceTests/paint/large-table*.html.

[Xianzhu, 2017-03-31 16:00:53]

Description was changed from

==========
Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item. Section also paints column backgrounds into its background
display item.

Previous paint invalidation logic invalidating cells on container
background change is removed.

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

to

==========
Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item. Section also paints column backgrounds into its background
display item.

Previous paint invalidation logic invalidating cells on container
background change is removed.

Perf try job 
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...
shows 5% to 20% performance improvement for
PerformanceTests/paint/large-table*.html.

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

[wkorman, 2017-03-31 22:15:41]

lgtm

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:492: void
addOverflowFromChild(const LayoutBox* child) {
If modifying signature and it won't be null perhaps just make it const&? Same
below.

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:38:
box.setDisplayItemsUncached();
Why this vs invalidateDisplayItemClient?

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

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2141: if
(table()->hasColElements())
Sometimes I expect table might have col elements but no need to paint anything
due to style, is that worth considering now or in future?

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:241: if
(section.table()->hasColElements())
If we change for prior comment would need to follow here similarly.

[chrishtr, 2017-03-31 22:59:07]

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:308:
cellVisualOverflowRect.moveBy(-location());
Why this change?

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

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1283: const auto*
cell = originatingCellAt(r, c);
Why the changes here?

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:239: if
(object.isTableSection()) {
This seems a weird place to invalidate paint phases.

[Xianzhu, 2017-03-31 23:26:30]

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:492: void
addOverflowFromChild(const LayoutBox* child) {
On 2017/03/31 22:15:41, wkorman wrote:
> If modifying signature and it won't be null perhaps just make it const&? Same
> below.

Done.

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:495: void
addOverflowFromChild(const LayoutBox* child, const LayoutSize& delta);
Done.

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:38:
box.setDisplayItemsUncached();
On 2017/03/31 22:15:41, wkorman wrote:
> Why this vs invalidateDisplayItemClient?

It should be invalidateDisplayItemClient. Done.

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:308:
cellVisualOverflowRect.moveBy(-location());
On 2017/03/31 22:59:06, chrishtr wrote:
> Why this change?

The original code happens to be correct for horizontal layout (when
location().x() is zero), but is incorrect for vertical layout. The changed code
is consistent with other places where we adjust cell location in row.

Found the issue when adding line 311-317 which needs the similar logic.

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

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1283: const auto*
cell = originatingCellAt(r, c);
On 2017/03/31 22:59:06, chrishtr wrote:
> Why the changes here?

I added originatingCellAt() in a previous CL. The old code is just to find
originatingCellAt().

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2141: if
(table()->hasColElements())
On 2017/03/31 22:15:41, wkorman wrote:
> Sometimes I expect table might have col elements but no need to paint anything
> due to style, is that worth considering now or in future?

paintedOutputOfObjectHasNoEffectRegardlessOfSize() should be super fast
otherwise the purpose would be defeated. Table cols are rare, and I think in
many cases cols are defined to add col backgrounds, so I prefer not checking col
backgrounds.

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:239: if
(object.isTableSection()) {
On 2017/03/31 22:59:06, chrishtr wrote:
> This seems a weird place to invalidate paint phases.

This is to update paint phase requirements of layers. Would you prefer moving
these code into layout classes implementing something like
LayoutObject::updatePaintingLayerPaintPhaseRequirements()?

[Xianzhu, 2017-03-31 23:26:56]

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

[commit-bot: I haz the power, 2017-03-31 23:27:33]

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-03-31 23:32:00]

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

[commit-bot: I haz the power, 2017-03-31 23:32:01]

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

[chrishtr, 2017-04-03 17:10:51]

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2786463004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:239: if
(object.isTableSection()) {
On 2017/03/31 at 23:26:30, Xianzhu wrote:
> On 2017/03/31 22:59:06, chrishtr wrote:
> > This seems a weird place to invalidate paint phases.
> 
> This is to update paint phase requirements of layers. Would you prefer moving
these code into layout classes implementing something like
LayoutObject::updatePaintingLayerPaintPhaseRequirements()?

Yes, that makes more sense, thanks.

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

https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:29: void
LayoutTableBoxComponent::MutableForPainting::updatePaintResult(
This method is new in this CL, right? I'm not sure why it is needed now
but not before.

[Xianzhu, 2017-04-03 17:23:05]

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

https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:29: void
LayoutTableBoxComponent::MutableForPainting::updatePaintResult(
On 2017/04/03 17:10:51, chrishtr wrote:
> This method is new in this CL, right? I'm not sure why it is needed now
> but not before.

Previously for any display item, we either paint it if it intersects paint rect
or skip painting of it. This ensures that we only need to invalidate a display
item client if required by changes of style, layout or compositing update.

This CL introduces table section and table row display items that can be bigger
than paint rect (interest rect) and clipped. As change of paint rect may affect
the display items, we also need to invalidate the display items when the
previous paint didn't paint the whole display item and paint rect changes. This
is like we invalidate a subsequence when paint rect changes.

[chrishtr, 2017-04-03 17:53:25]

Another question: what is the effect of this CL on raster time when changing a
cell's style?
Does this now incur raster of the row/section?

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

https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:37: auto& box
= static_cast<LayoutTableBoxComponent&>(m_layoutObject);
I think you need a setNeedsRepaint also on the paintingLayer.

[Xianzhu, 2017-04-03 18:23:13]

On 2017/04/03 17:53:25, chrishtr wrote:
> Another question: what is the effect of this CL on raster time when changing a
> cell's style?
> Does this now incur raster of the row/section?
> 

This doesn't change raster behavior on cell style change. We repaint/re-raster
row/section background only when the background changes (because of
row/section|col background style change or layout change).

>
https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp
(right):
> 
>
https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:37: auto&
box
> = static_cast<LayoutTableBoxComponent&>(m_layoutObject);
> I think you need a setNeedsRepaint also on the paintingLayer.

We are already in painting and setNeedsRepaint has already been checked. As
subsequence caching works in the same way, we can be sure we can reach here when
needed.

Actually your comment reminds me why I used setDisplayItemsUncached() instead of
invalidateDisplayItemClient() before wkorman@ asked the reason. This should be
setDisplayItemUncached() because the layer might not have needsRepaint flag set.

Changed to setDisplayItemUncached() and added check and comment.

[Xianzhu, 2017-04-03 18:23:32]

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

[commit-bot: I haz the power, 2017-04-03 18:24: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-03 18:31:02]

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

[commit-bot: I haz the power, 2017-04-03 18:31:03]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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_...)

[chrishtr, 2017-04-03 18:46:40]

On 2017/04/03 at 18:23:13, wangxianzhu wrote:
> On 2017/04/03 17:53:25, chrishtr wrote:
> > Another question: what is the effect of this CL on raster time when changing
a
> > cell's style?
> > Does this now incur raster of the row/section?
> > 
> 
> This doesn't change raster behavior on cell style change. We repaint/re-raster
row/section background only when the background changes (because of
row/section|col background style change or layout change).
> 
> >
https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp
(right):
> > 
> >
https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:37: auto&
box
> > = static_cast<LayoutTableBoxComponent&>(m_layoutObject);
> > I think you need a setNeedsRepaint also on the paintingLayer.
> 
> We are already in painting and setNeedsRepaint has already been checked. As
subsequence caching works in the same way, we can be sure we can reach here when
needed.
> 
> Actually your comment reminds me why I used setDisplayItemsUncached() instead
of invalidateDisplayItemClient() before wkorman@ asked the reason. This should
be setDisplayItemUncached() because the layer might not have needsRepaint flag
set.
> 
> Changed to setDisplayItemUncached() and added check and comment.

My main concern is that a cached subsequence containing this table will not
notice that it is partially painted.
I suppose the only way that the table section could be partially painted but the
subsequence doesn't know about
it is if the cull rect were updated outside of the PaintLayer tree (due to
something other than a clip). But this
seems fragile. Is there a reasonable way to plumb the PaintResult back up to the
containing layer, so that it
knows it was partially painted? Similar to how the various PaintLayerPainter
methods plumb similar bits.

[Xianzhu, 2017-04-03 18:55:05]

On 2017/04/03 18:46:40, chrishtr wrote:
> On 2017/04/03 at 18:23:13, wangxianzhu wrote:
> > On 2017/04/03 17:53:25, chrishtr wrote:
> > > Another question: what is the effect of this CL on raster time when
changing
> a
> > > cell's style?
> > > Does this now incur raster of the row/section?
> > > 
> > 
> > This doesn't change raster behavior on cell style change. We
repaint/re-raster
> row/section background only when the background changes (because of
> row/section|col background style change or layout change).
> > 
> > >
>
https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp
> (right):
> > > 
> > >
>
https://codereview.chromium.org/2786463004/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:37:
auto&
> box
> > > = static_cast<LayoutTableBoxComponent&>(m_layoutObject);
> > > I think you need a setNeedsRepaint also on the paintingLayer.
> > 
> > We are already in painting and setNeedsRepaint has already been checked. As
> subsequence caching works in the same way, we can be sure we can reach here
when
> needed.
> > 
> > Actually your comment reminds me why I used setDisplayItemsUncached()
instead
> of invalidateDisplayItemClient() before wkorman@ asked the reason. This should
> be setDisplayItemUncached() because the layer might not have needsRepaint flag
> set.
> > 
> > Changed to setDisplayItemUncached() and added check and comment.
> 
> My main concern is that a cached subsequence containing this table will not
> notice that it is partially painted.
> I suppose the only way that the table section could be partially painted but
the
> subsequence doesn't know about
> it is if the cull rect were updated outside of the PaintLayer tree (due to
> something other than a clip). But this
> seems fragile. Is there a reasonable way to plumb the PaintResult back up to
the
> containing layer, so that it
> knows it was partially painted? Similar to how the various PaintLayerPainter
> methods plumb similar bits.

It looks necessary to plumb PaintResult from object painters to layer painters.
Will create a patch for the plumbing.

[Xianzhu, 2017-04-04 03:05:57]

On 2017/04/03 18:55:05, Xianzhu wrote:
> On 2017/04/03 18:46:40, chrishtr wrote:
> > My main concern is that a cached subsequence containing this table will not
> > notice that it is partially painted.
> > I suppose the only way that the table section could be partially painted but
> the
> > subsequence doesn't know about
> > it is if the cull rect were updated outside of the PaintLayer tree (due to
> > something other than a clip). But this
> > seems fragile. Is there a reasonable way to plumb the PaintResult back up to
> the
> > containing layer, so that it
> > knows it was partially painted? Similar to how the various PaintLayerPainter
> > methods plumb similar bits.
> 
> It looks necessary to plumb PaintResult from object painters to layer
painters.
> Will create a patch for the plumbing.

I found that the situation doesn't change with this CL. If we change cull rect
outside of the PaintLayer tree, the original code will also have the same
problem because of the early returns in painter code. I think we are correct
about detecting partially painted layers, and this CL will not change that.

[chrishtr, 2017-04-04 18:09:13]

On 2017/04/04 at 03:05:57, wangxianzhu wrote:
> On 2017/04/03 18:55:05, Xianzhu wrote:
> > On 2017/04/03 18:46:40, chrishtr wrote:
> > > My main concern is that a cached subsequence containing this table will
not
> > > notice that it is partially painted.
> > > I suppose the only way that the table section could be partially painted
but
> > the
> > > subsequence doesn't know about
> > > it is if the cull rect were updated outside of the PaintLayer tree (due to
> > > something other than a clip). But this
> > > seems fragile. Is there a reasonable way to plumb the PaintResult back up
to
> > the
> > > containing layer, so that it
> > > knows it was partially painted? Similar to how the various
PaintLayerPainter
> > > methods plumb similar bits.
> > 
> > It looks necessary to plumb PaintResult from object painters to layer
painters.
> > Will create a patch for the plumbing.
> 
> I found that the situation doesn't change with this CL. If we change cull rect
outside of the PaintLayer tree, the original code will also have the same
problem because of the early returns in painter code. I think we are correct
about detecting partially painted layers, and this CL will not change that.

Ok, fair point. Is there a test that exercises these conditions for table
sections/rows?

[Xianzhu, 2017-04-04 18:12:49]

On 2017/04/04 18:09:13, chrishtr wrote:
> On 2017/04/04 at 03:05:57, wangxianzhu wrote:
> > On 2017/04/03 18:55:05, Xianzhu wrote:
> > > On 2017/04/03 18:46:40, chrishtr wrote:
> > > > My main concern is that a cached subsequence containing this table will
> not
> > > > notice that it is partially painted.
> > > > I suppose the only way that the table section could be partially painted
> but
> > > the
> > > > subsequence doesn't know about
> > > > it is if the cull rect were updated outside of the PaintLayer tree (due
to
> > > > something other than a clip). But this
> > > > seems fragile. Is there a reasonable way to plumb the PaintResult back
up
> to
> > > the
> > > > containing layer, so that it
> > > > knows it was partially painted? Similar to how the various
> PaintLayerPainter
> > > > methods plumb similar bits.
> > > 
> > > It looks necessary to plumb PaintResult from object painters to layer
> painters.
> > > Will create a patch for the plumbing.
> > 
> > I found that the situation doesn't change with this CL. If we change cull
rect
> outside of the PaintLayer tree, the original code will also have the same
> problem because of the early returns in painter code. I think we are correct
> about detecting partially painted layers, and this CL will not change that.
> 
> Ok, fair point. Is there a test that exercises these conditions for table
> sections/rows?

Yes, there are several new tests in this CL:
paint/tables/huge-table-composited-scroll.html
paint/tables/self-painting-row-background-vertical-clipped.html

[chrishtr, 2017-04-04 18:58:49]

lgtm

[Xianzhu, 2017-04-04 19:01:45]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-04-04 19:01:46]

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/2786463004/#ps80001
(title: "-")

[commit-bot: I haz the power, 2017-04-04 19:03:08]

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

[Xianzhu, 2017-04-04 19:03:37]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-04-04 19:03:38]

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

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

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 19:23:37]

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

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

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-04-04 19:45:23]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-04-04 19:45:25]

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

[commit-bot: I haz the power, 2017-04-04 19:46:25]

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 00:07:59]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491335123403610,
"parent_rev": "b226550dbd1834c2f7addec9d1df7d57498fb7fc", "commit_rev":
"62ad1c5bc2e538f2a57c2e16e06327414a6fbf7a"}

[commit-bot: I haz the power, 2017-04-05 00:08:51]

Description was changed from

==========
Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item. Section also paints column backgrounds into its background
display item.

Previous paint invalidation logic invalidating cells on container
background change is removed.

Perf try job 
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...
shows 5% to 20% performance improvement for
PerformanceTests/paint/large-table*.html.

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

to

==========
Paint backgrounds of a table section/row in one display item

Previously we painted backgrounds of table cell containers in small
display items each for each cell. This caused complexities in paint
invalidation.

Now paint backgrounds (including shadows) of a section/row in one
display item. Section also paints column backgrounds into its background
display item.

Previous paint invalidation logic invalidating cells on container
background change is removed.

Perf try job 
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...
shows 5% to 20% performance improvement for
PerformanceTests/paint/large-table*.html.

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

Review-Url: https://codereview.chromium.org/2786463004
Cr-Commit-Position: refs/heads/master@{#461907}
Committed:
https://chromium.googlesource.com/chromium/src/+/62ad1c5bc2e538f2a57c2e16e063...
==========

[commit-bot: I haz the power, 2017-04-05 00:08:55]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/62ad1c5bc2e538f2a57c2e16e063...