Using ScopedTempDir as the download directory used by the DownloadViaPost test.

chrome/browser/extensions/api/extension_action/browser_action_apitest.cc

 // Copyright (c) 2012 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 <stdint.h>
 
+#include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_icon_factory.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/extensions/extension_process_policy.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
@@ skipping 806 matching lines @@
       const GURL& target_url,
       ExpectedNavigationStatus expected_navigation_status) {
     std::string navigation_starting_script =
         "var form = document.getElementById('form');\n"
         "form.action = '" + target_url.spec() + "';\n"
         "form.submit();\n";
     TestPopupNavigation(target_url, expected_navigation_status,
                         navigation_starting_script);
   }
 
+  // Must be called after browser creation.  Creates a temporary
+  // directory for downloads that is auto-deleted on destruction.
+  // Returning false indicates a failure of the function, and should be asserted
+  // in the caller.
+  bool CreateAndSetDownloadsDirectory(Browser* browser) {
+    if (!browser)
+      return false;
+
+    if (!downloads_directory_.CreateUniqueTempDir())
+      return false;
+
+    browser->profile()->GetPrefs()->SetFilePath(
+        prefs::kDownloadDefaultDirectory, downloads_directory_.GetPath());
+    browser->profile()->GetPrefs()->SetFilePath(
+        prefs::kSaveFileDefaultDirectory, downloads_directory_.GetPath());
+
+    return true;
+  }
+
  private:
   void TestPopupNavigation(const GURL& target_url,
                            ExpectedNavigationStatus expected_navigation_status,
                            std::string navigation_starting_script) {
     // Were there any failures so far (e.g. in SetUpOnMainThread)?
     ASSERT_FALSE(HasFailure());
 
     // Simulate a click on the browser action to open the popup.
     WebContents* popup = OpenPopup(0);
     ASSERT_TRUE(popup);
@@ skipping 54 matching lines @@
     for (int i = 0; i < tabs->count(); i++) {
       WebContents* tab_contents = tabs->GetWebContentsAt(i);
       WaitForLoadStop(tab_contents);
       EXPECT_NE(target_url, tab_contents->GetLastCommittedURL())
           << "Navigating an extension pop-up should not affect tabs.";
     }
   }
 
   const Extension* popup_extension_;
   const Extension* other_extension_;
+  base::ScopedTempDir downloads_directory_;
 };
 
 // Tests that an extension pop-up cannot be navigated to a web page.
 IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, Webpage) {
   GURL web_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
 
   // With and without --isolate-extension the GET request will be blocked in
   // ExtensionViewHost::OpenURLFromTab (which silently drops navigations with
   // CURRENT_TAB disposition).
   TestPopupNavigationViaGet(web_url, EXPECTING_NAVIGATION_FAILURE);
@@ skipping 46 matching lines @@
 // works: No navigation, but download shelf visible + download goes through.
 //
 // Note - there is no "...ViaGet" flavour of this test, because we don't care
 // (yet) if GET succeeds with the download or not (it probably should succeed
 // for consistency with POST, but it always failed in M54 and before).  After
 // abandoing ShouldFork/OpenURL for all methods (not just for POST) [see comment
 // about https://crbug.com/646261 in ChromeContentRendererClient::ShouldFork]
 // GET should automagically start working for downloads.
 // TODO(lukasza): https://crbug.com/650694: Add a "Get" flavour of the test once
 // the download works both for GET and POST requests.
-
-// Disabled on Windows. See http://crbug.com/653856.
-#if defined(OS_WIN)
-#define MAYBE_DownloadViaPost DISABLED_DownloadViaPost
-#else
-#define MAYBE_DownloadViaPost DownloadViaPost
-#endif
-
-IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
-                       MAYBE_DownloadViaPost) {
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, DownloadViaPost) {
+  ASSERT_TRUE(CreateAndSetDownloadsDirectory(browser()));

[Devlin, 2016/10/07 21:11:38]

I don't feel too strongly, but if this is only used here, should we inline it?

[Łukasz Anforowicz, 2016/10/07 23:32:02]

Hmmm... not sure.

Arguments for inlining:
- More localized code
- Extra |downloads_directory_| field is not needed outside of the testcase.

Arguments for keeping as-is:
- Easy to code-search for CreateAndSetDownloadsDirectory
- The inlined setup code is not really core part of the test,
  so maybe better to keep it away from the main test logic

In the latest patchset I've decided to inline, because I really like having the
ScopedTempDir as a local variable (rather than a field of the test fixture). 
I've mentioned CreateAndSetDownloadsDirectory in a comment to hopefully still
enable searching the code for similar test code.

[Devlin, 2016/10/10 14:38:50]

Another option: create a method in the anonymous namespace that takes a
PrefService and returns a ScopedTempDir.  That would allow for separation of
logic while retaining isolation to this test and having the dir as a local
variable.

[Łukasz Anforowicz, 2016/10/10 15:08:57]

Good idea - thanks.  Done.

   content::DownloadTestObserverTerminal downloads_observer(
       content::BrowserContext::GetDownloadManager(browser()->profile()),
       1,  // == wait_count (only waiting for "download-test3.gif").
       content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
 
   // Navigate to a URL that replies with
   // Content-Disposition: attachment; filename=...
   // header.
   GURL download_url(
       embedded_test_server()->GetURL("foo.com", "/download-test3.gif"));
   TestPopupNavigationViaPost(download_url, EXPECTING_NAVIGATION_FAILURE);
 
   // Verify that "download-test3.gif got downloaded.
   downloads_observer.WaitForFinished();
   EXPECT_EQ(0u, downloads_observer.NumDangerousDownloadsSeen());
   EXPECT_EQ(1u, downloads_observer.NumDownloadsSeenInState(
                     content::DownloadItem::COMPLETE));
 
   // The test verification below is applicable only to scenarios where the
   // download shelf is supported - on ChromeOS, instead of the download shelf,
   // there is a download notification in the right-bottom corner of the screen.
 #if !defined(OS_CHROMEOS)
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 #endif
 }
 
 }  // namespace
 }  // namespace extensions