Make sure that Content Handled application can be connected multiple times.

mojo/application_manager/background_shell_application_loader.cc

Index: mojo/application_manager/background_shell_application_loader.cc
diff --git a/mojo/application_manager/background_shell_application_loader.cc b/mojo/application_manager/background_shell_application_loader.cc
index e6664db369b505a115b3364a6a7066d972726618..bb7e9b710ac2c73b1c4a865f566a3e4d12e7fde2 100644
--- a/mojo/application_manager/background_shell_application_loader.cc
+++ b/mojo/application_manager/background_shell_application_loader.cc
@@ -10,29 +10,6 @@
 
 namespace mojo {
 
-class BackgroundShellApplicationLoader::BackgroundLoader {
- public:
-  explicit BackgroundLoader(ApplicationLoader* loader) : loader_(loader) {}
-  ~BackgroundLoader() {}
-
-  void Load(ApplicationManager* manager,
-            const GURL& url,
-            ScopedMessagePipeHandle shell_handle) {
-    scoped_refptr<LoadCallbacks> callbacks(
-        new ApplicationLoader::SimpleLoadCallbacks(shell_handle.Pass()));
-    loader_->Load(manager, url, callbacks);
-  }
-
-  void OnApplicationError(ApplicationManager* manager, const GURL& url) {
-    loader_->OnApplicationError(manager, url);
-  }
-
- private:
-  ApplicationLoader* loader_;  // Owned by BackgroundShellApplicationLoader
-
-  DISALLOW_COPY_AND_ASSIGN(BackgroundLoader);
-};
-
 BackgroundShellApplicationLoader::BackgroundShellApplicationLoader(
     scoped_ptr<ApplicationLoader> real_loader,
     const std::string& thread_name,
@@ -40,8 +17,7 @@ BackgroundShellApplicationLoader::BackgroundShellApplicationLoader(
     : loader_(real_loader.Pass()),
       message_loop_type_(message_loop_type),
       thread_name_(thread_name),
-      message_loop_created_(true, false),
-      background_loader_(NULL) {
+      message_loop_created_(true, false) {
 }
 
 BackgroundShellApplicationLoader::~BackgroundShellApplicationLoader() {
@@ -52,11 +28,9 @@ BackgroundShellApplicationLoader::~BackgroundShellApplicationLoader() {
 void BackgroundShellApplicationLoader::Load(
     ApplicationManager* manager,
     const GURL& url,
-    scoped_refptr<LoadCallbacks> callbacks) {
-  ScopedMessagePipeHandle shell_handle = callbacks->RegisterApplication();
-  if (!shell_handle.is_valid())
-    return;
-
+    ScopedMessagePipeHandle shell_handle,
+    LoadCallback callbacks) {
+  DCHECK(shell_handle.is_valid());
   if (!thread_) {
     // TODO(tim): It'd be nice if we could just have each Load call
     // result in a new thread like DynamicService{Loader, Runner}. But some
@@ -72,12 +46,9 @@ void BackgroundShellApplicationLoader::Load(
 
   task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(
-          &BackgroundShellApplicationLoader::LoadOnBackgroundThread,
-          base::Unretained(this),
-          manager,
-          url,
-          base::Owned(new ScopedMessagePipeHandle(shell_handle.Pass()))));
+      base::Bind(&BackgroundShellApplicationLoader::LoadOnBackgroundThread,
+                 base::Unretained(this), manager, url,
+                 base::Passed(&shell_handle)));
 }
 
 void BackgroundShellApplicationLoader::OnApplicationError(
@@ -99,8 +70,6 @@ void BackgroundShellApplicationLoader::Run() {
   message_loop_created_.Signal();
   loop.Run();
 
-  delete background_loader_;
-  background_loader_ = NULL;
   // Destroy |loader_| on the thread it's actually used on.
   loader_.reset();
 }
@@ -108,20 +77,16 @@ void BackgroundShellApplicationLoader::Run() {
 void BackgroundShellApplicationLoader::LoadOnBackgroundThread(
     ApplicationManager* manager,
     const GURL& url,
-    ScopedMessagePipeHandle* shell_handle) {
+    ScopedMessagePipeHandle shell_handle) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (!background_loader_)
-    background_loader_ = new BackgroundLoader(loader_.get());
-  background_loader_->Load(manager, url, shell_handle->Pass());
+  loader_->Load(manager, url, shell_handle.Pass(), LoadCallback());

[Aaron Boodman, 2014/11/19 16:09:58]

I thought you said in a previous review that there was some problem with calling
Run() on an empty callback.

I like this better than binding to an empty function, though. Did you convince
yourself it was OK?

[qsr, 2014/11/19 16:37:22]

 Nope, it is clearly not ok. But this code was using SimpleLoadCallbacks, which
was using NOT_REACHED on LoadWithContentHandler, so the code is expecting this
callback to never be called.

[Aaron Boodman, 2014/11/20 00:59:01]

I see. In that case, we should probably make this more self-explanatory somehow.
Like passing a function that immediately NOTREACHED()'s.

[qsr, 2014/11/20 10:02:59]

Done.

 }
 
 void BackgroundShellApplicationLoader::OnApplicationErrorOnBackgroundThread(
     ApplicationManager* manager,
     const GURL& url) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (!background_loader_)
-    background_loader_ = new BackgroundLoader(loader_.get());
-  background_loader_->OnApplicationError(manager, url);
+  loader_->OnApplicationError(manager, url);
 }
 
 }  // namespace mojo