Raster display item lists via a visual rect RTree.

Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
TBR=sadrul
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/ccb9e13712b1632b889960d1d85d556c0139fd51
Cr-Commit-Position: refs/heads/master@{#409291}

[wkorman, 2015-11-30 22:03:39]

Description was changed from

==========
WIP: Performance test Blink visual rects with cc RTree usage.

Essentially, manual integration of http://crrev.com/1420283016 with
http://crrev.com/1459003003

BUG=529938
==========

to

==========
WIP: Performance test Blink visual rects with cc RTree usage.

Essentially, manual integration of http://crrev.com/1420283016 with
http://crrev.com/1459003003

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Charlie Harrison, 2015-12-08 19:40:11]

Description was changed from

==========
WIP: Performance test Blink visual rects with cc RTree usage.

Essentially, manual integration of http://crrev.com/1420283016 with
http://crrev.com/1459003003

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
WIP: Performance test Blink visual rects with cc RTree usage.

Essentially, manual integration of http://crrev.com/1420283016 with
http://crrev.com/1459003003

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[esprehn, 2015-12-08 19:58:30]

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

[esprehn, 2015-12-08 19:58:31]

https://codereview.chromium.org/1484163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1484163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:126: typedef
WTF::HashMap<DisplayItemClient, IntRect> DisplayItemRectMap;
Doesn't this approach the size of the layout tree if you invalidate paint on
many things? Every layout object is a client, so this map contains the entire
layout tree?

[wkorman, 2015-12-08 20:55:09]

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

[wkorman, 2015-12-08 20:55:10]

+ a few folks for the conversation with esprehn@. Feedback on current change
welcome but it is still in work as you'll see by the TODOs.

My intent is to submit this as a series of other changes, rather than this one
monolithic one.

The cc side is likely to be more monolithic due to cc team's desire to avoid
using a flag/experiment.

https://codereview.chromium.org/1484163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1484163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:126: typedef
WTF::HashMap<DisplayItemClient, IntRect> DisplayItemRectMap;
On 2015/12/08 at 19:58:31, esprehn wrote:
> Doesn't this approach the size of the layout tree if you invalidate paint on
many things? Every layout object is a client, so this map contains the entire
layout tree?

I believe you are correct. We obviously won't enable this code path until/unless
we gain comfort with the memory/performance ramifications. When we submit it
will be behind a flag.

We know that we may need to explore alternate data structure/approach for better
memory efficiency, but expectation is that there is a worthwhile performance
gain for memory increase.

What is your level of concern re: the degenerate case where we store the entire
layout tree, in the big picture of overall Blink/Chrome memory usage and
haraken@'s analysis?

https://docs.google.com/presentation/d/1_LRxXp30j60npShHSRVit0B5tLOS4TLiZ68gQ...

[Xianzhu, 2015-12-09 17:09:26]

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

[Xianzhu, 2015-12-09 17:09:27]

https://codereview.chromium.org/1484163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/1484163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:126: typedef
WTF::HashMap<DisplayItemClient, IntRect> DisplayItemRectMap;
On 2015/12/08 20:55:10, wkorman wrote:
> On 2015/12/08 at 19:58:31, esprehn wrote:
> > Doesn't this approach the size of the layout tree if you invalidate paint on
> many things? Every layout object is a client, so this map contains the entire
> layout tree?
> 
> I believe you are correct. We obviously won't enable this code path
until/unless
> we gain comfort with the memory/performance ramifications. When we submit it
> will be behind a flag.
> 
> We know that we may need to explore alternate data structure/approach for
better
> memory efficiency, but expectation is that there is a worthwhile performance
> gain for memory increase.
> 
> What is your level of concern re: the degenerate case where we store the
entire
> layout tree, in the big picture of overall Blink/Chrome memory usage and
> haraken@'s analysis?
> 
>
https://docs.google.com/presentation/d/1_LRxXp30j60npShHSRVit0B5tLOS4TLiZ68gQ...

FYI I have converted DisplayItemClient to an interface, making it possible to
let PaintController access client data. We can add a method
DisplayItemClient::previousPaintBounds() and LayoutObject can just return its
m_previousPaintInvalidaitonRect.

