Make ServiceManager own its ServiceLoaders

R=viettrungluu@chromium.org, viettrungluu
BUG=

Review URL: https://codereview.chromium.org/242203009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@265020 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 151 insertions, 109 deletions

diff --git a/mojo/service_manager/service_manager.cc b/mojo/service_manager/service_manager.cc
index f12249e..c34e83f 100644
--- a/mojo/service_manager/service_manager.cc
+++ b/mojo/service_manager/service_manager.cc
@@ -9,6 +9,7 @@
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/stl_util.h"
 #include "mojo/public/cpp/bindings/allocation_scope.h"
 #include "mojo/public/cpp/bindings/error_handler.h"
 #include "mojo/public/cpp/bindings/remote_ptr.h"
@@ -71,16 +72,13 @@ bool ServiceManager::TestAPI::HasFactoryForURL(const GURL& url) const {
 }
 
 ServiceManager::ServiceManager()
-    : default_loader_(NULL),
-      interceptor_(NULL) {
+    : interceptor_(NULL) {
 }
 
 ServiceManager::~ServiceManager() {
-  for (URLToServiceFactoryMap::iterator it = url_to_service_factory_.begin();
-       it != url_to_service_factory_.end(); ++it) {
-    delete it->second;
-  }
-  url_to_service_factory_.clear();
+  STLDeleteValues(&url_to_service_factory_);
+  STLDeleteValues(&url_to_loader_);
+  STLDeleteValues(&scheme_to_loader_);
 }
 
 // static
@@ -110,15 +108,20 @@ void ServiceManager::Connect(const GURL& url,
   }
 }
 
