Make DownloadHistory observe manager, items

chrome/browser/download/download_history.h

 // Copyright (c) 2012 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.
 
 #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/memory/scoped_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;
-}
+// Wraps HistoryService for easy mocking. The base implementation contains no
+// mechanisms for life time management, it's just a shim. DownloadHistory owns
+// its HSDA.  DownloadServiceFactory ensures that the downloads system is shut
+// down before HistoryService is destroyed.

[Randy Smith (Not in Mondays), 2012/08/27 18:25:11]

Hmmm.  I'd either like this entire system to not rely on lifetime relationships
with the download system, or to explicitly rely on the lifetime relationship and
not have any code (other than DCHECKs) to handle violations of the expected
lifetime relationship.  It seems like this is explicitly relying on the lifetime
relationship, and ~DownloadHistory has code to handle the download system not
being destroyed when it goes down.  Pick a direction and regularize?

[benjhayden, 2012/09/10 19:02:56]

It looks like you're talking about two different lifetime relationships:
HistoryService <> DownloadHistory and DownloadHistory <> DownloadManager.

DownloadHistory relies on HistoryService not disappearing because there's
nothing that it can do to detect HistoryService destruction. I suppose I could
make a virtual method on DownloadHistory that gets the HistoryService for this
profile every time it goes to call the HistoryService, and all the callers could
just bail if the HistoryService was destroyed. I'd be happy to do that, but I
thought you wanted to get away from that pattern.

DownloadHistory doesn't need to rely on outliving DM or DIs because there are
ways to deal with DM/DI destruction. It might as well use these mechanisms in
case our refactoring extends to ChromeDownloadManagerDelegate, and we decide to
do something slightly different with the CDMD<>DH relationship. It's just
defensive programming.

+class HistoryServiceDownloadAdapter {

[Randy Smith (Not in Mondays), 2012/08/27 18:25:11]

I'm not sure whether this makes sense or not--this interface is probably much
simpler--but given the lack of lifetime guarantees, couldn't you get rid of this
class entirely and just mock HistoryService in tests?

[benjhayden, 2012/09/10 19:02:56]

Done.

+ public:
+  explicit HistoryServiceDownloadAdapter(HistoryService* history_service);
+  virtual ~HistoryServiceDownloadAdapter();
+  virtual void QueryDownloads(const HistoryService::DownloadQueryCallback&);
+  virtual void GetVisibleVisitCountToHost(
+      const GURL& referrer_url,
+      const HistoryService::GetVisibleVisitCountToHostSimpleCallback& callback);
+  virtual void CreateDownload(
+      const DownloadPersistentStoreInfo& info,
+      const HistoryService::DownloadCreateCallback& callback);
+  virtual void UpdateDownload(const DownloadPersistentStoreInfo&);
+  virtual void RemoveDownloads(const std::set<int64>& handles);
 
-namespace content {
-class DownloadItem;
-}
+ private:
+  HistoryService* history_service_;
 
-// Interacts with the HistoryService on behalf of the download subsystem.
-class DownloadHistory {
+  DISALLOW_COPY_AND_ASSIGN(HistoryServiceDownloadAdapter);
+};
+
+// 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();
+  // Neither |manager| nor |history| may be NULL.
+  DownloadHistory(
+      content::DownloadManager* manager,
+      scoped_ptr<HistoryServiceDownloadAdapter> history);
 
-  // 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);
+  virtual ~DownloadHistory();
 
-  // Retrieves DownloadCreateInfos saved in the history.
-  void Load(const HistoryService::DownloadQueryCallback& callback);
-
-  // 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);
+  // content::DownloadManager::Observer
+  virtual void OnDownloadCreated(
+      content::DownloadManager* manager,
+      content::DownloadItem* item) OVERRIDE;
+  virtual void ManagerGoingDown(content::DownloadManager* manager) OVERRIDE;
 
-  // 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);
-
-  // 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::map<HistoryService::Handle, VisitedBeforeDoneCallback>
-      VisitedBeforeRequestsMap;
+  typedef std::set<int64> HandleSet;
+  typedef std::set<int32> IdSet;
+  typedef std::set<content::DownloadItem*> ItemSet;
 
-  void OnGotVisitCountToHost(HistoryService::Handle handle,
-                             bool found_visits,
-                             int count,
-                             base::Time first_visit);
+  void MaybeAddToHistory(content::DownloadItem* item);
+  void ItemAdded(int32 id, int64 db_handle);
+  void RemoveDownloadsBatch();
+  void OnGotVisitCountToHost(
+      const VisitedBeforeDoneCallback& callback,
+      bool found_visits, int count, base::Time first_visit);
+  void QueryCallback(
+      scoped_ptr<std::vector<DownloadPersistentStoreInfo> > infos);
 
-  Profile* profile_;
+  content::DownloadManager* manager_;
+  ItemSet observing_items_;
 
-  // 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_;
+  scoped_ptr<HistoryServiceDownloadAdapter> history_;
 
-  CancelableRequestConsumer history_consumer_;
+  // Whether QueryCallback() is in process of creating DownloadItems from the
+  // database.  OnDownloadCreated is called before QueryCallback() has a chance
+  // to attach a db_handle to the item, so this field is used to disable adding
+  // new items to the database.
+  bool loading_;
 
-  // The outstanding requests made by CheckVisitedReferrerBefore().
-  VisitedBeforeRequestsMap visited_before_requests_;
+  // |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);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_HISTORY_H_