Use new SafeBrowsing redirect tracking code in CWS pings.

chrome/browser/extensions/webstore_inline_installer_browsertest.cc

Index: chrome/browser/extensions/webstore_inline_installer_browsertest.cc
diff --git a/chrome/browser/extensions/webstore_inline_installer_browsertest.cc b/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
index 555ef2fd530f0a793edc4b7945e4c0c9640bc803..56a82afe5e3a3816b96150b611e68562c6ec2f44 100644
--- a/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
+++ b/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
@@ -116,7 +116,8 @@ class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
                                  content::RenderFrameHost* host,
                                  const std::string& extension_id,
                                  const GURL& requestor_url,
-                                 const Callback& callback)
+                                 const Callback& callback,
+                                 bool enable_safebrowsing_redirects)
       : WebstoreInlineInstaller(
             contents,
             host,
@@ -137,6 +138,10 @@ class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
     return WebstoreInlineInstaller::CheckRequestorAlive();
   }
 
+  bool SafeBrowsingNavigationEventsEnabled() const override {
+    return enable_safebrowsing_redirects_;
+  }
+
   // Tests that care about the actual arguments to the install callback can use
   // this to receive a copy in |install_result_target|.
   void set_install_result_target(
@@ -164,13 +169,22 @@ class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
   // arguments.
   std::unique_ptr<InstallResult>* install_result_target_;
 
+  // This can be set by tests that want to use the new SafeBrowsing redirect
+  // tracker.
+  bool enable_safebrowsing_redirects_;
+
   ProgrammableInstallPrompt* programmable_prompt_;
 };
 
 class WebstoreInlineInstallerForTestFactory :
     public WebstoreInlineInstallerFactory {
  public:
-  WebstoreInlineInstallerForTestFactory() : last_installer_(nullptr) {}
+  WebstoreInlineInstallerForTestFactory()
+      : last_installer_(nullptr), enable_safebrowsing_redirects_(false) {}
+  explicit WebstoreInlineInstallerForTestFactory(
+      bool enable_safebrowsing_redirects)
+      : last_installer_(nullptr),
+        enable_safebrowsing_redirects_(enable_safebrowsing_redirects) {}
   ~WebstoreInlineInstallerForTestFactory() override {}
 
   WebstoreInlineInstallerForTest* last_installer() { return last_installer_; }
@@ -182,13 +196,16 @@ class WebstoreInlineInstallerForTestFactory :
       const GURL& requestor_url,
       const WebstoreStandaloneInstaller::Callback& callback) override {
     last_installer_ = new WebstoreInlineInstallerForTest(
-        contents, host, webstore_item_id, requestor_url, callback);
+        contents, host, webstore_item_id, requestor_url, callback,
+        enable_safebrowsing_redirects_);
     return last_installer_;
   }
 
  private:
   // The last installer that was created.
   WebstoreInlineInstallerForTest* last_installer_;
+
+  bool enable_safebrowsing_redirects_;
 };
 
 IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest,
@@ -371,7 +388,9 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest, DoubleInlineInstallTest) {
   RunTest("runTest");
 }
 
-class WebstoreInlineInstallerRedirectTest : public WebstoreInlineInstallerTest {
+class WebstoreInlineInstallerRedirectTest
+    : public WebstoreInlineInstallerTest,
+      public ::testing::WithParamInterface<bool> {
  public:
   WebstoreInlineInstallerRedirectTest() : cws_request_received_(false) {}
   ~WebstoreInlineInstallerRedirectTest() override {}
@@ -400,8 +419,19 @@ class WebstoreInlineInstallerRedirectTest : public WebstoreInlineInstallerTest {
 
 // Test that an install from a page arrived at via redirects includes the
 // redirect information in the webstore request.
-IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
+IN_PROC_BROWSER_TEST_P(WebstoreInlineInstallerRedirectTest,
                        IncludesRedirectData) {
+  const bool using_safe_browsing_tracker = GetParam();
+
+  // GURL pre_navigate_url = GenerateTestServerUrl(kAppDomain, "install.html");

[Devlin, 2017/04/13 15:03:07]

dead code?

[robertshield, 2017/04/21 00:40:41]

Oops, yes, thanks.

+  // ui_test_utils::NavigateToURL(browser(), pre_navigate_url);
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  TabHelper* tab_helper = TabHelper::FromWebContents(web_contents);
+  WebstoreInlineInstallerForTestFactory* factory =
+      new WebstoreInlineInstallerForTestFactory(using_safe_browsing_tracker);
+  tab_helper->SetWebstoreInlineInstallerFactoryForTests(factory);
+
   // Hand craft a url that will cause the test server to issue redirects.
   const std::vector<std::string> redirects = {kRedirect1Domain,
                                               kRedirect2Domain};
@@ -418,7 +448,11 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
 
   AutoAcceptInstall();
   ui_test_utils::NavigateToURL(browser(), install_url);
-  RunTest("runTest");
+
+  RunTestAsync("runTest");

[Devlin, 2017/04/13 15:03:07]

Do we need to make this an async run?

[robertshield, 2017/04/21 00:40:41]

It seems to hang if we don't. This became true when I added the
WebstoreInlineInstallerForTestFactory above. 

+  while (!ProgrammableInstallPrompt::Ready())
+    base::RunLoop().RunUntilIdle();
+  web_contents->Close();
 
   EXPECT_TRUE(cws_request_received_);
   ASSERT_NE(nullptr, cws_request_json_data_);
@@ -428,18 +462,31 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
   ASSERT_NE(nullptr, redirect_list);
 
   // Check that the expected domains are in the redirect list.
-  const std::vector<std::string> expected_redirect_domains = {
+  const std::set<std::string> expected_redirect_domains = {
       kRedirect1Domain, kRedirect2Domain, kAppDomain};
-  ASSERT_EQ(expected_redirect_domains.size(), redirect_list->GetSize());
-  int i = 0;
+
+  // The SafeBrowsing tracker has a much more liberal definition of "redirect"
+  // and it may (based on timing) pick up additional navigations that occur
+  // shortly before the navigation we mainly care about here. Be somewhat
+  // permissive in what we accept as redirect results.
+  ASSERT_LE(expected_redirect_domains.size(), redirect_list->GetSize());
+
   for (const auto& value : *redirect_list) {
     std::string value_string;
     ASSERT_TRUE(value->GetAsString(&value_string));
     GURL redirect_url(value_string);
-    EXPECT_EQ(expected_redirect_domains[i++], redirect_url.host());
+    EXPECT_TRUE(expected_redirect_domains.find(redirect_url.host()) !=
+                expected_redirect_domains.end());
   }
 }
 
+INSTANTIATE_TEST_CASE_P(NetRedirectTracking,
+                        WebstoreInlineInstallerRedirectTest,
+                        testing::Values(false));

[Devlin, 2017/04/13 15:03:07]

Optional: If you don't care much about the naming, I think you can use a single
call to instantiate the tests with testing::Bool() instead of
testing::Values(true) and testing::Values(false).  Up to you.

[robertshield, 2017/04/21 00:40:41]

Ah, true.. I think on the balance having the two more explicit names here makes
it slightly easier to parse test log output. 

+INSTANTIATE_TEST_CASE_P(SafeBrowsingRedirectTracking,
+                        WebstoreInlineInstallerRedirectTest,
+                        testing::Values(true));
+
 class WebstoreInlineInstallerListenerTest : public WebstoreInlineInstallerTest {
  public:
   WebstoreInlineInstallerListenerTest() {}