Support for adding save page download items into downloads history.

chrome/browser/download/save_package.cc

Index: chrome/browser/download/save_package.cc
===================================================================
--- chrome/browser/download/save_package.cc	(revision 91473)
+++ chrome/browser/download/save_package.cc	(working copy)
@@ -21,6 +21,7 @@
 #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"
@@ -173,6 +174,7 @@
       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 ||
@@ -199,6 +201,7 @@
       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();
@@ -224,6 +227,7 @@
       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)) {
 }
 
@@ -315,18 +319,22 @@
   }
 
   // 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) {
@@ -691,6 +699,8 @@
 
   // 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() {
@@ -745,6 +755,8 @@
 
   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,
@@ -1474,3 +1486,14 @@
 
 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);
+}
+