Support for adding save page download items into downloads history.

chrome/browser/download/download_manager.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // The DownloadManager object manages the process of downloading, including
 // updates to the history system and providing the information for displaying
 // the downloads view in the Destinations tab. There is one DownloadManager per
 // active profile in Chrome.
 //
 // Download observers:
@@ skipping 75 matching lines @@
 
     // Called immediately after the DownloadManager puts up a select file
     // dialog.
     // |id| indicates which download opened the dialog.
     virtual void SelectFileDialogDisplayed(int32 id) {}
 
    protected:
     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);
 
   // Notifications sent from the download thread to the UI thread
   void StartDownload(int32 id);
   void UpdateDownload(int32 download_id, int64 size);
   // |hash| is sha256 hash for the downloaded file. It is empty when the hash
   // is not available.
   void OnResponseCompleted(int32 download_id, int64 size, int os_error,
@@ skipping 33 matching lines @@
   int RemoveDownloads(const base::Time remove_begin);
 
   // Remove all downloads will delete all downloads. The number of downloads
   // deleted is returned back to the caller.
   int RemoveAllDownloads();
 
   // Final download manager transition for download: Update the download
   // 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,
                    const std::string& referrer_encoding,
                    TabContents* tab_contents);
 
   // Download the object at the URL and save it to the specified path. The
   // download is treated as the temporary download and thus will not appear
   // in the download history. Used in cases such as drag and drop.
   void DownloadUrlToFile(const GURL& url,
@@ skipping 75 matching lines @@
   void CheckDownloadHashDone(int32 download_id, bool is_dangerous_hash);
 
   // Assert the named download item is on the correct queues
   // in the DownloadManager.  For debugging.
   void AssertQueueStateConsistent(DownloadItem* download);
 
   // Get the download item from the history map.  Useful after the item has
   // 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
   // DownloadManager to the observers of the incognito DownloadManager.
   class OtherDownloadManagerObserver : public Observer {
    public:
     explicit OtherDownloadManagerObserver(
         DownloadManager* observing_download_manager);
     virtual ~OtherDownloadManagerObserver();
 
     // Observer interface.
     virtual void ModelChanged();
     virtual void ManagerGoingDown();
 
    private:
     // The incognito download manager.
     DownloadManager* observing_download_manager_;
 
     // The original profile's download manager.
     DownloadManager* observed_download_manager_;
   };
 
   friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
   friend class DeleteTask<DownloadManager>;
   friend class OtherDownloadManagerObserver;
 
   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);
 
   // Called on the UI thread if the FILE thread detects the removal of
   // the downloaded file. The UI thread updates the state of the file
   // and then notifies this update to the file's observer.
   void OnFileRemovalDetected(int64 db_handle);
 
   // Called on the download thread to check whether the suggested file path
   // exists.  We don't check if the file exists on the UI thread to avoid UI
@@ skipping 35 matching lines @@
   // yet in the history map.
   DownloadItem* GetActiveDownloadItem(int id);
 
   // Debugging routine to confirm relationship between below
   // containers; no-op if NDEBUG.
   void AssertContainersConsistent() const;
 
   // 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
   // "save page as" downloads.  All other DownloadItem containers in
   // the DownloadManager are maps; they do not own the DownloadItems.
   // Note that this is the only place (with any functional implications;
   // see save_page_as_downloads_ below) that "save page as" downloads are
   // kept, as the DownloadManager's only job is to hold onto those
   // until destruction.
   //
@@ skipping 16 matching lines @@
   // When a download is created through a user action, the corresponding
   // DownloadItem* is placed in |active_downloads_| and remains there until the
   // download is in a terminal state (COMPLETE or CANCELLED).  It is also
   // placed in |in_progress_| and remains there until it has received a
   // valid handle from the history system. Once it has a valid handle, the
   // DownloadItem* is placed in the |history_downloads_| map.  When the
   // download reaches a terminal state, it is removed from |in_progress_|.
   // 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_;
 
   // Observers that want to be notified of changes to the set of downloads.
   ObserverList<Observer> observers_;
 
   // The current active profile.
   Profile* profile_;
 
   scoped_ptr<DownloadHistory> download_history_;
 
   scoped_ptr<DownloadPrefs> download_prefs_;
 
   // Non-owning pointer for handling file writing on the download_thread_.
   DownloadFileManager* file_manager_;
 
   // Non-owning pointer for updating the download status.
   base::WeakPtr<DownloadStatusUpdater> status_updater_;
 
   // The user's last choice for download directory. This is only used when the
   // 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_;
 
   scoped_ptr<OtherDownloadManagerObserver> other_download_manager_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManager);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_H_