[wkorman, 2015-12-09 17:51:15]

On 2015/12/09 at 17:09:27, wangxianzhu wrote:
> FYI I have converted DisplayItemClient to an interface, making it possible to
let PaintController access client data. We can add a method
DisplayItemClient::previousPaintBounds() and LayoutObject can just return its
m_previousPaintInvalidaitonRect.

I was excited to see that. IIUC this should mean we don't need to build and
maintain a hash set of DisplayItemClients -> visual_rects in PaintController,
since we can just retrieve them from the clients directly. I had assumed we
wanted to keep DisplayItemClient minimal for layering purposes, but I have only
a loose sense of compartmentalization for existing system currently.

What does this mean for csharrison@ re:

https://groups.google.com/a/chromium.org/d/msg/paint-dev/E9FIy0B2AkU/Js-83oIq...

Would he also add a method to DisplayItemClient for op count?

If you think we should pursue this path I'll send a change to add
previousPaintBounds() method.

[Xianzhu, 2015-12-09 18:03:26]

On 2015/12/09 17:51:15, wkorman wrote:
> On 2015/12/09 at 17:09:27, wangxianzhu wrote:
> > FYI I have converted DisplayItemClient to an interface, making it possible
to
> let PaintController access client data. We can add a method
> DisplayItemClient::previousPaintBounds() and LayoutObject can just return its
> m_previousPaintInvalidaitonRect.
> 
> I was excited to see that. IIUC this should mean we don't need to build and
> maintain a hash set of DisplayItemClients -> visual_rects in PaintController,
> since we can just retrieve them from the clients directly. I had assumed we
> wanted to keep DisplayItemClient minimal for layering purposes, but I have
only
> a loose sense of compartmentalization for existing system currently.
> 
> What does this mean for csharrison@ re:
> 
>
https://groups.google.com/a/chromium.org/d/msg/paint-dev/E9FIy0B2AkU/Js-83oIq...
> 
> Would he also add a method to DisplayItemClient for op count?
> 

I think op count should be still a PaintController thing which can be calculated
during commitNewDisplayItems.

> If you think we should pursue this path I'll send a change to add
> previousPaintBounds() method.

I'm working on a change adding CacheableDisplayItemClient to support client-side
invalidation flags. This change will be ready soon, then you can add
previousPaintBounds() into CacheableDisplayItemClient or a sub interface of it.

[esprehn, 2015-12-11 10:39:49]

That sounds like a great plan to me, we should reuse the storage in the layout
tree and not add these maps, the map bloat can be pretty bad for large
invalidations here, for example if you did document.style.color = "red" we'd
issue invalidations all over the page and end up with a huge map.

[wkorman, 2016-02-08 20:28:57]

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

[wkorman, 2016-02-08 20:30:34]

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

[wkorman, 2016-02-08 20:33:39]

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

[commit-bot: I haz the power, 2016-02-08 20:35:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484163002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484163002/360001

[commit-bot: I haz the power, 2016-02-08 22:09:56]

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

[commit-bot: I haz the power, 2016-02-08 22:09:57]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wkorman, 2016-02-22 19:29:53]

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

