Make the SavePackage use of the fake download item cleaner around cancellation.

content/browser/download/save_package.cc

Index: content/browser/download/save_package.cc
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index 0ef878efa2581dca506cbbea68a4b196f860784f..adf44d8a0259ec8dcf54cd5e9663f4f34980fe44 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -18,7 +18,6 @@
 #include "base/sys_string_conversions.h"
 #include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
-#include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/download/download_stats.h"
@@ -35,6 +34,8 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
 #include "content/public/browser/resource_context.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
@@ -333,12 +334,18 @@ void SavePackage::OnMHTMLGenerated(const FilePath& path, int64 size) {
     return;
   }
   wrote_to_completed_file_ = true;
-  download_->SetTotalBytes(size);
-  download_->UpdateProgress(size, size, DownloadItem::kEmptyFileHash);
-  // Must call OnAllDataSaved here in order for
-  // GDataDownloadObserver::ShouldUpload() to return true.
-  // ShouldCompleteDownload() may depend on the gdata uploader to finish.
-  download_->OnAllDataSaved(size, DownloadItem::kEmptyFileHash);
+
+  // Hack to avoid touching download_ after user cancel.
+  // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
+  // with SavePackage flow.
+  if (download_->IsInProgress()) {
+    download_->SetTotalBytes(size);
+    // Must call OnAllDataSaved here in order for
+    // GDataDownloadObserver::ShouldUpload() to return true.
+    // ShouldCompleteDownload() may depend on the gdata uploader to finish.
+    download_->OnAllDataSaved(size, DownloadItem::kEmptyFileHash);
+  }
+
   if (!download_manager_->GetDelegate() ||
       download_manager_->GetDelegate()->ShouldCompleteDownload(
           download_, base::Bind(&SavePackage::Finish, this))) {
@@ -740,13 +747,17 @@ void SavePackage::Finish() {
                  file_manager_,
                  save_ids));
 
-  if (download_) {
-    if (save_type_ != content::SAVE_PAGE_TYPE_AS_MHTML)
+  // Hack to avoid touching download_ after user cancel.
+  // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
+  // with SavePackage flow.
+  if (download_ && download_->IsInProgress()) {
+    if (save_type_ != content::SAVE_PAGE_TYPE_AS_MHTML) {
       download_->OnAllDataSaved(all_save_items_count_,
                                 DownloadItem::kEmptyFileHash);
+    }
     download_->MarkAsComplete();
-    FinalizeDownloadEntry();
   }
+  FinalizeDownloadEntry();
 }
 
 // Called for updating end state.
@@ -766,7 +777,10 @@ void SavePackage::SaveFinished(int32 save_id, int64 size, bool is_success) {
 
   // Inform the DownloadItem to update UI.
   // We use the received bytes as number of saved files.
-  if (download_)
+  // Hack to avoid touching download_ after user cancel.
+  // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
+  // with SavePackage flow.
+  if (download_ && download_->IsInProgress())
     download_->UpdateProgress(completed_count(), CurrentSpeed(), "");
 
   if (save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM &&
@@ -808,7 +822,10 @@ void SavePackage::SaveFailed(const GURL& save_url) {
 
   // Inform the DownloadItem to update UI.
   // We use the received bytes as number of saved files.
-  if (download_)
+  // Hack to avoid touching download_ after user cancel.
+  // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
+  // with SavePackage flow.
+  if (download_ && download_->IsInProgress())
     download_->UpdateProgress(completed_count(), CurrentSpeed(), "");
 
   if ((save_type_ == content::SAVE_PAGE_TYPE_AS_ONLY_HTML) ||
@@ -1096,7 +1113,10 @@ void SavePackage::OnReceivedSavableResourceLinksForCurrentPage(
                            static_cast<int>(frames_list.size());
 
   // We use total bytes as the total number of files we want to save.
-  if (download_)
+  // Hack to avoid touching download_ after user cancel.
+  // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
+  // with SavePackage flow.
+  if (download_ && download_->IsInProgress())
     download_->SetTotalBytes(all_save_items_count_);
 
   if (all_save_items_count_) {
@@ -1365,6 +1385,16 @@ void SavePackage::FinalizeDownloadEntry() {
   DCHECK(download_);
   DCHECK(download_manager_);
 
+  content::NotificationService::current()->Notify(
+      content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,

[benjhayden, 2012/09/10 14:39:58]

Did we talk about not using this? I thought we had figured out that we didn't
really need this.

[Randy Smith (Not in Mondays), 2012/09/10 17:53:16]

Looking at the code, I'd rather not make the change:
* There are locations using the notification outside of save_page_browsertest.cc
(automation providers, browser_encoding_browsertest).
* There isn't an easy way to get the DownloadItem associated with a save page;
you have to rely on nothing else going on so it's the only one showing up on the
download manager.  (And two of the uses *in* save_page_browsertest.cc don't
currently do that.)
* Making the switch relies for save page termination on code that's known to be
fragile, has in the past been flaky, has TODOs all over it, and is scheduled to
be nuked soon.

+      // We use the DownloadManager as the source as that's a
+      // central SavePackage related location that observers can
+      // get to if they want to wait for notifications for a
+      // particular BrowserContext.  Alternatively, we could make
+      // it come from the WebContents, which would be more specific
+      // but less useful to (current) customers.
+      content::Source<content::DownloadManager>(download_manager_),
+      content::Details<content::DownloadItem>(download_));
   download_manager_->SavePageDownloadFinished(download_);
   StopObservation();
 }