Support for adding save page download items into downloads history.

chrome/browser/download/download_manager.cc

Index: chrome/browser/download/download_manager.cc
===================================================================
--- chrome/browser/download/download_manager.cc	(revision 92431)
+++ chrome/browser/download/download_manager.cc	(working copy)
@@ -55,7 +55,8 @@
     : shutdown_needed_(false),
       profile_(NULL),
       file_manager_(NULL),
-      status_updater_(status_updater->AsWeakPtr()) {
+      status_updater_(status_updater->AsWeakPtr()),
+      next_save_page_id_(0) {
   if (status_updater_)
     status_updater_->AddDelegate(this);
 }
@@ -122,6 +123,7 @@
 
   in_progress_.clear();
   active_downloads_.clear();
+  save_page_downloads_.clear();

[Randy Smith (Not in Mondays), 2011/07/15 17:58:39]

If I'm reading the protocol right, save_page_downloads_ should always be empty
at this point (for the download maanger to be shutdown, its profile must be
shutting down, which requires all tabs associated with the profile to be killed,
which will have Stop()'d all SavePackage instances.)

[achuithb, 2011/07/16 20:15:32]

Didn't know that. I've deleted the line. Do you think I ought to add a DCHECK
here?

This does brings up two points. Firstly, if we're guaranteed that the
DownloadManager does not initiate the Shutdown sequence until all SavePackage
instances are gone, there's really no point in having SavePackage listen for
ManagerGoingDown(). Don't you agree? SavePackage should not be a
DownloadManager::Observer if it never receives the ManagerGoingDown callback.

The other point is that in AssertContainersConsistent, I should just assert that
save_page_downloads_ is empty and not include it in the set union consistency
checks. Do you agree?

[Randy Smith (Not in Mondays), 2011/07/17 18:25:47]

Yes, if you would.
Hmmm.  Fair enough.  I'd like some sort of check for that, though; if we're
wrong, we'll end up indirecting through a freed object.  I'm not sure how best
to do this--maybe leave in the manager doing down notification and called
NOTREACHED() there with a note?
Nope--AssertContainersConsistent is intended to be callable at any time about
any download item.  So it might be called when there is a save page in progress.

[achuithb, 2011/07/20 03:32:21]

Done.

   history_downloads_.clear();
 #if !defined(NDEBUG)
   save_page_as_downloads_.clear();
@@ -985,14 +987,18 @@
   return RemoveDownloadsBetween(base::Time(), base::Time());
 }
 
-void DownloadManager::SavePageAsDownloadStarted(DownloadItem* download) {
+void DownloadManager::SavePageDownloadStarted(DownloadItem* download) {
 #if !defined(NDEBUG)
   save_page_as_downloads_.insert(download);
 #endif
+  DCHECK(!ContainsKey(save_page_downloads_, download->id()));
   downloads_.insert(download);
-  // Add to history and notify observers.
-  AddDownloadItemToHistory(download, DownloadHistory::kUninitializedHandle);
-  NotifyModelChanged();
+  save_page_downloads_[download->id()] = download;
+
+  // Add this entry to the history service.
+  // Additionally, the UI is notified in the callback.
+  download_history_->AddEntry(download,
+      NewCallback(this, &DownloadManager::OnSavePageDownloadEntryAdded));
 }
 
 // Initiate a download of a specific URL. We send the request to the
@@ -1197,6 +1203,13 @@
 
   DCHECK(!ContainsKey(history_downloads_, download->db_handle()));
   history_downloads_[download->db_handle()] = download;
+
+  // Show in the appropriate browser UI.
+  // This includes buttons to save or cancel, for a dangerous download.
+  ShowDownloadInBrowser(download);
+
+  // Inform interested objects about the new download.
+  NotifyModelChanged();
 }
 
 // Once the new DownloadItem's creation info has been committed to the history
@@ -1215,13 +1228,6 @@
 
   AddDownloadItemToHistory(download, db_handle);
 
-  // Show in the appropriate browser UI.
-  // This includes buttons to save or cancel, for a dangerous download.
-  ShowDownloadInBrowser(download);
-
-  // Inform interested objects about the new download.
-  NotifyModelChanged();
-
   // If the download is still in progress, try to complete it.
   //
   // Otherwise, download has been cancelled or interrupted before we've
@@ -1367,3 +1373,38 @@
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }
+
+void DownloadManager::OnSavePageDownloadEntryAdded(int32 download_id,
+                                                   int64 db_handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadMap::const_iterator it = save_page_downloads_.find(download_id);
+  DCHECK(it != save_page_downloads_.end());

[Randy Smith (Not in Mondays), 2011/07/15 17:58:39]

I believe this DCHECK is inaccurate--specifically, if there's a race between the
TabContents closing (and hence SavePackage::Stop() being called) and the history
callback, the history callback could show up with nothing in
save_page_downloads_.  If you agree, could you copy the pattern in
http://codereview.chromium.org/7294013/ to turn around and yank the new entry
back out of the history system?  If you want to leave it in the history system
and mark it as cancelled, that's ok too; I think it's even ok to leave the code
as it is, as long as you document that you're relying on it showing up as
cancelled when it's read back in from the history system.

[achuithb, 2011/07/16 20:15:32]

The item remains in save_page_downloads_ until both the history callback and the
save_package Finalize have been called. The DCHECK should hold, unless I'm
mistaken. The item should not show up as canceled - it should be properly
finished, no matter how the race goes.

[Randy Smith (Not in Mondays), 2011/07/17 18:25:47]

Whoops, I'm sorry; I hadn't tracked that part of the protocol on the last
review.  

I'd really rather not do it that way--the invariant for the rest of the download
manager is that, on cancel, the download is tossed out of the pool (the
active_downloads_, in_progress_ queues) and that after shutdown, there are no
download items associated with the download manager anymore (or at least,
that'll be the invariant when I get my CL committed, hopefully within the next
day or two).  We need to do it that way for downloads because we may have
removed the download as well as cancelled it, which will result in the
DownloadItem being deleted, and we don't want to hold onto any pointers to a
deleted object.  

Would you be willing to shift this protocol to match the above?  I.e. on a
cancel, the download item is removed from save_page_downloads_, and the history
callback does the (a) right thing when it doesn't find the entry in
save_page_downloads_?

Note that my other comment about save_page_downloads_ being null on Shutdown()
relies on this pattern (== my misunderstanding of your original code); it's
perfectly possible for the history service callback to come back after shutdown.

[achuithb, 2011/07/20 03:32:21]

Ok, I've done what you suggest. I was following the existing pattern from
OnCreateDownloadEntryComplete. I see that in
http://codereview.chromium.org/7294013/, you are not allowing this race anymore,
and that you are no longer completing the download in
OnCreateDownloadEntryComplete.

The fact that the download remains in progress forever when the history callback
comes late is rather unfortunate. Are you sure this is the way you want it? 

+  if (it == save_page_downloads_.end())
+    return;
+
+  DownloadItem* download = it->second;
+  DCHECK(download);
+  if (!download)
+    return;
+
+  AddDownloadItemToHistory(download, db_handle);
+
+  // The download may have completed before we received the DB handle.

[Randy Smith (Not in Mondays), 2011/07/15 17:58:39]

Not requesting a change, just registering a disquiet here.  We put some effort
into removing (by serialization) all races not absolutely required in the
download system, and this is putting a new one in.  I think the code is correct
in that nothing particularly ugly will happen.  There is a shutdown race (save
page completes->Shutdown->history callback completes) that'll result in the save
looking like it's been cancelled in the history when it's actually finished, but
I don't think that's too big a deal.  And this basically parallels the one race
we've left in the download code (versus downloading the actual data).  So I
think I'm ok with having this in here until we unify the packages.  Just a bit
ootsy about it.

[achuithb, 2011/07/16 20:15:32]

I've been thinking about this, and no elegant solution presents itself. To get
to serialization, I have to AddRef the SavePackage to make sure it doesn't go
away, then in the history callback, I'd have to lookup a map of download ids to
SavePackage pointers to call back into SavePackage to allow it to proceed. I
need another data structure of pointers and some tricky lifetime management that
I'd rather avoid in this CL. These problems seem stem from the fact that
SavePackage is not more tightly integrated into the downloads system.

I understand and acknowledge your disquiet. If you can suggest a simple way to
fix this race, I'd be happy to implement it.

[Randy Smith (Not in Mondays), 2011/07/17 18:25:47]

Nope--else I would have.  I came to the same conclusion as you--better
integration would solve this problem, and short of that there's not a simple
solution.

+  if (!download->IsInProgress())
+    SavePageDownloadFinished(download);
+}
+
+void DownloadManager::SavePageDownloadFinished(DownloadItem* download) {
+  if (download->db_handle() > DownloadHistory::kUninitializedHandle) {
+    download_history_->UpdateEntry(download);
+    DCHECK(ContainsKey(save_page_downloads_, download->id()));
+    save_page_downloads_.erase(download->id());
+  }
+}
+
+int32 DownloadManager::GetNextSavePageId() {

[benjhayden, 2011/07/18 15:37:45]

Apologies if this was addressed elsewhere, but how are the semantics of this
counter different from DownloadFileManager::GetNextId()?
I'm making some changes to DFM::GNI() and hope to someday merge all of
DownloadManager's maps into one map, which means that save_page ids should be in
the same space as normal download ids. Perhaps my new next id counter will be
compatible with this one?
Not asking for any changes, I'm happy to figure out how to merge the counters
later. I'm just trying to understand how big that job will be.

[achuithb, 2011/07/18 18:33:18]

Not addressed elsewhere.

There's a GetNextId in both DownloadFileManager and SaveFileManager. However
both are created and used on the file thread, which is not appropriate for me. I
need ids on the UI thread...

I expect you'll still need ids on different threads, so you will need at least 2
id counters. To store them in the same map, maybe you can just offset one of the
starting ids by some large number, or add a non-numeral prefix?

[benjhayden, 2011/07/18 19:25:01]

If the multi-thread accessibility is the only issue, then it won't be any
trouble at all.
If you're curious, see the Description here for the current plan
http://codereview.chromium.org/7237034/

[achuith, 2011/07/18 19:44:38]

From reading the CL description, I feel like the integration of this counter
into your scheme should be straightforward. The only reason I introduced a new
one is because none existed on the UI thread and I was too lazy to make one of
the existing ones thread-safe.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return next_save_page_id_++;
+}
+