Add support for (de)serializing cc::Layer properties.

Add support for (de)serializing cc::Layer properties.

This CL adds a protocol buffer for serializing the Layer properties,
without any properties being serialized yet, except
needs_push_properties and num_dependents_need_push_properties.
It depends on the hierarchy already being up to date when the property
deserialization happens.

When properties are changed, only nodes with changed properties will be
serialized with their properties, and ascendants of such Layers will
be serialized for the two metadata properties mentioned above.

The base Layer class and all descendant classes will have their own
proto messages for the properties they require. The content of these
messages will be added in later CLs, but the base Layer class framework
and proto message is added in this CL to make it clear how this will
look like going forward.

Depends on https://codereview.chromium.org/1398443008/ for serialization
of the hierarchy.

BUG=538710
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[nyquist, 2015-10-21 18:17:08]

Description was changed from

==========
Added Layer properties (de)serialization

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

to

==========
Added Layer properties (de)serialization

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

[nyquist, 2015-10-21 18:17:08]

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

[nyquist, 2015-10-21 18:47:35]

Description was changed from

==========
Add support for (de)serializing cc::Layer properties.

This CL adds a protocol buffer for serializing the Layer properties,
without any properties being serialized yet, except
needs_push_properties and num_dependents_need_push_properties.
It depends on the hierarchy already being up to date when the property
deserialization happens.

When properties are changed, only nodes with changed properties will be
serialized with their properties, and ascendants of such Layers will
be serialized for the two metadata properties mentioned above.

Depends on https://codereview.chromium.org/1398443008/ for serialization
of the hierarchy.

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

to

==========
Add support for (de)serializing cc::Layer properties.

This CL adds a protocol buffer for serializing the Layer properties,
without any properties being serialized yet, except
needs_push_properties and num_dependents_need_push_properties.
It depends on the hierarchy already being up to date when the property
deserialization happens.

When properties are changed, only nodes with changed properties will be
serialized with their properties, and ascendants of such Layers will
be serialized for the two metadata properties mentioned above.

Depends on https://codereview.chromium.org/1398443008/ for serialization
of the hierarchy.

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

[nyquist, 2015-10-21 18:47:35]

nyquist@chromium.org changed reviewers:
- dtrainor@chromium.org

[nyquist, 2015-10-21 18:47:46]

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

[nyquist, 2015-10-21 21:02:13]

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

[commit-bot: I haz the power, 2015-10-21 21:04:43]

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

[commit-bot: I haz the power, 2015-10-21 21:21:00]

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

[commit-bot: I haz the power, 2015-10-21 21:21:01]

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/7...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[nyquist, 2015-11-07 00:11:17]

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

[commit-bot: I haz the power, 2015-11-07 00:13:21]

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

[commit-bot: I haz the power, 2015-11-07 01:21:18]

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

[commit-bot: I haz the power, 2015-11-07 01:21:18]

Dry run: This issue passed the CQ dry run.

[nyquist, 2015-11-07 14:17:34]

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

[commit-bot: I haz the power, 2015-11-07 14:17:41]

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

[nyquist, 2015-11-07 14:22:53]

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

[commit-bot: I haz the power, 2015-11-07 14:22:59]

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

[commit-bot: I haz the power, 2015-11-07 15:30:52]

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

[commit-bot: I haz the power, 2015-11-07 15:30:52]

Dry run: This issue passed the CQ dry run.

[nyquist, 2015-11-07 15:56:53]

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

[nyquist, 2015-11-07 15:56:54]

vmpstr: PTAL as cc/ OWNERS
dtrainor: PTAL on behalf of blimp

[nyquist, 2015-11-10 01:53:39]

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

[commit-bot: I haz the power, 2015-11-10 01:56:38]

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

[commit-bot: I haz the power, 2015-11-10 04:03:22]

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

[commit-bot: I haz the power, 2015-11-10 04:03:23]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2015-11-11 15:34:23]

lgtm!

https://chromiumcodereview.appspot.com/1423523002/diff/160001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://chromiumcodereview.appspot.com/1423523002/diff/160001/cc/layers/layer...
cc/layers/layer.h:616: // Deerialize all the necessary properties from
proto::LayerProperties into
Deerialize -> Deserialize

