cc: Add trace output for individual DisplayItems

cc: Add trace output for individual DisplayItems

This adds details about each individual DisplayItem to
DisplayItemList's trace output. Each DrawingDisplayItem's
SkPicture is also included in the trace output.

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

[pdr., 2015-02-02 22:22:15]

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

[pdr., 2015-02-02 22:22:16]

I think we need to have valid json output to make it easy to parse on the other
side, but otherwise this looks great.

https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
File cc/resources/clip_display_item.cc (right):

https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
cc/resources/clip_display_item.cc:59: for (size_t i = 0; i <
rounded_clip_rects_.size(); ++i) {
You can use a fancy c++11 iterator here to clean up the duplicated
rounded_clip_rects_[i] calls. (here, and elsewhere)

Something like:
for (auto clip_rect : rounded_clip_rects) {
 ...
}

https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
cc/resources/clip_display_item.cc:66: base::StringAppendF(&value, "[%f %f]",
upper_left_radius.x(),
Can you double-check that the output of these is valid json? I think we need a
comma here. (here, and elsewhere)

https://codereview.chromium.org/884473005/diff/20001/cc/resources/float_clip_...
File cc/resources/float_clip_display_item.cc (right):

https://codereview.chromium.org/884473005/diff/20001/cc/resources/float_clip_...
cc/resources/float_clip_display_item.cc:40:
array->AppendString(base::StringPrintf("FloatClipDisplayItem rect: [%s]",
This is another case I don't think will work in json.

[ajuma, 2015-02-02 23:02:21]

https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
File cc/resources/clip_display_item.cc (right):

https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
cc/resources/clip_display_item.cc:59: for (size_t i = 0; i <
rounded_clip_rects_.size(); ++i) {
On 2015/02/02 22:22:16, pdr wrote:
> You can use a fancy c++11 iterator here to clean up the duplicated
> rounded_clip_rects_[i] calls. (here, and elsewhere)
> 
> Something like:
> for (auto clip_rect : rounded_clip_rects) {
>  ...
> }

Done.

https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
cc/resources/clip_display_item.cc:66: base::StringAppendF(&value, "[%f %f]",
upper_left_radius.x(),
On 2015/02/02 22:22:16, pdr wrote:
> Can you double-check that the output of these is valid json? I think we need a
> comma here. (here, and elsewhere)

Added commas.

The overall output for the list is valid json -- each item is outputting a
single string (except for DrawingDIsplayItems, which are outputting a dictionary
since they're including an SkPicture in their output), so the contents of that
string can be arbitrary. If the plan is to parse each item's string (so that
each such string should itself be valid json), then switching it to a dictionary
might be better. What works better with your tracing patch?

[pdr., 2015-02-02 23:16:15]

On 2015/02/02 at 23:02:21, ajuma wrote:
>
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> File cc/resources/clip_display_item.cc (right):
> 
>
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> cc/resources/clip_display_item.cc:59: for (size_t i = 0; i <
rounded_clip_rects_.size(); ++i) {
> On 2015/02/02 22:22:16, pdr wrote:
> > You can use a fancy c++11 iterator here to clean up the duplicated
> > rounded_clip_rects_[i] calls. (here, and elsewhere)
> > 
> > Something like:
> > for (auto clip_rect : rounded_clip_rects) {
> >  ...
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> cc/resources/clip_display_item.cc:66: base::StringAppendF(&value, "[%f %f]",
upper_left_radius.x(),
> On 2015/02/02 22:22:16, pdr wrote:
> > Can you double-check that the output of these is valid json? I think we need
a
> > comma here. (here, and elsewhere)
> 
> Added commas.
> 
> The overall output for the list is valid json -- each item is outputting a
single string (except for DrawingDIsplayItems, which are outputting a dictionary
since they're including an SkPicture in their output), so the contents of that
string can be arbitrary. If the plan is to parse each item's string (so that
each such string should itself be valid json), then switching it to a dictionary
might be better. What works better with your tracing patch?

Lets go ahead with strings for the first pass but I realized it would be very
easy to parse the json on the other side and we might want to go that way as a
followup.

[pdr., 2015-02-02 23:19:13]

On 2015/02/02 at 23:16:15, pdr wrote:
> On 2015/02/02 at 23:02:21, ajuma wrote:
> >
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > File cc/resources/clip_display_item.cc (right):
> > 
> >
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > cc/resources/clip_display_item.cc:59: for (size_t i = 0; i <
rounded_clip_rects_.size(); ++i) {
> > On 2015/02/02 22:22:16, pdr wrote:
> > > You can use a fancy c++11 iterator here to clean up the duplicated
> > > rounded_clip_rects_[i] calls. (here, and elsewhere)
> > > 
> > > Something like:
> > > for (auto clip_rect : rounded_clip_rects) {
> > >  ...
> > > }
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > cc/resources/clip_display_item.cc:66: base::StringAppendF(&value, "[%f %f]",
upper_left_radius.x(),
> > On 2015/02/02 22:22:16, pdr wrote:
> > > Can you double-check that the output of these is valid json? I think we
need a
> > > comma here. (here, and elsewhere)
> > 
> > Added commas.
> > 
> > The overall output for the list is valid json -- each item is outputting a
single string (except for DrawingDIsplayItems, which are outputting a dictionary
since they're including an SkPicture in their output), so the contents of that
string can be arbitrary. If the plan is to parse each item's string (so that
each such string should itself be valid json), then switching it to a dictionary
might be better. What works better with your tracing patch?
> 
> Lets go ahead with strings for the first pass but I realized it would be very
easy to parse the json on the other side and we might want to go that way as a
followup.

Despite the WIP label, I think this patch is good to go as-is with an updated
change description.

[ajuma, 2015-02-02 23:24:52]

On 2015/02/02 23:19:13, pdr wrote:
> On 2015/02/02 at 23:16:15, pdr wrote:
> > On 2015/02/02 at 23:02:21, ajuma wrote:
> > >
>
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > > File cc/resources/clip_display_item.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > > cc/resources/clip_display_item.cc:59: for (size_t i = 0; i <
> rounded_clip_rects_.size(); ++i) {
> > > On 2015/02/02 22:22:16, pdr wrote:
> > > > You can use a fancy c++11 iterator here to clean up the duplicated
> > > > rounded_clip_rects_[i] calls. (here, and elsewhere)
> > > > 
> > > > Something like:
> > > > for (auto clip_rect : rounded_clip_rects) {
> > > >  ...
> > > > }
> > > 
> > > Done.
> > > 
> > >
>
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > > cc/resources/clip_display_item.cc:66: base::StringAppendF(&value, "[%f
%f]",
> upper_left_radius.x(),
> > > On 2015/02/02 22:22:16, pdr wrote:
> > > > Can you double-check that the output of these is valid json? I think we
> need a
> > > > comma here. (here, and elsewhere)
> > > 
> > > Added commas.
> > > 
> > > The overall output for the list is valid json -- each item is outputting a
> single string (except for DrawingDIsplayItems, which are outputting a
dictionary
> since they're including an SkPicture in their output), so the contents of that
> string can be arbitrary. If the plan is to parse each item's string (so that
> each such string should itself be valid json), then switching it to a
dictionary
> might be better. What works better with your tracing patch?
> > 
> > Lets go ahead with strings for the first pass but I realized it would be
very
> easy to parse the json on the other side and we might want to go that way as a
> followup.
> 
> Despite the WIP label, I think this patch is good to go as-is with an updated
> change description.

Description updated.

[pdr., 2015-02-02 23:27:36]

On 2015/02/02 at 23:24:52, ajuma wrote:
> On 2015/02/02 23:19:13, pdr wrote:
> > On 2015/02/02 at 23:16:15, pdr wrote:
> > > On 2015/02/02 at 23:02:21, ajuma wrote:
> > > >
> >
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > > > File cc/resources/clip_display_item.cc (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > > > cc/resources/clip_display_item.cc:59: for (size_t i = 0; i <
> > rounded_clip_rects_.size(); ++i) {
> > > > On 2015/02/02 22:22:16, pdr wrote:
> > > > > You can use a fancy c++11 iterator here to clean up the duplicated
> > > > > rounded_clip_rects_[i] calls. (here, and elsewhere)
> > > > > 
> > > > > Something like:
> > > > > for (auto clip_rect : rounded_clip_rects) {
> > > > >  ...
> > > > > }
> > > > 
> > > > Done.
> > > > 
> > > >
> >
https://codereview.chromium.org/884473005/diff/20001/cc/resources/clip_displa...
> > > > cc/resources/clip_display_item.cc:66: base::StringAppendF(&value, "[%f
%f]",
> > upper_left_radius.x(),
> > > > On 2015/02/02 22:22:16, pdr wrote:
> > > > > Can you double-check that the output of these is valid json? I think
we
> > need a
> > > > > comma here. (here, and elsewhere)
> > > > 
> > > > Added commas.
> > > > 
> > > > The overall output for the list is valid json -- each item is outputting
a
> > single string (except for DrawingDIsplayItems, which are outputting a
dictionary
> > since they're including an SkPicture in their output), so the contents of
that
> > string can be arbitrary. If the plan is to parse each item's string (so that
> > each such string should itself be valid json), then switching it to a
dictionary
> > might be better. What works better with your tracing patch?
> > > 
> > > Lets go ahead with strings for the first pass but I realized it would be
very
> > easy to parse the json on the other side and we might want to go that way as
a
> > followup.
> > 
> > Despite the WIP label, I think this patch is good to go as-is with an
updated
> > change description.
> 
> Description updated.

LGTM.

We may need to find another reviewer as I'm not an owner in this area.

[ajuma, 2015-02-02 23:31:06]

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

[danakj, 2015-02-02 23:31:11]

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

[danakj, 2015-02-02 23:31:11]

https://codereview.chromium.org/884473005/diff/40001/cc/resources/clip_displa...
File cc/resources/clip_display_item.cc (right):

https://codereview.chromium.org/884473005/diff/40001/cc/resources/clip_displa...
cc/resources/clip_display_item.cc:59: for (SkRRect rounded_rect :
rounded_clip_rects_) {
SkRRect&?
or const SkRRect&?
or const auto&?

[danakj, 2015-02-02 23:35:08]

one more style thinger. LGTM

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

https://codereview.chromium.org/884473005/diff/40001/cc/resources/display_ite...
cc/resources/display_item_list.cc:75: for (size_t i = 0; i < items_.size(); ++i)
{
nit: use iterator instead of array index?

can use for (foo : items_) ?

[ajuma, 2015-02-02 23:35:10]

https://codereview.chromium.org/884473005/diff/40001/cc/resources/clip_displa...
File cc/resources/clip_display_item.cc (right):

https://codereview.chromium.org/884473005/diff/40001/cc/resources/clip_displa...
cc/resources/clip_display_item.cc:59: for (SkRRect rounded_rect :
rounded_clip_rects_) {
On 2015/02/02 23:31:11, danakj wrote:
> SkRRect&?
> or const SkRRect&?
> or const auto&?

Changed to const SkRRect&.

[ajuma, 2015-02-02 23:41:53]

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

https://codereview.chromium.org/884473005/diff/40001/cc/resources/display_ite...
cc/resources/display_item_list.cc:75: for (size_t i = 0; i < items_.size(); ++i)
{
On 2015/02/02 23:35:08, danakj wrote:
> nit: use iterator instead of array index?
> 
> can use for (foo : items_) ?

Done.

[ajuma, 2015-02-02 23:42:07]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2015-02-02 23:42:32]

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

[vmpstr, 2015-02-02 23:49:31]

This is fine as is if it's just for display purposes. However, if we plan to do
anything with this in the frame viewer or something like that, it would be a lot
easier if everything is actual json structures (instead of a string "[5, 10]",
it's an array [5, 10], etc). Is there a plan to more jsonify this or is this
just for display as text?

[ajuma, 2015-02-02 23:57:57]

On 2015/02/02 23:49:31, vmpstr wrote:
> This is fine as is if it's just for display purposes. However, if we plan to
do
> anything with this in the frame viewer or something like that, it would be a
lot
> easier if everything is actual json structures (instead of a string "[5, 10]",
> it's an array [5, 10], etc). Is there a plan to more jsonify this or is this
> just for display as text?

Initially, it's just going to be displayed as text (pdr@ is working on wiring
this up on the trace viewer side), but we'll jsonify this once we want to add
fancier output.

[commit-bot: I haz the power, 2015-02-03 01:41:48]

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

[commit-bot: I haz the power, 2015-02-03 01:41:50]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (None)

[ajuma, 2015-02-03 01:57:19]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2015-02-03 01:59:06]

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

[commit-bot: I haz the power, 2015-02-03 03:03:00]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-02-03 03:05:11]

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