Make DownloadHistory observe manager, items

chrome/browser/download/download_history.h

Index: chrome/browser/download/download_history.h
diff --git a/chrome/browser/download/download_history.h b/chrome/browser/download/download_history.h
index 1a831ed706048439d371eb0bdce1c48ce5b11a4c..11299fda6b010eba1b28957af2bbbadaa2594ac6 100644
--- a/chrome/browser/download/download_history.h
+++ b/chrome/browser/download/download_history.h
@@ -5,87 +5,147 @@
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_HISTORY_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_HISTORY_H_
 
-#include <map>
+#include <set>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "chrome/browser/common/cancelable_request.h"
+#include "chrome/browser/download/all_download_item_notifier.h"
 #include "chrome/browser/history/history.h"
+#include "content/public/browser/download_item.h"
+#include "content/public/browser/download_manager.h"
 
-class Profile;
+struct DownloadPersistentStoreInfo;
 
-namespace base {
-class Time;
-}
+// Observes a single DownloadManager and all its DownloadItems, keeping the
+// DownloadDatabase up to date.
+class DownloadHistory : public AllDownloadItemNotifier::Observer {
+ public:
+  typedef std::set<int32> IdSet;
 
-namespace content {
-class DownloadItem;
-}
+  // Caller must guarantee that HistoryService outlives HistoryAdapter.
+  class HistoryAdapter {
+   public:
+    explicit HistoryAdapter(HistoryService* history);
+    virtual ~HistoryAdapter();
 
-// Interacts with the HistoryService on behalf of the download subsystem.
-class DownloadHistory {
- public:
-  typedef base::Callback<void(bool)> VisitedBeforeDoneCallback;
+    virtual void QueryDownloads(
+        const HistoryService::DownloadQueryCallback& callback);
+
+    virtual void CreateDownload(
+        const DownloadPersistentStoreInfo& info,
+        const HistoryService::DownloadCreateCallback& callback);
+
+    virtual void UpdateDownload(const DownloadPersistentStoreInfo& data);
 
-  explicit DownloadHistory(Profile* profile);
-  ~DownloadHistory();
+    virtual void RemoveDownloads(const std::set<int64>& db_handles);
 
-  // Retrieves the next_id counter from the sql meta_table.
-  // Should be much faster than Load so that we may delay downloads until after
-  // this call with minimal performance penalty.
-  void GetNextId(const HistoryService::DownloadNextIdCallback& callback);
+   private:
+    HistoryService* history_;
+    CancelableRequestConsumer consumer_;
+    DISALLOW_COPY_AND_ASSIGN(HistoryAdapter);
+  };
 
-  // Retrieves DownloadCreateInfos saved in the history.
-  void Load(const HistoryService::DownloadQueryCallback& callback);
+  class Observer {
+   public:
+    Observer();
+    virtual ~Observer();
 
-  // Checks whether |referrer_url| has been visited before today.  This takes
-  // ownership of |callback|.
-  void CheckVisitedReferrerBefore(int32 download_id,
-                                  const GURL& referrer_url,
-                                  const VisitedBeforeDoneCallback& callback);
+    // Fires when a download is added to or updated in the database. When
+    // downloads are first added, this fires after the callback from the
+    // database so that |info| includes the |db_handle|. When downloads are
+    // updated, this fires right after the message is sent to the database.
+    // |info| always includes the |db_handle|.
+    virtual void OnDownloadStored(content::DownloadItem* item,
+                                  const DownloadPersistentStoreInfo& info) {
+    }
 
-  // Adds a new entry for a download to the history database.
-  void AddEntry(content::DownloadItem* download_item,
-                const HistoryService::DownloadCreateCallback& callback);
+    // Fires when RemoveDownloads messages are sent to the DB thread.
+    virtual void OnDownloadsRemoved(const IdSet& ids) {}
 
-  // Updates the history entry for |download_item|.
-  void UpdateEntry(content::DownloadItem* download_item);
+    // Fires when the DownloadHistory is being destroyed so that implementors
+    // can RemoveObserver() and nullify their DownloadHistory*s.
+    virtual void OnDownloadHistoryDestroyed() {}
+  };
 
-  // Updates the download path for |download_item| to |new_path|.
-  void UpdateDownloadPath(content::DownloadItem* download_item,
-                          const FilePath& new_path);
+  // Returns true if the item is persisted.
+  static bool IsPersisted(content::DownloadItem* item);

[Randy Smith (Not in Mondays), 2012/11/09 21:36:39]

I'll call out that I'm uncomfortable that this method is static; I think of
statics as worth avoiding unless there's a better way.  The primary argument
I've heard against them is that they can't be mocked if someone wants to write a
test for some other class that uses the specified class.  I'll add a secondary
argument that this method being static reflects an implementation detail, which
is that we're storing the history related DownloadITem information as
ExternalData on the download item, as opposed to in a map inside the
DownloadHistory.  

But I consider both those arguments weak, so I'll make them and leave the choice
up to you.

[benjhayden, 2012/11/12 18:44:16]

There's also no way to mock DownloadHistory out of DownloadService, or mock
HistoryService.
This CL currently has no need to mock IsPersisted or DownloadHistory.
If a future CL needs to mock IsPersisted, then I'd suggest adding
static void SetIsPersistedForTesting(content::DownloadItem* item, bool
persisted);
If IsPersisted needs to move to being an instance method rather than a static
method, then it should be easy enough to migrate callers to call through
DownloadService.

 
-  // Removes |download_item| from the history database.
-  void RemoveEntry(content::DownloadItem* download_item);
+  // Neither |manager| nor |history| may be NULL.
+  // DownloadService creates DownloadHistory some time after DownloadManager is
+  // created and destroys DownloadHistory as DownloadManager is shutting down.
+  DownloadHistory(
+      content::DownloadManager* manager,
+      scoped_ptr<HistoryAdapter> history);
 
-  // Removes download-related history entries in the given time range.
-  void RemoveEntriesBetween(const base::Time remove_begin,
-                            const base::Time remove_end);
+  virtual ~DownloadHistory();
 
-  // Returns a new unique database handle which will not collide with real ones.
-  int64 GetNextFakeDbHandle();
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
 
  private:
-  typedef std::map<HistoryService::Handle, VisitedBeforeDoneCallback>
-      VisitedBeforeRequestsMap;
+  typedef std::set<int64> HandleSet;
+  typedef std::set<content::DownloadItem*> ItemSet;
+
+  // Callback from |history_| containing all entries in the downloads database
+  // table.
+  void QueryCallback(
+      std::vector<DownloadPersistentStoreInfo>* infos);
+
+  // May add |item| to |history_|.
+  void MaybeAddToHistory(content::DownloadItem* item);
+
+  // Callback from |history_| when an item was successfully inserted into the
+  // database.
+  void ItemAdded(int32 id, int64 db_handle);
+
+  // AllDownloadItemNotifier::Observer
+  virtual void OnDownloadCreated(
+      content::DownloadManager* manager, content::DownloadItem* item) OVERRIDE;
+  virtual void OnDownloadUpdated(
+      content::DownloadManager* manager, content::DownloadItem* item) OVERRIDE;
+  virtual void OnDownloadOpened(
+      content::DownloadManager* manager, content::DownloadItem* item) OVERRIDE;
+  virtual void OnDownloadRemoved(
+      content::DownloadManager* manager, content::DownloadItem* item) OVERRIDE;
+
+  // Schedule a record to be removed from |history_| the next time
+  // RemoveDownloadsBatch() runs. Schedule RemoveDownloadsBatch() to be run soon
+  // if it isn't already scheduled.
+  void ScheduleRemoveDownload(int32 download_id, int64 db_handle);
+
+  // Removes all |removing_handles_| from |history_|.
+  void RemoveDownloadsBatch();
+
+
+  AllDownloadItemNotifier notifier_;
+
+  scoped_ptr<HistoryAdapter> history_;
+
+  // |db_handle| of the item being created in response to QueryCallback(),
+  // matched up with created items in OnDownloadCreated() so that the item is
+  // not re-added to the database. For items not created by QueryCallback(),
+  // this is DownloadDatabase::kUninitializedHandle.
+  int64 loading_db_handle_;
 
-  void OnGotVisitCountToHost(HistoryService::Handle handle,
-                             bool found_visits,
-                             int count,
-                             base::Time first_visit);
+  // |db_handles| and |ids| of items that are scheduled for removal from
+  // history, to facilitate batching removals together for database efficiency.
+  HandleSet removing_handles_;
+  IdSet removing_ids_;
 
-  Profile* profile_;
+  // |GetId()|s of items that were removed while they were being added, so that
+  // they can be removed when their db_handles are received from the database.
+  IdSet removed_while_adding_;
 
-  // In case we don't have a valid db_handle, we use |fake_db_handle_| instead.
-  // This is useful for incognito mode or when the history database is offline.
-  // Downloads are expected to have unique handles, so we decrement the next
-  // fake handle value on every use.
-  int64 next_fake_db_handle_;
+  // Count the number of items in the history for UMA.
+  int64 history_size_;
 
-  CancelableRequestConsumer history_consumer_;
+  ObserverList<Observer> observers_;
 
-  // The outstanding requests made by CheckVisitedReferrerBefore().
-  VisitedBeforeRequestsMap visited_before_requests_;
+  base::WeakPtrFactory<DownloadHistory> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadHistory);
 };