Simplify resolution of mojo: URLs.

shell/context.cc

Index: shell/context.cc
diff --git a/shell/context.cc b/shell/context.cc
index af889d47d5e0a275099ee62b57557fd20e2dacd5..08fbe006f5c519c9ba8933137ff91c052c8f0233 100644
--- a/shell/context.cc
+++ b/shell/context.cc
@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
+#include "base/path_service.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
@@ -28,6 +29,7 @@
 #include "shell/application_manager/application_manager.h"
 #include "shell/dynamic_application_loader.h"
 #include "shell/external_application_listener.h"
+#include "shell/filename_util.h"
 #include "shell/in_process_dynamic_service_runner.h"
 #include "shell/out_of_process_dynamic_service_runner.h"
 #include "shell/switches.h"
@@ -37,12 +39,6 @@ namespace mojo {
 namespace shell {
 namespace {
 
-// These mojo: URLs are loaded directly from the local filesystem. They
-// correspond to shared libraries bundled alongside the mojo_shell.
-const char* kLocalMojoURLs[] = {
-    "mojo:network_service",
-};
-
 // Used to ensure we only init once.
 class Setup {
  public:
@@ -106,18 +102,58 @@ void InitContentHandlers(DynamicApplicationLoader* loader,
   }
 }
 
-bool ConfigureURLMappings(const std::string& mappings,
-                          mojo::shell::MojoURLResolver* resolver) {
-  base::StringPairs pairs;
-  if (!base::SplitStringIntoKeyValuePairs(mappings, '=', ',', &pairs))
+GURL AddTrailingSlashIfNeeded(const GURL& url) {
+  if (!url.has_path() || *url.path().rbegin() == '/')
+    return url;
+
+  std::string path(url.path() + '/');
+  GURL::Replacements replacements;
+  replacements.SetPathStr(path);
+  return url.ReplaceComponents(replacements);
+}
+
+bool ConfigureURLMappings(base::CommandLine *command_line,
+                          MojoURLResolver *resolver) {

[abarth-chromium, 2015/01/23 02:23:15]

s/base::CommandLine */base::CommandLine* /
Same for MojoURLResolver

[abarth-chromium, 2015/01/23 02:23:15]

s/base::CommandLine */base::CommandLine* /
Same for MojoURLResolver

[Nick Bray (chromium), 2015/01/24 01:09:04]

Done.

[Nick Bray (chromium), 2015/01/24 01:09:04]

Done.

+  // By default assume that the local apps reside alongside the shell.
+  base::FilePath shell_dir;
+  PathService::Get(base::DIR_MODULE, &shell_dir);
+  GURL shell_dir_url = AddTrailingSlashIfNeeded(FilePathToFileURL(shell_dir));

[Aaron Boodman, 2015/01/23 02:34:06]

Is there really ambiguity about whether FilePathToFileURL will return a trailing
slash or not? Seems like it would always do the same thing for directories.

[Nick Bray (chromium), 2015/01/24 01:09:04]

I believe it never returns a trailing slash.  There will still be ambiguity when
dealing with user input, and I'd be inclined to sanitize even unambiguous input
for self-similarity.  I see virtue in your question, however.  I could turn it
into a straight-up append, but with a mild preference not to.  What's your
inclination?

+
+  // Configure the resolution of unknown mojo: URLs.
+  GURL base_url;
+  if (command_line->HasSwitch(switches::kOrigin)) {
+    base_url = GURL(command_line->GetSwitchValueASCII(switches::kOrigin));
+    base_url = AddTrailingSlashIfNeeded(base_url);

[Aaron Boodman, 2015/01/23 02:34:06]

Why add the trailing slash in this case? Seems like the person providing the
switch should add the trailing slash if that's what they want.

[Nick Bray (chromium), 2015/01/24 01:09:04]

Ther difference is subtle:
GURL("http://foo/bar").Resolve("baz") => "http://foo/baz"
but
GURL("http://foo/bar/").Resolve("baz") => "http://foo/bar/baz"

It is almost certain the user does not intend the first, and if the user makes
that mistake the resulting behavior may be subtle an confounding.  Forcing the
trailing slash for something that should always be a directory is a good safety
measure.

[Aaron Boodman, 2015/01/27 00:37:13]

In that case, I think that we should ensure the caller does *not* add a trailing
slash, and spit out an error if they do. Then always add one.

[Nick Bray (chromium), 2015/01/27 01:44:35]

Requiring no slash will interact badly with tab completion.  That's the next
step - make mojo_shell behave as expected on the command line.
See: https://codereview.chromium.org/873253003/
This will make it easier to point mojo_shell at .nexe files while I bootstrap.

I tried doing it the other way - requiring a slash - but there are a bunch of
tests that pass in URLs without trailing slashes.  After changing a few I
backtracked and gave up for this CL.  This sort of interface change needs to be
done in a separate CL.

Also, either way, mojo_shell is pretty crappy about giving user feedback. 
Establishing a pattern for user feedback would be  another yak shave - likely
all the command line parameters would need to be interpreted, massaged, and
shoved into a config structure up front.

So, damned if you do, damned if you don't.  I think things are the way the are
for a reason. :)

[Aaron Boodman, 2015/01/27 07:29:54]

OK, I was initially going to suggest requiring a slash, but thought it would be
nicer to have to type less.

Either way, but I do think it would be a good change (later).
Agree.
Agree.
The reason is just that it hasn't been fixed yet.

+  } else {
+    base_url = shell_dir_url;
+  }
+  if (!base_url.is_valid()) {

[Aaron Boodman, 2015/01/23 02:34:06]

no braces.

[Nick Bray (chromium), 2015/01/24 01:09:03]

Sad panda about this style - goto fail, and all that, but you are correct. :/

     return false;
-  using StringPair = std::pair<std::string, std::string>;
-  for (const StringPair& pair : pairs) {
-    const GURL from(pair.first);
-    const GURL to(pair.second);
-    if (!from.is_valid() || !to.is_valid())
+  }
+  resolver->SetBaseURL(base_url);
+
+  // The network service must be loaded from the filesystem.
+  // This mapping is done before the command line URL mapping are processed, so
+  // that it can be overridden.
+  resolver->AddCustomMapping(GURL("mojo:network_service"),
+                             shell_dir_url.Resolve("network_service.mojo"));
+
+  // Command line URL mapping.
+  if (command_line->HasSwitch(switches::kURLMappings)) {
+    const std::string mappings =
+      command_line->GetSwitchValueASCII(switches::kURLMappings);
+
+    base::StringPairs pairs;
+    if (!base::SplitStringIntoKeyValuePairs(mappings, '=', ',', &pairs))
       return false;
-    resolver->AddCustomMapping(from, to);
+    using StringPair = std::pair<std::string, std::string>;
+    for (const StringPair& pair : pairs) {
+      const GURL from(pair.first);
+      const GURL to(pair.second);
+      if (!from.is_valid() || !to.is_valid())
+        return false;
+      resolver->AddCustomMapping(from, to);
+    }
   }
   return true;
 }
@@ -146,8 +182,9 @@ bool Context::Init() {
   task_runners_.reset(
       new TaskRunners(base::MessageLoop::current()->message_loop_proxy()));
 
-  for (size_t i = 0; i < arraysize(kLocalMojoURLs); ++i)
-    mojo_url_resolver_.AddLocalFileMapping(GURL(kLocalMojoURLs[i]));
+  if (!ConfigureURLMappings(command_line, &mojo_url_resolver_)) {

[Aaron Boodman, 2015/01/23 02:34:06]

This file uses the no-braces-for-one-line-if style. Please match.

[Nick Bray (chromium), 2015/01/24 01:09:04]

Done.

+    return false;
+  }
 
   if (command_line->HasSwitch(switches::kEnableExternalApplications)) {
     listener_.reset(new ExternalApplicationListener(
@@ -163,16 +200,6 @@ bool Context::Init() {
         base::Bind(&ApplicationManager::RegisterExternalApplication,
                    base::Unretained(&application_manager_)));
   }
-  if (command_line->HasSwitch(switches::kOrigin)) {
-    mojo_url_resolver()->SetBaseURL(
-        GURL(command_line->GetSwitchValueASCII(switches::kOrigin)));
-  }
-  if (command_line->HasSwitch(switches::kURLMappings) &&
-      !ConfigureURLMappings(
-          command_line->GetSwitchValueASCII(switches::kURLMappings),
-          mojo_url_resolver())) {
-    return false;
-  }
 
   scoped_ptr<DynamicServiceRunnerFactory> runner_factory;
   if (command_line->HasSwitch(switches::kEnableMultiprocess))