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..61fb082b72403cf3d171830f33844bfeceec52a0 100644
--- a/chrome/browser/download/download_history.h
+++ b/chrome/browser/download/download_history.h
@@ -6,86 +6,116 @@
 #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/hyperbolic_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;
-}
-
-namespace content {
-class DownloadItem;
-}
-
-// 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 HyperbolicDownloadItemNotifier::Observer {
  public:
   typedef base::Callback<void(bool)> VisitedBeforeDoneCallback;
+  typedef std::set<int64> HandleSet;
+
+  // Since there isn't (and shouldn't be) a general public way to access

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Why shouldn't there be?  It feels very weird for a file to have to include
download_history.h (for the observer definition), but not have any access to the
class.  

If the answer is "because we don't want consumers to have to know whether there
*is* a history or not", I can think of a couple of ways to deal with the issue. 
One is to make DH an ABC and have a null implementation.  Another is to make
DownloadHistoryObserver (no colon) a top level class, and expose it from the
CDMD (though I'll admit that feels a bit weird too).  I find myself leaning
towards the null class idea, but mostly away from the "declare in one class,
have as an argument to a complete different class".

[benjhayden, 2012/11/02 17:21:37]

Ok, yeah, I'll expose DownloadHistory so that we don't need to bounce
Add/RemoveHistoryObserver.
Before I do that, how do you feel about the suggestion that I made in another
comment about putting DownloadHistory in DownloadService instead of CDMD, and
making CDMD implement CheckVisitedReferrerBefore directly on top of
HistoryService?

[Randy Smith (Not in Mondays), 2012/11/02 23:31:24]

I think it's a good idea.

[benjhayden, 2012/11/06 20:01:14]

Done.

+  // DownloadHistory directly, observers should use
+  // DownloadServiceFactory::GetForProfile(profile)->
+  //   GetDownloadManagerDelegate()->
+  //     AddHistoryObserver(this);
+  //     RemoveHistoryObserver(this);
+  class Observer {
+   public:
+    Observer();
+    virtual ~Observer();
+
+    // Fires when CreateDownload/UpdateDownload messages are sent to the DB
+    // thread.
+    virtual void OnDownloadStored(const DownloadPersistentStoreInfo& info) {}
+
+    // Fires when RemoveDownloads messages are sent to the DB thread.
+    virtual void OnDownloadsRemoved(const HandleSet& db_handle) {}

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

The lack of an equivalent to OnDownloadDestroyed/ManagerGoingDown in this
interface means that all users of it *must* be destroyed before the
DownloadHistory itself.  That seems like an unnecessary restriction?  If you
feel like it keeps the interface cleaner, do document it at the
Add/RemoveObserver declarations--it's the kind of thing that's implicit in the
interface, but that might be missed if someone is in a hurry or not thinking
things through (maybe they missed the Balmer peak? :-})

[benjhayden, 2012/11/02 17:21:37]

I'm not sure who you're referring to by 'users'.
ADIN::Observers deal with ManagerGoingDown by checking ADIN::GetManager().
I'm not sure why a DH::O would want to match up OnDownloadsRemoved() calls with
DownloadItems. Wouldn't they just observe the item? Maybe
DH::O::OnDownloadsRemoved isn't all that useful...

[Randy Smith (Not in Mondays), 2012/11/02 23:31:24]

I'm confused about how you're saying relates to my comment--possibly we should
talk about this in person.  In parallel to the DM::O and DI::O interfaces (but
not ADIN::O), all users of this interface must call DH::RemoveObserver() before
they self-destruct.  That means they need to keep a pointer to DH around.  But
they don't want to keep a pointer to DH around if DH has been destroyed.  If
they don't get a warning when DH dies, they need to always die before DH does.  
So I'm suggesting that you either add a HistoryGoingDown() method (preferred) or
document that requirement at the Add/RemoveObserver interface (disliked, but you
could talk me into it).  

Does that make more sense than my first note?

[benjhayden, 2012/11/06 20:01:14]

Ah! Yes, thanks.
Done.

+  };
+
+  // Neither |manager| nor |history| may be NULL.
+  // Caller must guarantee that HistoryService outlives DownloadHistory.

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Suggestion: Add a note explicitly stating that we're independent of the
DownloadManager lifetime?

