[BrowserSwitcher] Go to chrome://browser-switch before switching

Instead of immediately switching browsers, Chrome will now navigate to
chrome://browser-switch, which takes care of switching browsers and
closing the tab.

Bug: 887005
Change-Id: I195b6ad80b18b7cd6fd2c42a42d03853c14a80a1
Reviewed-on: https://chromium-review.googlesource.com/c/1456680
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630122}

chrome/browser/browser_resources.grd

@@ -84,6 +84,8 @@
       <if expr="is_win or is_macosx or (is_linux and not is_chromeos)">
         <include name="IDR_BROWSER_SWITCHER_APP_HTML" file="resources\browser_switcher\app.html" compress="gzip" allowexternalscript="true" type="BINDATA" />
         <include name="IDR_BROWSER_SWITCHER_APP_JS" file="resources\browser_switcher\app.js" compress="gzip" type="BINDATA" />
+        <include name="IDR_BROWSER_SWITCHER_BROWSER_SWITCHER_PROXY_HTML" file="resources\browser_switcher\browser_switcher_proxy.html" compress="gzip" allowexternalscript="true" type="BINDATA" />
+        <include name="IDR_BROWSER_SWITCHER_BROWSER_SWITCHER_PROXY_JS" file="resources\browser_switcher\browser_switcher_proxy.js" compress="gzip" type="BINDATA" />
         <include name="IDR_BROWSER_SWITCHER_BROWSER_SWITCH_HTML" file="resources\browser_switcher\browser_switcher.html" compress="gzip" allowexternalscript="true" type="BINDATA" />
       </if>
       <if expr="is_win">

chrome/browser/browser_switcher/browser_switcher_navigation_throttle.cc

@@ -8,7 +8,6 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/metrics/histogram_macros.h"
 #include "base/task/post_task.h"
 #include "chrome/browser/browser_switcher/alternative_browser_driver.h"
 #include "chrome/browser/browser_switcher/browser_switcher_service.h"
@@ -16,33 +15,29 @@
 #include "chrome/browser/browser_switcher/browser_switcher_sitelist.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_list.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/webui_url_constants.h"
 #include "components/navigation_interception/intercept_navigation_throttle.h"
 #include "components/navigation_interception/navigation_params.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/guest_mode.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
+#include "net/base/url_util.h"
 
 namespace browser_switcher {
 
 namespace {
 
-// Returns true if there's only 1 tab left open in this profile. Incognito
-// window tabs count as the same profile.
-bool IsLastTab(const Profile* profile) {
-  profile = profile->GetOriginalProfile();
-  int tab_count = 0;
-  for (const Browser* browser : *BrowserList::GetInstance()) {
-    if (browser->profile()->GetOriginalProfile() != profile)
-      continue;
-    tab_count += browser->tab_strip_model()->count();
-    if (tab_count > 1)
-      return false;
-  }
-  return true;
+// Open 'chrome://browser-switch/?url=...' in the current tab.
+void OpenBrowserSwitchPage(content::WebContents* web_contents,
+                           const GURL& url,
+                           ui::PageTransition transition_type) {
+  GURL about_url(chrome::kChromeUIBrowserSwitchURL);
+  about_url = net::AppendQueryParameter(about_url, "url", url.spec());
+  content::OpenURLParams params(about_url, content::Referrer(),
+                                WindowOpenDisposition::CURRENT_TAB,
+                                transition_type, false);
+  web_contents->OpenURL(params);
 }
 
 bool MaybeLaunchAlternativeBrowser(
@@ -73,31 +68,11 @@ bool MaybeLaunchAlternativeBrowser(
     return true;
   }
 
-  // TODO(nicolaso): Once the chrome://browserswitch page is implemented, open
-  // that instead of immediately launching the browser/closing the tab.
-  bool success;
-  {
-    SCOPED_UMA_HISTOGRAM_TIMER("BrowserSwitcher.LaunchTime");
-    success = service->driver()->TryLaunch(url);
-    UMA_HISTOGRAM_BOOLEAN("BrowserSwitcher.LaunchSuccess", success);
-  }
-  if (success) {
-    const Profile* profile =
-        Profile::FromBrowserContext(web_contents->GetBrowserContext());
-    if (service->prefs().KeepLastTab() && IsLastTab(profile)) {
-      // TODO(nicolaso): Show the NTP after cancelling the navigation.
-    } else {
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::BindOnce(&content::WebContents::ClosePage,
-                                    base::Unretained(web_contents)));
-    }
-    return true;
-  }
-
-  // Launching the browser failed, fall back to loading the page normally.
-  //
-  // TODO(nicolaso): Show an error page instead.
-  return false;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&OpenBrowserSwitchPage, base::Unretained(web_contents),
+                     url, params.transition_type()));
+  return true;
 }
 
 }  // namespace