[commit-bot: I haz the power, 2016-02-22 19:30:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484163002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484163002/400001

[commit-bot: I haz the power, 2016-02-22 20:36:32]

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

[commit-bot: I haz the power, 2016-02-22 20:36:33]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[wkorman, 2016-03-24 20:17:35]

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

[commit-bot: I haz the power, 2016-03-24 20:18:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484163002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484163002/500001

[commit-bot: I haz the power, 2016-03-24 21:22:50]

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

[commit-bot: I haz the power, 2016-03-24 21:22:51]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[wkorman, 2016-04-05 20:50:11]

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

[commit-bot: I haz the power, 2016-04-05 20:50:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484163002/580001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484163002/580001

[commit-bot: I haz the power, 2016-04-05 21:33:13]

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

[commit-bot: I haz the power, 2016-04-05 21:33:14]

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

[wkorman, 2016-04-07 19:01:00]

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

[commit-bot: I haz the power, 2016-04-07 19:02:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484163002/600001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484163002/600001

[commit-bot: I haz the power, 2016-04-07 19:42:06]

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

[commit-bot: I haz the power, 2016-04-07 19:42:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[wkorman, 2016-05-23 05:31:24]

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

[commit-bot: I haz the power, 2016-05-23 05:31:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484163002/680001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484163002/680001

[commit-bot: I haz the power, 2016-05-23 06:16:26]

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

[commit-bot: I haz the power, 2016-05-23 06:16:28]

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

[wkorman, 2016-06-20 18:06:05]

Description was changed from

==========
WIP: Performance test Blink visual rects with cc RTree usage.

Essentially, manual integration of http://crrev.com/1420283016 with
http://crrev.com/1459003003

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use an RTree to raster in cc.

BUG=529938
==========

[wkorman, 2016-06-20 18:15:49]

Description was changed from

==========
Use an RTree to raster in cc.

BUG=529938
==========

to

==========
Use an RTree to raster in cc.

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[wkorman, 2016-06-22 18:24:45]

wkorman@chromium.org changed reviewers:
- esprehn@chromium.org, wangxianzhu@chromium.org

[wkorman, 2016-06-22 18:24:47]

Seeking review of changes under cc/ from vmpstr@ now, as we near wrapping up
remaining LayoutTest failures.

The changes under third_party/WebKit here (except one for a cc method signature
change in PaintArtifactCompositor.cpp) will be moved to a different change and
submitted ahead of this one.

One comment on this change: the cc DisplayItemListSettings proto and wrapping
class as cc/playback/display_item_list_settings.h is obsolete with this change.
I've discussed this previously with vmpstr@ and we agreed it should be removed.
I'd like to do that as a separate change just to keep things under control.

I've also previously discussed putting this change behind a flag, but cc team
would prefer to land and revert as needed.

As noted I am still looking at a few remaining layout test failures, and cluster
telemetry, and will go over those in person with vmpstr@/chrishtr@ before this
change lands.

[vmpstr, 2016-06-22 20:14:05]

vmpstr@chromium.org changed reviewers:
+ nyquist@chromium.org

[vmpstr, 2016-06-22 20:14:07]

looks good!

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:53: scoped_refptr<DisplayItemList>
DisplayItemList::Create(
Just as a very optional nit, I've been slowly removing ::Create functions that
just forward things to the ctor, so your call but it may be something you might
as well throw in here.

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:79: retain_visual_rects_(false),
nit: you can = false in the header (same for the rest of PODs)

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:100: for (const auto& item : items_) {
I wonder if we should add operator[] to ContiguousContainer and do a regular i =
0; i < items_.size() loop

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:151: // This is more permissive than Picture's
implementation, since none of the
nit: Add a TODO(someone) to make it clear that this needs to be fixed

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.h:121: bool retain_visual_rects_;
Where are we using this?

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
File cc/proto/display_item.proto (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
cc/proto/display_item.proto:24: // Deprecated.
I think at this point in time, we're free to remove things as need +nyquist@ for
an opinion

[nyquist, 2016-06-24 18:31:01]

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
File cc/proto/display_item.proto (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
cc/proto/display_item.proto:24: // Deprecated.
On 2016/06/22 20:14:07, vmpstr wrote:
> I think at this point in time, we're free to remove things as need +nyquist@
for
> an opinion

I think that right now it's fine to remove this field. I'd still add the new
field with a new ID like you've done though.

[wkorman, 2016-06-24 19:57:14]

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:53: scoped_refptr<DisplayItemList>
DisplayItemList::Create(
On 2016/06/22 at 20:14:07, vmpstr wrote:
> Just as a very optional nit, I've been slowly removing ::Create functions that
just forward things to the ctor, so your call but it may be something you might
as well throw in here.

It looks like we'd need to replace a bunch of callsites with
make_scoped_refptr(new DisplayItemList()), it's vaguely nice to have this method
instead as shorthand and for future flexibility?

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:79: retain_visual_rects_(false),
On 2016/06/22 at 20:14:07, vmpstr wrote:
> nit: you can = false in the header (same for the rest of PODs)

Done.

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:100: for (const auto& item : items_) {
On 2016/06/22 at 20:14:07, vmpstr wrote:
> I wonder if we should add operator[] to ContiguousContainer and do a regular i
= 0; i < items_.size() loop

Added TODO.

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:151: // This is more permissive than Picture's
implementation, since none of the
On 2016/06/22 at 20:14:07, vmpstr wrote:
> nit: Add a TODO(someone) to make it clear that this needs to be fixed

Done.

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.h:121: bool retain_visual_rects_;
On 2016/06/22 at 20:14:07, vmpstr wrote:
> Where are we using this?

In display_item_list_unittest, two main cases:

1. tests that use ValidateDisplayItemListSerialization() end up calling
cc::DisplayItemList::ToProtobuf() after Finalize() is called. Finalize() by
default clears out the visual rects, and ToProtobuf() has:

  DCHECK(items_.size() == visual_rects_.size())
      << "items.size() " << items_.size() << " visual_rects.size() "
      << visual_rects_.size();

which will fail unless we keep the visual rects around. It's a DCHECK so I
figure it is ok in release builds where Blimp will likely be calling
ToProtobuf() after Finalize() but if I'm off base let's discuss further.

2. There are a couple of unit tests for AsValue() which of course need the
visual rects to be around to make sure that part is working as intended.

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
File cc/proto/display_item.proto (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
cc/proto/display_item.proto:24: // Deprecated.
On 2016/06/24 at 18:31:00, nyquist wrote:
> On 2016/06/22 20:14:07, vmpstr wrote:
> > I think at this point in time, we're free to remove things as need +nyquist@
for
> > an opinion
> 
> I think that right now it's fine to remove this field. I'd still add the new
field with a new ID like you've done though.

OK, removed. There are varying opinions on best practices to remove a proto
field even when in nascent usage, I've just literally deleted its entry here.
Some would say to leave it commented out for history, others would say to leave
it with a prefix like DELETED. Open to whatever is preferred.

[vmpstr, 2016-06-24 20:15:33]

looks good!

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.cc:53: scoped_refptr<DisplayItemList>
DisplayItemList::Create(
On 2016/06/24 19:57:14, wkorman wrote:
> On 2016/06/22 at 20:14:07, vmpstr wrote:
> > Just as a very optional nit, I've been slowly removing ::Create functions
that
> just forward things to the ctor, so your call but it may be something you
might
> as well throw in here.
> 
> It looks like we'd need to replace a bunch of callsites with
> make_scoped_refptr(new DisplayItemList()), it's vaguely nice to have this
method
> instead as shorthand and for future flexibility?

Makes sense, and yeah in this case we also have CreateFromProto, so Create in
general makes sense.

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.h:121: bool retain_visual_rects_;
On 2016/06/24 19:57:14, wkorman wrote:
> On 2016/06/22 at 20:14:07, vmpstr wrote:
> > Where are we using this?
> 
> In display_item_list_unittest, two main cases:
> 
> 1. tests that use ValidateDisplayItemListSerialization() end up calling
> cc::DisplayItemList::ToProtobuf() after Finalize() is called. Finalize() by
> default clears out the visual rects, and ToProtobuf() has:
> 
>   DCHECK(items_.size() == visual_rects_.size())
>       << "items.size() " << items_.size() << " visual_rects.size() "
>       << visual_rects_.size();
> 
> which will fail unless we keep the visual rects around. It's a DCHECK so I
> figure it is ok in release builds where Blimp will likely be calling
> ToProtobuf() after Finalize() but if I'm off base let's discuss further.
> 
> 2. There are a couple of unit tests for AsValue() which of course need the
> visual rects to be around to make sure that part is working as intended.

Can you put a comment to make it clear that this is for tests?

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
File cc/proto/display_item.proto (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/proto/display_item....
cc/proto/display_item.proto:24: // Deprecated.
On 2016/06/24 19:57:14, wkorman wrote:
> On 2016/06/24 at 18:31:00, nyquist wrote:
> > On 2016/06/22 20:14:07, vmpstr wrote:
> > > I think at this point in time, we're free to remove things as need
+nyquist@
> for
> > > an opinion
> > 
> > I think that right now it's fine to remove this field. I'd still add the new
> field with a new ID like you've done though.
> 
> OK, removed. There are varying opinions on best practices to remove a proto
> field even when in nascent usage, I've just literally deleted its entry here.
> Some would say to leave it commented out for history, others would say to
leave
> it with a prefix like DELETED. Open to whatever is preferred.

I agree with deprecating fields in live code, since there would be clients in
existence that depend on the field. My recommendation here is only because this
is not launched yet, so we might as well do all the cleanup we can before it's
released.

[esprehn, 2016-06-24 21:12:25]

Can you write a more detailed change description explaining the awesome of this
patch? :)

[wkorman, 2016-06-24 21:56:36]

Description was changed from

==========
Use an RTree to raster in cc.

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use an RTree to raster in cc.

Rather than caching and playing back an entire SkPicture
when rastering a display item list, we instead retain
display items and use them in combination with their
visual rects to raster only what's needed.

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[wkorman, 2016-06-24 22:16:14]

Description was changed from

==========
Use an RTree to raster in cc.

Rather than caching and playing back an entire SkPicture
when rastering a display item list, we instead retain
display items and use them in combination with their
visual rects to raster only what's needed.

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[wkorman, 2016-06-24 22:39:21]

On 2016/06/24 at 21:12:25, esprehn wrote:
> Can you write a more detailed change description explaining the awesome of
this patch? :)

Done.

I'm still working on a few remaining layout tests for which fixes will land
separately and will ping reviewers for a really-final review when those are
done.

[wkorman, 2016-06-24 22:39:32]

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1484163002/diff/800001/cc/playback/display_it...
cc/playback/display_item_list.h:121: bool retain_visual_rects_;
On 2016/06/24 at 20:15:33, vmpstr wrote:
> On 2016/06/24 19:57:14, wkorman wrote:
> > On 2016/06/22 at 20:14:07, vmpstr wrote:
> > > Where are we using this?
> > 
> > In display_item_list_unittest, two main cases:
> > 
> > 1. tests that use ValidateDisplayItemListSerialization() end up calling
> > cc::DisplayItemList::ToProtobuf() after Finalize() is called. Finalize() by
> > default clears out the visual rects, and ToProtobuf() has:
> > 
> >   DCHECK(items_.size() == visual_rects_.size())
> >       << "items.size() " << items_.size() << " visual_rects.size() "
> >       << visual_rects_.size();
> > 
> > which will fail unless we keep the visual rects around. It's a DCHECK so I
> > figure it is ok in release builds where Blimp will likely be calling
> > ToProtobuf() after Finalize() but if I'm off base let's discuss further.
> > 
> > 2. There are a couple of unit tests for AsValue() which of course need the
> > visual rects to be around to make sure that part is working as intended.
> 
> Can you put a comment to make it clear that this is for tests?

Done.

[chrishtr, 2016-07-07 19:49:22]

https://codereview.chromium.org/1484163002/diff/940001/cc/blink/web_content_l...
File cc/blink/web_content_layer_impl.cc (left):

https://codereview.chromium.org/1484163002/diff/940001/cc/blink/web_content_l...
cc/blink/web_content_layer_impl.cc:78:
cc::DisplayItemList::Create(PaintableRegion(), settings);
PaintableRegion may not start at 0,0.

[wkorman, 2016-07-12 00:00:48]

https://codereview.chromium.org/1484163002/diff/940001/cc/blink/web_content_l...
File cc/blink/web_content_layer_impl.cc (left):

https://codereview.chromium.org/1484163002/diff/940001/cc/blink/web_content_l...
cc/blink/web_content_layer_impl.cc:78:
cc::DisplayItemList::Create(PaintableRegion(), settings);
On 2016/07/07 at 19:49:22, chrishtr wrote:
> PaintableRegion may not start at 0,0.

Three affected LayoutTests identified via CHECK, to investigate further:

compositing/squashing/vertical-writing-mode-squashed.html
fast/forms/huge-mac-input-clamped-width.html
fast/forms/huge-mac-input-clamped-height.html

[wkorman, 2016-07-26 20:38:15]

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

[commit-bot: I haz the power, 2016-07-26 20:38:42]

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, 2016-07-26 20:38:45]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-26 21:20:56]

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

[commit-bot: I haz the power, 2016-07-26 21:20:59]

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, 2016-07-28 21:17:19]

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

[commit-bot: I haz the power, 2016-07-28 21:17:45]

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, 2016-07-28 21:17:49]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-28 21:20:14]

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

[commit-bot: I haz the power, 2016-07-28 21:20:15]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

[wkorman, 2016-07-29 18:23:17]

Description was changed from

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
==========

[wkorman, 2016-07-29 20:00:01]

Description was changed from

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
==========

to

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[wkorman, 2016-07-30 00:33:08]

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

[commit-bot: I haz the power, 2016-07-30 00:33:47]

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, 2016-07-30 03:54:37]

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

[commit-bot: I haz the power, 2016-07-30 03:54:38]

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

[chrishtr, 2016-08-01 20:22:20]

lgtm

https://codereview.chromium.org/1484163002/diff/1280001/third_party/WebKit/So...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/1484163002/diff/1280001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:86:
static gfx::Rect largeRect(-20000, -20000, 40000, 40000);
Recommend expanding by 10-100x in each direction, for safety.

[wkorman, 2016-08-01 20:32:59]

vmpstr@ PTAL, requesting LGTM, would like to commit this patch and will watch
raster perf benchmarks subsequently.

+sky@ for ui/{compositor,views} OWNERS. Changes there are purely parameter for
method signature change.

https://codereview.chromium.org/1484163002/diff/1280001/third_party/WebKit/So...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/1484163002/diff/1280001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:86:
static gfx::Rect largeRect(-20000, -20000, 40000, 40000);
On 2016/08/01 at 20:22:19, chrishtr wrote:
> Recommend expanding by 10-100x in each direction, for safety.

Done.

[wkorman, 2016-08-01 20:33:24]

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

[wkorman, 2016-08-01 20:33:27]

Actually +sky@ for ui/{compositor,views} OWNERS this time. Changes there are
purely parameter for method signature change.

[wkorman, 2016-08-01 20:36:06]

wkorman@chromium.org changed reviewers:
+ sadrul@chromium.org
- sky@chromium.org

[wkorman, 2016-08-01 20:36:09]

+sadrul@ for ui/ OWNERS rather than sky@ for load balancing.

[wkorman, 2016-08-01 20:42:37]

Description was changed from

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
TBR=sadrul
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sky, 2016-08-02 03:17:06]

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

[sky, 2016-08-02 03:17:10]

LGTM

[sadrul, 2016-08-02 16:25:30]

Already reviewed by sky@, but ++lgtm

[vmpstr, 2016-08-02 17:34:21]

cc lgtm with nits

https://codereview.chromium.org/1484163002/diff/1300001/cc/blink/web_display_...
File cc/blink/web_display_item_list_impl.cc (right):

https://codereview.chromium.org/1484163002/diff/1300001/cc/blink/web_display_...
cc/blink/web_display_item_list_impl.cc:161: // TODO(wkorman): Should we
translate the visual rect as well? Create a test
nit: can you file a bug if one doesn't exist?

https://codereview.chromium.org/1484163002/diff/1300001/cc/playback/display_i...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1484163002/diff/1300001/cc/playback/display_i...
cc/playback/display_item_list.cc:124: static bool GetCanvasClipBounds(SkCanvas*
canvas, gfx::Rect* clip_bounds) {
move this to the top of the file please (and maybe put it in anonymous namespace
instead of keyword static)

https://codereview.chromium.org/1484163002/diff/1300001/cc/playback/display_i...
cc/playback/display_item_list.cc:140: for (size_t index : indices) {
Is there a plan to skip non-drawing sets of items?

[wkorman, 2016-08-02 18:15:09]

https://codereview.chromium.org/1484163002/diff/1300001/cc/blink/web_display_...
File cc/blink/web_display_item_list_impl.cc (right):

https://codereview.chromium.org/1484163002/diff/1300001/cc/blink/web_display_...
cc/blink/web_display_item_list_impl.cc:161: // TODO(wkorman): Should we
translate the visual rect as well? Create a test
On 2016/08/02 at 17:34:21, vmpstr (dnd) wrote:
> nit: can you file a bug if one doesn't exist?

Done.

https://codereview.chromium.org/1484163002/diff/1300001/cc/playback/display_i...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1484163002/diff/1300001/cc/playback/display_i...
cc/playback/display_item_list.cc:124: static bool GetCanvasClipBounds(SkCanvas*
canvas, gfx::Rect* clip_bounds) {
On 2016/08/02 at 17:34:21, vmpstr (dnd) wrote:
> move this to the top of the file please (and maybe put it in anonymous
namespace instead of keyword static)

Done.

https://codereview.chromium.org/1484163002/diff/1300001/cc/playback/display_i...
cc/playback/display_item_list.cc:140: for (size_t index : indices) {
On 2016/08/02 at 17:34:21, vmpstr (dnd) wrote:
> Is there a plan to skip non-drawing sets of items?

Paint code checks drawsContent() for various cases, see for example:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

but if you explain more what you mean specifically and why it's material here we
can make sure to track/file a bug.

[wkorman, 2016-08-02 18:15:24]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-08-02 18:15:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, sadrul@chromium.org, vmpstr@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1484163002/#ps1320001
(title: "Code review feedback.")

[commit-bot: I haz the power, 2016-08-02 18:16:27]

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, 2016-08-02 20:07:27]

Committed patchset #67 (id:1320001)

[commit-bot: I haz the power, 2016-08-02 20:09:01]

Description was changed from

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
TBR=sadrul
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Raster display item lists via a visual rect RTree.

Rather than caching and playing back an entire SkPicture
when rastering a display item list for a particular
playback rect, instead retain display items and query
them via an RTree of their visual rects to find and
raster only what's needed.

Display item lists no longer support the notion of a
bounding "layer rect" with mutable origin.

DisplayItemListSettings proto is obsolete after this
change as it's comprised solely of one field to allow
switching whether to use the aforementioned now-deleted
cached SkPicture code path. It will be deleted in a
subsequent patch.

BUG=529938
TBR=sadrul
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/ccb9e13712b1632b889960d1d85d556c0139fd51
Cr-Commit-Position: refs/heads/master@{#409291}
==========

[commit-bot: I haz the power, 2016-08-02 20:09:06]

Patchset 67 (id:??) landed as
https://crrev.com/ccb9e13712b1632b889960d1d85d556c0139fd51
Cr-Commit-Position: refs/heads/master@{#409291}

[Khushal, 2016-08-02 23:47:54]

https://codereview.chromium.org/1484163002/diff/1320001/cc/playback/display_i...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1484163002/diff/1320001/cc/playback/display_i...
cc/playback/display_item_list.cc:162:
std::vector<gfx::Rect>().swap(inputs_.visual_rects);
Looks like the visual rects are being dropped here and the
DisplayItemList::ToProtobuf which runs after Finalize is trying to serialize
them.

[wkorman, 2016-08-02 23:51:26]

https://codereview.chromium.org/1484163002/diff/1320001/cc/playback/display_i...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1484163002/diff/1320001/cc/playback/display_i...
cc/playback/display_item_list.cc:162:
std::vector<gfx::Rect>().swap(inputs_.visual_rects);
On 2016/08/02 at 23:47:54, Khushal wrote:
> Looks like the visual rects are being dropped here and the
DisplayItemList::ToProtobuf which runs after Finalize is trying to serialize
them.

Yes, possible, this is a memory optimization but perhaps it is not compatible
with Blimp's current control flow. If you comment this out, does it fix the
issue?