arc: Use the new InstanceHolder for unittests

arc: Use the new InstanceHolder for unittests

Instead of having unit tests go through Mojo and jump through hoops to
ensure that all messages have propagated to all the threads, we can use
the new arc::ArcBridgeService::InstanceHolder<T>::SetInstance() to
directly set the concrete fake (and optional version). This simplifies
testing quite a bit.

BUG=626695
TEST=trybots

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

[Luis Héctor Chávez, 2016-07-08 22:44:57]

lhchavez@chromium.org changed reviewers:
+ hidehiko@chromium.org, stevenjb@chromium.org

[Luis Héctor Chávez, 2016-07-08 22:44:58]

PTAL

This is the second (and last) part of the ArcBridgeService cleanup. It only
saves 73 lines, but it simplifies tests a bit and is a prerequisite for both the
Bluetooth and SyncManager tests.

hidehiko: *arc/*
stevenjb: chrome/browser

[hidehiko, 2016-07-11 06:05:18]

Looks a good direction to me. Can I prioritize to review your first one?

https://codereview.chromium.org/2138513002/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_manager_unittest.cc (right):

https://codereview.chromium.org/2138513002/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_manager_unittest.cc:5: #include <vector>
nit: <string> and <memory>, too?

[Luis Héctor Chávez, 2016-07-11 17:15:08]

Sure, I just send this together to both provide mode context for the previous
change and try to reduce the number of cross-timezone roundtrips.

https://codereview.chromium.org/2138513002/diff/1/ui/arc/notification/arc_not...
File ui/arc/notification/arc_notification_manager_unittest.cc (right):

https://codereview.chromium.org/2138513002/diff/1/ui/arc/notification/arc_not...
ui/arc/notification/arc_notification_manager_unittest.cc:5: #include <vector>
On 2016/07/11 06:05:18, hidehiko wrote:
> nit: <string> and <memory>, too?

Addressed in previous change.

[stevenjb, 2016-07-11 19:57:11]

c/b/ui lgtm

[hidehiko, 2016-07-12 18:27:36]

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_service_impl.cc:197: void
ArcBridgeServiceImpl::OnAppInstanceReady(mojom::AppInstancePtr app_ptr) {
As you moved ABHost to Impl, could you also extract "real" implementation in
InstanceHolder to impl files? Specifically

- CloseChannel
- OnInstanceReady, and
- OnVersionReady.

If you'll work on it in a separate CL, it is also ok for me.

[Luis Héctor Chávez, 2016-07-12 18:34:06]

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_service_impl.cc:197: void
ArcBridgeServiceImpl::OnAppInstanceReady(mojom::AppInstancePtr app_ptr) {
On 2016/07/12 18:27:36, hidehiko wrote:
> As you moved ABHost to Impl, could you also extract "real" implementation in
> InstanceHolder to impl files? Specifically
> 
> - CloseChannel
> - OnInstanceReady, and
> - OnVersionReady.
> 
> If you'll work on it in a separate CL, it is also ok for me.

I prefer to work on it separately to land this today :) It's going to be a very
localized change so it won't be painful.

[Luis Héctor Chávez, 2016-07-12 22:14:29]

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_service_impl.cc:197: void
ArcBridgeServiceImpl::OnAppInstanceReady(mojom::AppInstancePtr app_ptr) {
On 2016/07/12 18:34:06, Luis Héctor Chávez wrote:
> On 2016/07/12 18:27:36, hidehiko wrote:
> > As you moved ABHost to Impl, could you also extract "real" implementation in
> > InstanceHolder to impl files? Specifically
> > 
> > - CloseChannel
> > - OnInstanceReady, and
> > - OnVersionReady.
> > 
> > If you'll work on it in a separate CL, it is also ok for me.
> 
> I prefer to work on it separately to land this today :) It's going to be a
very
> localized change so it won't be painful.

Wait, but that wouldn't work: the InstanceHolder is instantiated in
ArcBridgeService (and I'd like to keep it that way to avoid making the getters
virtual), so InstanceHolder needs to keep the real implementation.

[hidehiko, 2016-07-13 08:08:12]

LGTM with assuming you'll work on separation of InstanceHolder in a follow up
CL.

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/2138513002/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_service_impl.cc:197: void
ArcBridgeServiceImpl::OnAppInstanceReady(mojom::AppInstancePtr app_ptr) {
On 2016/07/12 22:14:29, Luis Héctor Chávez wrote:
> On 2016/07/12 18:34:06, Luis Héctor Chávez wrote:
> > On 2016/07/12 18:27:36, hidehiko wrote:
> > > As you moved ABHost to Impl, could you also extract "real" implementation
in
> > > InstanceHolder to impl files? Specifically
> > > 
> > > - CloseChannel
> > > - OnInstanceReady, and
> > > - OnVersionReady.
> > > 
> > > If you'll work on it in a separate CL, it is also ok for me.
> > 
> > I prefer to work on it separately to land this today :) It's going to be a
> very
> > localized change so it won't be painful.
> 
> Wait, but that wouldn't work: the InstanceHolder is instantiated in
> ArcBridgeService (and I'd like to keep it that way to avoid making the getters
> virtual), so InstanceHolder needs to keep the real implementation.

You can touch them via SetInstance().

[Luis Héctor Chávez, 2016-07-13 13:48:51]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2016-07-13 13:48:52]

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

[commit-bot: I haz the power, 2016-07-13 13:49:04]

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-07-13 14:01:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-13 14:02:08]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 14:03:59]

Description was changed from

==========
arc: Use the new InstanceHolder for unittests

Instead of having unit tests go through Mojo and jump through hoops to
ensure that all messages have propagated to all the threads, we can use
the new arc::ArcBridgeService::InstanceHolder<T>::SetInstance() to
directly set the concrete fake (and optional version). This simplifies
testing quite a bit.

BUG=626695
TEST=trybots
==========

to

==========
arc: Use the new InstanceHolder for unittests

Instead of having unit tests go through Mojo and jump through hoops to
ensure that all messages have propagated to all the threads, we can use
the new arc::ArcBridgeService::InstanceHolder<T>::SetInstance() to
directly set the concrete fake (and optional version). This simplifies
testing quite a bit.

BUG=626695
TEST=trybots

Committed: https://crrev.com/c5fffc64c14a0464f8870bbe91df7e945f42c3ca
Cr-Commit-Position: refs/heads/master@{#405138}
==========

[commit-bot: I haz the power, 2016-07-13 14:04:01]

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

[engedy, 2016-07-13 15:50:49]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2146573005/ by engedy@chromium.org.

The reason for reverting is: Caused unit_tests failure on Linux ChromiumOS Tests
(1):

ArcAppModelBuilderTest.RemoveAppCleanUpFolder.