Support for adding save page download items into downloads history.

chrome/browser/download/download_manager.h

Index: chrome/browser/download/download_manager.h
===================================================================
--- chrome/browser/download/download_manager.h	(revision 94123)
+++ chrome/browser/download/download_manager.h	(working copy)
@@ -93,24 +93,25 @@
     virtual ~Observer() {}
   };
 
+  typedef std::vector<DownloadItem*> DownloadVector;
   // Return all temporary downloads that reside in the specified directory.
   void GetTemporaryDownloads(const FilePath& dir_path,
-                             std::vector<DownloadItem*>* result);
+                             DownloadVector* result);
 
   // Return all non-temporary downloads in the specified directory that are
   // are in progress or have completed.
   void GetAllDownloads(const FilePath& dir_path,
-                       std::vector<DownloadItem*>* result);
+                       DownloadVector* result);
 
   // Return all non-temporary downloads in the specified directory that are
   // in-progress (including dangerous downloads waiting for user confirmation).
   void GetCurrentDownloads(const FilePath& dir_path,
-                           std::vector<DownloadItem*>* result);
+                           DownloadVector* result);
 
   // Returns all non-temporary downloads matching |query|. Empty query matches
   // everything.
   void SearchDownloads(const string16& query,
-                       std::vector<DownloadItem*>* result);
+                       DownloadVector* result);
 
   // Returns true if initialized properly.
   bool Init(Profile* profile);
@@ -164,10 +165,6 @@
   // history and remove the download from |active_downloads_|.
   void DownloadCompleted(int32 download_id);
 
-  // Called when a Save Page As download is started. Transfers ownership
-  // of |download_item| to the DownloadManager.
-  void SavePageAsDownloadStarted(DownloadItem* download_item);
-
   // Download the object at the URL. Used in cases such as "Save Link As..."
   void DownloadUrl(const GURL& url,
                    const GURL& referrer,
@@ -263,10 +260,24 @@
   // been removed from the active map, or was retrieved from the history DB.
   DownloadItem* GetDownloadItem(int id);
 
+  // Called when Save Page download starts. Transfers ownership of |download|
+  // to the DownloadManager.
+  void SavePageDownloadStarted(DownloadItem* download);
+
+  // Callback when Save Page As entry is commited to the history system.
+  void OnSavePageDownloadEntryAdded(int32 download_id, int64 db_handle);
+
+  // Called when Save Page download is done.
+  void SavePageDownloadFinished(DownloadItem* download);
+
+  // Save Page Ids.
+  int32 GetNextSavePageId();
+
  private:
   // For testing.
   friend class DownloadManagerTest;
   friend class MockDownloadManager;
+  friend class SavePageBrowserTest;
 
   // This class is used to let an incognito DownloadManager observe changes to
   // a normal DownloadManager, to propagate ModelChanged() calls from the parent
@@ -295,6 +306,8 @@
 
   virtual ~DownloadManager();
 
+  DownloadHistory* download_history() const { return download_history_.get(); }
+
   // Called on the FILE thread to check the existence of a downloaded file.
   void CheckForFileRemovalOnFileThread(int64 db_handle, const FilePath& path);
 
@@ -350,6 +363,11 @@
   // Add a DownloadItem to history_downloads_.
   void AddDownloadItemToHistory(DownloadItem* item, int64 db_handle);
 
+  typedef std::set<DownloadItem*> DownloadSet;
+  typedef base::hash_map<int64, DownloadItem*> DownloadMap;
+
+  int RemoveDownloadItems(const DownloadSet& items);

[Randy Smith (Not in Mondays), 2011/07/28 21:16:04]

Actually, thinking a bit more about this function, I think I'd like an interface
change.  This isn't a strong feeling; if you disagree, let me know.  But my
thoughts go like this: RemoveDownloadItems is currently called from two places,
one of which removes a single download item, and one of which removes several
download items.  So the requirement for the function to take multiple download
items just comes from one place.  If we hoist the loop traversal from
RemoveDownloadItems into that single caller, we've not increased the amount of
code, and we've simplified the interface (because RemoveDownlaodItems would then
be singular, taking a DownloadItem).  WDYT?

[achuithb, 2011/07/28 22:23:27]

I can't get this to work.

In addition to removing the items from the map, RemoveDownloadItems should call
NotifyModelChanged and delete the item as side-effects, since these actions
logically accompany the removal.

I can implement a RemoveDownloadItem method that removes a single item, calls
NotifyModelChanged and deletes the item. RemoveDownloadsBetween then calls
RemoveDownloadItem in a loop. The problem is that NotifyModelChanged gets called
a number of times. Is this safe and efficient? I worry that in the case of
RemoveAllDownloads with a large history, this could be an unnecessary
performance hit with n invocations of NotifyModelChanged where 1 would suffice.

The other option is to have RemoveDownload strictly remove the item from the
maps, and not call NotifyModelChanged or delete, and have those be done in
RemoveDownloadsBetween and RemoveDownload. I'm afraid I don't like this option
since it calls for code duplication :/

I've gone back to using a DownloadVector - I think it's better to be able to
create a temporary container with the 1 object instead of devoting 2 additional
lines of code to do this, as in the case of the set. 

Let me know what you think.

[Randy Smith (Not in Mondays), 2011/07/28 23:24:05]

Good points.  The way that you've got it LGTM.

+
   // |downloads_| is the owning set for all downloads known to the
   // DownloadManager.  This includes downloads started by the user in
   // this session, downloads initialized from the history system, and
@@ -386,16 +404,12 @@
   // Downloads from past sessions read from a persisted state from the
   // history system are placed directly into |history_downloads_| since
   // they have valid handles in the history system.
-  typedef std::set<DownloadItem*> DownloadSet;
-  typedef base::hash_map<int64, DownloadItem*> DownloadMap;
 
   DownloadSet downloads_;
   DownloadMap history_downloads_;
   DownloadMap in_progress_;
   DownloadMap active_downloads_;
-#if !defined(NDEBUG)
-  DownloadSet save_page_as_downloads_;
-#endif
+  DownloadMap save_page_downloads_;
 
   // True if the download manager has been initialized and requires a shutdown.
   bool shutdown_needed_;
@@ -420,6 +434,9 @@
   // user wants us to prompt for a save location for each download.
   FilePath last_download_path_;
 
+  // Save Page Ids.
+  int32 next_save_page_id_;
+
   // The "Save As" dialog box used to ask the user where a file should be
   // saved.
   scoped_refptr<SelectFileDialog> select_file_dialog_;