Support for adding save page download items into downloads history.

chrome/browser/download/save_package.cc

 // Copyright (c) 2011 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 "chrome/browser/download/save_package.h"
 
 #include <algorithm>
 
 #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-inl.h"
 #include "base/string_piece.h"
 #include "base/string_split.h"
 #include "base/sys_string_conversions.h"
 #include "base/task.h"
 #include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_item.h"
 #include "chrome/browser/download/download_item_model.h"
+#include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/download/save_file.h"
 #include "chrome/browser/download/save_file_manager.h"
 #include "chrome/browser/download/save_item.h"
 #include "chrome/browser/net/url_fixer_upper.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/prefs/pref_member.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ skipping 132 matching lines @@
       saved_main_directory_path_(directory_full_path),
       title_(tab_contents()->GetTitle()),
       finished_(false),
       user_canceled_(false),
       disk_error_occurred_(false),
       save_type_(save_type),
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(tab_contents()->GetRenderProcessHost()->id()),
       unique_id_(g_save_package_id++),
+      download_manager_(tab_contents()->profile()->GetDownloadManager()),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(page_url_.is_valid());
   DCHECK(save_type_ == SAVE_AS_ONLY_HTML ||
          save_type_ == SAVE_AS_COMPLETE_HTML);
   DCHECK(!saved_main_file_path_.empty() &&
          saved_main_file_path_.value().length() <= kMaxFilePathLength);
   DCHECK(!saved_main_directory_path_.empty() &&
          saved_main_directory_path_.value().length() < kMaxFilePathLength);
   InternalInit();
 }
 
 SavePackage::SavePackage(TabContentsWrapper* wrapper)
     : TabContentsObserver(wrapper->tab_contents()),
       wrapper_(wrapper),
       file_manager_(NULL),
       download_(NULL),
       page_url_(GetUrlToBeSaved()),
       title_(tab_contents()->GetTitle()),
       finished_(false),
       user_canceled_(false),
       disk_error_occurred_(false),
       save_type_(SAVE_TYPE_UNKNOWN),
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(tab_contents()->GetRenderProcessHost()->id()),
       unique_id_(g_save_package_id++),
+      download_manager_(tab_contents()->profile()->GetDownloadManager()),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(page_url_.is_valid());
   InternalInit();
 }
 
 // This is for testing use. Set |finished_| as true because we don't want
 // method Cancel to be be called in destructor in test mode.
 // We also don't call InternalInit().
 SavePackage::SavePackage(TabContentsWrapper* wrapper,
                          const FilePath& file_full_path,
                          const FilePath& directory_full_path)
     : TabContentsObserver(wrapper->tab_contents()),
       wrapper_(wrapper),
       file_manager_(NULL),
       download_(NULL),
       saved_main_file_path_(file_full_path),
       saved_main_directory_path_(directory_full_path),
       finished_(true),
       user_canceled_(false),
       disk_error_occurred_(false),
       save_type_(SAVE_TYPE_UNKNOWN),
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(0),
       unique_id_(g_save_package_id++),
