Added some cancel/close tab tests to SavePackage.

chrome/browser/download/save_page_browsertest.cc

Index: chrome/browser/download/save_page_browsertest.cc
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index 92ed9b39dfeeac30cb8f7995808452cd24cff96e..c2cba5a8e3c1c804b36161199ab1298b8ba86f4b 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -36,6 +36,7 @@
 #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"
 
@@ -65,6 +66,67 @@ static const char* kAppendedExtension =
 
 }  // 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() {}
@@ -232,6 +294,57 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
       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(