7 files changed, 414 insertions, 63 deletions

diff --git a/base/test/test_file_util.h b/base/test/test_file_util.h
index 362c525..2de74e7 100644
--- a/base/test/test_file_util.h
+++ b/base/test/test_file_util.h
@@ -30,6 +30,14 @@ bool EvictFileFromSystemCache(const FilePath& file);
 bool CopyRecursiveDirNoCache(const FilePath& source_dir,
                              const FilePath& dest_dir);
 
+#if defined(OS_WIN)
+// Returns true if the volume supports Alternate Data Streams.
+bool VolumeSupportsADS(const FilePath& path);
+
+// Returns true if the ZoneIdentifier is correctly set to "Internet" (3).
+bool HasInternetZoneIdentifier(const FilePath& full_path);
+#endif  // defined(OS_WIN)
+
 }  // namespace file_util
 
 #endif  // BASE_TEST_TEST_FILE_UTIL_H_
diff --git a/base/test/test_file_util_win.cc b/base/test/test_file_util_win.cc
index 8e5b37b..fc3d018 100644
--- a/base/test/test_file_util_win.cc
+++ b/base/test/test_file_util_win.cc
@@ -4,6 +4,7 @@
 
 #include "base/test/test_file_util.h"
 
+#include <shlwapi.h>
 #include <windows.h>
 
 #include <vector>
@@ -173,4 +174,61 @@ bool CopyRecursiveDirNoCache(const FilePath& source_dir,
   return true;
 }
 