+      download_manager_(tab_contents()->profile()->GetDownloadManager()),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
 }
 
 SavePackage::~SavePackage() {
   // Stop receiving saving job's updates
   if (!finished_ && !canceled()) {
     // Unexpected quit.
     Cancel(true);
   }
 
@@ skipping 71 matching lines @@
   wait_state_ = START_PROCESS;
 
   // Initialize the request context and resource dispatcher.
   Profile* profile = tab_contents()->profile();
   if (!profile) {
     NOTREACHED();
     return false;
   }
 
   // Create the fake DownloadItem and display the view.
-  DownloadManager* download_manager =
-      tab_contents()->profile()->GetDownloadManager();
-  download_ = new DownloadItem(download_manager,
+  if (!download_manager_) {
+    NOTREACHED();
+    return false;
+  }
+  download_ = new DownloadItem(download_manager_,
                                saved_main_file_path_,
                                page_url_,
                                profile->IsOffTheRecord());
 
   // Transfer the ownership to the download manager. We need the DownloadItem
   // to be alive as long as the Profile is alive.
-  download_manager->SavePageAsDownloadStarted(download_);
+  download_manager_->SavePageAsDownloadStarted(download_);
 
-  wrapper_->download_tab_helper()->OnStartDownload(download_);
+  // Add this entry to the history service, which also notifies the UI.
+  download_manager_->download_history()->AddEntry(download_,
+      NewCallback(this, &SavePackage::OnDownloadEntryAdded));

[Randy Smith (Not in Mondays), 2011/07/08 16:56:25]

I'm concerned that there's a race here if SavePackage::Stop() or
SavePackage::Finish() get called before the history system callback.  In that
case, there won't be a db_handle() on the download, and the history system won't
be updated.  That might be the correct (or least incorrect) thing to do, but the
code should at least have comments documenting what the behavior will be in
those cases and why it's acceptable. 

[achuithb, 2011/07/08 21:23:54]

Done.

 
   // Check save type and process the save page job.
   if (save_type_ == SAVE_AS_COMPLETE_HTML) {
     // Get directory
     DCHECK(!saved_main_directory_path_.empty());
     GetAllSavableResourceLinksForCurrentPage();
   } else {
     wait_state_ = NET_FILES;
     SaveFileCreateInfo::SaveFileSource save_source = page_url_.SchemeIsFile() ?
         SaveFileCreateInfo::SAVE_FILE_FROM_FILE :
@@ skipping 344 matching lines @@
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(file_manager_,
                         &SaveFileManager::RemoveSavedFileFromFileMap,
                         save_ids));
 
   finished_ = true;
   wait_state_ = FAILED;
 
   // Inform the DownloadItem we have canceled whole save page job.
   download_->Cancel(false);
+  if (download_manager_)
+    download_manager_->download_history()->UpdateEntry(download_);
 }
 
 void SavePackage::CheckFinish() {
   if (in_process_count() || finished_)
     return;
 
   FilePath dir = (save_type_ == SAVE_AS_COMPLETE_HTML &&
                   saved_success_items_.size() > 1) ?
                   saved_main_directory_path_ : FilePath();
 
@@ skipping 34 matching lines @@
     save_ids.push_back(it->second->save_id());
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(file_manager_,
                         &SaveFileManager::RemoveSavedFileFromFileMap,
                         save_ids));
 
   download_->OnAllDataSaved(all_save_items_count_);
   download_->MarkAsComplete();
+  if (download_manager_)
+    download_manager_->download_history()->UpdateEntry(download_);
 
   NotificationService::current()->Notify(
       NotificationType::SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
       Source<SavePackage>(this),
       Details<GURL>(&page_url_));
 }
 
 // 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,
@@ skipping 709 matching lines @@
 }
 
 // SelectFileDialog::Listener interface.
 void SavePackage::FileSelected(const FilePath& path,
                                int index, void* params) {
   ContinueSave(path, index);
 }
 
 void SavePackage::FileSelectionCanceled(void* params) {
 }
+
+void SavePackage::ManagerGoingDown() {

[Randy Smith (Not in Mondays), 2011/07/08 16:56:25]

Where is the SavePackage registered as an observer of the download manager? 
(DownloadManager::AddObserver)  And, arguably more importantly given the below,
where is the SavePackage *de*registered as an observer of DownloadManager?  If
the SavePackages complets and is destroyed before the download manager is
shutdown (common case, I think) that means any download manager notifications
will be indirecting through invalid pointers.

[achuithb, 2011/07/08 21:23:54]

I left it out by mistake. I've added the calls to AddObserver/RemoveObserver,
and re-done this code a bit.
Done.

+  download_manager_ = NULL;

[Randy Smith (Not in Mondays), 2011/07/08 16:56:25]

I believe that it isn't possible for the download manager to go away while a
save page in in progress.  Specifically, for the download manager to go away,
the profile owning it has to go away, and I think that requires all tabs
associated with the profile to have been killed (which would have resulted in
SavePackage::Stop()).  Given that the consequences are indirecting through a
pointer to garbage memory, I don't mind the belt-and-suspenders of the
observering the download manager and setting the internal pointer to null if it
does go down, but if you agree with my logic could you put DCHECKs that the
download_manager_ is still alive before each use?

[achuithb, 2011/07/08 21:23:54]

Makes sense. DCHECKs added. Done.

+}
+
+void SavePackage::OnDownloadEntryAdded(int32 download_id, int64 db_handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (download_manager_)
+    download_manager_->AddDownloadItemToHistory(download_, db_handle);
+}
+