-void ServiceManager::SetLoaderForURL(ServiceLoader* loader, const GURL& url) {
-  DCHECK(url_to_loader_.find(url) == url_to_loader_.end());
-  url_to_loader_[url] = loader;
+void ServiceManager::SetLoaderForURL(scoped_ptr<ServiceLoader> loader,
+                                     const GURL& url) {
+  URLToLoaderMap::iterator it = url_to_loader_.find(url);
+  if (it != url_to_loader_.end())
+    delete it->second;
+  url_to_loader_[url] = loader.release();
 }
 
-void ServiceManager::SetLoaderForScheme(ServiceLoader* loader,
+void ServiceManager::SetLoaderForScheme(scoped_ptr<ServiceLoader> loader,
                                         const std::string& scheme) {
-  DCHECK(scheme_to_loader_.find(scheme) == scheme_to_loader_.end());
-  scheme_to_loader_[scheme] = loader;
+  SchemeToLoaderMap::iterator it = scheme_to_loader_.find(scheme);
+  if (it != scheme_to_loader_.end())
+    delete it->second;
+  scheme_to_loader_[scheme] = loader.release();
 }
 
 void ServiceManager::SetInterceptor(Interceptor* interceptor) {
@@ -134,7 +137,7 @@ ServiceLoader* ServiceManager::GetLoaderForURL(const GURL& url) {
   if (scheme_it != scheme_to_loader_.end())
     return scheme_it->second;
   DCHECK(default_loader_);
-  return default_loader_;
+  return default_loader_.get();
 }
 
 void ServiceManager::OnServiceFactoryError(ServiceFactory* service_factory) {
diff --git a/mojo/service_manager/service_manager.h b/mojo/service_manager/service_manager.h
index 6d9ab5d..40cb8cd 100644
--- a/mojo/service_manager/service_manager.h
+++ b/mojo/service_manager/service_manager.h
@@ -10,7 +10,9 @@
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
 #include "mojo/public/interfaces/shell/shell.mojom.h"
+#include "mojo/service_manager/service_loader.h"
 #include "mojo/service_manager/service_manager_export.h"
 #include "url/gurl.h"
 
@@ -20,8 +22,6 @@ namespace content {
 
 namespace mojo {
 
-class ServiceLoader;
-
 class MOJO_SERVICE_MANAGER_EXPORT ServiceManager {
  public:
   // API for testing.
@@ -57,16 +57,16 @@ class MOJO_SERVICE_MANAGER_EXPORT ServiceManager {
   // Loads a service if necessary and establishes a new client connection.
   void Connect(const GURL& url, ScopedMessagePipeHandle client_handle);
 
-  // Sets the default Loader to be used if not overridden by
-  // SetLoaderForURL() or SetLoaderForScheme().
-  // Does not take ownership of |loader|.
-  void set_default_loader(ServiceLoader* loader) { default_loader_ = loader; }
+  // Sets the default Loader to be used if not overridden by SetLoaderForURL()
+  // or SetLoaderForScheme().
+  void set_default_loader(scoped_ptr<ServiceLoader> loader) {
+    default_loader_ = loader.Pass();
+  }
   // Sets a Loader to be used for a specific url.
-  // Does not take ownership of |loader|.
-  void SetLoaderForURL(ServiceLoader* loader, const GURL& url);
+  void SetLoaderForURL(scoped_ptr<ServiceLoader> loader, const GURL& url);
   // Sets a Loader to be used for a specific url scheme.
-  // Does not take ownership of |loader|.
-  void SetLoaderForScheme(ServiceLoader* loader, const std::string& scheme);
+  void SetLoaderForScheme(scoped_ptr<ServiceLoader> loader,
+                          const std::string& scheme);
   // Allows to interpose a debugger to service connections.
   void SetInterceptor(Interceptor* interceptor);
 
@@ -87,7 +87,7 @@ class MOJO_SERVICE_MANAGER_EXPORT ServiceManager {
   // Loader management.
   URLToLoaderMap url_to_loader_;
   SchemeToLoaderMap scheme_to_loader_;
-  ServiceLoader* default_loader_;
+  scoped_ptr<ServiceLoader> default_loader_;
   Interceptor* interceptor_;
 
   URLToServiceFactoryMap url_to_service_factory_;
diff --git a/mojo/service_manager/service_manager_unittest.cc b/mojo/service_manager/service_manager_unittest.cc
index 6ba9d97..54a6b7d 100644
--- a/mojo/service_manager/service_manager_unittest.cc
+++ b/mojo/service_manager/service_manager_unittest.cc
@@ -19,9 +19,10 @@ namespace {
 const char kTestURLString[] = "test:testService";
 
 struct TestContext {
-  TestContext() : num_impls(0) {}
+  TestContext() : num_impls(0), num_loader_deletes(0) {}
   std::string last_test_string;
   int num_impls;
+  int num_loader_deletes;
 };
 
 class TestServiceImpl :
@@ -30,7 +31,8 @@ class TestServiceImpl :
   TestServiceImpl() {}
 
   virtual ~TestServiceImpl() {
-    --context()->num_impls;
+    if (context())
+      --context()->num_impls;
   }
 
   virtual void Test(const mojo::String& test_string) OVERRIDE {
@@ -39,11 +41,12 @@ class TestServiceImpl :
   }
 
   void Initialize(
-      ServiceConnector<TestServiceImpl, TestContext>* service_connector,
+      ServiceConnector<TestServiceImpl, TestContext>* service_factory,
       ScopedMessagePipeHandle client_handle) {
     ServiceConnection<TestService, TestServiceImpl, TestContext>::Initialize(
-        service_connector, client_handle.Pass());
-    ++context()->num_impls;
+        service_factory, client_handle.Pass());
+    if (context())
+      ++context()->num_impls;
   }
 };
 
@@ -72,74 +75,51 @@ class TestClientImpl : public TestClient {
   bool quit_after_ack_;
   DISALLOW_COPY_AND_ASSIGN(TestClientImpl);
 };
-}  // namespace
 
-class ServiceManagerTest : public testing::Test, public ServiceLoader {
+class TestServiceLoader : public ServiceLoader {
  public:
-  ServiceManagerTest() {}
-
-  virtual ~ServiceManagerTest() {}
-
-  virtual void SetUp() OVERRIDE {
-    GURL test_url(kTestURLString);
-    service_manager_.reset(new ServiceManager);
-    service_manager_->SetLoaderForURL(this, test_url);
-
-    InterfacePipe<TestService, AnyInterface> pipe;
-    test_client_.reset(new TestClientImpl(pipe.handle_to_self.Pass()));
-    service_manager_->Connect(test_url, pipe.handle_to_peer.Pass());
+  TestServiceLoader()
+      : context_(NULL),
+        num_loads_(0),
+        quit_after_error_(false) {
   }
 
-  virtual void TearDown() OVERRIDE {
-    test_client_.reset(NULL);
+  virtual ~TestServiceLoader() {
+    if (context_)
+      ++context_->num_loader_deletes;
     test_app_.reset(NULL);
-    service_manager_.reset(NULL);
   }
 
+  void set_context(TestContext* context) { context_ = context; }
+  void set_quit_after_error(bool quit_after_error) {
+    quit_after_error_ = quit_after_error;
+  }
+
+  int num_loads() const { return num_loads_; }
+
+ private:
   virtual void LoadService(ServiceManager* manager,
                            const GURL& url,
                            ScopedShellHandle shell_handle) OVERRIDE {
+    ++num_loads_;
     test_app_.reset(new Application(shell_handle.Pass()));
     test_app_->AddServiceConnector(
-        new ServiceConnector<TestServiceImpl, TestContext>(&context_));
+        new ServiceConnector<TestServiceImpl, TestContext>(context_));
   }
 
   virtual void OnServiceError(ServiceManager* manager,
                               const GURL& url) OVERRIDE {
-    base::MessageLoop::current()->PostTask(FROM_HERE,
-                                           base::MessageLoop::QuitClosure());
+    if (quit_after_error_) {
+      base::MessageLoop::current()->PostTask(FROM_HERE,
+                                             base::MessageLoop::QuitClosure());
+    }
   }
 
-  bool HasFactoryForTestURL() {
-    ServiceManager::TestAPI manager_test_api(service_manager_.get());
-    return manager_test_api.HasFactoryForURL(GURL(kTestURLString));
-  }
 
- protected:
-  mojo::Environment env_;
-  base::MessageLoop loop_;
-  TestContext context_;
   scoped_ptr<Application> test_app_;
-  scoped_ptr<TestClientImpl> test_client_;
-  scoped_ptr<ServiceManager> service_manager_;
-  DISALLOW_COPY_AND_ASSIGN(ServiceManagerTest);
-};
-
-class TestServiceLoader : public ServiceLoader {
- public:
-  TestServiceLoader() : num_loads_(0) {}
-  int num_loads() const { return num_loads_; }
-
- private:
-  virtual void LoadService(ServiceManager* manager,
-                           const GURL& url,
-                           ScopedShellHandle service_handle) OVERRIDE {
-    ++num_loads_;
-  }
-  virtual void OnServiceError(ServiceManager* manager, const GURL& url)
-      OVERRIDE {}
-
+  TestContext* context_;
   int num_loads_;
+  bool quit_after_error_;
   DISALLOW_COPY_AND_ASSIGN(TestServiceLoader);
 };
 
@@ -170,6 +150,47 @@ class TestServiceInterceptor : public ServiceManager::Interceptor {
   DISALLOW_COPY_AND_ASSIGN(TestServiceInterceptor);
 };
 
+}  // namespace
+
+class ServiceManagerTest : public testing::Test {
+ public:
+  ServiceManagerTest() {}
+
+  virtual ~ServiceManagerTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    GURL test_url(kTestURLString);
+    service_manager_.reset(new ServiceManager);
+
+    InterfacePipe<TestService, AnyInterface> pipe;
+    test_client_.reset(new TestClientImpl(pipe.handle_to_self.Pass()));
+    TestServiceLoader* default_loader = new TestServiceLoader;
+    default_loader->set_context(&context_);
+    default_loader->set_quit_after_error(true);
+    service_manager_->set_default_loader(
+        scoped_ptr<ServiceLoader>(default_loader));
+    service_manager_->Connect(test_url, pipe.handle_to_peer.Pass());
+  }
+
+  virtual void TearDown() OVERRIDE {
+    test_client_.reset(NULL);
+    service_manager_.reset(NULL);
+  }
+
+  bool HasFactoryForTestURL() {
+    ServiceManager::TestAPI manager_test_api(service_manager_.get());
+    return manager_test_api.HasFactoryForURL(GURL(kTestURLString));
+  }
+
+ protected:
+  mojo::Environment env_;
+  base::MessageLoop loop_;
+  TestContext context_;
+  scoped_ptr<TestClientImpl> test_client_;
+  scoped_ptr<ServiceManager> service_manager_;
+  DISALLOW_COPY_AND_ASSIGN(ServiceManagerTest);
+};
+
 TEST_F(ServiceManagerTest, Basic) {
   test_client_->Test("test");
   loop_.Run();
@@ -187,43 +208,67 @@ TEST_F(ServiceManagerTest, ClientError) {
   EXPECT_FALSE(HasFactoryForTestURL());
 }
 
+TEST_F(ServiceManagerTest, Deletes) {
+  {
+    ServiceManager sm;
+    TestServiceLoader* default_loader = new TestServiceLoader;
+    default_loader->set_context(&context_);
+    TestServiceLoader* url_loader1 = new TestServiceLoader;
+    TestServiceLoader* url_loader2 = new TestServiceLoader;
+    url_loader1->set_context(&context_);
+    url_loader2->set_context(&context_);
+    TestServiceLoader* scheme_loader1 = new TestServiceLoader;
+    TestServiceLoader* scheme_loader2 = new TestServiceLoader;
+    scheme_loader1->set_context(&context_);
+    scheme_loader2->set_context(&context_);
+    sm.set_default_loader(scoped_ptr<ServiceLoader>(default_loader));
+    sm.SetLoaderForURL(scoped_ptr<ServiceLoader>(url_loader1),
+                       GURL("test:test1"));
+    sm.SetLoaderForURL(scoped_ptr<ServiceLoader>(url_loader2),
+                       GURL("test:test1"));
+    sm.SetLoaderForScheme(scoped_ptr<ServiceLoader>(scheme_loader1), "test");
+    sm.SetLoaderForScheme(scoped_ptr<ServiceLoader>(scheme_loader2), "test");
+  }
+  EXPECT_EQ(5, context_.num_loader_deletes);
+}
+
 // Confirm that both urls and schemes can have their loaders explicitly set.
 TEST_F(ServiceManagerTest, SetLoaders) {
   ServiceManager sm;
-  TestServiceLoader default_loader;
-  TestServiceLoader url_loader;
-  TestServiceLoader scheme_loader;
-  sm.set_default_loader(&default_loader);
-  sm.SetLoaderForURL(&url_loader, GURL("test:test1"));
-  sm.SetLoaderForScheme(&scheme_loader, "test");
+  TestServiceLoader* default_loader = new TestServiceLoader;
+  TestServiceLoader* url_loader = new TestServiceLoader;
+  TestServiceLoader* scheme_loader = new TestServiceLoader;
+  sm.set_default_loader(scoped_ptr<ServiceLoader>(default_loader));
+  sm.SetLoaderForURL(scoped_ptr<ServiceLoader>(url_loader), GURL("test:test1"));
+  sm.SetLoaderForScheme(scoped_ptr<ServiceLoader>(scheme_loader), "test");
 
   // test::test1 should go to url_loader.
   InterfacePipe<TestService, AnyInterface> pipe1;
   sm.Connect(GURL("test:test1"), pipe1.handle_to_peer.Pass());
-  EXPECT_EQ(1, url_loader.num_loads());
-  EXPECT_EQ(0, scheme_loader.num_loads());
-  EXPECT_EQ(0, default_loader.num_loads());
+  EXPECT_EQ(1, url_loader->num_loads());
+  EXPECT_EQ(0, scheme_loader->num_loads());
+  EXPECT_EQ(0, default_loader->num_loads());
 
   // test::test2 should go to scheme loader.
   InterfacePipe<TestService, AnyInterface> pipe2;
   sm.Connect(GURL("test:test2"), pipe2.handle_to_peer.Pass());
-  EXPECT_EQ(1, url_loader.num_loads());
-  EXPECT_EQ(1, scheme_loader.num_loads());
-  EXPECT_EQ(0, default_loader.num_loads());
+  EXPECT_EQ(1, url_loader->num_loads());
+  EXPECT_EQ(1, scheme_loader->num_loads());
+  EXPECT_EQ(0, default_loader->num_loads());
 
   // http::test1 should go to default loader.
   InterfacePipe<TestService, AnyInterface> pipe3;
   sm.Connect(GURL("http:test1"), pipe3.handle_to_peer.Pass());
-  EXPECT_EQ(1, url_loader.num_loads());
-  EXPECT_EQ(1, scheme_loader.num_loads());
-  EXPECT_EQ(1, default_loader.num_loads());
+  EXPECT_EQ(1, url_loader->num_loads());
+  EXPECT_EQ(1, scheme_loader->num_loads());
+  EXPECT_EQ(1, default_loader->num_loads());
 }
 
 TEST_F(ServiceManagerTest, Interceptor) {
   ServiceManager sm;
-  TestServiceLoader default_loader;
   TestServiceInterceptor interceptor;
-  sm.set_default_loader(&default_loader);
+  TestServiceLoader* default_loader = new TestServiceLoader;
+  sm.set_default_loader(scoped_ptr<ServiceLoader>(default_loader));
   sm.SetInterceptor(&interceptor);
 
   std::string url("test:test3");
@@ -231,7 +276,7 @@ TEST_F(ServiceManagerTest, Interceptor) {
   sm.Connect(GURL(url), pipe1.handle_to_peer.Pass());
   EXPECT_EQ(1, interceptor.call_count());
   EXPECT_EQ(url, interceptor.url_spec());
-  EXPECT_EQ(1, default_loader.num_loads());
+  EXPECT_EQ(1, default_loader->num_loads());
 }
 
 }  // namespace mojo
diff --git a/mojo/shell/android/mojo_main.cc b/mojo/shell/android/mojo_main.cc
index a912864..88ce11e 100644
--- a/mojo/shell/android/mojo_main.cc
+++ b/mojo/shell/android/mojo_main.cc
@@ -54,9 +54,6 @@ class NativeViewportServiceLoader : public ServiceLoader {
   scoped_ptr<Application> app_;
 };
 
-LazyInstance<scoped_ptr<NativeViewportServiceLoader> >
-    g_viewport_service_loader = LAZY_INSTANCE_INITIALIZER;
-
 }  // namspace
 
 static void Init(JNIEnv* env, jclass clazz, jobject context) {
@@ -98,9 +95,8 @@ static void Start(JNIEnv* env, jclass clazz, jobject context, jstring jurl) {
 
   shell::Context* shell_context = new shell::Context();
   shell_context->set_activity(activity.obj());
-  g_viewport_service_loader.Get().reset(new NativeViewportServiceLoader());
   shell_context->service_manager()->SetLoaderForURL(
-      g_viewport_service_loader.Get().get(),
+      make_scoped_ptr<ServiceLoader>(new NativeViewportServiceLoader),
       GURL("mojo:mojo_native_viewport_service"));
 
   g_context.Get().reset(shell_context);
diff --git a/mojo/shell/context.cc b/mojo/shell/context.cc
index 2ba4aef..df86d14 100644
--- a/mojo/shell/context.cc
+++ b/mojo/shell/context.cc
@@ -59,12 +59,12 @@ Context::Context()
   else
     runner_factory.reset(new InProcessDynamicServiceRunnerFactory());
 
-  dynamic_service_loader_.reset(
-      new DynamicServiceLoader(this, runner_factory.Pass()));
-  service_manager_.set_default_loader(dynamic_service_loader_.get());
-  native_viewport_service_loader_.reset(new NativeViewportServiceLoader(this));
-  service_manager_.SetLoaderForURL(native_viewport_service_loader_.get(),
-                                    GURL("mojo:mojo_native_viewport_service"));
+  service_manager_.set_default_loader(
+      scoped_ptr<ServiceLoader>(
+          new DynamicServiceLoader(this, runner_factory.Pass())));
+  service_manager_.SetLoaderForURL(
+      scoped_ptr<ServiceLoader>(new NativeViewportServiceLoader(this)),
+      GURL("mojo:mojo_native_viewport_service"));
 
   if (cmdline->HasSwitch(switches::kSpy)) {
     spy_.reset(new mojo::Spy(&service_manager_,
@@ -73,7 +73,7 @@ Context::Context()
 }
 
 Context::~Context() {
-  service_manager_.set_default_loader(NULL);
+  service_manager_.set_default_loader(scoped_ptr<ServiceLoader>());
 }
 
 }  // namespace shell
diff --git a/mojo/shell/context.h b/mojo/shell/context.h
index 57ed5d5..c9acdc4 100644
--- a/mojo/shell/context.h
+++ b/mojo/shell/context.h
@@ -46,9 +46,7 @@ class Context {
   Storage storage_;
   Loader loader_;
   ServiceManager service_manager_;
-  scoped_ptr<DynamicServiceLoader> dynamic_service_loader_;
   scoped_ptr<Spy> spy_;
-  scoped_ptr<NativeViewportServiceLoader> native_viewport_service_loader_;
 #if defined(OS_ANDROID)
   base::android::ScopedJavaGlobalRef<jobject> activity_;
 #endif  // defined(OS_ANDROID)