cc : Make tree synchronization independent of layer tree hierarchy (2)

cc : Make tree synchronization independent of layer tree hierarchy (2)

This CL :
  * Stores layers that need to push properties in LayerTreeHost(layer_set)
  * Deletes bools needs_push_properties and dependants_needs_push_properties
    from Layer.
  * Iterate the layer_set tp push properties during commit.
  * Changes what layers are serialized (Both dirty layers and their ancestors
    are serialized currently. With this CL, only dirty layers serialize).

BUG=568874
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9234e405c7089c47ca9d30b34846f54b1fd9b8fd
Cr-Commit-Position: refs/heads/master@{#382379}

[jaydasika, 2016-03-17 23:24:42]

Description was changed from

==========
cc : Make tree synchronization independent of layer tree hierarchy (2)

This CL :
  * Stores layers that need to push properties in LayerTreeHost(layer_set)
  * Deletes bools needs_push_properties and dependants_needs_push_properties
    from Layer.
  * Iterate the layer_set tp push properties during commit.
  * Changes what layers are serialized (Both dirty layers and their ancestors
    are serialized currently. With this CL, only dirty layers serialize).

BUG=568874
==========

to

==========
cc : Make tree synchronization independent of layer tree hierarchy (2)

This CL :
  * Stores layers that need to push properties in LayerTreeHost(layer_set)
  * Deletes bools needs_push_properties and dependants_needs_push_properties
    from Layer.
  * Iterate the layer_set tp push properties during commit.
  * Changes what layers are serialized (Both dirty layers and their ancestors
    are serialized currently. With this CL, only dirty layers serialize).

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

[jaydasika, 2016-03-17 23:29:19]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, enne@chromium.org, vollick@chromium.org,
weiliangc@chromium.org

[jaydasika, 2016-03-17 23:38:37]

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer.cc#newcode151
cc/layers/layer.cc:151: layer_tree_host_ = host;
This is to ensure that layer is added to right set when we invalidate property
tree indices.

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
File cc/layers/layer_proto_converter_unittest.cc (left):

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
cc/layers/layer_proto_converter_unittest.cc:353: /* Testing deserialization of
properties for a tree that looks like this:
this test changes needs_push_properties directly on the proto, so removed it.

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_unittest.cc
File cc/layers/layer_unittest.cc (left):

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_unittest.cc...
cc/layers/layer_unittest.cc:2512: TEST_F(LayerTest,
SimplePropertiesSerialization) {
Deleted this as what gets serialized is no longer decided by
ToLayerPropertiesProto

[jaydasika, 2016-03-17 23:39:33]

+dtrainor : This CL changes what layers get serialized.

[ajuma, 2016-03-18 14:33:16]

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
File cc/layers/layer_proto_converter.h (right):

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
cc/layers/layer_proto_converter.h:40: // that are dirty will contain the list of
dirty properties.
Since the update only contains dirty nodes now, all nodes will have a list of
dirty properties, right?

Also, this comment should mention that the list of layers needing push
properties is cleared out by this method.

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
File cc/layers/layer_proto_converter_unittest.cc (left):

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
cc/layers/layer_proto_converter_unittest.cc:353: /* Testing deserialization of
properties for a tree that looks like this:
On 2016/03/17 23:38:37, jaydasika wrote:
> this test changes needs_push_properties directly on the proto, so removed it.

Is there an equivalent test somewhere that after deserializing layers that need
push properties, these layers really do get added to their LayerTreeHost's set
of layers needing push properties?

https://codereview.chromium.org/1808373002/diff/1/cc/test/fake_layer_tree_hos...
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/1808373002/diff/1/cc/test/fake_layer_tree_hos...
cc/test/fake_layer_tree_host.cc:124: layer->PushPropertiesTo(impl_layer);
How about putting this in a method in TreeSynchronizer so that it doesn't have
to be duplicated here and in LayerTreeHost (and so that this test code tests the
real push properties logic)? This also looks the same as the logic for pushing
properties between LayerTreeImpls, so perhaps (with templating) we can avoid
duplication there too.

https://codereview.chromium.org/1808373002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1808373002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1297: RemoveLayerShouldPushProperties(layer);
Do we have a test that removes a layer needing push properties from its host,
verifying that this layer's id is no longer in that host's set of layers needing
push properties?

https://codereview.chromium.org/1808373002/diff/1/cc/trees/tree_synchronizer.cc
File cc/trees/tree_synchronizer.cc (left):

https://codereview.chromium.org/1808373002/diff/1/cc/trees/tree_synchronizer....
cc/trees/tree_synchronizer.cc:181: static void
CheckScrollAndClipPointersRecursive(Layer* layer,
This checks are pretty useful. It'd be nice to keep them in some form (e.g.
using LayerListIterator instead of an explicit recursive tree walk).

[jaydasika, 2016-03-18 18:25:05]

Patchset #2 (id:20001) has been deleted

[jaydasika, 2016-03-18 21:08:28]

Patchset #3 (id:60001) has been deleted

[jaydasika, 2016-03-18 22:02:59]

Patchset #3 (id:80001) has been deleted

[jaydasika, 2016-03-18 22:03:09]

Patchset #2 (id:40001) has been deleted

[jaydasika, 2016-03-19 02:17:58]

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
File cc/layers/layer_proto_converter.h (right):

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
cc/layers/layer_proto_converter.h:40: // that are dirty will contain the list of
dirty properties.
On 2016/03/18 14:33:15, ajuma wrote:
> Since the update only contains dirty nodes now, all nodes will have a list of
> dirty properties, right?
> 
> Also, this comment should mention that the list of layers needing push
> properties is cleared out by this method.

Done.

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
File cc/layers/layer_proto_converter_unittest.cc (left):

https://codereview.chromium.org/1808373002/diff/1/cc/layers/layer_proto_conve...
cc/layers/layer_proto_converter_unittest.cc:353: /* Testing deserialization of
properties for a tree that looks like this:
On 2016/03/18 14:33:15, ajuma wrote:
> On 2016/03/17 23:38:37, jaydasika wrote:
> > this test changes needs_push_properties directly on the proto, so removed
it.
> 
> Is there an equivalent test somewhere that after deserializing layers that
need
> push properties, these layers really do get added to their LayerTreeHost's set
> of layers needing push properties?

Added one.

https://codereview.chromium.org/1808373002/diff/1/cc/test/fake_layer_tree_hos...
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/1808373002/diff/1/cc/test/fake_layer_tree_hos...
cc/test/fake_layer_tree_host.cc:124: layer->PushPropertiesTo(impl_layer);
On 2016/03/18 14:33:15, ajuma wrote:
> How about putting this in a method in TreeSynchronizer so that it doesn't have
> to be duplicated here and in LayerTreeHost (and so that this test code tests
the
> real push properties logic)? This also looks the same as the logic for pushing
> properties between LayerTreeImpls, so perhaps (with templating) we can avoid
> duplication there too.

Done.

https://codereview.chromium.org/1808373002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1808373002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1297: RemoveLayerShouldPushProperties(layer);
On 2016/03/18 14:33:16, ajuma wrote:
> Do we have a test that removes a layer needing push properties from its host,
> verifying that this layer's id is no longer in that host's set of layers
needing
> push properties?

There are many tests that check this implicitly. More than 40 tests were
crashing when InvalidatePropertyTrees in layer.cc was trying to add the layer
back to the old layer tree host's set.

https://codereview.chromium.org/1808373002/diff/1/cc/trees/tree_synchronizer.cc
File cc/trees/tree_synchronizer.cc (left):

https://codereview.chromium.org/1808373002/diff/1/cc/trees/tree_synchronizer....
cc/trees/tree_synchronizer.cc:181: static void
CheckScrollAndClipPointersRecursive(Layer* layer,
On 2016/03/18 14:33:16, ajuma wrote:
> This checks are pretty useful. It'd be nice to keep them in some form (e.g.
> using LayerListIterator instead of an explicit recursive tree walk).

Done.

[ajuma, 2016-03-21 13:41:20]

Just a couple more things.

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_serialization.cc (right):

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_serialization.cc:53:
std::unordered_set<Layer*> layers_that_should_push_properties_src_ =
Nit: local variable name shouldn't end with an underscore

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/tree_synchron...
File cc/trees/tree_synchronizer_unittest.cc (right):

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/tree_synchron...
cc/trees/tree_synchronizer_unittest.cc:168: void
PushLayerPropertiesTo(LayerTreeHost* tree_host, LayerTreeImpl* tree_impl) {
Is this still needed now that we have a method in TreeSyncrhonizer with the same
logic?

[jaydasika, 2016-03-21 17:32:03]

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_serialization.cc (right):

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_serialization.cc:53:
std::unordered_set<Layer*> layers_that_should_push_properties_src_ =
On 2016/03/21 13:41:20, ajuma wrote:
> Nit: local variable name shouldn't end with an underscore

Done.

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/tree_synchron...
File cc/trees/tree_synchronizer_unittest.cc (right):

https://codereview.chromium.org/1808373002/diff/160001/cc/trees/tree_synchron...
cc/trees/tree_synchronizer_unittest.cc:168: void
PushLayerPropertiesTo(LayerTreeHost* tree_host, LayerTreeImpl* tree_impl) {
On 2016/03/21 13:41:20, ajuma wrote:
> Is this still needed now that we have a method in TreeSyncrhonizer with the
same
> logic?

Removed.

[ajuma, 2016-03-21 17:35:56]

Thanks, lgtm.

[jaydasika, 2016-03-21 17:56:50]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-03-21 17:57:12]

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

[enne (OOO), 2016-03-21 18:10:16]

lgtm % ajuma

Exciting!

[commit-bot: I haz the power, 2016-03-21 19:40:11]

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

[commit-bot: I haz the power, 2016-03-21 19:40:13]

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

[jaydasika, 2016-03-21 20:37:50]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-03-21 20:38:15]

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

[commit-bot: I haz the power, 2016-03-21 20:44:30]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-03-21 20:45:29]

Description was changed from

==========
cc : Make tree synchronization independent of layer tree hierarchy (2)

This CL :
  * Stores layers that need to push properties in LayerTreeHost(layer_set)
  * Deletes bools needs_push_properties and dependants_needs_push_properties
    from Layer.
  * Iterate the layer_set tp push properties during commit.
  * Changes what layers are serialized (Both dirty layers and their ancestors
    are serialized currently. With this CL, only dirty layers serialize).

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

to

==========
cc : Make tree synchronization independent of layer tree hierarchy (2)

This CL :
  * Stores layers that need to push properties in LayerTreeHost(layer_set)
  * Deletes bools needs_push_properties and dependants_needs_push_properties
    from Layer.
  * Iterate the layer_set tp push properties during commit.
  * Changes what layers are serialized (Both dirty layers and their ancestors
    are serialized currently. With this CL, only dirty layers serialize).

BUG=568874
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9234e405c7089c47ca9d30b34846f54b1fd9b8fd
Cr-Commit-Position: refs/heads/master@{#382379}
==========

[commit-bot: I haz the power, 2016-03-21 20:45:30]

Patchset 6 (id:??) landed as
https://crrev.com/9234e405c7089c47ca9d30b34846f54b1fd9b8fd
Cr-Commit-Position: refs/heads/master@{#382379}