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 73 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);
+  void GetTemporaryDownloads(const FilePath& dir_path, 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);
+  void GetAllDownloads(const FilePath& dir_path, 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);
+  void GetCurrentDownloads(const FilePath& dir_path, DownloadVector* result);
 
   // Returns all non-temporary downloads matching |query|. Empty query matches
   // everything.
-  void SearchDownloads(const string16& query,
-                       std::vector<DownloadItem*>* result);
+  void SearchDownloads(const string16& query, 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);
+
+  // Id for next Save Page.
+  int32 GetNextSavePageId();
+
  private:
+  typedef std::set<DownloadItem*> DownloadSet;
+  typedef base::hash_map<int64, DownloadItem*> DownloadMap;
+
   // 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() { return download_history_.get(); }

[Paweł Hajdan Jr., 2011/08/01 18:22:59]

nit: Why private? If it's needed just go ahead and make it public. It's totally
fine.

[achuithb, 2011/08/01 21:19:54]

I had it as public but Randy preferred to have this be private. Randy, what was
your rationale for wanting this to be private?

[achuithb, 2011/08/02 19:00:41]

I forgot Randy is on vacation. I'm going to make this public as I originally had
it. Making it private is a small change that we can do later if Randy wants it
that way. 

[Randy Smith (Not in Mondays), 2011/08/03 16:03:09]

My memory is that it was only needed by tests.  I care a lot about the interface
to a class, and I consider private functions part of the implementation rather
than the interface, so if at all possible, I like to avoid putting things in the
public interface to classes.  Specifically, if something's only needed for
tests, I don't want to pollute the interface (that later code may target) with
it.  

Remind me what uses this?  (I'm tossing this comment in in hopes of resolving
this before you commit.)

[Paweł Hajdan Jr., 2011/08/03 19:07:46]

I think it's better to use #ifdef UNIT_TEST then rather than private. Friending
gives you access to many more internals.

[achuithb, 2011/08/03 19:52:43]

Pawel: It's not a unit test. Is UNIT_TEST also applicable to browser tests?

Randy: It's only needed by SavePageBrowserTest.

I've made this private again.

[Paweł Hajdan Jr., 2011/08/03 20:49:40]

If it isn't it's a bug.

[achuithb, 2011/08/03 21:05:28]

Done.

+
   // 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);
 
+  // Remove from internal maps.
+  int RemoveDownloadItems(const DownloadVector& pending_deletes);
+
   // |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.

[Paweł Hajdan Jr., 2011/08/01 18:22:59]

nit: Hey, please actually see my earlier comment about this and fix it.

Or did you just forgot to upload the fix?

[achuithb, 2011/08/01 21:19:54]

Sorry, I changed the comments of the public GetNextSavePageIds() method but not
of the private variable. I'll upload it with the next patchset

+  int32 next_save_page_id_;
+
   scoped_ptr<OtherDownloadManagerObserver> other_download_manager_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManager);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_H_