https://chromiumcodereview.appspot.com/1423523002/diff/160001/cc/layers/layer...
File cc/layers/layer_unittest.cc (right):

https://chromiumcodereview.appspot.com/1423523002/diff/160001/cc/layers/layer...
cc/layers/layer_unittest.cc:1574: a*   b*+[mask:*]
[mask:*,replica]?

https://chromiumcodereview.appspot.com/1423523002/diff/160001/cc/layers/layer...
cc/layers/layer_unittest.cc:1577: Layers marked with * have changed properties.
Add the same description to the layer_proto_converter_unittest file.

[nyquist, 2015-11-11 18:31:57]

Done with all comments from dtrainor.

vmpstr: PTAL. Should be ready for review :-)

https://codereview.chromium.org/1423523002/diff/160001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1423523002/diff/160001/cc/layers/layer.h#newc...
cc/layers/layer.h:616: // Deerialize all the necessary properties from
proto::LayerProperties into
On 2015/11/11 15:34:22, David Trainor wrote:
> Deerialize -> Deserialize

Done.

https://codereview.chromium.org/1423523002/diff/160001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/1423523002/diff/160001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:1574: a*   b*+[mask:*]
On 2015/11/11 15:34:22, David Trainor wrote:
> [mask:*,replica]?

Good idea! Done.

https://codereview.chromium.org/1423523002/diff/160001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:1577: Layers marked with * have changed properties.
On 2015/11/11 15:34:22, David Trainor wrote:
> Add the same description to the layer_proto_converter_unittest file.

Done (with the values from that test of course).

[nyquist, 2015-11-17 19:18:45]

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

[commit-bot: I haz the power, 2015-11-17 19:19:35]

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

[commit-bot: I haz the power, 2015-11-17 20:25:46]

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

