Don't prompt to save for filetypes marked as auto-open.

Check if the generated file extension is in the auto-open list. If so, don't bother displaying the save as dialog even if the always prompt for download option is on.

Original patch by James Simonsen (see http://codereview.chromium.org/2910003 ), r=me.

BUG=18587
TEST=Turn on prompt for download option. Mark a filetype as always open. Download a file of that type and make sure no dialog appears.
Review URL: http://codereview.chromium.org/2823043

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@52559 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 111 insertions, 28 deletions

diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 4fe4d5a..c43f18e 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -360,7 +360,8 @@ void DownloadManager::Shutdown() {
   DCHECK(shutdown_needed_) << "Shutdown called when not needed.";
 
   // Stop receiving download updates
-  file_manager_->RemoveDownloadManager(this);
+  if (file_manager_)
+    file_manager_->RemoveDownloadManager(this);
 
   // Stop making history service requests
   cancelable_consumer_.CancelAllRequests();
@@ -568,16 +569,13 @@ bool DownloadManager::Init(Profile* profile) {
   // a no op.
   CleanUpInProgressHistoryEntries();
 
+  // In test mode, there may be no ResourceDispatcherHost.  In this case it's
+  // safe to avoid setting |file_manager_| because we only call a small set of
+  // functions, none of which need it.
   ResourceDispatcherHost* rdh = g_browser_process->resource_dispatcher_host();
-  if (!rdh) {
-    NOTREACHED();
-    return false;
-  }
-
-  file_manager_ = rdh->download_file_manager();
-  if (!file_manager_) {
-    NOTREACHED();
-    return false;
+  if (rdh) {
+    file_manager_ = rdh->download_file_manager();
+    DCHECK(file_manager_);
   }
 
   // Get our user preference state.
@@ -654,26 +652,28 @@ void DownloadManager::StartDownload(DownloadCreateInfo* info) {
       info->is_extension_install = true;
   }
 
-  // Freeze the user's preference for showing a Save As dialog.  We're going to
-  // bounce around a bunch of threads and we don't want to worry about race
-  // conditions where the user changes this pref out from under us.
-  if (*prompt_for_download_) {
-    // But never obey the preference for the following scenarios:
-    // 1) Extension installation. Note that we only care here about the case
-    //    where an extension is installed, not when one is downloaded with
-    //    "save as...".
-    // 2) Drag-out download. Since we will save to the destination folder that
-    //    is dropped to, we should not pop up a Save As dialog.
-    if (!info->is_extension_install && info->save_info.file_path.empty())
-      info->save_as = true;
-  }
-
   if (info->save_info.file_path.empty()) {
+    FilePath generated_name;
+    GenerateFileNameFromInfo(info, &generated_name);
+
+    // Freeze the user's preference for showing a Save As dialog.  We're going
+    // to bounce around a bunch of threads and we don't want to worry about race
+    // conditions where the user changes this pref out from under us.
+    if (*prompt_for_download_) {
+      // But ignore the user's preference for the following scenarios:
+      // 1) Extension installation. Note that we only care here about the case
+      //    where an extension is installed, not when one is downloaded with
+      //    "save as...".
+      // 2) Filetypes marked "always open." If the user just wants this file
+      //    opened, don't bother asking where to keep it.
+      if (!info->is_extension_install &&
+          !ShouldOpenFileBasedOnExtension(generated_name))
+        info->save_as = true;
+    }
+
     // Determine the proper path for a download, by either one of the following:
     // 1) using the default download directory.
     // 2) prompting the user.
-    FilePath generated_name;
-    GenerateFileNameFromInfo(info, &generated_name);
     if (info->save_as && !last_download_path_.empty())
       info->suggested_path = last_download_path_;
     else
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 8429be2..5ff1786 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -6,7 +6,10 @@
 
 #include "base/string_util.h"
 #include "build/build_config.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/download/download_manager.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/testing_profile.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
@@ -32,8 +35,17 @@
 
 class DownloadManagerTest : public testing::Test {
  public:
-  DownloadManagerTest() {
-    download_manager_ = new DownloadManager();
+  DownloadManagerTest()
+      : profile_(new TestingProfile()),
+        download_manager_(new DownloadManager()),
+        ui_thread_(ChromeThread::UI, &message_loop_) {
+    download_manager_->Init(profile_.get());
+  }
+
+  ~DownloadManagerTest() {
+    // profile_ must outlive download_manager_, so we explicitly delete
+    // download_manager_ first.
+    download_manager_.release();
   }
 
   void GetGeneratedFilename(const std::string& content_disposition,
@@ -52,8 +64,10 @@ class DownloadManagerTest : public testing::Test {
   }
 
  protected:
+  scoped_ptr<TestingProfile> profile_;
   scoped_refptr<DownloadManager> download_manager_;
   MessageLoopForUI message_loop_;
+  ChromeThread ui_thread_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerTest);
 };
@@ -610,3 +624,72 @@ TEST_F(DownloadManagerTest, GetSafeFilename) {
   }
 }
 #endif  // defined(OS_WIN) || defined(OS_MACOSX)
+
+namespace {
+
+const struct {
+  const char* url;
+  const char* mime_type;
+  bool save_as;
+  bool prompt_for_download;
+  bool expected_save_as;
+} kStartDownloadCases[] = {
+  { "http://www.foo.com/dont-open.html",
+    "text/html",
+    false,
+    false,
+    false, },
+  { "http://www.foo.com/save-as.html",
+    "text/html",
+    true,
+    false,
+    true, },
+  { "http://www.foo.com/always-prompt.html",
+    "text/html",
+    false,
+    true,
+    true, },
+  { "http://www.foo.com/wrong_mime_extension.user.js",
+    "text/html",
+    false,
+    true,
+    false, },
+  { "http://www.foo.com/extensionless-extension",
+    "application/x-chrome-extension",
+    true,
+    false,
+    true, },
+  { "http://www.foo.com/save-as.pdf",
+    "application/pdf",
+    true,
+    false,
+    true, },
+  { "http://www.foo.com/auto-open.pdf",
+    "application/pdf",
+    false,
+    true,
+    false, },
+};
+
+}  // namespace
+
+TEST_F(DownloadManagerTest, StartDownload) {
+  PrefService* prefs = profile_->GetPrefs();
+  prefs->SetFilePath(prefs::kDownloadDefaultDirectory, FilePath());
+  download_manager_->OpenFilesBasedOnExtension(
+      FilePath(FILE_PATH_LITERAL("example.pdf")), true);
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kStartDownloadCases); ++i) {
+    prefs->SetBoolean(prefs::kPromptForDownload,
+                      kStartDownloadCases[i].prompt_for_download);
+
+    DownloadCreateInfo info;
+    info.save_as = kStartDownloadCases[i].save_as;
+    info.url = GURL(kStartDownloadCases[i].url);
+    info.mime_type = kStartDownloadCases[i].mime_type;
+
+    download_manager_->StartDownload(&info);
+
+    EXPECT_EQ(kStartDownloadCases[i].expected_save_as, info.save_as);
+  }
+}