MacViews: Enable views based Download recovery dialog behind secondary-ui-md flag.

chrome/browser/download/download_danger_prompt_browsertest.cc

Index: chrome/browser/download/download_danger_prompt_browsertest.cc
diff --git a/chrome/browser/download/download_danger_prompt_browsertest.cc b/chrome/browser/download/download_danger_prompt_browsertest.cc
index 49eeccfd342bdb53c2e7467aba6b67aeb3b1dea4..d28b92a7eee11ff3596fec6c4bb365b1cbbae660 100644
--- a/chrome/browser/download/download_danger_prompt_browsertest.cc
+++ b/chrome/browser/download/download_danger_prompt_browsertest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "build/build_config.h"
@@ -21,6 +22,7 @@
 #include "content/public/test/mock_download_item.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/ui_base_switches.h"
 #include "url/gurl.h"
 
 using ::testing::_;
@@ -29,23 +31,48 @@ using ::testing::Eq;
 using ::testing::Return;
 using ::testing::ReturnRef;
 using ::testing::SaveArg;
-using safe_browsing::ClientDownloadResponse;
-using safe_browsing::ClientSafeBrowsingReportRequest;
-using safe_browsing::SafeBrowsingService;
 
 namespace safe_browsing {
 
+namespace {
+
 const char kTestDownloadUrl[] = "http://evildownload.com";
 const char kDownloadResponseToken[] = "default_token";
 
-class DownloadDangerPromptTest : public InProcessBrowserTest {
+// An enum to parameterize the tests so that the tests can be run with and
+// without the secondary-ui-md flag.
+enum class SecondaryUiMd {
+  ENABLED,
+  DISABLED,
+};
+
+// Functor to generate the test name suffix depending on the value of the

[tapted, 2017/01/25 00:32:17]

Can this be a regular function? See e.g.
https://codereview.chromium.org/2394123002/diff/370001/ui/views/controls/menu...

[karandeepb, 2017/01/25 00:51:51]

Done.

+// SecondaryUiMd param.
+struct PrintSecondaryUiMdStatus {
+  template <class ParamType>
+  std::string operator()(const testing::TestParamInfo<ParamType>& info) const {
+    switch (info.param) {
+      case SecondaryUiMd::ENABLED:
+        return "SecondaryUiMdEnabled";
+      case SecondaryUiMd::DISABLED:
+        return "SecondaryUiMdDisabled";
+    }
+    NOTREACHED();
+    return std::string();
+  }
+};
+
+}  // namespace
+
+class DownloadDangerPromptTest
+    : public InProcessBrowserTest,
+      public ::testing::WithParamInterface<SecondaryUiMd> {
  public:
   DownloadDangerPromptTest()
       : prompt_(nullptr),
         expected_action_(DownloadDangerPrompt::CANCEL),
         did_receive_callback_(false),
-        test_safe_browsing_factory_(
-            new safe_browsing::TestSafeBrowsingServiceFactory()) {}
+        test_safe_browsing_factory_(new TestSafeBrowsingServiceFactory()) {}

[tapted, 2017/01/25 00:32:17]

nit: (since you're changing it), I think the preference is for

base::MakeUnique<TestSafeBrowsingServiceFactory>()

[karandeepb, 2017/01/25 00:51:51]

Done.

 
   ~DownloadDangerPromptTest() override {}
 
@@ -59,6 +86,13 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
     InProcessBrowserTest::TearDown();
   }
 
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // TODO(crbug.com/630357): Remove when secondary-ui-md is enabled by default

[tapted, 2017/01/25 00:32:17]

I don't think we need the TODO. When that happens,
switches::kExtendMdToSecondaryUi will disappear, so it will "automatically" get
cleaned up.

But! What might be missed is that the whole WithParamInterface can go too. So
perhaps update the TODO to mention that. Then when someone gets a compile error
here, hopefully they see the TODO and remember to remove WithParamInterface as
well as just delete the SetUpCommandLine override.

[karandeepb, 2017/01/25 00:51:51]

Done.

+    // on all platforms.
+    if (GetParam() == SecondaryUiMd::ENABLED)
+      command_line->AppendSwitch(switches::kExtendMdToSecondaryUi);

[karandeepb, 2017/01/24 09:17:21]

Tried using MaterialDesignControllerTestApi but it didn't work. It's difficult
to hook into the point MaterialDesignController is initialized for a browser
test.

[tapted, 2017/01/25 00:32:17]

This is good - that's how I've been doing it

+  }
+
   // Opens a new tab and waits for navigations to finish. If there are pending
   // navigations, the constrained prompt might be dismissed when the navigation
   // completes.
@@ -123,12 +157,10 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
     EXPECT_CALL(download_, GetFileNameToReportUser()).WillRepeatedly(Return(
         base::FilePath(FILE_PATH_LITERAL("evil.exe"))));
     EXPECT_CALL(download_, GetDangerType()).WillRepeatedly(Return(danger_type));
-    safe_browsing::DownloadProtectionService::DownloadPingToken* token_obj
-        = new safe_browsing::DownloadProtectionService::DownloadPingToken(
-            token);
-    download_.SetUserData(
-        safe_browsing::DownloadProtectionService::kDownloadPingTokenKey,
-        token_obj);
+    DownloadProtectionService::DownloadPingToken* token_obj =
+        new DownloadProtectionService::DownloadPingToken(token);
+    download_.SetUserData(DownloadProtectionService::kDownloadPingTokenKey,
+                          token_obj);
   }
 
   void SetUpSafeBrowsingReportExpectations(
@@ -172,8 +204,7 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
   DownloadDangerPrompt* prompt_;
   DownloadDangerPrompt::Action expected_action_;
   bool did_receive_callback_;
-  std::unique_ptr<safe_browsing::TestSafeBrowsingServiceFactory>
-      test_safe_browsing_factory_;
+  std::unique_ptr<TestSafeBrowsingServiceFactory> test_safe_browsing_factory_;
   std::string expected_serialized_report_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadDangerPromptTest);
@@ -185,7 +216,7 @@ class DownloadDangerPromptTest : public InProcessBrowserTest {
 #else
 #define MAYBE_TestAll TestAll
 #endif
-IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, MAYBE_TestAll) {
+IN_PROC_BROWSER_TEST_P(DownloadDangerPromptTest, MAYBE_TestAll) {
   // ExperienceSampling: Set default actions for DownloadItem methods we need.
   GURL download_url(kTestDownloadUrl);
   ON_CALL(download(), GetURL()).WillByDefault(ReturnRef(download_url));
@@ -302,4 +333,12 @@ IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, MAYBE_TestAll) {
   VerifyExpectations(true);
 }
 
+// Prefix for test instantiations intentionally left blank since the test
+// fixture class has a single parameterization.
+INSTANTIATE_TEST_CASE_P(,
+                        DownloadDangerPromptTest,
+                        ::testing::Values(SecondaryUiMd::ENABLED,
+                                          SecondaryUiMd::DISABLED),
+                        PrintSecondaryUiMdStatus());
+
 }  // namespace safe_browsing