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 b1f827b4c0f64bf7eb3bcf224b45800ae46bbc86..f038d28cd00eadbb9bb0955039f4f0ebc3a30667 100644
--- a/chrome/browser/download/download_history.h
+++ b/chrome/browser/download/download_history.h
@@ -6,37 +6,56 @@
 #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 "chrome/browser/cancelable_request.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;
-}
+// Wrap HistoryService for easy mocking and to hide CancelableRequestConsumer.

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Document the lifetime guarantees that this class provides.  I think the primary
one that needs to be documented is that no callback passed to this class will be
called after the class is destroyed.  

Secondary question: Would it be a good idea to change this lifetime guarantee
and say that the callbacks have to provide their own protection, e.g. via
WeakPtrs?  I don't have a vision of what that implementation would look like,
but I could imagine it was as simple as doing a WeakPtrFactory on
DownloadHistory and using it, and I suspect someone familiar with chrome
patterns would find it easier code to read.

[benjhayden, 2012/08/17 18:20:45]

Done.
Added the WeakPtrFactory to DownloadHistory.

+class HistoryServiceDownloadAdapter {

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Why in .h file rather than a forward decl?

[benjhayden, 2012/08/17 18:20:45]

The test needs to be able to override the methods.

+ public:
+  explicit HistoryServiceDownloadAdapter(HistoryService* history_service);
+  virtual ~HistoryServiceDownloadAdapter();
+  virtual void QueryDownloads(const HistoryService::DownloadQueryCallback&);
+  virtual HistoryService::Handle GetVisibleVisitCountToHost(
+      const GURL& referrer_url,
+      const HistoryService::GetVisibleVisitCountToHostCallback& callback);
+  virtual void CreateDownload(
+      int32 id,
+      const DownloadPersistentStoreInfo& info,
+      const HistoryService::DownloadCreateCallback& callback);
+  virtual void UpdateDownload(const DownloadPersistentStoreInfo&);
+  virtual void RemoveDownloads(const std::set<int64>& handles);
+  virtual void OnDownloadHistoryDestroyed();
+
+ private:
+  CancelableRequestConsumer history_consumer_;
+  HistoryService* history_service_;
 
-namespace content {
-class DownloadItem;
-}
+  DISALLOW_COPY_AND_ASSIGN(HistoryServiceDownloadAdapter);
+};
 
-// Interacts with the HistoryService on behalf of the download subsystem.
-class DownloadHistory {
+// Observes a single DownloadManager and all its DownloadItems, keeping the
+// DownloadDatabase up to date.
+class DownloadHistory: public content::DownloadManager::Observer,
+                       public content::DownloadItem::Observer,
+                       public base::SupportsWeakPtr<DownloadHistory> {

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

We've talked about my concerns about SupportsWeakPtr.  What's the argument for
using this, rather than WeakPtrFactory?  Does it need to be part of the public
interface of the class?

[benjhayden, 2012/08/17 18:20:45]

Done.

  public:
   typedef base::Callback<void(bool)> VisitedBeforeDoneCallback;
+  typedef std::vector<DownloadPersistentStoreInfo> InfoVector;

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

InfoVector isn't used in the public portion of the interface--why is it a public
typedef?

[benjhayden, 2012/08/17 18:20:45]

Done.

+  typedef std::set<int64> HandleSet;

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Ditto HandleSet.

[benjhayden, 2012/08/17 18:20:45]

Done.

 
-  explicit DownloadHistory(Profile* profile);
-  ~DownloadHistory();
-
-  // 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);
+  DownloadHistory(
+      content::DownloadManager* manager,
+      HistoryServiceDownloadAdapter* history);
 
-  // Retrieves DownloadCreateInfos saved in the history.
-  void Load(const HistoryService::DownloadQueryCallback& callback);
+  virtual ~DownloadHistory();
 
   // Checks whether |referrer_url| has been visited before today.  This takes
   // ownership of |callback|.

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Not this CL's problem, but: "Takes ownership of |callback|"?  What does that
mean?  a) callbacks are values, and copyable, and b) it's a const reference.  Do
you have any idea what's going on here?

[benjhayden, 2012/08/17 18:20:45]

