cc: Include pictures in trace output for DisplayItemLists

cc: Include pictures in trace output for DisplayItemLists

This makes DisplayItemList::AsValue() include a serialized SkPicture
containing the list's items. This is needed so that layers backed by
DisplayItemLists can have their contents and draw operations inspected
in Frame Viewer (this will still require a follow-up patch in
trace-viewer).

BUG=444181
Committed: https://crrev.com/d7dd21abc088c4fbb0d67dcde2ae9f8f0120ec76
Cr-Commit-Position: refs/heads/master@{#310645}

[ajuma, 2015-01-07 22:45:49]

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

[vmpstr, 2015-01-08 21:44:34]

https://codereview.chromium.org/816443003/diff/40001/cc/debug/picture_debug_u...
File cc/debug/picture_debug_util.h (right):

https://codereview.chromium.org/816443003/diff/40001/cc/debug/picture_debug_u...
cc/debug/picture_debug_util.h:16: static void SerializeInBase64(const SkPicture*
picture, std::string* output);
nit: Sounds a bit awkward.. maybe SerializeToBase64 or SerializeAsBase64? Up to
you.

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_ite...
File cc/resources/display_item_list.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_ite...
cc/resources/display_item_list.cc:82: for (size_t i = 0; i < items_.size(); ++i)
{
nit: braces optional

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_lis...
File cc/resources/display_list_recording_source.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_lis...
cc/resources/display_list_recording_source.cc:90:
display_list_->set_layer_rect(recorded_viewport_);
Unlike picture pile, this is guaranteed to be the actual recorded viewport,
right? (just confirming)

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
File cc/resources/drawing_display_item.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
cc/resources/drawing_display_item.cc:34: void
DrawingDisplayItem::RasterForTracing(SkCanvas* canvas) const {
This seems to be basically ::Raster with nullptr as callback (except it would
use drawPicture instead of playback). 

Is there a reason this has to be a different function?

[ajuma, 2015-01-08 22:20:58]

https://codereview.chromium.org/816443003/diff/40001/cc/debug/picture_debug_u...
File cc/debug/picture_debug_util.h (right):

https://codereview.chromium.org/816443003/diff/40001/cc/debug/picture_debug_u...
cc/debug/picture_debug_util.h:16: static void SerializeInBase64(const SkPicture*
picture, std::string* output);
On 2015/01/08 21:44:34, vmpstr wrote:
> nit: Sounds a bit awkward.. maybe SerializeToBase64 or SerializeAsBase64? Up
to
> you.

Changed to SerializeAsBase64.

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_ite...
File cc/resources/display_item_list.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_ite...
cc/resources/display_item_list.cc:82: for (size_t i = 0; i < items_.size(); ++i)
{
On 2015/01/08 21:44:34, vmpstr wrote:
> nit: braces optional

Removed.

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_lis...
File cc/resources/display_list_recording_source.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/display_lis...
cc/resources/display_list_recording_source.cc:90:
display_list_->set_layer_rect(recorded_viewport_);
On 2015/01/08 21:44:34, vmpstr wrote:
> Unlike picture pile, this is guaranteed to be the actual recorded viewport,
> right? (just confirming)

Yes.

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
File cc/resources/drawing_display_item.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
cc/resources/drawing_display_item.cc:34: void
DrawingDisplayItem::RasterForTracing(SkCanvas* canvas) const {
On 2015/01/08 21:44:34, vmpstr wrote:
> This seems to be basically ::Raster with nullptr as callback (except it would
> use drawPicture instead of playback). 
> 
> Is there a reason this has to be a different function?

We need playback instead of drawPicture to make the tracing output useful. If we
use drawPicture, the resulting SkPicture is just a sequence of drawPicture ops,
one for each DrawingDisplayItem. The picture debugger in about:tracing doesn't
go deeper for drawPicture ops, so all we see is a sequence of drawPicture ops
and not much else. With playback, the SkPicture consists of the individual ops.

[vmpstr, 2015-01-08 22:46:00]

lgtm

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
File cc/resources/drawing_display_item.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
cc/resources/drawing_display_item.cc:34: void
DrawingDisplayItem::RasterForTracing(SkCanvas* canvas) const {
On 2015/01/08 22:20:58, ajuma wrote:
> On 2015/01/08 21:44:34, vmpstr wrote:
> > This seems to be basically ::Raster with nullptr as callback (except it
would
> > use drawPicture instead of playback). 
> > 
> > Is there a reason this has to be a different function?
> 
> We need playback instead of drawPicture to make the tracing output useful. If
we
> use drawPicture, the resulting SkPicture is just a sequence of drawPicture
ops,
> one for each DrawingDisplayItem. The picture debugger in about:tracing doesn't
> go deeper for drawPicture ops, so all we see is a sequence of drawPicture ops
> and not much else. With playback, the SkPicture consists of the individual
ops. 

Ah I see. FWIW, you can just Raster and give is some small class DontAbort :
public SkDrawPictureCallback { bool abortDrawing() override { return false; } };
type of thing. However, this works as well.

Either way, can you make a comment here that explains why this should be
different (or why we need to pass a don't abort callback if you go that route)?

[ajuma, 2015-01-08 23:16:11]

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
File cc/resources/drawing_display_item.cc (right):

https://codereview.chromium.org/816443003/diff/40001/cc/resources/drawing_dis...
cc/resources/drawing_display_item.cc:34: void
DrawingDisplayItem::RasterForTracing(SkCanvas* canvas) const {
On 2015/01/08 22:45:59, vmpstr wrote:
> On 2015/01/08 22:20:58, ajuma wrote:
> > On 2015/01/08 21:44:34, vmpstr wrote:
> > > This seems to be basically ::Raster with nullptr as callback (except it
> would
> > > use drawPicture instead of playback). 
> > > 
> > > Is there a reason this has to be a different function?
> > 
> > We need playback instead of drawPicture to make the tracing output useful.
If
> we
> > use drawPicture, the resulting SkPicture is just a sequence of drawPicture
> ops,
> > one for each DrawingDisplayItem. The picture debugger in about:tracing
doesn't
> > go deeper for drawPicture ops, so all we see is a sequence of drawPicture
ops
> > and not much else. With playback, the SkPicture consists of the individual
> ops. 
> 
> Ah I see. FWIW, you can just Raster and give is some small class DontAbort :
> public SkDrawPictureCallback { bool abortDrawing() override { return false; }
};
> type of thing. However, this works as well.
> 
> Either way, can you make a comment here that explains why this should be
> different (or why we need to pass a don't abort callback if you go that
route)?

Added a comment.

[ajuma, 2015-01-08 23:16:18]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2015-01-08 23:17:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/816443003/80001

[commit-bot: I haz the power, 2015-01-09 00:57:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-09 00:59:16]

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