[BrowserSwitcher] Add Mac OS support

This enables the build on Mac OS X, and refactors
AlternativeBrowserDriver to work correctly on Mac OS.

Bug: 881589
Change-Id: I6e73d33d83af5bc1c0d8bb3ec3e76fcf8409ee2e
Reviewed-on: https://chromium-review.googlesource.com/c/1283835Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601570}

chrome/browser/BUILD.gn

@@ -3438,10 +3438,9 @@ jumbo_split_static_library("browser") {
     ]
   }
 
-  if (is_win || (is_linux && !is_chromeos)) {
+  if (is_win || is_mac || is_desktop_linux) {
     sources += [
       "browser_switcher/alternative_browser_driver.h",
-      "browser_switcher/alternative_browser_driver_linux.cc",
       "browser_switcher/alternative_browser_driver_win.cc",
       "browser_switcher/alternative_browser_launcher.cc",
       "browser_switcher/alternative_browser_launcher.h",
@@ -3458,6 +3457,9 @@ jumbo_split_static_library("browser") {
       "browser_switcher/ieem_sitelist_parser.cc",
       "browser_switcher/ieem_sitelist_parser.h",
     ]
+    if (is_mac || is_desktop_linux) {
+      sources += [ "browser_switcher/alternative_browser_driver_posix.cc" ]
+    }
   }
 
   if (is_chromeos && use_cras) {

chrome/browser/browser_switcher/alternative_browser_driver.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_BROWSER_SWITCHER_ALTERNATIVE_BROWSER_DRIVER_H_
 #define CHROME_BROWSER_BROWSER_SWITCHER_ALTERNATIVE_BROWSER_DRIVER_H_
 
+#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/strings/string_piece_forward.h"
@@ -51,17 +52,13 @@ class AlternativeBrowserDriverImpl : public AlternativeBrowserDriver {
   void SetBrowserParameters(const base::ListValue* parameters) override;
   bool TryLaunch(const GURL& url) override;
 
-  using StringType = base::FilePath::StringType;
-
-  // Appends the arguments in |raw_args| to |argv|, performing substitution of
-  // "${url}" at the same time. If none of |raw_args| contains "${url}", the URL
-  // is appended as the last argument.
-  static void AppendCommandLineArguments(
-      std::vector<StringType>* argv,
-      const std::vector<StringType>& raw_args,
-      const GURL& url);
+  // Create the CommandLine object that would be used to launch an external
+  // process.
+  base::CommandLine CreateCommandLine(const GURL& url);
 
  private:
+  using StringType = base::FilePath::StringType;
+
 #if defined(OS_WIN)
   bool TryLaunchWithDde(const GURL& url);
   bool TryLaunchWithExec(const GURL& url);

chrome/browser/browser_switcher/alternative_browser_driver_linux.cc → chrome/browser/browser_switcher/alternative_browser_driver_posix.cc

@@ -4,11 +4,11 @@
 
 #include "chrome/browser/browser_switcher/alternative_browser_driver.h"
 
-#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/process/launch.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "chrome/browser/browser_switcher/alternative_browser_launcher.h"
 #include "url/gurl.h"
 
@@ -22,13 +22,23 @@ namespace {
 
 const char kUrlVarName[] = "${url}";
 
+#if defined(OS_MACOSX)
+const char kChromeExecutableName[] = "Google Chrome";
+const char kFirefoxExecutableName[] = "Firefox";
+const char kOperaExecutableName[] = "Opera";
+const char kSafariExecutableName[] = "Safari";
+#else
 const char kChromeExecutableName[] = "google-chrome";
 const char kFirefoxExecutableName[] = "firefox";
 const char kOperaExecutableName[] = "opera";
+#endif
 
 const char kChromeVarName[] = "${chrome}";
 const char kFirefoxVarName[] = "${firefox}";
 const char kOperaVarName[] = "${opera}";
+#if defined(OS_MACOSX)
+const char kSafariVarName[] = "${safari}";
+#endif
 
 const struct {
   const char* var_name;
@@ -37,6 +47,9 @@ const struct {
     {kChromeVarName, kChromeExecutableName},
     {kFirefoxVarName, kFirefoxExecutableName},
     {kOperaVarName, kOperaExecutableName},
+#if defined(OS_MACOSX)
+    {kSafariVarName, kSafariExecutableName},
+#endif
 };
 
 bool ExpandUrlVarName(std::string* arg, const GURL& url) {
@@ -81,6 +94,33 @@ void ExpandEnvironmentVariables(std::string* arg) {
   std::swap(out, *arg);
 }
 
+#if defined(OS_MACOSX)
+bool ContainsUrlVarName(const std::vector<std::string>& tokens) {
+  return std::any_of(tokens.begin(), tokens.end(),
+                     [](const std::string& token) {
+                       return token.find(kUrlVarName) != std::string::npos;
+                     });
+}
+#endif  // defined(OS_MACOSX)
+
+// static
+void AppendCommandLineArguments(base::CommandLine* cmd_line,
+                                const std::vector<std::string>& raw_args,
+                                const GURL& url,
+                                bool always_append_url) {
+  bool contains_url = false;
+  for (const auto& arg : raw_args) {
+    std::string expanded_arg = arg;
+    ExpandTilde(&expanded_arg);
+    ExpandEnvironmentVariables(&expanded_arg);
+    if (ExpandUrlVarName(&expanded_arg, url))
+      contains_url = true;
+    cmd_line->AppendArg(expanded_arg);
+  }
+  if (always_append_url && !contains_url)
+    cmd_line->AppendArg(url.spec());
+}
+
 }  // namespace
 
 AlternativeBrowserDriver::~AlternativeBrowserDriver() {}
@@ -90,6 +130,13 @@ AlternativeBrowserDriverImpl::~AlternativeBrowserDriverImpl() {}
 
 void AlternativeBrowserDriverImpl::SetBrowserPath(base::StringPiece path) {
   browser_path_ = path.as_string();
+#if defined(OS_MACOSX)
+  // Unlike most POSIX platforms, MacOS always has another browser than Chrome,
+  // so admins don't have to explicitly configure one.
+  if (browser_path_.empty()) {
+    browser_path_ = kSafariExecutableName;
+  }
+#endif
   for (const auto& mapping : kBrowserVarMappings) {
     if (!browser_path_.compare(mapping.var_name)) {
       browser_path_ = mapping.executable_name;
@@ -120,16 +167,7 @@ bool AlternativeBrowserDriverImpl::TryLaunch(const GURL& url) {
 
   CHECK(url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile());
 
-  const int max_num_args = browser_params_.size() + 2;
-  std::vector<std::string> argv;
-  argv.reserve(max_num_args);
-  std::string path = browser_path_;
-  ExpandTilde(&path);
-  ExpandEnvironmentVariables(&path);
-  argv.push_back(path);
-  AppendCommandLineArguments(&argv, browser_params_, url);
-
-  base::CommandLine cmd_line = base::CommandLine(argv);
+  auto cmd_line = CreateCommandLine(url);
   base::LaunchOptions options;
   // Don't close the alternative browser when Chrome exits.
   options.new_process_group = true;
@@ -140,23 +178,45 @@ bool AlternativeBrowserDriverImpl::TryLaunch(const GURL& url) {
   return true;
 }
 
-// static
-void AlternativeBrowserDriverImpl::AppendCommandLineArguments(
-    std::vector<std::string>* argv,
-    const std::vector<std::string>& raw_args,
+base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine(
     const GURL& url) {
-  // TODO(crbug/882520): Do environment variable and tilde expansion.
-  bool contains_url = false;
-  for (const auto& arg : raw_args) {
-    std::string expanded_arg = arg;
-    ExpandTilde(&expanded_arg);
-    ExpandEnvironmentVariables(&expanded_arg);
-    if (ExpandUrlVarName(&expanded_arg, url))
-      contains_url = true;
-    argv->push_back(expanded_arg);
+  std::string path = browser_path_;
+  ExpandTilde(&path);
+  ExpandEnvironmentVariables(&path);
+
+#if defined(OS_MACOSX)
+  // On MacOS, if the path doesn't start with a '/', it's probably not an
+  // executable path. It is probably a name for an application, e.g. "Safari" or
+  // "Google Chrome". Those can be launched using the `open(1)' command.
+  //
+  // It may use the following syntax (first syntax):
+  //     open -a <browser_path> <url> [--args <browser_params...>]
+  //
+  // Or, if |browser_params| contains "${url}" (second syntax):
+  //     open -a <browser_path> --args <browser_params...>
+  //
+  // Safari only supports the first syntax.
+  if (!browser_path_.empty() && browser_path_[0] != '/') {
+    base::CommandLine cmd_line(std::vector<std::string>{"open"});
+    cmd_line.AppendArg("-a");
+    cmd_line.AppendArg(path);
+    if (!ContainsUrlVarName(browser_params_)) {
+      // First syntax.
+      cmd_line.AppendArg(url.spec());
+    }
+    if (!browser_params_.empty()) {
+      // First or second syntax, depending on what is in |browser_params_|.
+      cmd_line.AppendArg("--args");
+      AppendCommandLineArguments(&cmd_line, browser_params_, url,
+                                 /* always_append_url */ false);
+    }
+    return cmd_line;
   }
-  if (!contains_url)
-    argv->push_back(url.spec());
+#endif
+  base::CommandLine cmd_line(std::vector<std::string>{path});
+  AppendCommandLineArguments(&cmd_line, browser_params_, url,
+                             /* always_append_url */ true);
+  return cmd_line;
 }
 
 }  // namespace browser_switcher

chrome/browser/browser_switcher/alternative_browser_driver_win.cc

@@ -4,7 +4,6 @@
 
 #include "chrome/browser/browser_switcher/alternative_browser_driver.h"
 
-#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/process/launch.h"
 #include "base/strings/utf_string_conversions.h"
@@ -113,6 +112,23 @@ void ExpandEnvironmentVariables(std::wstring* arg) {
     *arg = out.get();
 }
 
+void AppendCommandLineArguments(base::CommandLine* cmd_line,
+                                const std::vector<std::wstring>& raw_args,
+                                const GURL& url) {
+  std::wstring url_spec = base::UTF8ToWide(url.spec());
+  std::vector<std::wstring> command_line;
+  bool contains_url = false;
+  for (const auto& arg : raw_args) {
+    std::wstring expanded_arg = arg;
+    ExpandEnvironmentVariables(&expanded_arg);
+    if (ExpandUrlVarName(&expanded_arg, url_spec))
+      contains_url = true;
+    cmd_line->AppendArgNative(expanded_arg);
+  }
+  if (!contains_url)
+    cmd_line->AppendArgNative(url_spec);
+}
+
 }  // namespace
 
 AlternativeBrowserDriver::~AlternativeBrowserDriver() {}
@@ -217,17 +233,8 @@ bool AlternativeBrowserDriverImpl::TryLaunchWithExec(const GURL& url) {
 
   CHECK(url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile());
 
-  // We know that there will be at most browser_params_.size() arguments, plus
-  // one for the executable, and possibly one for the URL.
-  const int max_num_args = browser_params_.size() + 2;
-  std::vector<std::wstring> argv;
-  argv.reserve(max_num_args);
-  std::wstring path = browser_path_;
-  ExpandEnvironmentVariables(&path);
-  argv.push_back(path);
-  AppendCommandLineArguments(&argv, browser_params_, url);
+  auto cmd_line = CreateCommandLine(url);
 
-  base::CommandLine cmd_line = base::CommandLine(argv);
   base::LaunchOptions options;
   if (!base::LaunchProcess(cmd_line, options).IsValid()) {
     LOG(ERROR) << "Could not start the alternative browser! Error: "
@@ -237,23 +244,13 @@ bool AlternativeBrowserDriverImpl::TryLaunchWithExec(const GURL& url) {
   return true;
 }
 
-void AlternativeBrowserDriverImpl::AppendCommandLineArguments(
-    std::vector<std::wstring>* argv,
-    const std::vector<std::wstring>& raw_args,
+base::CommandLine AlternativeBrowserDriverImpl::CreateCommandLine(
     const GURL& url) {
-  // TODO(crbug/882520): Do environment variable expansion.
-  std::wstring url_spec = base::UTF8ToWide(url.spec());
-  std::vector<std::wstring> command_line;
-  bool contains_url = false;
-  for (const auto& arg : raw_args) {
-    std::wstring expanded_arg = arg;
-    ExpandEnvironmentVariables(&expanded_arg);
-    if (ExpandUrlVarName(&expanded_arg, url_spec))
-      contains_url = true;
-    argv->push_back(expanded_arg);
-  }
-  if (!contains_url)
-    argv->push_back(url_spec);
+  std::wstring path = browser_path_;
+  ExpandEnvironmentVariables(&path);
+  base::CommandLine cmd_line(std::vector<std::wstring>{path});
+  AppendCommandLineArguments(&cmd_line, browser_params_, url);
+  return cmd_line;
 }
 
 }  // namespace browser_switcher

chrome/browser/chrome_content_browser_client.cc

@@ -412,11 +412,8 @@
 #include "chrome/browser/webshare/share_service_impl.h"
 #endif
 
-#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
-#include "chrome/browser/browser_switcher/browser_switcher_navigation_throttle.h"
-#endif
-
-#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
 #include "chrome/browser/browser_switcher/browser_switcher_navigation_throttle.h"
 #endif
 
@@ -4173,7 +4170,8 @@ ChromeContentBrowserClient::CreateThrottlesForNavigation(
   if (previews_lite_page_throttle)
     throttles.push_back(std::move(previews_lite_page_throttle));
 
-#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
   std::unique_ptr<content::NavigationThrottle> browser_switcher_throttle =
       browser_switcher::BrowserSwitcherNavigationThrottle ::
           MaybeCreateThrottleFor(handle);

chrome/browser/prefs/browser_prefs.cc

@@ -311,7 +311,8 @@
 #include "chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.h"
 #endif
 
-#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
 #include "chrome/browser/browser_switcher/browser_switcher_prefs.h"
 #endif
 
@@ -743,7 +744,8 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   safe_browsing::PostCleanupSettingsResetter::RegisterProfilePrefs(registry);
 #endif
 
-#if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
   browser_switcher::prefs::RegisterProfilePrefs(registry);
 #endif
 

chrome/test/BUILD.gn

@@ -2032,10 +2032,6 @@ test("browser_tests") {
         "../browser/renderer_context_menu/spelling_options_submenu_observer_browsertest.cc",
       ]
     }
-    if (is_win || (is_linux && !is_chromeos)) {
-      sources +=
-          [ "../browser/browser_switcher/ieem_sitelist_parser_browsertest.cc" ]
-    }
     if (!is_posix || is_chromeos) {
       sources -= [ "../common/time_format_browsertest.cc" ]
     }
@@ -2043,6 +2039,7 @@ test("browser_tests") {
     if (is_mac || is_win || (is_linux && !is_chromeos)) {
       sources += [
         # Tests for non mobile and non CrOS (includes Linux, Win, Mac).
+        "../browser/browser_switcher/ieem_sitelist_parser_browsertest.cc",
         "../browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc",
         "../browser/profiles/profile_statistics_browsertest.cc",
       ]
@@ -3346,13 +3343,8 @@ test("unit_tests") {
   }
 
   if (is_win || (is_linux && !is_chromeos)) {
-    sources += [
-      "../browser/browser_switcher/alternative_browser_driver_unittest.cc",
-      "../browser/browser_switcher/alternative_browser_launcher_unittest.cc",
-      "../browser/browser_switcher/browser_switcher_navigation_throttle_unittest.cc",
-      "../browser/browser_switcher/browser_switcher_sitelist_unittest.cc",
-      "../browser/ui/views/quit_instruction_bubble_controller_unittest.cc",
-    ]
+    sources +=
+        [ "../browser/ui/views/quit_instruction_bubble_controller_unittest.cc" ]
   }
 
   if (enable_native_notifications) {
@@ -4440,7 +4432,13 @@ test("unit_tests") {
   }
 
   if (is_win || is_mac || (is_linux && !is_chromeos)) {
-    sources += [ "../browser/password_manager/password_store_signin_notifier_impl_unittest.cc" ]
+    sources += [
+      "../browser/browser_switcher/alternative_browser_driver_unittest.cc",
+      "../browser/browser_switcher/alternative_browser_launcher_unittest.cc",
+      "../browser/browser_switcher/browser_switcher_navigation_throttle_unittest.cc",
+      "../browser/browser_switcher/browser_switcher_sitelist_unittest.cc",
+      "../browser/password_manager/password_store_signin_notifier_impl_unittest.cc",
+    ]
   }
 
   if (enable_widevine && is_win) {