cc/blimp: Set up the framework for LayerTreeTests in remote mode.

cc/blimp: Set up the framework for LayerTreeTests in remote mode.

This sets up the LayerTreeHostRemoteForTesting class which can be used
to mock out the external transport between the RemoteCompositorBridge
on the engine and LayerTreeHostInProcess on the client. The class
implements a simple binding layer that passes through state updates
between these 2.

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

Committed: https://crrev.com/04b10facfd83a405009f155cb58b7d7732d52f43
Cr-Commit-Position: refs/heads/master@{#424299}

[Khushal, 2016-10-06 18:33:23]

Description was changed from

==========
cc/blimp: Set up the framework for LayerTreeTests in remote mode.

This sets up the LayerTreeHostRemoteForTesting class which can be used
to mock out the external transport between the RemoteCompositorBridge
on the engine and LayerTreeHostInProcess on the client. The class
implements a simple binding layer that passes through state updates
between these 2.

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

to

==========
cc/blimp: Set up the framework for LayerTreeTests in remote mode.

This sets up the LayerTreeHostRemoteForTesting class which can be used
to mock out the external transport between the RemoteCompositorBridge
on the engine and LayerTreeHostInProcess on the client. The class
implements a simple binding layer that passes through state updates
between these 2.

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

[Khushal, 2016-10-06 18:36:43]

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

[Khushal, 2016-10-06 18:36:44]

This depends on a couple of patches but is otherwise done. I tested on a couple
of random tests. PTAL.

[Khushal, 2016-10-06 18:39:07]

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
File cc/test/layer_tree_host_remote_for_testing.h (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.h:26: class
LayerTreeHostRemoteForTesting : public LayerTreeHostRemote,
Also I'm hoping to use this class for Layout tests too, the only thing missing
for them is piping for CopyOutputRequests. It would be great if you could review
from that standpoint as well. Thanks!

[enne (OOO), 2016-10-06 20:19:55]

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
File cc/test/layer_tree_host_remote_for_testing.cc (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.cc:42: void
ProcessCompositorStateUpdate(
override?

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.cc:228: // Override the LayerFactory
since a lot of tests rely on the fact that Layers
Why are layer ids different for Blimp?

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
File cc/test/layer_tree_host_remote_for_testing.h (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.h:26: class
LayerTreeHostRemoteForTesting : public LayerTreeHostRemote,
On 2016/10/06 at 18:39:07, Khushal wrote:
> Also I'm hoping to use this class for Layout tests too, the only thing missing
for them is piping for CopyOutputRequests. It would be great if you could review
from that standpoint as well. Thanks!

I think it might be worth running layout tests manually once, but I really don't
want to get to adding virtual test suites to layout tests for blimp.  I think
layer tree pixel tests and cc unit tests cover most everything that should be
needed.

We probably are going to want separate serialization unit tests anyway for
display lists and such, and I think those should cover the rest of the missing
pieces, in my opinion.

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_test.h#n...
cc/test/layer_tree_test.h:144: LayerTreeHost* layer_tree_host() const;
style nit: functions that return non-const pointers shouldn't be const.

[enne (OOO), 2016-10-06 20:20:18]

Meant to also add that this all looks generally good to me.  It's about what I
expected.  Thanks!

[Khushal, 2016-10-06 21:09:42]

Thanks enne! \o/

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
File cc/test/layer_tree_host_remote_for_testing.cc (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.cc:42: void
ProcessCompositorStateUpdate(
On 2016/10/06 20:19:55, enne wrote:
> override?

Its there right?

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.cc:228: // Override the LayerFactory
since a lot of tests rely on the fact that Layers
On 2016/10/06 20:19:55, enne wrote:
> Why are layer ids different for Blimp?

Layers on the engine, and those on the client have different ids. So we have,
EngineLayer -> ClientLayer -> LayerImpl.

Now tests would set the EngineLayer and then may compare it with the
corresponding LayerImpl, which will have an id matching the ClientLayer.

I had thought about forcing the id on the ClientLayer to match the EngineLayer,
but as we go forward to doing UI things, we will be mutating the LayerTree on
the client (adding layers etc.), so there was a possibility of id collision. So
instead I settled on keeping a mapping in CompositorStateDeserializer and having
the same ids for tests only.

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
File cc/test/layer_tree_host_remote_for_testing.h (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.h:26: class
LayerTreeHostRemoteForTesting : public LayerTreeHostRemote,
On 2016/10/06 20:19:55, enne wrote:
> On 2016/10/06 at 18:39:07, Khushal wrote:
> > Also I'm hoping to use this class for Layout tests too, the only thing
missing
> for them is piping for CopyOutputRequests. It would be great if you could
review
> from that standpoint as well. Thanks!
> 
> I think it might be worth running layout tests manually once, but I really
don't
> want to get to adding virtual test suites to layout tests for blimp.  I think
> layer tree pixel tests and cc unit tests cover most everything that should be
> needed.

I'll try with a few layout tests to see how feasible/stable using an approach
like this is. The aim is to get maximum coverage from the existing test
frameworks to make sure things don't fail silently. Its at least worth
experimenting with an FYI bot I guess.

> 
> We probably are going to want separate serialization unit tests anyway for
> display lists and such, and I think those should cover the rest of the missing
> pieces, in my opinion.

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_test.h#n...
cc/test/layer_tree_test.h:144: LayerTreeHost* layer_tree_host() const;
On 2016/10/06 20:19:55, enne wrote:
> style nit: functions that return non-const pointers shouldn't be const.

Removed. Added it because the proxy accessor was yelling at me so I figured the
pattern in cc was to const.

[enne (OOO), 2016-10-06 21:32:18]

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
File cc/test/layer_tree_host_remote_for_testing.h (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.h:26: class
LayerTreeHostRemoteForTesting : public LayerTreeHostRemote,
On 2016/10/06 at 21:09:42, Khushal wrote:
> On 2016/10/06 20:19:55, enne wrote:
> > On 2016/10/06 at 18:39:07, Khushal wrote:
> > > Also I'm hoping to use this class for Layout tests too, the only thing
missing
> > for them is piping for CopyOutputRequests. It would be great if you could
review
> > from that standpoint as well. Thanks!
> > 
> > I think it might be worth running layout tests manually once, but I really
don't
> > want to get to adding virtual test suites to layout tests for blimp.  I
think
> > layer tree pixel tests and cc unit tests cover most everything that should
be
> > needed.
> 
> I'll try with a few layout tests to see how feasible/stable using an approach
like this is. The aim is to get maximum coverage from the existing test
frameworks to make sure things don't fail silently. Its at least worth
experimenting with an FYI bot I guess.

As I said, I think running layout tests manually is going to be the best
approach here.  Which functionality in particular are you worried about failing
that cc unit tests are not going to catch?

The only reason that there are even threaded vs single threaded layout tests is
because Blink itself set up animations differently based on this distinction and
nobody took the time to write the code to make it possible to have single
threaded animations on the compositor thread and it's been too thorny to remove
ever since.  I think in general these are implementation details in cc that
should not make a different to Blink, and so should be tested elsewhere by more
targeted unit tests.  I would also hope that Blimp integration tests would
involve more of the pipeline and not a LayerTreeRemoteForTesting.

If you want to run an FYI bot, you might want to chrishtr, as he's been doing
that for the slimming paint project.

[enne (OOO), 2016-10-06 21:34:23]

lgtm

[Khushal, 2016-10-06 22:48:51]

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
File cc/test/layer_tree_host_remote_for_testing.h (right):

https://codereview.chromium.org/2401703002/diff/1/cc/test/layer_tree_host_rem...
cc/test/layer_tree_host_remote_for_testing.h:26: class
LayerTreeHostRemoteForTesting : public LayerTreeHostRemote,
On 2016/10/06 21:32:18, enne wrote:
> On 2016/10/06 at 21:09:42, Khushal wrote:
> > On 2016/10/06 20:19:55, enne wrote:
> > > On 2016/10/06 at 18:39:07, Khushal wrote:
> > > > Also I'm hoping to use this class for Layout tests too, the only thing
> missing
> > > for them is piping for CopyOutputRequests. It would be great if you could
> review
> > > from that standpoint as well. Thanks!
> > > 
> > > I think it might be worth running layout tests manually once, but I really
> don't
> > > want to get to adding virtual test suites to layout tests for blimp.  I
> think
> > > layer tree pixel tests and cc unit tests cover most everything that should
> be
> > > needed.
> > 
> > I'll try with a few layout tests to see how feasible/stable using an
approach
> like this is. The aim is to get maximum coverage from the existing test
> frameworks to make sure things don't fail silently. Its at least worth
> experimenting with an FYI bot I guess.
> 
> As I said, I think running layout tests manually is going to be the best
> approach here.  Which functionality in particular are you worried about
failing
> that cc unit tests are not going to catch?

Sorry, that's what I meant. I'll try a few tests manually first. After I hook up
CopyOutputRequests, which is necessary for cc pixel tests as well anyway.

Regarding functionality, I don't have a good answer tbh. I'm just concerned
about cases where, for instance, a property is added to cc but its a trivial
push through to commit/activation so doesn't end up in a cc LayerTreeTest, but
does end up including a layout test. Though now that I think about it such cases
would not be covered in layout tests in single threaded mode as well, since that
code path will not cover the activation path?

So I guess its a distant possibility and I'm just being paranoid. Thanks for the
guidance on this. I'm not as familiar with layout tests to make a correct
judgement here so I'll put this on the back of the list for now, to come back
later when its clearer what code is where.

> 
> The only reason that there are even threaded vs single threaded layout tests
is
> because Blink itself set up animations differently based on this distinction
and
> nobody took the time to write the code to make it possible to have single
> threaded animations on the compositor thread and it's been too thorny to
remove
> ever since.  I think in general these are implementation details in cc that
> should not make a different to Blink, and so should be tested elsewhere by
more
> targeted unit tests.  I would also hope that Blimp integration tests would
> involve more of the pipeline and not a LayerTreeRemoteForTesting.

The integration tests would. We already have one which tests the complete
pipeline end-to-end
(https://cs.chromium.org/chromium/src/blimp/engine/browser_tests/integration_t...),
but these should be be more akin to the browser tests right? As you mentioned,
given that we don't even use threaded compositor for layout tests (I assume
because they tend to be flaky?), I was worried adding more parts of the blimp
pipeline to them is going to have the same problem. Also, one reason for
considering using a test suite like this was because setting up the code to have
more of the blimp pipeline without content shell and existing setup for the
blink test runner is not trivial.

But now I realize that if we don't include them then having layout tests is not
even worth it... 

> 
> If you want to run an FYI bot, you might want to chrishtr, as he's been doing
> that for the slimming paint project.

[Khushal, 2016-10-10 19:03:27]

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

[commit-bot: I haz the power, 2016-10-10 19:03:55]

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-10 20:06:23]

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

[commit-bot: I haz the power, 2016-10-10 20:06:24]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-10-10 23:06:07]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-10-10 23:06:07]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2401703002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-10 23:06:23]

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-11 00:20:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-11 00:22:58]

Description was changed from

==========
cc/blimp: Set up the framework for LayerTreeTests in remote mode.

This sets up the LayerTreeHostRemoteForTesting class which can be used
to mock out the external transport between the RemoteCompositorBridge
on the engine and LayerTreeHostInProcess on the client. The class
implements a simple binding layer that passes through state updates
between these 2.

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

to

==========
cc/blimp: Set up the framework for LayerTreeTests in remote mode.

This sets up the LayerTreeHostRemoteForTesting class which can be used
to mock out the external transport between the RemoteCompositorBridge
on the engine and LayerTreeHostInProcess on the client. The class
implements a simple binding layer that passes through state updates
between these 2.

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

Committed: https://crrev.com/04b10facfd83a405009f155cb58b7d7732d52f43
Cr-Commit-Position: refs/heads/master@{#424299}
==========

[commit-bot: I haz the power, 2016-10-11 00:22:59]

Patchset 4 (id:??) landed as
https://crrev.com/04b10facfd83a405009f155cb58b7d7732d52f43
Cr-Commit-Position: refs/heads/master@{#424299}

[stgao, 2016-10-18 04:17:37]

FYI: this CL seems cause the flakiness in the test
LayerTreeHostCopyRequestTestDeleteTexture.RunSingleThread_DelegatingRenderer

The flakiness trend diagram is shown in the link below, and clicking the red
data point shows the link to the first build cycle that the flake started.

https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

[Khushal, 2016-10-18 04:49:43]

I highly doubt this change would cause this flakiness. It didn't change any of
the setup for single/threaded test modes.

If its that build, then the only other suspected change could be this one:
https://codereview.chromium.org/2402173002/.

+danakj, do you mind taking a look? Here is an instance of the failing build:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....