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 91883)
+++ 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_(NULL),
       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_(NULL),
       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_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
 }
 
@@ -314,19 +318,27 @@
     return false;
   }
 
+  // Get the download manager and add ourselves as an observer.
+  download_manager_ = tab_contents()->profile()->GetDownloadManager();
+  if (!download_manager_) {
+    NOTREACHED();
+    return false;
+  }
+  download_manager_->AddObserver(this);
+
   // Create the fake DownloadItem and display the view.
-  DownloadManager* download_manager =
-      tab_contents()->profile()->GetDownloadManager();
-  download_ = new DownloadItem(download_manager,
+  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.

[Randy Smith (Not in Mondays), 2011/07/10 19:37:40]

I think it's worthwhile making clear that the UI is notified in the callback,
not directly by the history service.

[achuithb, 2011/07/13 21:48:16]

Done.

+  download_manager_->download_history()->AddEntry(download_,
+      NewCallback(this, &SavePackage::OnDownloadEntryAdded));

[Randy Smith (Not in Mondays), 2011/07/10 19:37:40]

I'm not an expert on this, but I think that NewCallback doesn't take a reference
on the object passed, which means that this call has no protection against the
callback arriving at a freed object.  (The way this works in DownloadManager is
the download_history_'s lifetime is tied to DownloadManager, so the callback
chain gets blocked at download_history_ if DownloadManager's gone away.)  Could
you confirm that you're safe against the callback being delayed past object
destruction?

[achuithb, 2011/07/13 21:48:16]

I decided to move this code into download_manager. The other option was to
AddRef() here and Release() in the callback to ensure that the save_package
doesn't go away. But I think having this code in the download_manager is more
natural, and also makes it unnecessary to expose the history_service (except for
testing).

 
   // Check save type and process the save page job.
   if (save_type_ == SAVE_AS_COMPLETE_HTML) {
@@ -691,6 +703,7 @@
 
   // Inform the DownloadItem we have canceled whole save page job.
   download_->Cancel(false);
+  FinalizeDownloadEntry();
 }
 
 void SavePackage::CheckFinish() {
@@ -745,6 +758,7 @@
 
   download_->OnAllDataSaved(all_save_items_count_);
   download_->MarkAsComplete();
+  FinalizeDownloadEntry();
 
   NotificationService::current()->Notify(
       NotificationType::SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
@@ -1474,3 +1488,26 @@
 
 void SavePackage::FileSelectionCanceled(void* params) {
 }
+
+void SavePackage::ManagerGoingDown() {
+  download_manager_ = NULL;
+}
+
+void SavePackage::OnDownloadEntryAdded(int32 download_id, int64 db_handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // download_manager_ can be null if Finalize got called before this callback.
+  // In that case, this item would not get added to the history db.

[Randy Smith (Not in Mondays), 2011/07/10 19:37:40]

Nope, it's been added to the DB, it just won't be added to the now-non-existent
download manager.  No harm, no foul.

[achuithb, 2011/07/13 21:48:16]

This code is now in DownloadManager.

+  DCHECK(download_manager_);

[Randy Smith (Not in Mondays), 2011/07/10 19:37:40]

This is the one case in which I think download_manager_ can indeed be null in
normal operation (because callbacks may be arbitrarily delayed).  So no DCHECK()
here.

[achuithb, 2011/07/13 21:48:16]

This code is now in DownloadManager.

+  if (download_manager_)
+    download_manager_->AddDownloadItemToHistory(download_, db_handle);
+}
+
+void SavePackage::FinalizeDownloadEntry() {
+  DCHECK(download_manager_);
+  if (download_manager_) {
+    download_manager_->download_history()->UpdateEntry(download_);

[Randy Smith (Not in Mondays), 2011/07/10 19:37:40]

So there's a pretty subtle but important difference between the flow in the
DownloadManager vs. the flow in SavePackage with this CL.  In the
DownloadManager, we delay just about anything happening until the history
callback comes back--the download item can be cancelled (new CL going in to do
this properly which you just made ChromeOS review comments on; you may want to
examine the history interactions in download_manager.cc in
http://codereview.chromium.org/7294013/), but we don't let it progress in any
meaningful way (bytes downloaded and dumped into a file, but that's it) until
the history db gets back to us.  

So the main downloads code allows a race between history callback and cancel
(either one can complete first) but forces completion to be strictly after
history callback.  This CL, if I'm reading it right, allows races between
history callback and both cancellation and completion.  And I don't see any
special code to handle either race.  In the cancellation race case I think we
end up with a started download in the history that's never marked as
completing/canceling, which isn't horrible--there are corner cases that do that
for the main code.  In the completion case we'll end up with a completed
download that isn't completed in the history, and will later show up as
cancelled--I don't think that's ok.

I'm afraid I'd like to request that you mimic the main download code, and hold
off completion until you get the history callback--de-racing that code was, to
my mind, the key thing that we did to deflake the tests, and I don't want to
create new, racy code here.  And putting in explicit comments about what occurs
and why it's ok when the history/cancel race happens in the wrong order (cancel
occurs before history callback) would also be very good.

[Randy Smith (Not in Mondays), 2011/07/10 19:37:40]

If you could also take a look at how hard it would be to put in tests which
explore each of these races (history versus cancel, history versus completion)
I'd be grateful.  There may not be good ways--the way I eventually want to do it
for the main download system is by mocking the history system and using a test
mock object that delays the callback until explicitly triggered.  But there may
be ways to make it likely that one or another event happens first based on the
current state of the system.

[achuithb, 2011/07/13 21:48:16]

I'm now handling the race between completion and history callback. Cancellation
doesn't work at all for save_package, but I believe the code is correct for that
case as well. 

[achuithb, 2011/07/13 21:48:16]

I think adding a test for the races should be doable. I'm going to take a look.
I'm concerned that the test itself may end up being tricky, racy and/or flaky so
I'd like to do it separate from this CL.

+    download_manager_->RemoveObserver(this);
+    download_manager_ = NULL;

[Randy Smith (Not in Mondays), 2011/07/10 19:37:40]

I'd like a CHECK() in the destructor that the download_manager_ is null at that
point; this CL has tied observing/non-observing state to download_manager_
not-null/null, and we want to be really really sure that we don't stay observing
the download manager after destruction.  I'm requesting a CHECK() rather that a
DCHECK, because if we are observing the download manager after destruction,
that's a very dangerous security issue (function pointer indirection through
freed memory).

[achuithb, 2011/07/13 21:48:16]

Done.

+  }
+}
+