Fix ArcBridgeBootstrap race issues.

components/arc/arc_bridge_service_impl.h

Index: components/arc/arc_bridge_service_impl.h
diff --git a/components/arc/arc_bridge_service_impl.h b/components/arc/arc_bridge_service_impl.h
index a9b5c44ccc5f21aa823ace2ef3e07fd1c160e56f..4800dc1575b87c9945596a845edfc04518e60c41 100644
--- a/components/arc/arc_bridge_service_impl.h
+++ b/components/arc/arc_bridge_service_impl.h
@@ -27,13 +27,23 @@ namespace arc {
 class ArcBridgeServiceImpl : public ArcBridgeService,
                              public ArcBridgeBootstrap::Delegate {
  public:
-  explicit ArcBridgeServiceImpl(std::unique_ptr<ArcBridgeBootstrap> bootstrap);
+  // This is the delegate interface for testing purpose only.
+  class Delegate {

[Luis Héctor Chávez, 2016/09/07 23:38:32]

Wouldn't this make more sense to be called "Factory" and be in
ArcBridgeBootstrap?

[hidehiko, 2016/09/08 16:53:41]

Replaced by Factory style with base::Callback.
Can we keep the factory in this class? The factory is only for testing with
ArcBridgeServiceImpl so that it should be independent from ArcBridgeBootstrap?

[Luis Héctor Chávez, 2016/09/15 23:08:49]

Sure, after the refactor and in callback-style it makes more sense to have it
here.

+   public:
+    // Creates ArcBridgeBootstrap instance.
+    virtual std::unique_ptr<ArcBridgeBootstrap> CreateBootstrap() = 0;

[Luis Héctor Chávez, 2016/09/07 23:38:32]

this needs a virtual destructor.

[hidehiko, 2016/09/08 16:53:41]

Acknowledged.

+  };
+
+  ArcBridgeServiceImpl();
   ~ArcBridgeServiceImpl() override;
 
   void HandleStartup() override;
 
   void Shutdown() override;
 
+  // |delegate| is managed by the caller. This is only for testing purpose.
+  void SetDelegateForTesting(Delegate* delegate) { delegate_ = delegate; }
+
   // Normally, reconnecting after connection shutdown happens after a short
   // delay. When testing, however, we'd like it to happen immediately to avoid
   // adding unnecessary delays.
@@ -72,6 +82,9 @@ class ArcBridgeServiceImpl : public ArcBridgeService,
   // Delay the reconnection.
   bool use_delay_before_reconnecting_ = true;
 
+  // Delegate to inject a fake ArcBridgeBootstrap instance for testing.
+  Delegate* delegate_ = nullptr;

[Luis Héctor Chávez, 2016/09/07 23:38:32]

If you make the Delegate a Factory, maybe make this a std::unique_ptr<Factory>.
That way we can remove the ArcBridgeBootstrap::Create and instead instantiate an
ArcBridgeBootstrap::Factory() as the default value for this. In tests, instead
of passing "this" as the factory, it's probably better to replace it with a
FakeArcBridgeBootstrap::Factory().

[hidehiko, 2016/09/08 16:53:41]

Replaced by Callback, and now life-time should be managed as value.

+
   // WeakPtrFactory to use callbacks.
   base::WeakPtrFactory<ArcBridgeServiceImpl> weak_factory_;