Enable more layer_tree_host_unittest for LayerTreeHostRemote.

Enable more cc unit tests for LayerTreeHostRemote test.

We have added test classes for LayerTreeHostRemote for Blimp and would
like to reuse cc unit tests to test the following flow:

Engine cc ==> Client cc main thread ==> Client cc impl thread.

This CL goes through about 50 unit test cases in
layer_tree_host_unittest.cc.

BUG=653371
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/3c9b4cc4fe631cec765af7dca9ce4c4c2b7e064d
Cr-Commit-Position: refs/heads/master@{#429396}

[xingliu, 2016-10-27 17:35:12]

Description was changed from

==========
Enable more layer_tree_host_unittest for LayerTreeHostRemote.

We have added test classes for LayerTreeHostRemote for Blimp and would
like to reuse cc unit tests to test the following flow:

Engine cc ==> Client cc main thread ==> Client cc impl thread.

This CL goes through about 50 unit test cases in
layer_tree_host_unittest.cc.

BUG=653371
==========

to

==========
Enable more layer_tree_host_unittest for LayerTreeHostRemote.

We have added test classes for LayerTreeHostRemote for Blimp and would
like to reuse cc unit tests to test the following flow:

Engine cc ==> Client cc main thread ==> Client cc impl thread.

This CL goes through about 50 unit test cases in
layer_tree_host_unittest.cc.

BUG=653371
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[xingliu, 2016-10-27 17:36:15]

Description was changed from

==========
Enable more layer_tree_host_unittest for LayerTreeHostRemote.

We have added test classes for LayerTreeHostRemote for Blimp and would
like to reuse cc unit tests to test the following flow:

Engine cc ==> Client cc main thread ==> Client cc impl thread.

This CL goes through about 50 unit test cases in
layer_tree_host_unittest.cc.

BUG=653371
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Enable more cc unit tests for LayerTreeHostRemote test.

We have added test classes for LayerTreeHostRemote for Blimp and would
like to reuse cc unit tests to test the following flow:

Engine cc ==> Client cc main thread ==> Client cc impl thread.

This CL goes through about 50 unit test cases in
layer_tree_host_unittest.cc.

BUG=653371
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[xingliu, 2016-10-27 17:40:21]

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

[commit-bot: I haz the power, 2016-10-27 17:40:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 17:55:26]

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

[commit-bot: I haz the power, 2016-10-27 17:55:27]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-10-27 18:07:28]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org, khushalsagar@chromium.org

[xingliu, 2016-10-27 18:07:29]

Please help take a look, more unit tests for LTH remote.

[Khushal, 2016-10-27 18:37:51]

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

[Khushal, 2016-10-27 18:37:52]

https://codereview.chromium.org/2456093003/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2456093003/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:3317: class PushPropertiesCountingLayer :
public Layer {
The tests which use this would definitely be nice to have. Because the code on
the engine side does rely on Layers that need push properties to decide what is
going to be serialized.

The ToLayerPropertiesProto method is almost the equivalent of PushPropertiesTo
for pushing into a proto. May be we could add this layer as a type for
serialization, like the fake picture layer, but do what we do in
PushPropertiesTo in ToLayerPropertiesProto also. It's fine to do in both, since
only one of them should ever run, and it will create the correct runtime
LayerImpl on the client side.

+ajuma, wdyt?

[ajuma, 2016-10-27 19:40:03]

https://codereview.chromium.org/2456093003/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2456093003/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:3317: class PushPropertiesCountingLayer :
public Layer {
On 2016/10/27 18:37:52, Khushal wrote:
> The tests which use this would definitely be nice to have. Because the code on
> the engine side does rely on Layers that need push properties to decide what
is
> going to be serialized.
> 
> The ToLayerPropertiesProto method is almost the equivalent of PushPropertiesTo
> for pushing into a proto. May be we could add this layer as a type for
> serialization, like the fake picture layer, but do what we do in
> PushPropertiesTo in ToLayerPropertiesProto also. It's fine to do in both,
since
> only one of them should ever run, and it will create the correct runtime
> LayerImpl on the client side.
> 
> +ajuma, wdyt?

That sounds like a good plan.

[xingliu, 2016-10-28 20:04:09]

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

[commit-bot: I haz the power, 2016-10-28 20:04:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-10-28 20:05:03]

Thanks for the review, did some modification on PushPropertiesCountingLayer
related tests.

[commit-bot: I haz the power, 2016-10-28 20:23:22]

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

[commit-bot: I haz the power, 2016-10-28 20:23:22]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[xingliu, 2016-10-28 20:54:11]

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

[commit-bot: I haz the power, 2016-10-28 20:54:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 21:13:53]

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

[commit-bot: I haz the power, 2016-10-28 21:13:54]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[Khushal, 2016-10-28 23:11:51]

https://codereview.chromium.org/2456093003/diff/20001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2456093003/diff/20001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:492:
layer_tree_->LayersThatShouldPushProperties());
Do you mind adding a small test for this in layer_tree_host_remote_unittest?

Create a tree with 3 layers or so and in the first frame, all layers would be
serialized. Then modify a single layer and make sure in the second frame only
that layer is serialized?

[xingliu, 2016-10-31 18:43:56]

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

[commit-bot: I haz the power, 2016-10-31 18:44:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-10-31 18:48:06]

Added a unit test in layer_tree_host_remote_unittest.cc.
Please help take a look.

https://codereview.chromium.org/2456093003/diff/20001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2456093003/diff/20001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:492:
layer_tree_->LayersThatShouldPushProperties());
On 2016/10/28 23:11:50, Khushal wrote:
> Do you mind adding a small test for this in layer_tree_host_remote_unittest?
> 
> Create a tree with 3 layers or so and in the first frame, all layers would be
> serialized. Then modify a single layer and make sure in the second frame only
> that layer is serialized?

