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

content/browser/download/save_package.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 "content/browser/download/save_package.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/i18n/file_util_icu.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/string_piece.h"
 #include "base/string_split.h"
 #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"
 #include "content/browser/download/save_file.h"
 #include "content/browser/download/save_file_manager.h"
 #include "content/browser/download/save_item.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/resource_dispatcher_host_impl.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #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"
 #include "net/base/io_buffer.h"
 #include "net/base/mime_util.h"
 #include "net/base/net_util.h"
 #include "net/url_request/url_request_context.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebPageSerializerClient.h"
 
 using base::Time;
@@ skipping 278 matching lines @@
 
   return true;
 }
 
 void SavePackage::OnMHTMLGenerated(const FilePath& path, int64 size) {
   if (size <= 0) {
     Cancel(false);
     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))) {
     Finish();
   }
 }
 
 // On POSIX, the length of |pure_file_name| + |file_name_ext| is further
 // restricted by NAME_MAX. The maximum allowed path looks like:
 // '/path/to/save_dir' + '/' + NAME_MAX.
@@ skipping 381 matching lines @@
   for (SaveUrlItemMap::iterator it = saved_failed_items_.begin();
        it != saved_failed_items_.end(); ++it)
     save_ids.push_back(it->second->save_id());
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&SaveFileManager::RemoveSavedFileFromFileMap,
                  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.
 void SavePackage::SaveFinished(int32 save_id, int64 size, bool is_success) {
   // Because we might have canceled this saving job before,
   // so we might not find corresponding SaveItem. Just ignore it.
   SaveItem* save_item = LookupItemInProcessBySaveId(save_id);
   if (!save_item)
     return;
 
   // Let SaveItem set end state.
   save_item->Finish(size, is_success);
   // Remove the associated save id and SavePackage.
   file_manager_->RemoveSaveFile(save_id, save_item->url(), this);
 
   PutInProgressItemToSavedMap(save_item);
 
   // 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 &&
       save_item->url() == page_url_ && !save_item->received_bytes()) {
     // If size of main HTML page is 0, treat it as disk error.
     Cancel(false);
     return;
   }
 
   if (canceled()) {
@@ skipping 21 matching lines @@
     return;
   }
   SaveItem* save_item = it->second;
 
   save_item->Finish(0, false);
 
   PutInProgressItemToSavedMap(save_item);
 
   // 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) ||
       (save_type_ == content::SAVE_PAGE_TYPE_AS_MHTML) ||
       (save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM)) {
     // We got error when saving page. Treat it as disk error.
     Cancel(true);
   }
 
   if (canceled()) {
@@ skipping 267 matching lines @@
     const std::vector<content::Referrer>& referrers_list,
     const std::vector<GURL>& frames_list) {
   if (wait_state_ != RESOURCES_LIST)
     return;
 
   DCHECK(resources_list.size() == referrers_list.size());
   all_save_items_count_ = static_cast<int>(resources_list.size()) +
                            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_) {
     // Put all sub-resources to wait list.
     for (int i = 0; i < static_cast<int>(resources_list.size()); ++i) {
       const GURL& u = resources_list[i];
       DCHECK(u.is_valid());
       SaveFileCreateInfo::SaveFileSource save_source = u.SchemeIsFile() ?
           SaveFileCreateInfo::SAVE_FILE_FROM_FILE :
           SaveFileCreateInfo::SAVE_FILE_FROM_NET;
@@ skipping 248 matching lines @@
 }
 
 void SavePackage::OnDownloadDestroyed(DownloadItem* download) {
   StopObservation();
 }
 
 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();
 }