chrome/browser/browser_switcher/browser_switcher_navigation_throttle_unittest.cc

@@ -9,13 +9,11 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
-#include "chrome/browser/browser_switcher/alternative_browser_driver.h"
 #include "chrome/browser/browser_switcher/browser_switcher_prefs.h"
 #include "chrome/browser/browser_switcher/browser_switcher_service.h"
 #include "chrome/browser/browser_switcher/browser_switcher_service_factory.h"
 #include "chrome/browser/browser_switcher/browser_switcher_sitelist.h"
 #include "chrome/browser/browser_switcher/ieem_sitelist_parser.h"
-#include "chrome/browser/browser_switcher/mock_alternative_browser_driver.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/navigation_throttle.h"
@@ -60,11 +58,6 @@ class BrowserSwitcherNavigationThrottleTest
         BrowserSwitcherServiceFactory::GetForBrowserContext(
             web_contents()->GetBrowserContext());
 
-    std::unique_ptr<MockAlternativeBrowserDriver> driver =
-        std::make_unique<MockAlternativeBrowserDriver>();
-    driver_ = driver.get();
-    service->SetDriverForTesting(std::move(driver));
-
     std::unique_ptr<MockBrowserSwitcherSitelist> sitelist =
         std::make_unique<MockBrowserSwitcherSitelist>();
     sitelist_ = sitelist.get();
@@ -81,11 +74,9 @@ class BrowserSwitcherNavigationThrottleTest
     return BrowserSwitcherNavigationThrottle::MaybeCreateThrottleFor(handle);
   }
 
-  MockAlternativeBrowserDriver* driver() { return driver_; }
   MockBrowserSwitcherSitelist* sitelist() { return sitelist_; }
 
  private:
-  MockAlternativeBrowserDriver* driver_;
   MockBrowserSwitcherSitelist* sitelist_;
 };
 
@@ -100,7 +91,6 @@ TEST_F(BrowserSwitcherNavigationThrottleTest, ShouldIgnoreNavigation) {
 
 TEST_F(BrowserSwitcherNavigationThrottleTest, LaunchesOnStartRequest) {
   EXPECT_CALL(*sitelist(), ShouldSwitch(_)).WillOnce(Return(true));
-  EXPECT_CALL(*driver(), TryLaunch(_)).WillOnce(Return(true));
   std::unique_ptr<MockNavigationHandle> handle =
       CreateMockNavigationHandle(GURL("https://example.com/"));
   std::unique_ptr<NavigationThrottle> throttle =
@@ -114,7 +104,6 @@ TEST_F(BrowserSwitcherNavigationThrottleTest, LaunchesOnRedirectRequest) {
   EXPECT_CALL(*sitelist(), ShouldSwitch(_))
       .WillOnce(Return(false))
       .WillOnce(Return(true));
-  EXPECT_CALL(*driver(), TryLaunch(_)).WillOnce(Return(true));
   std::unique_ptr<MockNavigationHandle> handle =
       CreateMockNavigationHandle(GURL("https://yahoo.com/"));
   std::unique_ptr<NavigationThrottle> throttle =
@@ -126,14 +115,4 @@ TEST_F(BrowserSwitcherNavigationThrottleTest, LaunchesOnRedirectRequest) {
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_F(BrowserSwitcherNavigationThrottleTest, FallsBackToLoadingNormally) {
-  EXPECT_CALL(*sitelist(), ShouldSwitch(_)).WillOnce(Return(true));
-  EXPECT_CALL(*driver(), TryLaunch(_)).WillOnce(Return(false));
-  std::unique_ptr<MockNavigationHandle> handle =
-      CreateMockNavigationHandle(GURL("https://yahoo.com/"));
-  std::unique_ptr<NavigationThrottle> throttle =
-      CreateNavigationThrottle(handle.get());
-  EXPECT_EQ(NavigationThrottle::PROCEED, throttle->WillStartRequest());
-}
-
 }  // namespace browser_switcher

chrome/browser/resources/browser_switcher/BUILD.gn

@@ -8,6 +8,7 @@ js_type_check("closure_compile") {
   deps = [
     ":app",
     ":browser_switcher",
+    ":browser_switcher_proxy",
   ]
 }
 
@@ -19,3 +20,9 @@ js_library("app") {
 
 js_library("browser_switcher") {
 }
+
+js_library("browser_switcher_proxy") {
+  deps = [
+    "//ui/webui/resources/js:cr",
+  ]
+}

chrome/browser/resources/browser_switcher/app.html

@@ -2,6 +2,7 @@
 
 <link rel="import" href="chrome://resources/html/cr.html">
 <link rel="import" href="chrome://resources/cr_elements/shared_vars_css.html">
+<link rel="import" href="browser_switcher_proxy.html">
 
 <dom-module id="browser-switcher-app">
   <template>

chrome/browser/resources/browser_switcher/app.js

@@ -31,10 +31,8 @@ Polymer({
       return;
     }
 
-    // TODO(nicolaso): Use a BrowserProxy to make testing easier.
-    cr.sendWithPromise('launchAlternativeBrowser', this.url_).then(() => {
-      chrome.send('closeTab');
-    });
+    const proxy = browser_switcher.BrowserSwitcherProxyImpl.getInstance();
+    proxy.launchAlternativeBrowserAndCloseTab(this.url_);
   },
 });
 })();

