Add protobuf serialization to DisplayItemList

Add protobuf serialization to DisplayItemList

Serialize the following classes:
- DisplayItemListSettings
- DisplayItemList
- DisplayItem and all subclasses
  - Did not fully serialize FilterDisplayItem.  Serializing
    FilterOperations will require more work.  Will do in a follow up CL.

Added unit tests for all DisplayItems serialized.

BUG=541321
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2191e4d3cb6b574fd532547a8f93e02f9168a706
Cr-Commit-Position: refs/heads/master@{#356563}

[David Trainor- moved to gerrit, 2015-10-14 22:33:38]

dtrainor@chromium.org changed reviewers:
+ danakj@chromium.org, vmpstr@chromium.org

[David Trainor- moved to gerrit, 2015-10-14 22:33:38]

ptal... most is in display_item_list_unittests.cc and display_item.proto I
swear! :(

[David Trainor- moved to gerrit, 2015-10-14 23:09:28]

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

https://codereview.chromium.org/1407793002/diff/20001/cc/playback/clip_displa...
cc/playback/clip_display_item.cc:43: DCHECK_EQ(0,
details->rounded_rects_size());
TODO: details->clear_rounded_rects()?

https://codereview.chromium.org/1407793002/diff/20001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1407793002/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:124: // Build the DrawingDisplayItem.
TODO(me): Pull out the common code for these unit tests?  Should be able to
generate the two DrawingDisplayItems with just the DisplayItemList:

void AppendFirstTestPicture(scoped_refptr<DisplayItemList> list);
void AppendSecondTestPicture(scoped_refptr<DisplayItemList> list);

?

[David Trainor- moved to gerrit, 2015-10-20 20:26:49]

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

https://codereview.chromium.org/1407793002/diff/20001/cc/playback/clip_displa...
cc/playback/clip_display_item.cc:43: DCHECK_EQ(0,
details->rounded_rects_size());
On 2015/10/14 23:09:28, David Trainor wrote:
> TODO: details->clear_rounded_rects()?

Done.

https://codereview.chromium.org/1407793002/diff/20001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1407793002/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:124: // Build the DrawingDisplayItem.
On 2015/10/14 23:09:28, David Trainor wrote:
> TODO(me): Pull out the common code for these unit tests?  Should be able to
> generate the two DrawingDisplayItems with just the DisplayItemList:
> 
> void AppendFirstTestPicture(scoped_refptr<DisplayItemList> list);
> void AppendSecondTestPicture(scoped_refptr<DisplayItemList> list);
> 
> ?

Done.

[David Trainor- moved to gerrit, 2015-10-26 16:04:35]

adding kmarshall@ and wez@ to cc for FYI, but feel free to review if you'd like.

[vmpstr, 2015-10-26 17:46:17]

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
File cc/playback/clip_display_item.cc (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_display_item.cc:43: details->clear_rounded_rects();
dcheck that the size is 0 instead

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_display_item.cc:44: for (auto& rrect : rounded_clip_rects_) {
const auto&

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
File cc/playback/clip_path_display_item.cc (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_path_display_item.cc:46: details->set_clip_path(buffer.get(),
path_size);
What's the parameter to set_clip_path? Is this a copy? It's weird to just pass a
raw ptr to a scoped_ptr thing that is deleted immediately.

Can you make it std::string(buffer.get()) to make it explicit?

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_path_display_item.cc:59:
clip_path.readFromMemory(details.clip_path().c_str(),
Does readFromMemory return some sort of a status code? If so, can you dcheck
that it succeeded?

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
File cc/playback/display_item.h (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
cc/playback/display_item.h:35: virtual void ToProtobuf(proto::DisplayItem*
proto) = 0;
Make this const, please.

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
File cc/playback/display_item_list.cc (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
cc/playback/display_item_list.cc:65: item->FromProtobuf(item_proto);
Maybe FromProtobuf functionality can be a part of the "Construct" aspect of
AllocateAndConstruct? (ie be inside that call)

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
File cc/playback/display_item_list.h (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
cc/playback/display_item_list.h:44: static scoped_refptr<DisplayItemList>
Create(
CreateFromProto

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
File cc/playback/display_item_proto_factory.h (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
cc/playback/display_item_proto_factory.h:19: class DisplayItemProtoFactory {
I wonder if it's possible to make this patch more similar to how
https://codereview.chromium.org/1398443008/ is.. Something like having a
DisplayItemProtoConverter and have most of the functionality that deals with
general structure of the display item list in there. Wdyt?

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/proto/display...
File cc/proto/display_item.proto (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/proto/display...
cc/proto/display_item.proto:48: optional ClipDisplayItem details_clip = 1000;
Why does this start from 1000?

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/proto/display...
cc/proto/display_item.proto:52: optional FilterDisplayItem details_filter =
1004;
maybe instead of details_foo, it can be foo_item?

[David Trainor- moved to gerrit, 2015-10-26 20:42:56]

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
File cc/playback/clip_display_item.cc (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_display_item.cc:43: details->clear_rounded_rects();
On 2015/10/26 17:46:17, vmpstr wrote:
> dcheck that the size is 0 instead

Done.

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_display_item.cc:44: for (auto& rrect : rounded_clip_rects_) {
On 2015/10/26 17:46:17, vmpstr wrote:
> const auto&

Done.

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
File cc/playback/clip_path_display_item.cc (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_path_display_item.cc:46: details->set_clip_path(buffer.get(),
path_size);
On 2015/10/26 17:46:17, vmpstr wrote:
> What's the parameter to set_clip_path? Is this a copy? It's weird to just pass
a
> raw ptr to a scoped_ptr thing that is deleted immediately.
> 
> Can you make it std::string(buffer.get()) to make it explicit?

I believe protobufs always copy the data passed in so they're self-contained
objects.  I can cast it to a string if you'd prefer that though.  It'll just do
the copy to the string constructor then ref the data internally in the protobuf.

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/clip...
cc/playback/clip_path_display_item.cc:59:
clip_path.readFromMemory(details.clip_path().c_str(),
On 2015/10/26 17:46:17, vmpstr wrote:
> Does readFromMemory return some sort of a status code? If so, can you dcheck
> that it succeeded?

It returns the bytes read to build the SkPath.  I can DCHECK that it's the same
as the size I'm passing in.

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
File cc/playback/display_item.h (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
cc/playback/display_item.h:35: virtual void ToProtobuf(proto::DisplayItem*
proto) = 0;
On 2015/10/26 17:46:17, vmpstr wrote:
> Make this const, please.

Sure.  This might change in the future if we start pushing deltas, but I think
we can abstract that above the level of each individual DisplayItem, so maybe it
won't :).

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
File cc/playback/display_item_list.cc (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
cc/playback/display_item_list.cc:65: item->FromProtobuf(item_proto);
On 2015/10/26 17:46:17, vmpstr wrote:
> Maybe FromProtobuf functionality can be a part of the "Construct" aspect of
> AllocateAndConstruct? (ie be inside that call)

I was trying to hide the subclasses from this file, but otherwise keep all
serialization/deserialization here.  Let me know if you feel strongly about this
and I'll move it.

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
File cc/playback/display_item_list.h (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/playback/disp...
cc/playback/display_item_list.h:44: static scoped_refptr<DisplayItemList>
Create(
On 2015/10/26 17:46:17, vmpstr wrote:
> CreateFromProto

Done.

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/proto/display...
File cc/proto/display_item.proto (right):

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/proto/display...
cc/proto/display_item.proto:48: optional ClipDisplayItem details_clip = 1000;
On 2015/10/26 17:46:17, vmpstr wrote:
> Why does this start from 1000?

It's a little easier to keep the number spaces fairly separate for specific type
of message data (ie: if we add more 'base class' fields, they'll just get = 2, =
3, etc.).

https://chromiumcodereview.appspot.com/1407793002/diff/80001/cc/proto/display...
cc/proto/display_item.proto:52: optional FilterDisplayItem details_filter =
1004;
On 2015/10/26 17:46:17, vmpstr wrote:
> maybe instead of details_foo, it can be foo_item?

Done.

[vmpstr, 2015-10-27 21:49:47]

Looks good, just a few comments.

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

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:55: gfx::Rect layer_rect =
ProtoToRect(proto.layer_rect());
As a separate thing, can you file a bug and/or a todo to put ProtoToRect and
friends into some class and make them static (or put them in some namespace).
I'd prefer to see where this function is coming from more or less... like for
example we have MathUtil and there we have like MathUtil::Deg2Rad, etc.. it's a
bit clearer for me.

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:58: DisplayItemListSettings(proto.settings()));
There's some work to remove the cached_picture path, which would make the
display item list settings empty. Would it be a straight-forward patch to remove
it?

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:99: // TODO(dtrainor): Send only changed state.
Can you file a bug and put a reference to it here?

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
File cc/playback/display_item_proto_factory.cc (right):

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_proto_factory.cc:49: default:
Don't put the default case, let the compiler emit the error (and that also means
you don't have to handle the null case). If the compiler complains, then at the
end of the function we usually do NOTREACHED(); return nullptr; or something
like that.

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/drawing_di...
File cc/playback/drawing_display_item.cc (right):

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/drawing_di...
cc/playback/drawing_display_item.cc:45: // TODO(dtrainor, nyquist): Add an
SkPixelSerializer to not serialize images
Bug number?

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/filter_dis...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/filter_dis...
cc/playback/filter_display_item.cc:45: // TODO(dtrainor): Support serializing
FilterOperations.
Bug number please :)

[David Trainor- moved to gerrit, 2015-10-27 22:49:24]

filed the bugs :).

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

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:55: gfx::Rect layer_rect =
ProtoToRect(proto.layer_rect());
On 2015/10/27 21:49:47, vmpstr wrote:
> As a separate thing, can you file a bug and/or a todo to put ProtoToRect and
> friends into some class and make them static (or put them in some namespace).
> I'd prefer to see where this function is coming from more or less... like for
> example we have MathUtil and there we have like MathUtil::Deg2Rad, etc.. it's
a
> bit clearer for me.

Yeah I can do that.  I was trying to be consistent with the existing gfx helper
methods (rect_conversions, point_conversions).  When I fix the bug not allowing
me to put proto files in different directories I plan on migrating all of these
to their rightful places too (gfx:: or skia::).

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:58: DisplayItemListSettings(proto.settings()));
On 2015/10/27 21:49:47, vmpstr wrote:
> There's some work to remove the cached_picture path, which would make the
> display item list settings empty. Would it be a straight-forward patch to
remove
> it?

Yeah it would just be deleting the .proto file when we delete the
display_item_list_settings.cc/.h files (and the unit test/reference in the
display_item_list message).

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:99: // TODO(dtrainor): Send only changed state.
On 2015/10/27 21:49:47, vmpstr wrote:
> Can you file a bug and put a reference to it here?

Removed TODO.  Put bug number in the header file TODO which basically says the
same thing.

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
File cc/playback/display_item_proto_factory.cc (right):

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/display_it...
cc/playback/display_item_proto_factory.cc:49: default:
On 2015/10/27 21:49:47, vmpstr wrote:
> Don't put the default case, let the compiler emit the error (and that also
means
> you don't have to handle the null case). If the compiler complains, then at
the
> end of the function we usually do NOTREACHED(); return nullptr; or something
> like that.

Ah yeah sorry I forgot to fix this one.  Good point :).

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/drawing_di...
File cc/playback/drawing_display_item.cc (right):

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/drawing_di...
cc/playback/drawing_display_item.cc:45: // TODO(dtrainor, nyquist): Add an
SkPixelSerializer to not serialize images
On 2015/10/27 21:49:47, vmpstr wrote:
> Bug number?

Done.

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/filter_dis...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/1407793002/diff/100001/cc/playback/filter_dis...
cc/playback/filter_display_item.cc:45: // TODO(dtrainor): Support serializing
FilterOperations.
On 2015/10/27 21:49:47, vmpstr wrote:
> Bug number please :)

Done.

[vmpstr, 2015-10-27 23:03:01]

https://codereview.chromium.org/1407793002/diff/120001/cc/playback/display_it...
File cc/playback/display_item_proto_factory.cc (right):

https://codereview.chromium.org/1407793002/diff/120001/cc/playback/display_it...
cc/playback/display_item_proto_factory.cc:49: default:
<_<

[David Trainor- moved to gerrit, 2015-10-27 23:09:56]

https://codereview.chromium.org/1407793002/diff/120001/cc/playback/display_it...
File cc/playback/display_item_proto_factory.cc (right):

https://codereview.chromium.org/1407793002/diff/120001/cc/playback/display_it...
cc/playback/display_item_proto_factory.cc:49: default:
On 2015/10/27 23:03:01, vmpstr wrote:
> <_<

 (╯°□°）╯︵ ┻━┻

[vmpstr, 2015-10-27 23:35:24]

lgtm

[David Trainor- moved to gerrit, 2015-10-28 14:51:59]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2015-10-28 14:52:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407793002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407793002/140001

[commit-bot: I haz the power, 2015-10-28 16:02:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-28 16:03:45]

Patchset 8 (id:??) landed as
https://crrev.com/2191e4d3cb6b574fd532547a8f93e02f9168a706
Cr-Commit-Position: refs/heads/master@{#356563}