ExtensionNavigationThrottle: Enforce the same rules on redirect as we

chrome/browser/extensions/extension_navigation_throttle_unittest.cc

Index: chrome/browser/extensions/extension_navigation_throttle_unittest.cc
diff --git a/chrome/browser/extensions/extension_navigation_throttle_unittest.cc b/chrome/browser/extensions/extension_navigation_throttle_unittest.cc
index 35a546bc2a0bef1734d17c771b2f89279e7c6c50..d241173060b667300eb8370da8c9920d7077e689 100644
--- a/chrome/browser/extensions/extension_navigation_throttle_unittest.cc
+++ b/chrome/browser/extensions/extension_navigation_throttle_unittest.cc
@@ -62,19 +62,44 @@ class ExtensionNavigationThrottleUnitTest
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
-  // Checks that trying to navigate the given |host| to |url| results in the
-  // |expected_result|.
+  // Checks that trying to navigate the given |host| to |extension_url| results
+  // in the |expected_result|.
   void CheckTestCase(content::RenderFrameHost* host,
-                     const GURL& url,
+                     const GURL& extension_url,
                      NavigationThrottle::ThrottleCheckResult expected_result) {
+    // First subtest: direct navigation to |extension_url|.
     std::unique_ptr<content::NavigationHandle> handle =
-        content::NavigationHandle::CreateNavigationHandleForTesting(url, host);
+        content::NavigationHandle::CreateNavigationHandleForTesting(
+            extension_url, host);
     EXPECT_EQ(expected_result,
               handle->CallWillStartRequestForTesting(
                   /*is_post=*/false, content::Referrer(),
                   /*has_user_gesture=*/false, ui::PAGE_TRANSITION_LINK,
                   /*is_external_protocol=*/false))
-        << url;
+        << extension_url;
+
+    // Reset the handle for a second subtest: server redirect to
+    // |extension_url|. Currently, the same rules apply, but we cancel rather
+    // than block such requests.
+    GURL http_url("https://example.com");
+    handle = content::NavigationHandle::CreateNavigationHandleForTesting(
+        http_url, host);
+    NavigationThrottle::ThrottleCheckResult expected_redirect_result =

[Devlin, 2017/05/18 15:22:44]

This is a bit strange, because now |expected_result| isn't always the actual
expected result.  I would suggest either:
a) passing in a bool "expect_succeed", and then constructing a
direct_navigation_expected_result = success ? PROCEED : BLOCK_REQUEST and a
redirect_navigation_expected_result = success ? PROCEED : CANCEL, or
b) passing in both a direct and redirect expected result.

Since it looks like the expected result is currently always either proceed or
block, a) would probably be sufficient and is a little less verbose.

[ncarter (slow), 2017/05/22 22:55:59]

Really, it's just a limitation of the throttle system in non-plznavigate that we
need to round the BLOCKs down to CANCELs currently.

My hope is that in the future, we'll be able to use the same errors for
WillRedirect as we do for WillStartRequest.

I've added some TODO comments w/ bug links to clarify what's going on.

+        (expected_result == NavigationThrottle::PROCEED)
+            ? NavigationThrottle::PROCEED
+            : NavigationThrottle::CANCEL;
+    EXPECT_EQ(NavigationThrottle::PROCEED,
+              handle->CallWillStartRequestForTesting(
+                  /*is_post=*/false, content::Referrer(),
+                  /*has_user_gesture=*/false, ui::PAGE_TRANSITION_LINK,
+                  /*is_external_protocol=*/false))
+        << http_url;
+    EXPECT_EQ(expected_redirect_result,
+              handle->CallWillRedirectRequestForTesting(
+                  extension_url,
+                  /*new_method_is_post=*/false, http_url,
+                  /*new_is_external_protocol=*/false))
+        << extension_url;
   }
 
   const Extension* extension() { return extension_.get(); }