+// Checks if the volume supports Alternate Data Streams. This is required for
+// the Zone Identifier implementation.
+bool VolumeSupportsADS(const FilePath& path) {
+  wchar_t drive[MAX_PATH] = {0};
+  wcscpy_s(drive, MAX_PATH, path.value().c_str());
+
+  if (!PathStripToRootW(drive))
+    return false;
+
+  DWORD fs_flags = 0;
+  if (!GetVolumeInformationW(drive, NULL, 0, 0, NULL, &fs_flags, NULL, 0))
+    return false;
+
+  if (fs_flags & FILE_NAMED_STREAMS)
+    return true;
+
+  return false;
+}
+
+// Return whether the ZoneIdentifier is correctly set to "Internet" (3)
+bool HasInternetZoneIdentifier(const FilePath& full_path) {
+  std::wstring path = full_path.value() + L":Zone.Identifier";
+
+  // This polling and sleeping here is a very bad pattern. But due to how
+  // Windows file semantics work it's really hard to do it other way. We are
+  // reading a file written by a different process, using a different handle.
+  // Windows does not guarantee that we will get the same contents even after
+  // the other process closes the handle, flushes the buffers, etc.
+  for (int i = 0; i < 20; i++) {
+    PlatformThread::Sleep(1000);
+
+    const DWORD kShare = FILE_SHARE_READ |
+        FILE_SHARE_WRITE |
+        FILE_SHARE_DELETE;
+    HANDLE file = CreateFile(path.c_str(), GENERIC_READ, kShare, NULL,
+                             OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+    if (file == INVALID_HANDLE_VALUE)
+      continue;
+
+    char buffer[100] = {0};
+    DWORD read = 0;
+    BOOL read_result = ::ReadFile(file, buffer, 100, &read, NULL);
+    CloseHandle(file);
+
+    if (!read_result)
+      continue;
+
+    const char kIdentifier[] = "[ZoneTransfer]\nZoneId=3";
+    if (read != arraysize(kIdentifier))
+      continue;
+
+    if (strcmp(kIdentifier, buffer) == 0)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace file_util
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
new file mode 100644
index 0000000..1f42fbb
--- /dev/null
+++ b/chrome/browser/download/download_browsertest.cc
@@ -0,0 +1,340 @@
+// Copyright (c) 2010 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 "base/file_path.h"
+#include "base/file_util.h"
+#include "base/path_service.h"
+#include "base/scoped_temp_dir.h"
+#include "base/test/test_file_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/browser_window.h"
+#include "chrome/browser/download/download_item.h"
+#include "chrome/browser/download/download_manager.h"
+#include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/net/url_request_mock_http_job.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profile.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/notification_service.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Variation of DownloadsCompleteObserver from ui_test_utils.cc; the
+// specifically targeted download tests need finer granularity on waiting.
+// Construction of this class defines a system state, based on some number
+// of downloads being seen in a particular state + other events that
+// may occur in the download system.  That state will be recorded if it
+// occurs at any point after construction.  When that state occurs, the class
+// is considered finished.  Callers may either probe for the finished state, or
+// wait on it.
+class DownloadsObserver : public DownloadManager::Observer,
+                          public DownloadItem::Observer {
+ public:
+  // The action which should be considered the completion of the download.
+  enum DownloadFinishedSignal { COMPLETE, FILE_RENAME };
+
+  // Create an object that will be considered completed when |wait_count|
+  // download items have entered state |download_finished_signal|.
+  // If |finish_on_select_file| is true, the object will also be
+  // considered finished if the DownloadManager raises a
+  // SelectFileDialogDisplayed() notification.
+
+  // TODO(rdsmith): Add option of "dangerous accept/reject dialog" as
+  // a unblocking event; if that shows up when you aren't expecting it,
+  // it'll result in a hang/timeout as we'll never get to final rename.
+  // This probably means rewriting the interface to take a list of events
+  // to treat as completion events.
+  DownloadsObserver(DownloadManager* download_manager,
+                    size_t wait_count,
+                    DownloadFinishedSignal download_finished_signal,
+                    bool finish_on_select_file)
+      : download_manager_(download_manager),
+        wait_count_(wait_count),
+        finished_downloads_at_construction_(0),
+        waiting_(false),
+        download_finished_signal_(download_finished_signal),
+        finish_on_select_file_(finish_on_select_file),
+        select_file_dialog_seen_(false) {
+    download_manager_->AddObserver(this);  // Will call initial ModelChanged().
+    finished_downloads_at_construction_ = finished_downloads_.size();
+  }
+
+  ~DownloadsObserver() {
+    std::set<DownloadItem*>::iterator it = downloads_observed_.begin();
+    for (; it != downloads_observed_.end(); ++it) {
+      (*it)->RemoveObserver(this);
+    }
+    download_manager_->RemoveObserver(this);
+  }
+
+  // State accessors.
+  bool select_file_dialog_seen() { return select_file_dialog_seen_; }
+
+  // Wait for whatever state was specified in the constructor.
+  void WaitForFinished() {
+    if (!IsFinished()) {
+      waiting_ = true;
+      ui_test_utils::RunMessageLoop();
+      waiting_ = false;
+    }
+  }
+
+  // Return true if everything's happened that we're configured for.
+  bool IsFinished() {
+    if (finished_downloads_.size() - finished_downloads_at_construction_
+        >= wait_count_)
+      return true;
+    return (finish_on_select_file_ && select_file_dialog_seen_);
+  }
+
+  // DownloadItem::Observer
+  virtual void OnDownloadUpdated(DownloadItem* download) {
+    if (download_finished_signal_ == COMPLETE &&
+        download->state() == DownloadItem::COMPLETE)
+      DownloadInFinalState(download);
+  }
+
+  virtual void OnDownloadFileCompleted(DownloadItem* download) {
+    if (download_finished_signal_ == FILE_RENAME)
+      DownloadInFinalState(download);
+  }
+
+  virtual void OnDownloadOpened(DownloadItem* download) {}
+
+  // DownloadManager::Observer
+  virtual void ModelChanged() {
+    // Regenerate DownloadItem observers.  If there are any download items
+    // in the COMPLETE state and that's our final state, note them in
+    // finished_downloads_ (done by OnDownloadUpdated).
+    std::vector<DownloadItem*> downloads;
+    download_manager_->SearchDownloads(string16(), &downloads);
+
+    std::vector<DownloadItem*>::iterator it = downloads.begin();
+    for (; it != downloads.end(); ++it) {
+      OnDownloadUpdated(*it);  // Safe to call multiple times; checks state.
+
+      std::set<DownloadItem*>::const_iterator
+          finished_it(finished_downloads_.find(*it));
+      std::set<DownloadItem*>::const_iterator
+          observed_it(downloads_observed_.find(*it));
+
+      // If it isn't finished and we're aren't observing it, start.
+      if (finished_it == finished_downloads_.end() &&
+          observed_it == downloads_observed_.end()) {
+        (*it)->AddObserver(this);
+        downloads_observed_.insert(*it);
+        continue;
+      }
+
+      // If it is finished and we are observing it, stop.
+      if (finished_it != finished_downloads_.end() &&
+          observed_it != downloads_observed_.end()) {
+        (*it)->RemoveObserver(this);
+        downloads_observed_.erase(observed_it);
+        continue;
+      }
+    }
+  }
+
+  virtual void SelectFileDialogDisplayed() {
+    select_file_dialog_seen_ = true;
+    SignalIfFinished();
+  }
+
+ private:
+  // Called when we know that a download item is in a final state.
+  // Note that this is not the same as it first transitioning in to the
+  // final state; in the case of DownloadItem::COMPLETE, multiple
+  // notifications may occur once the item is in that state.  So we
+  // keep our own track of transitions into final.
+  void DownloadInFinalState(DownloadItem* download) {
+    if (finished_downloads_.find(download) != finished_downloads_.end()) {
+      // We've already seen terminal state on this download.
+      return;
+    }
+
+    // Record the transition.
+    finished_downloads_.insert(download);
+
+    SignalIfFinished();
+  }
+
+  void SignalIfFinished() {
+    if (waiting_ && IsFinished())
+      MessageLoopForUI::current()->Quit();
+  }
+
+  // The observed download manager.
+  DownloadManager* download_manager_;
+
+  // The set of DownloadItem's that have transitioned to their finished state
+  // since construction of this object.  When the size of this array
+  // reaches wait_count_, we're done.
+  std::set<DownloadItem*> finished_downloads_;
+
+  // The set of DownloadItem's we are currently observing.  Generally there
+  // won't be any overlap with the above; once we see the final state
+  // on a DownloadItem, we'll stop observing it.
+  std::set<DownloadItem*> downloads_observed_;
+
+  // The number of downloads to wait on completing.
+  size_t wait_count_;
+
+  // The number of downloads entered in final state in initial
+  // ModelChanged().  We use |finished_downloads_| to track the incoming
+  // transitions to final state we should ignore, and to track the
+  // number of final state transitions that occurred between
+  // construction and return from wait.  But if the final state is the
+  // COMPLETE state, some downloads may be in it (and thus be entered
+  // into finished_downloads_) when we construct this class.  We don't
+  // want to count those;
+  int finished_downloads_at_construction_;
+
+  // Whether an internal message loop has been started and must be quit upon
+  // all downloads completing.
+  bool waiting_;
+
+  // The action on which to consider the DownloadItem finished.
+  DownloadFinishedSignal download_finished_signal_;
+
+  // True if we should transition the DownloadsObserver to finished if
+  // the select file dialog comes up.
+  bool finish_on_select_file_;
+
+  // True if we've seen the select file dialog.
+  bool select_file_dialog_seen_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadsObserver);
+};
+
+class DownloadTest : public InProcessBrowserTest {
+ protected:
+  void SetUpInProcessBrowserTestFixture() {
+    ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_));
+  }
+
+  // Must be called after browser creation.  Creates a temporary
+  // directory for downloads that is auto-deleted on destruction.
+  bool CreateAndSetDownloadsDirectory() {
+    if (downloads_directory_.CreateUniqueTempDir()) {
+      browser()->profile()->GetPrefs()->SetFilePath(
+          prefs::kDownloadDefaultDirectory,
+          downloads_directory_.path());
+      return true;
+    }
+    return false;
+  }
+
+  // May only be called inside of an individual test; browser() is NULL
+  // outside of that context.
+  FilePath GetDownloadDirectory() {
+    DownloadManager* download_mananger =
+        browser()->profile()->GetDownloadManager();
+    return download_mananger->download_prefs()->download_path();
+  }
+
+  DownloadsObserver* CreateWaiter(int num_downloads) {
+    DownloadManager* download_manager =
+        browser()->profile()->GetDownloadManager();
+    return new DownloadsObserver(
+        download_manager, num_downloads,
+        DownloadsObserver::FILE_RENAME,  // Really done
+        true);                          // Bail on select file
+  }
+
+  // Should only be called when the download is known to have finished
+  // (in error or not).
+  void CheckDownload(const FilePath& downloaded_filename,
+                     const FilePath& origin_filename) {
+    // Find the path to which the data will be downloaded.
+    FilePath downloaded_file =
+        GetDownloadDirectory().Append(downloaded_filename);
+
+    // Find the origin path (from which the data comes).
+    FilePath origin_file(test_dir_.Append(origin_filename));
+    ASSERT_TRUE(file_util::PathExists(origin_file));
+
+    // Confirm the downloaded data file exists.
+    ASSERT_TRUE(file_util::PathExists(downloaded_file));
+    int64 origin_file_size = 0;
+    int64 downloaded_file_size = 0;
+    EXPECT_TRUE(file_util::GetFileSize(origin_file, &origin_file_size));
+    EXPECT_TRUE(file_util::GetFileSize(downloaded_file, &downloaded_file_size));
+    EXPECT_EQ(origin_file_size, downloaded_file_size);
+    EXPECT_TRUE(file_util::ContentsEqual(downloaded_file, origin_file));
+
+#if defined(OS_WIN)
+    // Check if the Zone Identifier is correctly set.
+    if (file_util::VolumeSupportsADS(downloaded_file))
+      EXPECT_TRUE(file_util::HasInternetZoneIdentifier(downloaded_file));
+#endif
+
+    // Delete the downloaded copy of the file.
+    EXPECT_TRUE(file_util::DieFileDie(downloaded_file, false));
+  }
+
+ private:
+  // Location of the test data.
+  FilePath test_dir_;
+
+  // Location of the downloads directory for these tests
+  ScopedTempDir downloads_directory_;
+};
+
+// Test is believed good (non-flaky) in itself, but it
+// sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  Until that bug is
+// fixed, this test should be considered flaky.
+// (Note that if 63237 does cause a failure, it'll be a timeout.
+// This isn't ideal, but getting stats on this tests failure is
+// worth the timeout test costs.)
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_DownloadMimeType) {
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  ASSERT_TRUE(CreateAndSetDownloadsDirectory());
+
+  EXPECT_EQ(1, browser()->tab_count());
+
+  // Setup notification, navigate, and block.
+  scoped_ptr<DownloadsObserver> observer(CreateWaiter(1));
+  ui_test_utils::NavigateToURL(
+      browser(), URLRequestMockHTTPJob::GetMockUrl(file));
+  observer->WaitForFinished();
+
+  // Download should be finished; check state.
+  EXPECT_FALSE(observer->select_file_dialog_seen());
+  EXPECT_EQ(1, browser()->tab_count());
+  CheckDownload(file, file);
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+}
+
+// This test runs correctly, but leaves behind turds in the test user's
+// download directory because of http://crbug.com/62099.  No big loss; it
+// was primarily confirming DownloadsObserver wait on select file dialog
+// functionality anyway.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DownloadMimeTypeSelect) {
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  ASSERT_TRUE(CreateAndSetDownloadsDirectory());
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload, true);
+
+  EXPECT_EQ(1, browser()->tab_count());
+
+  // Setup notification, navigate, and block.
+  scoped_ptr<DownloadsObserver> observer(CreateWaiter(1));
+  ui_test_utils::NavigateToURL(
+      browser(), URLRequestMockHTTPJob::GetMockUrl(file));
+  observer->WaitForFinished();
+
+  // Download should not be finished; check state.
+  EXPECT_TRUE(observer->select_file_dialog_seen());
+  EXPECT_EQ(1, browser()->tab_count());
+  EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+}
+
+}  // namespace
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index a451d41..3211507 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -414,6 +414,7 @@ void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) {
                                     info->suggested_path,
                                     &file_type_info, 0, FILE_PATH_LITERAL(""),
                                     owning_window, info);
