Added some cancel/close tab tests to SavePackage.

chrome/browser/download/save_page_browsertest.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 "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
@@ skipping 18 matching lines @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/download_persistent_store_info.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/test_utils.h"
 #include "content/test/net/url_request_mock_http_job.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/download/save_package_file_picker_chromeos.h"
 #else
 #include "chrome/browser/download/save_package_file_picker.h"
 #endif
 
 using content::BrowserContext;
 using content::BrowserThread;
 using content::DownloadItem;
 using content::DownloadManager;
 using content::DownloadPersistentStoreInfo;
 using content::WebContents;
 
 namespace {
 
 static const FilePath::CharType* kTestDir = FILE_PATH_LITERAL("save_page");
 
 static const char* kAppendedExtension =
 #if defined(OS_WIN)
     ".htm";
 #else
     ".html";
 #endif
 
 }  // namespace
 
+// Loosely based on logic in DownloadTestObserver.
+class DownloadItemCreatedObserver : public DownloadManager::Observer {
+ public:
+  explicit DownloadItemCreatedObserver(DownloadManager* manager)
+      : waiting_(false), manager_(manager), created_item_(NULL) {
+    manager->AddObserver(this);
+  }
+
+  ~DownloadItemCreatedObserver() {
+    if (manager_)
+      manager_->RemoveObserver(this);
+  }
+
+  // Wait for the first download item created after object creation.
+  // Note that this class provides no protection against the download
+  // being destroyed between creation and return of WaitForNewDownloadItem();
+  // the caller must guarantee that in some other fashion.
+  DownloadItem* WaitForNewDownloadItem() {
+    if (!manager_) {
+      // The manager went away before we were asked to wait; return
+      // what we have, even if it's null.
+      return created_item_;
+    }
+
+    if (!created_item_) {
+      waiting_ = true;
+      content::RunMessageLoop();
+      waiting_ = false;
+    }
+    return created_item_;
+  }
+
+ private:
+
+  // DownloadManager::Observer
+  void OnDownloadCreated(DownloadManager* manager, DownloadItem* item) {
+    DCHECK_EQ(manager, manager_);
+    if (!created_item_)
+      created_item_ = item;
+
+    if (waiting_)
+      MessageLoopForUI::current()->Quit();
+
+    manager_->RemoveObserver(this);

[benjhayden, 2012/08/21 20:00:24]

Why do this here?

[Randy Smith (Not in Mondays), 2012/08/21 22:00:37]

We have no interest in hearing anything from the DownloadManager anymore; after
we get our first item, our work is done.  OTOH, there are fewer lines of code
without it, and this is test code, so we don't care about performance or
gratuitous calls, really.  Ok, removed.

+    manager_ = NULL;
+  }
+
+  void ManagerGoingDownload(DownloadManager* manager) {
+    manager_->RemoveObserver(this);
+    manager_ = NULL;
+    if (waiting_)
+      MessageLoopForUI::current()->Quit();
+  }
+
+  bool waiting_;
+  DownloadManager* manager_;
+  DownloadItem* created_item_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadItemCreatedObserver);
+};
+
 class SavePageBrowserTest : public InProcessBrowserTest {
  public:
   SavePageBrowserTest() {}
   virtual ~SavePageBrowserTest();
 
  protected:
   void SetUp() OVERRIDE {
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_));
     ASSERT_TRUE(save_dir_.CreateUniqueTempDir());
     InProcessBrowserTest::SetUp();
