Allow local file to run though content handler.

mojo/shell/dynamic_application_loader.cc

Index: mojo/shell/dynamic_application_loader.cc
diff --git a/mojo/shell/dynamic_application_loader.cc b/mojo/shell/dynamic_application_loader.cc
index 71dcada0d129f2125307e42199d59ae6da248cc3..b7d745c770e1b67c9571615fde76f7d071bc5ab7 100644
--- a/mojo/shell/dynamic_application_loader.cc
+++ b/mojo/shell/dynamic_application_loader.cc
@@ -8,12 +8,16 @@
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/format_macros.h"
+#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "mojo/common/common_type_converters.h"
 #include "mojo/common/data_pipe_utils.h"
+#include "mojo/public/cpp/system/data_pipe.h"
 #include "mojo/services/public/interfaces/network/url_loader.mojom.h"
 #include "mojo/shell/context.h"
 #include "mojo/shell/filename_util.h"
@@ -23,6 +27,77 @@
 namespace mojo {
 namespace shell {
 
+namespace {
+
+void IgnoreResult(bool result) {
+}
+
+class LoaderResponse {

[Aaron Boodman, 2014/11/03 19:20:15]

Is there some better way to factor this?

It seems weird that we have the Loader/LocalLoader/RemoteLoader split, but then
this shared LoaderResponse that contains smarts about how to handle both local
and remote results.

Can the impl of converting to URL response go in the two loader implementations
somehow?

[qsr, 2014/11/04 11:58:34]

 Not easily. But I can definitively make this class virtual and have 2
implementation for the 2 cases. It wll prevent all of those tests.

[Aaron Boodman, 2014/11/04 18:05:01]

Really? It seems like all the methods of this class could become abstract on
Loader, then implemented in each of the two loader impls. What am I missing?

[qsr, 2014/11/05 10:21:54]

 We could do this, but it would mean adding the field of this class to each of
the loader. Those fields would only be valid in a certain phase of the loader,
and the associated method would only be called in this phase. This makes harder
to use the API, and I find it quite confusing. Using an external object for the
response that is created when needed and use as a parameter of the OnResponse
method seems way more natural for me.

+ public:
+  explicit LoaderResponse(URLResponsePtr response)
+      : file_needs_cleanup_(true), response_(response.Pass()) {}
+  LoaderResponse(const GURL& url, base::FilePath path)
+      : file_needs_cleanup_(false), url_(url), path_(path) {}
+
+  ~LoaderResponse() {
+    if (file_needs_cleanup_ && !path_.empty())
+      base::DeleteFile(path_, false);
+  }
+
+  URLResponsePtr AsURLResponse(base::TaskRunner* task_runner) {
+    if (response_ || path_.empty())
+      return response_.Pass();
+    URLResponsePtr response(URLResponse::New());
+    response->url = String::From(url_);
+    DataPipe data_pipe;
+    response->body = data_pipe.consumer_handle.Pass();
+    int64 file_size;
+    if (base::GetFileSize(path_, &file_size)) {
+      response->headers = Array<String>(1);
+      response->headers[0] =
+          base::StringPrintf("Content-Length: %" PRId64, file_size);
+    }
+    common::CopyFromFile(path_,
+                         data_pipe.producer_handle.Pass(),
+                         task_runner,
+                         base::Bind(&IgnoreResult));

[Aaron Boodman, 2014/11/04 18:05:00]

I think you can also do this?

base::Callback<void(bool)> foo;
common::CopyFromFile(..., foo);

[qsr, 2014/11/05 10:21:54]

Not really. See the comment in base/task_runner_util.h:30 and
http://crbug.com/162712

The bug is quite old, but given that the comment is still there, I prefer to
consider that the API should not allow for null callbacks.

+    return response.Pass();
+  }
+
+  void AsPath(base::TaskRunner* task_runner,
+              base::Callback<void(const base::FilePath&, bool)> callback) {
+    if (!path_.empty() || !response_) {
+      callback.Run(path_, base::PathExists(path_));
+      return;
+    }
+    base::CreateTemporaryFile(&path_);
+    common::CopyToFile(response_->body.Pass(),
+                       path_,
+                       task_runner,
+                       base::Bind(callback, path_));
+  }
+
+  std::string MimeType() {
+    if (response_)
+      return response_->mime_type;
+    return "";
+  }
+
+  std::string Url() {
+    if (response_)
+      return response_->url;
+    return String::From(url_);
+  }
+
+ private:
+  bool file_needs_cleanup_;
+  URLResponsePtr response_;
+  GURL url_;
+  base::FilePath path_;
+};
+
+}  // namespace
+
 // Encapsulates loading and running one individual application.
 //
 // Loaders are owned by DynamicApplicationLoader. DynamicApplicationLoader must
@@ -35,12 +110,14 @@ namespace shell {
 class DynamicApplicationLoader::Loader {
  public:
   Loader(Context* context,
+         MimeTypeToURLMap* mime_type_to_url,
          DynamicServiceRunnerFactory* runner_factory,
          scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks,
          const LoaderCompleteCallback& loader_complete_callback)
       : load_callbacks_(load_callbacks),
         loader_complete_callback_(loader_complete_callback),
         context_(context),
+        mime_type_to_url_(mime_type_to_url),
         runner_factory_(runner_factory),
         weak_ptr_factory_(this) {}
 
@@ -71,9 +148,34 @@ class DynamicApplicationLoader::Loader {
 
   void LoaderComplete() { loader_complete_callback_.Run(this); }
 
+  void OnResponse(scoped_ptr<LoaderResponse> response) {
+    MimeTypeToURLMap::iterator iter =
+        mime_type_to_url_->find(response->MimeType());
+    if (iter != mime_type_to_url_->end()) {
+      load_callbacks_->LoadWithContentHandler(
+          iter->second,
+          response->AsURLResponse(context_->task_runners()->blocking_pool()));
+      return;
+    }
+
+    LOG(INFO) << "Failed to find content handler for " << response->Url()
+              << " (mimetype: " << response->MimeType() << ")" << std::endl
+              << "Attempting to load as native library instead...";
+
+    // TODO(aa): Santify check that the thing we got looks vaguely like a mojo
+    // application. That could either mean looking for the platform-specific dll
+    // header, or looking for some specific mojo signature prepended to the
+    // library.
+
+    response->AsPath(
+        context_->task_runners()->blocking_pool(),
+        base::Bind(&Loader::RunLibrary, weak_ptr_factory_.GetWeakPtr()));
+  }
+
   scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks_;
   LoaderCompleteCallback loader_complete_callback_;
   Context* context_;
+  MimeTypeToURLMap* mime_type_to_url_;
 
  private:
   DynamicServiceRunnerFactory* runner_factory_;
@@ -86,14 +188,15 @@ class DynamicApplicationLoader::LocalLoader : public Loader {
  public:
   LocalLoader(const GURL& url,
               Context* context,
+              MimeTypeToURLMap* mime_type_to_url,
               DynamicServiceRunnerFactory* runner_factory,
               scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks,
               const LoaderCompleteCallback& loader_complete_callback)
       : Loader(context,
+               mime_type_to_url,
                runner_factory,
                load_callbacks,
-               loader_complete_callback),
-        weak_ptr_factory_(this) {
+               loader_complete_callback) {
     DCHECK(url.SchemeIsFile());
     url::RawCanonOutputW<1024> output;
     url::DecodeURLEscapeSequences(
@@ -106,19 +209,10 @@ class DynamicApplicationLoader::LocalLoader : public Loader {
     base::FilePath path(base::UTF16ToUTF8(decoded_path));
 #endif
 
-    // Async for consistency with network case.
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&LocalLoader::RunLibrary,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   path,
-                   base::PathExists(path)));
+    OnResponse(make_scoped_ptr(new LoaderResponse(url, path)));
   }
 
   ~LocalLoader() override {}
-
- private:
-  base::WeakPtrFactory<LocalLoader> weak_ptr_factory_;
 };
 
 // A loader for network files.
@@ -132,10 +226,10 @@ class DynamicApplicationLoader::NetworkLoader : public Loader {
                 scoped_refptr<ApplicationLoader::LoadCallbacks> load_callbacks,
                 const LoaderCompleteCallback& loader_complete_callback)
       : Loader(context,
+               mime_type_to_url,
                runner_factory,
                load_callbacks,
                loader_complete_callback),
-        mime_type_to_url_(mime_type_to_url),
         weak_ptr_factory_(this) {
     URLRequestPtr request(URLRequest::New());
     request->url = String::From(url);
@@ -152,11 +246,6 @@ class DynamicApplicationLoader::NetworkLoader : public Loader {
                                   weak_ptr_factory_.GetWeakPtr()));
   }
 
-  ~NetworkLoader() override {
-    if (!file_.empty())
-      base::DeleteFile(file_, false);
-  }
-
  private:
   void OnLoadComplete(URLResponsePtr response) {
     if (response->error) {
@@ -167,34 +256,10 @@ class DynamicApplicationLoader::NetworkLoader : public Loader {
       return;
     }
 
-    MimeTypeToURLMap::iterator iter =
-        mime_type_to_url_->find(response->mime_type);
-    if (iter != mime_type_to_url_->end()) {
-      load_callbacks_->LoadWithContentHandler(iter->second, response.Pass());
-      return;
-    }
-
-    LOG(INFO) << "Failed to find content handler for " << response->url
-              << " (mimetype: " << response->url << ")" << std::endl
-              << "Attempting to load as native library instead...";
-
-    // TODO(aa): Santify check that the thing we got looks vaguely like a mojo
-    // application. That could either mean looking for the platform-specific dll
-    // header, or looking for some specific mojo signature prepended to the
-    // library.
-
-    base::CreateTemporaryFile(&file_);
-    common::CopyToFile(
-        response->body.Pass(),
-        file_,
-        context_->task_runners()->blocking_pool(),
-        base::Bind(
-            &NetworkLoader::RunLibrary, weak_ptr_factory_.GetWeakPtr(), file_));
+    OnResponse(make_scoped_ptr(new LoaderResponse(response.Pass())));
   }
 
-  MimeTypeToURLMap* mime_type_to_url_;
   URLLoaderPtr url_loader_;
-  base::FilePath file_;
   base::WeakPtrFactory<NetworkLoader> weak_ptr_factory_;
 };
 
@@ -234,6 +299,7 @@ void DynamicApplicationLoader::Load(
   if (resolved_url.SchemeIsFile()) {
     loaders_.push_back(new LocalLoader(resolved_url,
                                        context_,
+                                       &mime_type_to_url_,
                                        runner_factory_.get(),
                                        load_callbacks,
                                        loader_complete_callback_));