[commit-bot: I haz the power, 2015-11-17 20:25:47]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2015-11-17 21:05:44]

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1432: if (needs_push_properties_)
Is this to reduce the size of the proto? That is, we have skeleton type of nodes
when needs_push_properties_ is false, just so we can have the
num_dependents_need_push_properties_?

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1449: void Layer::FromLayerPropertiesProto(const
proto::LayerProperties& proto) {
It's a little bit confusing that ToLayerPropertiesProto serializes the
properties of this layer, and the properties of the children. 

However, the reverse function FromLayerPropertiesProto only deserializes the
properties of this layer. Where are the properties of children deserialized?

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter.cc (right):

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.cc:45: void
LayerProtoConverter::SerializeLayerProperties(
I think these should be more symmetrical wrt to children serialization.

[nyquist, 2015-11-18 00:24:34]

vmpstr: PTAL

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1432: if (needs_push_properties_)
On 2015/11/17 21:05:44, vmpstr wrote:
> Is this to reduce the size of the proto? That is, we have skeleton type of
nodes
> when needs_push_properties_ is false, just so we can have the
> num_dependents_need_push_properties_?

Yes. I do think that we could optimize this in the future though, at the cost of
some complexity.

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1449: void Layer::FromLayerPropertiesProto(const
proto::LayerProperties& proto) {
On 2015/11/17 21:05:44, vmpstr wrote:
> It's a little bit confusing that ToLayerPropertiesProto serializes the
> properties of this layer, and the properties of the children. 
> 
> However, the reverse function FromLayerPropertiesProto only deserializes the
> properties of this layer. Where are the properties of children deserialized?

Yeah, that's in LayerProtoConverter. I've made them symmetric now.

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter.cc (right):

https://codereview.chromium.org/1423523002/diff/180001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.cc:45: void
LayerProtoConverter::SerializeLayerProperties(
On 2015/11/17 21:05:44, vmpstr wrote:
> I think these should be more symmetrical wrt to children serialization.

Done.

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter_unittest.cc (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter_unittest.cc:141:
LayerProtoConverter::SerializeLayerProperties(layer_src_root, &layer_update);
This test is literally the same as LayerTest::RecursivePropertiesSerialization
from patch set 10, except the following change to this line:

-  layer_src_root->ToLayerPropertiesProto(&layer_update);
+  LayerProtoConverter::SerializeLayerProperties(layer_src_root, &layer_update);

[vmpstr, 2015-11-18 04:05:25]

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1436: num_dependents_need_push_properties_ = 0;
nit: you can just save a bool to return before clearing it. I think it looks
better than accessing the proto file that you just created.

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter.cc (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.cc:53: scoped_refptr<Layer> existing_root,
Can you pass a raw pointer here instead?

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.cc:74: if (!serialize_descendants)
Are mask/replica considered descendants? That is, do you need to serialize
mask/replica even if this returns false?

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter.h (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.h:62: const scoped_refptr<Layer> root_layer,
nit: const scoped_refptr<Layer>& root_layer

Or better yet, can you pass a non-owning raw pointer here? Just Layer*
root_layer?

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter_unittest.cc (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter_unittest.cc:110: TEST(LayerProtoConverterTest,
RecursivePropertiesSerialization) {
Can you add a test for

root+ ----- b* [mask:*] ---- c

to ensure that you get "b" and "mask"

[nyquist, 2015-11-18 07:15:14]

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

[commit-bot: I haz the power, 2015-11-18 07:15:53]

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

[nyquist, 2015-11-18 07:15:59]

vmpstr: PTAL

I couldn't find a good name for the extra test you wanted me to add. Feel free
to suggest a better one!

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1436: num_dependents_need_push_properties_ = 0;
On 2015/11/18 04:05:25, vmpstr wrote:
> nit: you can just save a bool to return before clearing it. I think it looks
> better than accessing the proto file that you just created.

Done.

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter.cc (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.cc:53: scoped_refptr<Layer> existing_root,
On 2015/11/18 04:05:25, vmpstr wrote:
> Can you pass a raw pointer here instead?

Done.

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.cc:74: if (!serialize_descendants)
On 2015/11/18 04:05:25, vmpstr wrote:
> Are mask/replica considered descendants? That is, do you need to serialize
> mask/replica even if this returns false?

According to Layer::SetMaskLayer(...) (
https://chromium.googlesource.com/chromium/src/+/cc1c0830a3ad08c3e1c346fdf23a...
) and Layer::SetReplicaLayer(...) (
https://chromium.googlesource.com/chromium/src/+/cc1c0830a3ad08c3e1c346fdf23a...
) both the mask and replica layer gets the current layer as their parent, which
from what I can tell means that SetNeedsPushProperties will traverse up the
tree.

So, if I've understood that correctly, there's not really a difference from mask
and replica layers versus normal children, and they will all need to be looked
at if num_dependents_need_push_properties_ > 0.

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter.h (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter.h:62: const scoped_refptr<Layer> root_layer,
On 2015/11/18 04:05:25, vmpstr wrote:
> nit: const scoped_refptr<Layer>& root_layer
> 
> Or better yet, can you pass a non-owning raw pointer here? Just Layer*
> root_layer?

Done.

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
File cc/layers/layer_proto_converter_unittest.cc (right):

https://codereview.chromium.org/1423523002/diff/220001/cc/layers/layer_proto_...
cc/layers/layer_proto_converter_unittest.cc:110: TEST(LayerProtoConverterTest,
RecursivePropertiesSerialization) {
On 2015/11/18 04:05:25, vmpstr wrote:
> Can you add a test for
> 
> root+ ----- b* [mask:*] ---- c
> 
> to ensure that you get "b" and "mask" 

Yeah, but from what I understand, for the b-node, this will be:
b*+[mask:*] (note the added +)
since mask is considered a descendant of b.

[vmpstr, 2015-11-18 07:33:52]

lgtm

[commit-bot: I haz the power, 2015-11-18 08:26:35]

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

[commit-bot: I haz the power, 2015-11-18 08:26:36]

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

[nyquist, 2015-11-18 15:05:02]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2015-11-18 15:05:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1423523002/#ps240001
(title: "Addressed comments about raw pointers and new test.")

[commit-bot: I haz the power, 2015-11-18 15:06:34]

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

[commit-bot: I haz the power, 2015-11-18 16:10:03]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-11-18 16:10:54]

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