@@ skipping 147 matching lines @@
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
   CheckDownloadHistory(url, full_file_name, 1);  // a.htm is 1 file.
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   EXPECT_FALSE(file_util::PathExists(dir));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).Append(FILE_PATH_LITERAL("a.htm")),
       full_file_name));
 }
 
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyCancel) {
+  GURL url = NavigateToMockURL("a");

[benjhayden, 2012/08/21 20:00:24]

I'm just curious. Why "GURL url = ..." instead of "GURL url(...)", and "DM*
dm(...)" instead of "DM* dm = ..."? Seems like there's a part of C++ that I
don't know.

[Randy Smith (Not in Mondays), 2012/08/21 22:00:37]

I don't think there's any difference at the C++ level (you'd think it would use
the assignment operator rather than constructing it directly, but I think it
constructs it directly).  It's this way because I copied from another test in
the same file, and I'm inclined to leave it that way unless you want me to
change the entire file (following the "consistency within files" guideline). 
But I'm happy to change the whole file if you'd like.

+  DownloadManager* manager(GetDownloadManager());
+  std::vector<DownloadItem*> downloads;
+  manager->SearchDownloads(string16(), &downloads);
+  ASSERT_EQ(0u, downloads.size());
+
+  FilePath full_file_name, dir;
+  GetDestinationPaths("a", &full_file_name, &dir);
+  DownloadItemCreatedObserver creation_observer(manager);
+  ASSERT_TRUE(GetCurrentTab()->SavePage(full_file_name, dir,
+                                        content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
+  DownloadItem* item = creation_observer.WaitForNewDownloadItem();
+  item->Cancel(true);
+
+  // TODO(rdsmith): Fix DII::Cancel() to actually cancel the save package.
+  // Currently it's ignored.
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  CheckDownloadHistory(url, full_file_name, 1);  // a.htm is 1 file.
+
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_FALSE(file_util::PathExists(dir));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(FILE_PATH_LITERAL("a.htm")),
+      full_file_name));
+}
+
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyTabDestroy) {
+  GURL url = NavigateToMockURL("a");
+  DownloadManager* manager(GetDownloadManager());
+  std::vector<DownloadItem*> downloads;
+  manager->SearchDownloads(string16(), &downloads);
+  ASSERT_EQ(0u, downloads.size());
+
+  FilePath full_file_name, dir;
+  GetDestinationPaths("a", &full_file_name, &dir);
+  DownloadItemCreatedObserver creation_observer(manager);
+  ASSERT_TRUE(GetCurrentTab()->SavePage(full_file_name, dir,
+                                        content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
+  DownloadItem* item = creation_observer.WaitForNewDownloadItem();
+
+  // Close the tab; does this cancel the download?
+  GetCurrentTab()->Close();
+  EXPECT_TRUE(item->IsCancelled());
+
+  EXPECT_FALSE(file_util::PathExists(full_file_name));

[benjhayden, 2012/08/21 20:00:24]

What happens if you WaitForSavePackageToFinish()? Is that what it should do?

[Randy Smith (Not in Mondays), 2012/08/21 22:00:37]

Well, I didn't try it, but a tab close should result in a SavePackage::Stop(),
which shouldn't ever send the notification that we've successfully finished.  So
I assume it would hang forever.  If you think I should create a new notification
and listen for it, you may well be right :-J--is that where you were going?

[benjhayden, 2012/08/22 13:23:13]

If you think it would help your DFM CL. If not, then don't worry about it.

[Randy Smith (Not in Mondays), 2012/08/22 14:50:43]

Nope; from the DFM perspective, the question is "Do these tests crash?" :-} :-|.

Thanks!

+  EXPECT_FALSE(file_util::PathExists(dir));
+}
+
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveViewSourceHTMLOnly) {
   FilePath file_name(FILE_PATH_LITERAL("a.htm"));
   GURL view_source_url = URLRequestMockHTTPJob::GetMockViewSourceUrl(
       FilePath(kTestDir).Append(file_name));
   GURL actual_page_url = URLRequestMockHTTPJob::GetMockUrl(
       FilePath(kTestDir).Append(file_name));
   ui_test_utils::NavigateToURL(browser(), view_source_url);
 
   FilePath full_file_name, dir;
   GetDestinationPaths("a", &full_file_name, &dir);
@@ skipping 163 matching lines @@
         content::NotificationService::AllSources());
   chrome::SavePage(browser());
   observer.Wait();
   CheckDownloadHistory(url, full_file_name, -1);
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   int64 actual_file_size = -1;
   EXPECT_TRUE(file_util::GetFileSize(full_file_name, &actual_file_size));
   EXPECT_LE(kFileSizeMin, actual_file_size);
 }