[WebLayer] Open captive portal in a new tab

WebLayer now has support for opening in a new tab since
http://crrev.com/c/2228365. We can now fix the TODO about captive
portals opening in a new tab.

Bug: 1047130
Change-Id: I1d2bf894caef6d0cc8f536db6d687fce0397ce0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264879Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782106}

weblayer/browser/ssl_browsertest.cc

@@ -7,12 +7,15 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "base/scoped_observer.h"
 #include "build/build_config.h"
 #include "components/network_time/network_time_tracker.h"
 #include "components/security_interstitials/content/ssl_error_handler.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "weblayer/browser/browser_process.h"
 #include "weblayer/browser/weblayer_security_blocking_page_factory.h"
+#include "weblayer/public/browser.h"
+#include "weblayer/public/browser_observer.h"
 #include "weblayer/shell/browser/shell.h"
 #include "weblayer/test/interstitial_utils.h"
 #include "weblayer/test/load_completion_observer.h"
@@ -20,6 +23,37 @@
 #include "weblayer/test/weblayer_browser_test_utils.h"
 
 namespace weblayer {
+namespace {
+
+#if defined(OS_ANDROID)
+// Waits for a new tab to be created, and then load |url|.
+class NewTabWaiter : public BrowserObserver {
+ public:
+  NewTabWaiter(Browser* browser, const GURL& url) : url_(url) {
+    observer_.Add(browser);
+  }
+
+  void OnTabAdded(Tab* tab) override {
+    navigation_observer_ = std::make_unique<TestNavigationObserver>(
+        url_, TestNavigationObserver::NavigationEvent::kStart, tab);
+    run_loop_.Quit();
+  }
+
+  void Wait() {
+    if (!navigation_observer_)
+      run_loop_.Run();
+    navigation_observer_->Wait();
+  }
+
+ private:
+  GURL url_;
+  std::unique_ptr<TestNavigationObserver> navigation_observer_;
+  base::RunLoop run_loop_;
+  ScopedObserver<Browser, BrowserObserver> observer_{this};
+};
+#endif
+
+}  // namespace
 
 class SSLBrowserTest : public WebLayerBrowserTest {
  public:
@@ -166,13 +200,12 @@ class SSLBrowserTest : public WebLayerBrowserTest {
 
     // Note: The embedded test server cannot actually load the captive portal
     // login URL, so simply detect the start of the navigation to the page.
-    TestNavigationObserver navigation_observer(
-        WebLayerSecurityBlockingPageFactory::
-            GetCaptivePortalLoginPageUrlForTesting(),
-        TestNavigationObserver::NavigationEvent::kStart, shell());
+    NewTabWaiter waiter(shell()->browser(),
+                        WebLayerSecurityBlockingPageFactory::
+                            GetCaptivePortalLoginPageUrlForTesting());
     ExecuteScript(shell(), "window.certificateErrorPageController.openLogin();",
                   false /*use_separate_isolate*/);
-    navigation_observer.Wait();
+    waiter.Wait();
   }
 #endif
 

weblayer/browser/tab_impl.cc

@@ -151,16 +151,11 @@ NewTabType NewTabTypeFromWindowDisposition(WindowOpenDisposition disposition) {
 // Opens a captive portal login page in |web_contents|.
 void OpenCaptivePortalLoginTabInWebContents(
     content::WebContents* web_contents) {
-  // In Chrome this opens in a new tab, but WebLayer's TabImpl has no support
-  // for opening new tabs (its OpenURLFromTab() method DCHECKs if the
-  // disposition is not |CURRENT_TAB|).
-  // TODO(crbug.com/1047130): Revisit if TabImpl gets support for opening URLs
-  // in new tabs.
   content::OpenURLParams params(
       CaptivePortalServiceFactory::GetForBrowserContext(
           web_contents->GetBrowserContext())
           ->test_url(),
-      content::Referrer(), WindowOpenDisposition::CURRENT_TAB,
+      content::Referrer(), WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui::PAGE_TRANSITION_LINK, false);
   web_contents->OpenURL(params);
 }

weblayer/browser/weblayer_security_blocking_page_factory.cc

@@ -38,14 +38,10 @@ void OpenLoginPage(content::WebContents* web_contents) {
   // ChromeSecurityBlockingPageFactory::OpenLoginPage(), from which this is
   // adapted.
 #if defined(OS_ANDROID)
-  // In Chrome this opens in a new tab, but WebLayer's TabImpl has no support
-  // for opening new tabs (its OpenURLFromTab() method DCHECKs if the
-  // disposition is not |CURRENT_TAB|).
-  // TODO(crbug.com/1047130): Revisit if TabImpl gets support for opening URLs
-  // in new tabs.
-  content::OpenURLParams params(
-      GetCaptivePortalLoginPageUrlInternal(), content::Referrer(),
-      WindowOpenDisposition::CURRENT_TAB, ui::PAGE_TRANSITION_LINK, false);
+  content::OpenURLParams params(GetCaptivePortalLoginPageUrlInternal(),
+                                content::Referrer(),
+                                WindowOpenDisposition::NEW_FOREGROUND_TAB,
+                                ui::PAGE_TRANSITION_LINK, false);
   web_contents->OpenURL(params);
 #else
   NOTIMPLEMENTED();