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..4fb2fd88d5730be8ff94ed0c6244e326af478ec4 100644
--- a/chrome/browser/download/download_history.h
+++ b/chrome/browser/download/download_history.h
@@ -6,87 +6,123 @@
 #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.
+// Theoretically, CancelableRequestConsumer guarantees that callbacks will not
+// be called after this object is destroyed. As with all guarantees, you should
+// go dig for the code before relying on it.

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

I understand why you feel this way, but I think this isn't a reasonable set of
instructions--it would make software engineering impossible if you had to grok
the code of all producer classes before relying on them.  If it's part of the
interface that is relied on it should be documented (and ideally unit tested,
but I'm not sure I see a way to unit test this).  If it's not part of the
interface, it shouldn't be relied on.  

I think this makes me a conservative software engineer, but I'm not sure :-}.

Also, I was asking you to document the lifetime guarantees that
HistoryServiceDownloadAdapter provides.  

[benjhayden, 2012/08/24 15:32:15]

Done.

+class HistoryServiceDownloadAdapter {
+ 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:
   typedef base::Callback<void(bool)> VisitedBeforeDoneCallback;
 
-  explicit DownloadHistory(Profile* profile);
-  ~DownloadHistory();
+  // Disable persisting this DownloadItem. Static so you don't need to know the
+  // item's profile or manager or expose this from anywhere.

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

What is this used for?  

(If the IsTemporary() rewrite, I'd much rather it went into that CL than this
one--plans change regularly and semi-randomly, and with rare exceptions putting
things in anticipatorially (sic) over time results in dead code that rots.  It's
also easier to track purposes in the CL/revision record if it goes in with the
rest of the feature, and/or in association with the bug that describes the
feature.)

[benjhayden, 2012/08/24 15:32:15]

Done.

+  static void Disable(content::DownloadItem* item);
 
-  // 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);
+  // Neither |manager| nor |history| may be NULL.
+  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|.
+  // Checks whether |referrer_url| has been visited before today.
   void CheckVisitedReferrerBefore(int32 download_id,
                                   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);
-
-  // Removes |download_item| from the history database.
-  void RemoveEntry(content::DownloadItem* download_item);
+  // content::DownloadManager::Observer
+  virtual void OnDownloadCreated(
+      content::DownloadManager* manager,
+      content::DownloadItem* item) OVERRIDE;
+  virtual void ManagerGoingDown(content::DownloadManager* manager) OVERRIDE;
 
-  // 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<int64> HandleSet;
+  typedef std::set<int32> IdSet;
+  typedef std::vector<DownloadPersistentStoreInfo> InfoVector;
+  typedef std::set<content::DownloadItem*> ItemSet;
   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_;
-
-  // 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_;
+  content::DownloadManager* manager_;
+  ItemSet observing_items_;
 
-  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, to
+  // facilitate batching removals together for database efficiency.
+  HandleSet removing_;
+
+  // |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_;
+
+  // Count the number of items in the history for UMA.
+  int64 history_size_;
+
+  base::WeakPtrFactory<DownloadHistory> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadHistory);
 };