chrome/browser/resources/browser_switcher/browser_switcher_proxy.html

+<link rel="import" href="chrome://resources/html/cr.html">
+<script src="browser_switcher_proxy.js"></script>

chrome/browser/resources/browser_switcher/browser_switcher_proxy.js

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+cr.define('browser_switcher', function() {
+  /** @interface */
+  class BrowserSwitcherProxy {
+    /**
+     * @param {string} url URL to open in alternative browser.
+     * @return {Promise} A promise that can fail if unable to launch. It will
+     *     never resolve, because the tab closes if this succeeds.
+     */
+    launchAlternativeBrowserAndCloseTab(url) {}
+  }
+
+  /** @implements {settings.BrowserSwitcherProxy} */
+  class BrowserSwitcherProxyImpl {
+    /** @override */
+    launchAlternativeBrowserAndCloseTab(url) {
+      return cr.sendWithPromise('launchAlternativeBrowserAndCloseTab', url);
+    }
+  }
+
+  cr.addSingletonGetter(BrowserSwitcherProxyImpl);
+
+  return {
+    BrowserSwitcherProxy: BrowserSwitcherProxy,
+    BrowserSwitcherProxyImpl: BrowserSwitcherProxyImpl
+  };
+});

chrome/browser/ui/webui/browser_switcher/browser_switch_ui.cc

@@ -8,8 +8,16 @@
 
 #include "base/bind.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/values.h"
+#include "chrome/browser/browser_switcher/alternative_browser_driver.h"
+#include "chrome/browser/browser_switcher/browser_switcher_service.h"
+#include "chrome/browser/browser_switcher/browser_switcher_service_factory.h"
+#include "chrome/browser/browser_switcher/browser_switcher_sitelist.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/dark_mode_handler.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/grit/browser_resources.h"
@@ -23,6 +31,27 @@
 
 namespace {
 
+// Returns true if there's only 1 tab left open in this profile. Incognito
+// window tabs count as the same profile.
+bool IsLastTab(const Profile* profile) {
+  profile = profile->GetOriginalProfile();
+  int tab_count = 0;
+  for (const Browser* browser : *BrowserList::GetInstance()) {
+    if (browser->profile()->GetOriginalProfile() != profile)
+      continue;
+    tab_count += browser->tab_strip_model()->count();
+    if (tab_count > 1)
+      return false;
+  }
+  return true;
+}
+
+browser_switcher::BrowserSwitcherService* GetBrowserSwitcherService(
+    content::WebUI* web_ui) {
+  return browser_switcher::BrowserSwitcherServiceFactory::GetForBrowserContext(
+      web_ui->GetWebContents()->GetBrowserContext());
+}
+
 content::WebUIDataSource* CreateBrowserSwitchUIHTMLSource(
     content::WebUI* web_ui) {
   content::WebUIDataSource* source =
@@ -37,6 +66,11 @@ content::WebUIDataSource* CreateBrowserSwitchUIHTMLSource(
   source->AddResourcePath("app.js", IDR_BROWSER_SWITCHER_APP_JS);
   source->AddResourcePath("browser_switch.html",
                           IDR_BROWSER_SWITCHER_BROWSER_SWITCH_HTML);
+  source->AddResourcePath("browser_switcher_proxy.html",
+                          IDR_BROWSER_SWITCHER_BROWSER_SWITCHER_PROXY_HTML);
+  source->AddResourcePath("browser_switcher_proxy.js",
+                          IDR_BROWSER_SWITCHER_BROWSER_SWITCHER_PROXY_JS);
+
   source->SetDefaultResource(IDR_BROWSER_SWITCHER_BROWSER_SWITCH_HTML);
   source->UseGzip();
 
@@ -53,12 +87,12 @@ class BrowserSwitchHandler : public content::WebUIMessageHandler {
 
  private:
   // Launches the given URL in the configured alternative browser. Acts as a
-  // bridge for |AlternativeBrowserDriver::TryLaunch()|.
-  void HandleLaunchAlternativeBrowser(const base::ListValue* args);
-
-  // Closes the current tab, since calling |window.close()| from JavaScript is
-  // not normally allowed.
-  void HandleCloseTab(const base::ListValue* args);
+  // bridge for |AlternativeBrowserDriver::TryLaunch()|. Then, if that succeeds,
+  // closes the current tab.
+  //
+  // If it fails, the JavaScript promise is rejected. If it succeeds, the
+  // JavaScript promise is not resolved, because we close the tab anyways.
+  void HandleLaunchAlternativeBrowserAndCloseTab(const base::ListValue* args);
 
   DISALLOW_COPY_AND_ASSIGN(BrowserSwitchHandler);
 };
@@ -68,22 +102,57 @@ BrowserSwitchHandler::~BrowserSwitchHandler() = default;
 
 void BrowserSwitchHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
-      "launchAlternativeBrowser",
-      base::BindRepeating(&BrowserSwitchHandler::HandleLaunchAlternativeBrowser,
-                          base::Unretained(this)));
-
-  web_ui()->RegisterMessageCallback(
-      "closeTab", base::BindRepeating(&BrowserSwitchHandler::HandleCloseTab,
-                                      base::Unretained(this)));
+      "launchAlternativeBrowserAndCloseTab",
+      base::BindRepeating(
+          &BrowserSwitchHandler::HandleLaunchAlternativeBrowserAndCloseTab,
+          base::Unretained(this)));
 }
 