Done, make sense. Also tweak UpdateTrackingRemoteCompositorBridge in
layer_tree_host_remote_unittest.cc, not sure if this is really needed.

[commit-bot: I haz the power, 2016-10-31 19:24:32]

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

[commit-bot: I haz the power, 2016-10-31 19:24:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Khushal, 2016-10-31 23:43:27]

https://codereview.chromium.org/2456093003/diff/40001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2456093003/diff/40001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:57: compositor_proto_state_ =
std::move(compositor_proto_state);
This looks perfect. Thanks! :)

https://codereview.chromium.org/2456093003/diff/40001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:432:
scoped_refptr<PushPropertiesCountingLayer> child_layer =
To be honest, I'd say just use Layer for this test. PushPropertiesCountingLayer
looks like unnecessary test magic, and I think the serialization testing would
be better if we actually put the ids for layers serialized in the proto in a
set, and then verified that only the expected ones are serialized. Wdyt?

https://codereview.chromium.org/2456093003/diff/40001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:452: DCHECK(compositor_proto_state);
nit: Can do without the DCHECK? We'll crash on the next line anyway right?

[xingliu, 2016-11-01 01:30:00]

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

[commit-bot: I haz the power, 2016-11-01 01:30:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-11-01 01:31:13]

Tweak on the unit tests, thanks for the feedback.

https://codereview.chromium.org/2456093003/diff/40001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2456093003/diff/40001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:432:
scoped_refptr<PushPropertiesCountingLayer> child_layer =
On 2016/10/31 23:43:27, Khushal wrote:
> To be honest, I'd say just use Layer for this test.
PushPropertiesCountingLayer
> looks like unnecessary test magic, and I think the serialization testing would
> be better if we actually put the ids for layers serialized in the proto in a
> set, and then verified that only the expected ones are serialized. Wdyt?

Yeah, I think PushPropertiesCountingLayer looks redundant. Removed.

https://codereview.chromium.org/2456093003/diff/40001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:452: DCHECK(compositor_proto_state);
On 2016/10/31 23:43:27, Khushal wrote:
> nit: Can do without the DCHECK? We'll crash on the next line anyway right?

Removed the DCHECK.

[commit-bot: I haz the power, 2016-11-01 02:32:46]

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

[commit-bot: I haz the power, 2016-11-01 02:32:47]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-11-01 03:26:50]

lgtm. Thanks!

https://codereview.chromium.org/2456093003/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2456093003/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:56: // Cache the proto here for
verification.
nit: Its fine without the comment too. Pretty obvious.

[ajuma, 2016-11-01 13:51:55]

lgtm too

[David Trainor- moved to gerrit, 2016-11-02 17:00:04]

lgtm

https://codereview.chromium.org/2456093003/diff/60001/cc/test/push_properties...
File cc/test/push_properties_counting_layer.cc (right):

https://codereview.chromium.org/2456093003/diff/60001/cc/test/push_properties...
cc/test/push_properties_counting_layer.cc:51: SetIsDrawable(draws_content);
This method feels unnecessary, but it looks like you only moved it.

[xingliu, 2016-11-02 17:33:23]

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

[xingliu, 2016-11-02 17:33:53]

Some minor polish, start to put it into CQ.

https://codereview.chromium.org/2456093003/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2456093003/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:56: // Cache the proto here for
verification.
On 2016/11/01 03:26:50, Khushal wrote:
> nit: Its fine without the comment too. Pretty obvious.

Done.

https://codereview.chromium.org/2456093003/diff/60001/cc/test/push_properties...
File cc/test/push_properties_counting_layer.cc (right):

https://codereview.chromium.org/2456093003/diff/60001/cc/test/push_properties...
cc/test/push_properties_counting_layer.cc:51: SetIsDrawable(draws_content);
On 2016/11/02 17:00:04, David Trainor (OOO to oct 31) wrote:
> This method feels unnecessary, but it looks like you only moved it.

Done, sorry didn't notice this method.

[commit-bot: I haz the power, 2016-11-02 17:34:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 20:35:10]

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

[commit-bot: I haz the power, 2016-11-02 20:35:11]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-11-02 20:44:25]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2016-11-02 20:44:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, ajuma@chromium.org, khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2456093003/#ps80001
(title: "Minor polish on refactored test class.")

[commit-bot: I haz the power, 2016-11-02 20:45:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 20:53:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-02 20:56:20]

Description was changed from

==========
Enable more cc unit tests for LayerTreeHostRemote test.

We have added test classes for LayerTreeHostRemote for Blimp and would
like to reuse cc unit tests to test the following flow:

Engine cc ==> Client cc main thread ==> Client cc impl thread.

This CL goes through about 50 unit test cases in
layer_tree_host_unittest.cc.

BUG=653371
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Enable more cc unit tests for LayerTreeHostRemote test.

We have added test classes for LayerTreeHostRemote for Blimp and would
like to reuse cc unit tests to test the following flow:

Engine cc ==> Client cc main thread ==> Client cc impl thread.

This CL goes through about 50 unit test cases in
layer_tree_host_unittest.cc.

BUG=653371
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/3c9b4cc4fe631cec765af7dca9ce4c4c2b7e064d
Cr-Commit-Position: refs/heads/master@{#429396}
==========

[commit-bot: I haz the power, 2016-11-02 20:56:21]

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