[benjhayden, 2012/11/02 17:21:37]

What do you mean by 'independent'?
ADIN::Os deal with ManagerGoingDown by checking ADIN::GetManager.

[Randy Smith (Not in Mondays), 2012/11/02 23:31:24]

Yep, that's how the implementation deals, but users of DownloadHistory shouldn't
have to know about the implementation, that's sorta the point.  I was suggesting
(your call; it's sorta implied) that while the caller must guarantee that
HistoryService outlives DownloadHistory, the caller doesn't have to worry about
the DownloadManager lifetime in relationship to DownloadHistory.  I think of it
as an obvious question for consumers, since DownloadHistory is acting as a
bridge between HistoryService and DownloadManager.

[benjhayden, 2012/11/06 20:01:14]

Done.

+  DownloadHistory(
+      content::DownloadManager* manager,
+      HistoryService* history);
+
+  virtual ~DownloadHistory();
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // Checks whether |referrer_url| has been visited before today.
+  void CheckVisitedReferrerBefore(const GURL& referrer_url,
+                                  const VisitedBeforeDoneCallback& callback);
 
-  explicit DownloadHistory(Profile* profile);
-  ~DownloadHistory();
+ private:
+  typedef std::set<int32> IdSet;
+  typedef std::set<content::DownloadItem*> ItemSet;
 
-  // 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);
+  // HyperbolicDownloadItemNotifier::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;
 
-  // Retrieves DownloadCreateInfos saved in the history.
-  void Load(const HistoryService::DownloadQueryCallback& callback);
+  void MaybeAddToHistory(content::DownloadItem* item);
 
-  // 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);
+  void ItemAdded(int32 id, int64 db_handle);
 
-  // Adds a new entry for a download to the history database.
-  void AddEntry(content::DownloadItem* download_item,
-                const HistoryService::DownloadCreateCallback& callback);
+  void RemoveDownloadsBatch();
 
-  // Updates the history entry for |download_item|.
-  void UpdateEntry(content::DownloadItem* download_item);
+  void OnGotVisitCountToHost(
+      const VisitedBeforeDoneCallback& callback,
+      HistoryService::Handle unused_handle,
+      bool found_visits, int count, base::Time first_visit);
 
-  // Updates the download path for |download_item| to |new_path|.
-  void UpdateDownloadPath(content::DownloadItem* download_item,
-                          const FilePath& new_path);
+  void QueryCallback(
+      std::vector<DownloadPersistentStoreInfo>* infos);
 
-  // Removes |download_item| from the history database.
-  void RemoveEntry(content::DownloadItem* download_item);
+  HyperbolicDownloadItemNotifier notifier_;
 
-  // Removes download-related history entries in the given time range.
-  void RemoveEntriesBetween(const base::Time remove_begin,
-                            const base::Time remove_end);
+  HistoryService* history_;
 
-  // Returns a new unique database handle which will not collide with real ones.
-  int64 GetNextFakeDbHandle();
+  // |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_;
 
- private:
-  typedef std::map<HistoryService::Handle, VisitedBeforeDoneCallback>
-      VisitedBeforeRequestsMap;
+  // |db_handles| of items that are scheduled for removal from history, to
+  // facilitate batching removals together for database efficiency.
+  HandleSet removing_;
 
-  void OnGotVisitCountToHost(HistoryService::Handle handle,
-                             bool found_visits,
-                             int count,
-                             base::Time first_visit);
+  // |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_;
 
-  Profile* profile_;
+  // Count the number of items in the history for UMA.
+  int64 history_size_;
 
-  // 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_;
+  ObserverList<Observer> observers_;
 
   CancelableRequestConsumer history_consumer_;
 
-  // The outstanding requests made by CheckVisitedReferrerBefore().
-  VisitedBeforeRequestsMap visited_before_requests_;
+  base::WeakPtrFactory<DownloadHistory> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadHistory);
 };