I think it meant that it copied the callback to an internal map from
HistoryService::Handle to the callback. But that's an implementation detail, so
I deleted the comment.

[Randy Smith (Not in Mondays), 2012/08/20 18:57:04]

Thanks.

@@ -44,49 +63,56 @@ class DownloadHistory {
                                   const GURL& referrer_url,
                                   const VisitedBeforeDoneCallback& callback);
 
-  // Adds a new entry for a download to the history database.
-  void AddEntry(content::DownloadItem* download_item,
-                const HistoryService::DownloadCreateCallback& callback);
-
-  // Updates the history entry for |download_item|.
-  void UpdateEntry(content::DownloadItem* download_item);
-
-  // Updates the download path for |download_item| to |new_path|.
-  void UpdateDownloadPath(content::DownloadItem* download_item,
-                          const FilePath& new_path);
+  // content::DownloadManager::Observer
+  virtual void OnDownloadCreated(
+      content::DownloadManager* manager,
+      content::DownloadItem* item) OVERRIDE;
+  virtual void ManagerGoingDown(content::DownloadManager* manager) OVERRIDE;
 
-  // Removes |download_item| from the history database.
-  void RemoveEntry(content::DownloadItem* download_item);
-
-  // Removes download-related history entries in the given time range.
-  void RemoveEntriesBetween(const base::Time remove_begin,
-                            const base::Time remove_end);
-
-  // Returns a new unique database handle which will not collide with real ones.
-  int64 GetNextFakeDbHandle();
+  // content::DownloadItem::Observer
+  virtual void OnDownloadUpdated(content::DownloadItem* item) OVERRIDE;
+  virtual void OnDownloadOpened(content::DownloadItem* item) OVERRIDE;
+  virtual void OnDownloadRemoved(content::DownloadItem* item) OVERRIDE;
+  virtual void OnDownloadDestroyed(content::DownloadItem* item) OVERRIDE;
 
  private:
+  typedef std::set<int32> IdSet;
+  typedef std::map<int32, DownloadPersistentStoreInfo> InfoMap;
   typedef std::map<HistoryService::Handle, VisitedBeforeDoneCallback>
       VisitedBeforeRequestsMap;
 
+  void MaybeAddToHistory(content::DownloadItem* item);
+  void ItemAdded(int32 id, int64 db_handle);
+  void RemoveDownloadsBatch();
   void OnGotVisitCountToHost(HistoryService::Handle handle,
                              bool found_visits,
                              int count,
                              base::Time first_visit);
+  void QueryCallback(InfoVector* infos);
 
-  Profile* profile_;
+  content::DownloadManager* manager_;
 
-  // 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_;
-
-  CancelableRequestConsumer history_consumer_;
+  HistoryServiceDownloadAdapter* history_;
 
   // The outstanding requests made by CheckVisitedReferrerBefore().
   VisitedBeforeRequestsMap visited_before_requests_;
 
+  // Whether we are in process of creating DownloadItems from the database.
+  // OnDownloadCreated is called before we have a chance to attach a
+  // DownloadHistoryData to the item, so this field is used to disable adding
+  // new items to the database.
+  bool loading_;
+
+  // |db_handles| of items that are scheduled for removal from history.
+  // Cannot be merged into DownloadHistoryData.

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Document that the motivation for this is batching removes (dependent on
investigation of possibility of remove -> MaybeAddToHistory).

[benjhayden, 2012/08/17 18:20:45]

Done.

+  HandleSet removing_;
+
+  // |GetId()|s of items that were removed while they were being added.

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

I wince because these comments aren't really understandable (at least by me)
without reading the .cc file.  In some ways that's inevitable, but if there is a
way to give a just .h file understandable description of these items and move
the implementation specific details into the .cc file (e.g. references to
DownloadHistoryData) that would be better.  Failing that, a one line description
of what a DownloadHistoryData is here would help.

[benjhayden, 2012/08/17 18:20:45]

Done.

+  std::set<int32> removed_while_adding_;

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Document that the motivation is that we can't remove a download until we get the
add callback giving us the DB handle.

[benjhayden, 2012/08/17 18:20:45]

Done.

+
+  // Count the number of items in the history for UMA.
+  int64 history_size_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadHistory);
 };