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)
@@ -164,10 +164,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,
@@ -202,9 +198,9 @@
     return static_cast<int>(in_progress_.size());
   }
 
-  Profile* profile() { return profile_; }
+  Profile* profile() const { return profile_; }

[Paweł Hajdan Jr., 2011/07/27 16:53:51]

Why? It is risky (in terms of hard to misuse API) to make a const method return
a non-const pointer. Same for below.

[Randy Smith (Not in Mondays), 2011/07/27 21:08:35]

This was my fault; an earlier patchset was exposing download_history() and I was
struggling for some way to limit it's usage so I suggested making it const.  I'm
good with pulling the consts back off these methods.

[achuithb, 2011/07/28 00:45:58]

If you are saying that the const does nothing of value, I agree with that - it
is somewhat misleading since the pointer is not const. Anyway, I've removed the
consts.

 
-  DownloadPrefs* download_prefs() { return download_prefs_.get(); }
+  DownloadPrefs* download_prefs() const { return download_prefs_.get(); }
 
   // Creates the download item.  Must be called on the UI thread.
   void CreateDownloadItem(DownloadCreateInfo* info);
@@ -263,10 +259,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 +305,8 @@
 
   virtual ~DownloadManager();
 
+  DownloadHistory* download_history() const { return download_history_.get(); }

[Randy Smith (Not in Mondays), 2011/07/27 21:08:35]

I don't think this is used anymore; can we remove it?

[achuithb, 2011/07/28 00:45:58]

It is used in SavePageBrowserTest::QueryDownloadHistory(). I can make this
method non-const (doesn't really matter). I can also directly access
download_history_ since SavePageBrowserTest is a friend, but I think the
accessor is nicer.

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

Nope, that makes sense.  I don't care a lot about private accessors, and this
balance is right anyway from my POV--it's not used by code inside of
DownloadManager, so it shouldn't use the member variables, but it's from a test,
so a friend test + private restricts access.  Thanks for the explanation.

+
   // Called on the FILE thread to check the existence of a downloaded file.
   void CheckForFileRemovalOnFileThread(int64 db_handle, const FilePath& path);
 
@@ -350,6 +362,9 @@
   // Add a DownloadItem to history_downloads_.
   void AddDownloadItemToHistory(DownloadItem* item, int64 db_handle);
 
+  typedef std::vector<DownloadItem*> DownloadVec;

[Paweł Hajdan Jr., 2011/07/27 16:53:51]

nit: Please avoid abbreviations like Vec. Use Vector, the full name. Also, don't
we already have some typedef for this? Just checking.

Or maybe we could just use DownloadSet for simplicity?

[Randy Smith (Not in Mondays), 2011/07/27 21:08:35]

I don't think we have a typedef for this--before this PS, there were several
methods in DM that took an explicit std::vector<DownloadItem*>.  I wouldn't mind
having a public typedef for this, but I agree with Pawel; it shouldn't be
abbreviated.
I'd rather not do that; DownloadSet is a private typedef having to do with the
internal implementation of the DownloadManager, and I don't want to expose it. 
I also think that vectors are more comfortable to programmers than sets, and
vectors are already in the DM interface (see, e.g., GetAllDownloads).  

[achuithb, 2011/07/28 00:45:58]

I created a public typedef for DownloadVector and used it in the other methods
that return a vector of DownloadItem*. This change has nothing to do with this
CL, so I can revert it if you don't like it.
For RemoveDownloadItem, I went with using DownloadSet, since the order doesn't
really matter.

+  int RemoveDownloadItems(const DownloadVec& items);
+
   // |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
@@ -393,9 +408,7 @@
   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 +433,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_;