[HBD] Intercept navigation to Flash download page

chrome/browser/plugins/flash_download_interception_unittest.cc

+// Copyright 2016 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.
+
+#include "chrome/browser/plugins/flash_download_interception.h"
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/browser/navigation_handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+using content::NavigationHandle;
+using content::NavigationThrottle;
+
+class FlashDownloadInterceptionTest : public ChromeRenderViewHostTestHarness {
+ public:
+  FlashDownloadInterceptionTest() {}
+};
+
+TEST_F(FlashDownloadInterceptionTest, MaybeCreateThrottleFor) {
+  std::unique_ptr<NavigationThrottle> throttle;

[tommycli, 2016/09/01 01:08:20]

If you split up the two cases below, these common setup things can be put into
the SetUp() function of your test class

[trizzofo, 2016/09/01 01:56:14]

Done.

+  std::unique_ptr<NavigationHandle> flash_navigation_handle_ =
+      NavigationHandle::CreateNavigationHandleForTesting(
+          GURL("https://get.adobe.com/flashplayer"), main_rfh());
+  std::unique_ptr<NavigationHandle> flash_slash_navigation_handle_ =
+      NavigationHandle::CreateNavigationHandleForTesting(
+          GURL("https://get.adobe.com/flashplayer/"), main_rfh());
+  std::unique_ptr<NavigationHandle> example_navigation_handle_ =
+      NavigationHandle::CreateNavigationHandleForTesting(
+          GURL("https://www.example.com"), main_rfh());
+  // Changes the status of the navigation handles. This is needed because
+  // the NavigationHandle's status needs to be different from INITIAL so that
+  // the HasUserGesture method can be used.
+  flash_navigation_handle_->CallWillProcessResponseForTesting(main_rfh());
+  flash_slash_navigation_handle_->CallWillProcessResponseForTesting(main_rfh());
+  example_navigation_handle_->CallWillProcessResponseForTesting(main_rfh());
+
+  {

[tommycli, 2016/09/01 01:08:20]

These scoped portions suggest to me it should be divided into two tests:

TEST_F(FlashDownloadInterceptionTest, PreferHtmlOverPluginsOff)

and

TEST_F(FlashDownloadInterceptionTest, PreferHtmlOverPluginsOn)

[trizzofo, 2016/09/01 01:56:14]

Done.

+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndDisableFeature(features::kPreferHtmlOverPlugins);
+
+    flash_navigation_handle_->SetHasUserGestureForTesting(true);
+    throttle = FlashDownloadInterception::MaybeCreateThrottleFor(
+        flash_navigation_handle_.get());
+    EXPECT_EQ(nullptr, throttle);
+  }
+
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeature(features::kPreferHtmlOverPlugins);
+
+    flash_navigation_handle_->SetHasUserGestureForTesting(false);
+    throttle = FlashDownloadInterception::MaybeCreateThrottleFor(
+        flash_navigation_handle_.get());
+    EXPECT_EQ(nullptr, throttle);
+
+    flash_navigation_handle_->SetHasUserGestureForTesting(true);
+    throttle = FlashDownloadInterception::MaybeCreateThrottleFor(
+        flash_navigation_handle_.get());
+    EXPECT_NE(nullptr, throttle);

[tommycli, 2016/09/01 01:08:20]

To give this test a bit more teeth in the non-nullptr case, also test the result
of throttle->WillStartRequest(); and potentially... WillRedirectRequest, though
I'm not sure what's supposed to happen in that case.

[trizzofo, 2016/09/01 17:58:56]

Done.

+
+    flash_slash_navigation_handle_->SetHasUserGestureForTesting(true);
+    throttle = FlashDownloadInterception::MaybeCreateThrottleFor(
+        flash_navigation_handle_.get());
+    EXPECT_NE(nullptr, throttle);
+
+    example_navigation_handle_->SetHasUserGestureForTesting(true);
+    throttle = FlashDownloadInterception::MaybeCreateThrottleFor(
+        example_navigation_handle_.get());
+    EXPECT_EQ(nullptr, throttle);
+  }
+}