Make interstitial links open in a new tab

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

Index: chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
index dc3f6d722288f88ae490741a783d25070d77a73b..bd2e6e52ae24bb5e4d4ed905b5fa7c199e2b25d3 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
@@ -42,6 +42,7 @@
 #include "components/safe_browsing_db/database_manager.h"
 #include "components/safe_browsing_db/test_database_manager.h"
 #include "components/safe_browsing_db/util.h"
+#include "components/security_interstitials/content/security_interstitial_controller_client.h"
 #include "components/security_interstitials/core/controller_client.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/security_interstitials/core/urls.h"
@@ -57,6 +58,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/mock_cert_verifier.h"
@@ -729,6 +731,25 @@ class SafeBrowsingBlockingPageBrowserTest
         ->hit_report_sent();
   }
 
+  void MockHelpCenterUrl(InterstitialPage* interstitial_page) {

[estark, 2017/06/27 00:36:21]

nit: maybe add a brief comment above this method to explain why it exists (e.g.:
"Modify the interstitial to use a test server URL for its help center link.")

[sperigo, 2017/06/27 17:31:58]

Done.

+    ASSERT_TRUE(https_server_.Start());
+    scoped_refptr<net::X509Certificate> cert(https_server_.GetCertificate());
+    net::CertVerifyResult verify_result;
+    verify_result.is_issued_by_known_root = true;
+    verify_result.verified_cert = cert;
+    verify_result.cert_status = 0;
+    mock_cert_verifier()->AddResultForCert(cert.get(), verify_result, net::OK);
+
+    SafeBrowsingBlockingPage* sb_interstitial =
+        static_cast<SafeBrowsingBlockingPage*>(
+            interstitial_page->GetDelegateForTesting());
+    security_interstitials::SecurityInterstitialControllerClient* client =
+        sb_interstitial->controller();
+
+    const GURL mock_help_center_url = https_server_.GetURL("/title1.html");

[estark, 2017/06/27 00:36:21]

nit: const GURL&
or just inline it below:
SetBaseHelpCenterUrlForTesting(https_server_.GetURL(...))

[sperigo, 2017/06/27 17:31:58]

Done.

+    client->SetBaseHelpCenterUrlForTesting(mock_help_center_url);
+  }
+
  protected:
   TestThreatDetailsFactory details_factory_;
 
@@ -795,12 +816,33 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, DontProceed) {
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, VisitWhitePaper) {
   SetupWarningAndNavigate(browser());
 
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+  WebContents* interstitial_tab =

[estark, 2017/06/27 00:36:21]

nit: ASSERT_TRUE(interstitial_tab) after this line to make the test fail nicely
instead of crashing if for some reason the tab doesn't exist

[sperigo, 2017/06/27 17:31:58]

Done.

+      browser()->tab_strip_model()->GetActiveWebContents();
+
   EXPECT_EQ(VISIBLE, GetVisibility("whitepaper-link"));
-  EXPECT_TRUE(ClickAndWaitForDetach("whitepaper-link"));
+  content::TestNavigationObserver nav_observer(nullptr);
+  nav_observer.StartWatchingNewWebContents();
+  EXPECT_TRUE(Click("whitepaper-link"));
 
-  AssertNoInterstitial(false);  // Assert the interstitial is gone
-  EXPECT_EQ(GetWhitePaperUrl(),
-            browser()->tab_strip_model()->GetActiveWebContents()->GetURL());
+  nav_observer.Wait();
+
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+  EXPECT_EQ(1, browser()->tab_strip_model()->active_index());
+
+  AssertNoInterstitial(false);  // Assert the interstitial is not present in

[estark, 2017/06/27 00:36:21]

nit: comment formatting is a little weird, you can probably just it on the line
above

[sperigo, 2017/06/27 17:31:59]

Done.

+                                // the foreground tab.
+
+  // Foreground tab displays the help center.
+  WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents();

[estark, 2017/06/27 00:36:21]

nit: ASSERT_TRUE(new_tab) after this line to make the test fail nicely instead
of crashing if for some reason the tab doesn't exist

[sperigo, 2017/06/27 17:31:58]

Done.

+  EXPECT_EQ(GetWhitePaperUrl(), new_tab->GetURL());
+
+  // Interstitial should still display in the background tab.
+  browser()->tab_strip_model()->ActivateTabAt(0, true);
+  EXPECT_EQ(0, browser()->tab_strip_model()->active_index());
+  EXPECT_EQ(interstitial_tab,
+            browser()->tab_strip_model()->GetActiveWebContents());
+  EXPECT_TRUE(YesInterstitial());
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, Proceed) {
@@ -1109,17 +1151,34 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, LearnMore) {
   SetupWarningAndNavigate(browser());
-  EXPECT_TRUE(ClickAndWaitForDetach("learn-more-link"));
-  AssertNoInterstitial(false);  // Assert the interstitial is gone
 
-  // We are in the help page.
-  EXPECT_EQ(
-      GURL("https://support.google.com/chrome/answer/99020").GetWithEmptyPath(),
-      browser()
-          ->tab_strip_model()
-          ->GetActiveWebContents()
-          ->GetURL()
-          .GetWithEmptyPath());
+  WebContents* interstitial_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(interstitial_tab);
+
+  MockHelpCenterUrl(interstitial_tab->GetInterstitialPage());
+
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+
+  content::TestNavigationObserver nav_observer(nullptr);
+  nav_observer.StartWatchingNewWebContents();
+  SendCommand(security_interstitials::CMD_OPEN_HELP_CENTER);
+  nav_observer.Wait();
+
+  // A new tab has been opened.
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+
+  // Interstitial does not display in the foreground tab.
+  EXPECT_EQ(1, browser()->tab_strip_model()->active_index());
+  WebContents* new_tab = browser()->tab_strip_model()->GetWebContentsAt(1);

[estark, 2017/06/27 00:36:21]

same nit about ASSERT_TRUE(new_tab)

[sperigo, 2017/06/27 17:31:58]

Done.

+  EXPECT_FALSE(new_tab->ShowingInterstitialPage());
+
+  // Interstitial still displays in the background tab.
+  browser()->tab_strip_model()->ActivateTabAt(0, true);
+  EXPECT_EQ(0, browser()->tab_strip_model()->active_index());
+  EXPECT_EQ(interstitial_tab,
+            browser()->tab_strip_model()->GetActiveWebContents());
+  EXPECT_TRUE(YesInterstitial());
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,