-void BrowserSwitchHandler::HandleLaunchAlternativeBrowser(
+void BrowserSwitchHandler::HandleLaunchAlternativeBrowserAndCloseTab(
     const base::ListValue* args) {
-  // Not yet implemented.
-}
-
-void BrowserSwitchHandler::HandleCloseTab(const base::ListValue* args) {
-  // Not yet implemented.
+  DCHECK(args);
+  AllowJavascript();
+
+  std::string callback_id = args->GetList()[0].GetString();
+  std::string url_spec = args->GetList()[1].GetString();
+  GURL url(url_spec);
+
+  auto* service = GetBrowserSwitcherService(web_ui());
+  bool should_switch = service->sitelist()->ShouldSwitch(url);
+  if (!url.is_valid() || !should_switch) {
+    // This URL shouldn't open in an alternative browser. Abort launch, because
+    // something weird is going on (e.g. race condition from a new sitelist
+    // being loaded).
+    RejectJavascriptCallback(args->GetList()[0], base::Value());
+    return;
+  }
+
+  bool success;
+  {
+    SCOPED_UMA_HISTOGRAM_TIMER("BrowserSwitcher.LaunchTime");
+    success = service->driver()->TryLaunch(url);
+    UMA_HISTOGRAM_BOOLEAN("BrowserSwitcher.LaunchSuccess", success);
+  }
+
+  if (!success) {
+    RejectJavascriptCallback(args->GetList()[0], base::Value());
+    return;
+  }
+
+  // TODO(nicolaso): Find a fix: when the last tab closes, restarting Chrome
+  // causes it to immediately open the alternative browser, and then close
+  // Chrome again.
+  auto* profile = Profile::FromWebUI(web_ui());
+
+  if (service->prefs().KeepLastTab() && IsLastTab(profile)) {
+    // TODO(nicolaso): Show the NTP after cancelling the navigation.
+  } else {
+    // We don't need to resolve the promise, because the tab will close anyways.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&content::WebContents::ClosePage,
+                       base::Unretained(web_ui()->GetWebContents())));
+  }
 }
 
 }  // namespace

chrome/common/webui_url_constants.cc

@@ -266,6 +266,7 @@ const char kChromeUISandboxHost[] = "sandbox";
 #if defined(OS_WIN) || defined(OS_MACOSX) || \
     (defined(OS_LINUX) && !defined(OS_CHROMEOS))
 const char kChromeUIBrowserSwitchHost[] = "browser-switch";
+const char kChromeUIBrowserSwitchURL[] = "chrome://browser-switch/";
 #endif
 
 #if (defined(OS_LINUX) && defined(TOOLKIT_VIEWS)) || defined(USE_AURA)

chrome/common/webui_url_constants.h

@@ -262,6 +262,7 @@ extern const char kChromeUISandboxHost[];
 #if defined(OS_WIN) || defined(OS_MACOSX) || \
     (defined(OS_LINUX) && !defined(OS_CHROMEOS))
 extern const char kChromeUIBrowserSwitchHost[];
+extern const char kChromeUIBrowserSwitchURL[];
 #endif
 
 #if (defined(OS_LINUX) && defined(TOOLKIT_VIEWS)) || defined(USE_AURA)