+    FOR_EACH_OBSERVER(Observer, observers_, SelectFileDialogDisplayed());
   } else {
     // No prompting for download, just continue with the suggested name.
     CreateDownloadItem(info, info->suggested_path);
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index a6cdd90..e5f08bf 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -85,6 +85,10 @@ class DownloadManager
     // from calling back to a stale pointer.
     virtual void ManagerGoingDown() {}
 
+    // Called immediately after the DownloadManager puts up a select file
+    // dialog.
+    virtual void SelectFileDialogDisplayed() {}
+
    protected:
     virtual ~Observer() {}
   };
diff --git a/chrome/browser/download/download_uitest.cc b/chrome/browser/download/download_uitest.cc
index e37bfb4..c92ef6d 100644
--- a/chrome/browser/download/download_uitest.cc
+++ b/chrome/browser/download/download_uitest.cc
@@ -33,26 +33,6 @@ namespace {
 
 const wchar_t kDocRoot[] = L"chrome/test/data";
 
-#if defined(OS_WIN)
-// Checks if the volume supports Alternate Data Streams. This is required for
-// the Zone Identifier implementation.
-bool VolumeSupportsADS(const std::wstring path) {
-  wchar_t drive[MAX_PATH] = {0};
-  wcscpy_s(drive, MAX_PATH, path.c_str());
-
-  EXPECT_TRUE(PathStripToRootW(drive));
-
-  DWORD fs_flags = 0;
-  EXPECT_TRUE(GetVolumeInformationW(drive, NULL, 0, 0, NULL, &fs_flags, NULL,
-                                    0));
-
-  if (fs_flags & FILE_NAMED_STREAMS)
-    return true;
-
-  return false;
-}
-#endif  // defined(OS_WIN)
-
 class DownloadTest : public UITest {
  protected:
   DownloadTest() : UITest() {}
@@ -74,8 +54,8 @@ class DownloadTest : public UITest {
 
 #if defined(OS_WIN)
     // Check if the Zone Identifier is correctly set.
-    if (VolumeSupportsADS(file_on_client.value()))
-      CheckZoneIdentifier(file_on_client.value());
+    if (file_util::VolumeSupportsADS(file_on_client))
+      EXPECT_TRUE(file_util::HasInternetZoneIdentifier(file_on_client));
 #endif
 
     // Delete the client copy of the file.
@@ -135,47 +115,6 @@ class DownloadTest : public UITest {
     EXPECT_FALSE(file_util::PathExists(download_path));
   }
 
-#if defined(OS_WIN)
-  // Checks if the ZoneIdentifier is correctly set to "Internet" (3)
-  void CheckZoneIdentifier(const std::wstring full_path) {
-    std::wstring path = full_path + L":Zone.Identifier";
-
-    // This polling and sleeping here is a very bad pattern. But due to how
-    // Windows file semantics work it's really hard to do it other way. We are
-    // reading a file written by a different process, using a different handle.
-    // Windows does not guarantee that we will get the same contents even after
-    // the other process closes the handle, flushes the buffers, etc.
-    for (int i = 0; i < 20; i++) {
-      PlatformThread::Sleep(sleep_timeout_ms());
-
-      const DWORD kShare = FILE_SHARE_READ |
-                           FILE_SHARE_WRITE |
-                           FILE_SHARE_DELETE;
-      HANDLE file = CreateFile(path.c_str(), GENERIC_READ, kShare, NULL,
-                               OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
-      if (file == INVALID_HANDLE_VALUE)
-        continue;
-
-      char buffer[100] = {0};
-      DWORD read = 0;
-      BOOL read_result = ReadFile(file, buffer, 100, &read, NULL);
-      CloseHandle(file);
-
-      if (!read_result)
-        continue;
-
-      const char kIdentifier[] = "[ZoneTransfer]\nZoneId=3";
-      if (read != arraysize(kIdentifier))
-        continue;
-
-      if (strcmp(kIdentifier, buffer) == 0)
-        return;
-    }
-
-    FAIL() << "Could not detect Internet ZoneIndentifier";
-  }
-#endif  // defined(OS_WIN)
-
   FilePath download_prefix_;
 };
 
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 22b524e..bdd161a 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1957,6 +1957,7 @@
         'browser/device_orientation/device_orientation_browsertest.cc',
         'browser/dom_ui/file_browse_browsertest.cc',
         'browser/dom_ui/mediaplayer_browsertest.cc',
+        'browser/download/download_browsertest.cc',
         'browser/download/save_page_browsertest.cc',
         'browser/extensions/alert_apitest.cc',
         'browser/extensions/all_urls_apitest.cc',