Make DownloadHistory observe manager, items

chrome/browser/download/download_history.cc

Index: chrome/browser/download/download_history.cc
diff --git a/chrome/browser/download/download_history.cc b/chrome/browser/download/download_history.cc
index d98945f259a03181206a35f977cb271234386688..0252ffc50a9fa0311dc0b0d7736b2214da182518 100644
--- a/chrome/browser/download/download_history.cc
+++ b/chrome/browser/download/download_history.cc
@@ -4,152 +4,323 @@
 
 #include "chrome/browser/download/download_history.h"
 
-#include "base/logging.h"
-#include "chrome/browser/download/download_crx_util.h"
-#include "chrome/browser/history/history_marshaling.h"
-#include "chrome/browser/history/history_service_factory.h"
-#include "chrome/browser/profiles/profile.h"
+#include "base/metrics/histogram.h"
+#include "chrome/browser/cancelable_request.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item.h"
+#include "content/public/browser/download_manager.h"
 #include "content/public/browser/download_persistent_store_info.h"
 
+using content::BrowserThread;
 using content::DownloadItem;
+using content::DownloadManager;
 using content::DownloadPersistentStoreInfo;
 
-DownloadHistory::DownloadHistory(Profile* profile)
-    : profile_(profile),
-      next_fake_db_handle_(DownloadItem::kUninitializedHandle - 1) {
-  DCHECK(profile);
+namespace {
+
+class DownloadHistoryData : public base::SupportsUserData::Data {
+ public:
+  static const int64 kUninitializedHandle = -1;
+
+  static DownloadHistoryData* Get(DownloadItem* item) {
+    base::SupportsUserData::Data* data = item->GetUserData(kKey);
+    return (data == NULL) ? NULL :
+      static_cast<DownloadHistoryData*>(data);
+  }
+
+  explicit DownloadHistoryData(DownloadItem* item)
+    : is_persisted_(false),
+      db_handle_(kUninitializedHandle) {
+    item->SetUserData(kKey, this);
+  }
+
+  virtual ~DownloadHistoryData() {
+  }
+
+  bool is_persisted() const { return is_persisted_; }
+  void set_is_persisted(bool p) { is_persisted_ = p; }
+
+  // TODO(benjhayden) Merge db_handle with DownloadItem::GetId(), then this
+  // class can turn into a simple bool.
+  int64 db_handle() const { return db_handle_; }
+  void set_db_handle(int64 h) { db_handle_ = h; }
+
+ private:
+  static const char kKey[];
+
+  bool is_persisted_;
+  int64 db_handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadHistoryData);
+};
+
+const char DownloadHistoryData::kKey[] =
+  "DownloadItem DownloadHistoryData";
+
+DownloadPersistentStoreInfo GetPersistentStoreInfo(DownloadItem* item) {
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  int64 db_handle = ((data != NULL) ?
+                     data->db_handle() :
+                     DownloadHistoryData::kUninitializedHandle);
+  return DownloadPersistentStoreInfo(
+      item->GetFullPath(),
+      item->GetURL(),
+      item->GetReferrerUrl(),
+      item->GetStartTime(),
+      item->GetEndTime(),
+      item->GetReceivedBytes(),
+      item->GetTotalBytes(),
+      item->GetState(),
+      db_handle,
+      item->GetOpened());
 }
 
-DownloadHistory::~DownloadHistory() {}
+}  // anonymous namespace
 
-void DownloadHistory::GetNextId(
-    const HistoryService::DownloadNextIdCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
+DownloadHistory::DownloadHistory(
+    DownloadManager* manager,
+    HistoryServiceDownloadInterface* history)
+  : manager_(manager),
+    history_(history),
+    history_size_(0) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(manager_);
+  DCHECK(history_);
+  manager_->AddObserver(this);
+}
 
-  hs->GetNextDownloadId(&history_consumer_, callback);
+DownloadHistory::~DownloadHistory() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  history_->OnDownloadHistoryDestroyed();
+  if (manager_)
+    manager_->RemoveObserver(this);
 }
 
 void DownloadHistory::Load(
     const HistoryService::DownloadQueryCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
-
-  hs->QueryDownloads(&history_consumer_, callback);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  history_->QueryDownloads(base::Bind(
+      &DownloadHistory::LoadCallback, AsWeakPtr(), callback));
+}
 
-  // This is the initial load, so do a cleanup of corrupt in-progress entries.
-  hs->CleanUpInProgressEntries();
+void DownloadHistory::LoadCallback(
+    const HistoryService::DownloadQueryCallback& callback,
+    DownloadHistory::InfoVector* infos) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  loaded_infos_.clear();
+  loaded_infos_.resize(infos->size());
+  std::copy(infos->begin(), infos->end(), loaded_infos_.begin());
+  callback.Run(infos);
 }
 
 void DownloadHistory::CheckVisitedReferrerBefore(
     int32 download_id,
     const GURL& referrer_url,
     const VisitedBeforeDoneCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (referrer_url.is_valid()) {
-    HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-        profile_, Profile::EXPLICIT_ACCESS);
-    if (hs) {
-      HistoryService::Handle handle =
-          hs->GetVisibleVisitCountToHost(referrer_url, &history_consumer_,
-              base::Bind(&DownloadHistory::OnGotVisitCountToHost,
-                         base::Unretained(this)));
-      visited_before_requests_[handle] = callback;
-      return;
-    }
+    HistoryService::Handle handle = history_->GetVisibleVisitCountToHost(
+        referrer_url,
+        base::Bind(&DownloadHistory::OnGotVisitCountToHost,
+                   AsWeakPtr()));
+    visited_before_requests_[handle] = callback;
+    return;
   }
   callback.Run(false);
 }
 
-void DownloadHistory::AddEntry(
-    DownloadItem* download_item,
-    const HistoryService::DownloadCreateCallback& callback) {
-  DCHECK(download_item);
-  // Do not store the download in the history database for a few special cases:
-  // - incognito mode (that is the point of this mode)
-  // - extensions (users don't think of extension installation as 'downloading')
-  // - temporary download, like in drag-and-drop
-  // - history service is not available (e.g. in tests)
-  // We have to make sure that these handles don't collide with normal db
-  // handles, so we use a negative value. Eventually, they could overlap, but
-  // you'd have to do enough downloading that your ISP would likely stab you in
-  // the neck first. YMMV.
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (download_item->IsOtr() ||
-      download_crx_util::IsExtensionDownload(*download_item) ||
-      download_item->IsTemporary() || !hs) {
-    callback.Run(download_item->GetId(), GetNextFakeDbHandle());
-    return;
+void DownloadHistory::OnGotVisitCountToHost(
+    HistoryService::Handle handle,
+    bool found_visits,
+    int count,
+    base::Time first_visit) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VisitedBeforeRequestsMap::iterator request =
+      visited_before_requests_.find(handle);
+  DCHECK(request != visited_before_requests_.end());
+  VisitedBeforeDoneCallback callback = request->second;
+  visited_before_requests_.erase(request);
+  callback.Run(found_visits && count &&
+      (first_visit.LocalMidnight() < base::Time::Now().LocalMidnight()));
+}
+
+void DownloadHistory::OnDownloadCreated(
+    DownloadManager* manager, DownloadItem* item) {
+  DCHECK(manager);
+  DCHECK(manager_);
+  DCHECK_EQ(manager_, manager);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // Observe even temporary downloads in case they are marked not temporary.
+  item->AddObserver(this);
+  DownloadPersistentStoreInfo info = GetPersistentStoreInfo(item);
+  infos_[item->GetId()] = info;
+  // All downloads should pass through OnDownloadCreated exactly once.
+  DCHECK(!DownloadHistoryData::Get(item));
+  DownloadHistoryData* data = new DownloadHistoryData(item);
+
+  // Try to match up |item| with |loaded_infos_| by all fields except
+  // |db_handle|, because DownloadItem doesn't know its |db_handle|.

[Randy Smith (Not in Mondays), 2012/08/02 22:47:56]

Refactor load path into DM so that the DH creates each DI one at a time, and the
DM returns the DI, at which point the db handle can be added directly; no need
for matching.

[benjhayden, 2012/08/14 21:31:47]

Done.

+  // Theoretically, DownloadManager will run through infos in order, so this
+  // loop should either match the first info or else loaded_infos_ should be
+  // empty, but we shouldn't rely on that. This matching mechanism relies on
+  // DownloadItem reflecting its info accurately, without "fixing" any fields
+  // except |state|, which is changed from IN_PROGRESS to CANCELLED.
+  for (InfoVector::iterator it = loaded_infos_.begin();
+       it != loaded_infos_.end(); ++it) {
+    LOG(INFO) << "occam " << __FUNCTION__
+              << " " << data->is_persisted() << " " << data->db_handle()
+              << " " << info.db_handle << " " << it->db_handle
+              << " " << info.path.value() << " " << it->path.value()
+              << " " << info.url.spec() << " " << it->url.spec()
+              << " " << info.referrer_url.spec() << " " << it->referrer_url.spec()
+              << " " << info.start_time.ToTimeT() << " " << it->start_time.ToTimeT()
+              << " " << info.end_time.ToTimeT() << " " << it->end_time.ToTimeT()
+              << " " << info.received_bytes << " " << it->received_bytes
+              << " " << info.total_bytes << " " << it->total_bytes
+              << " " << info.state << " " << it->state
+              << " " << info.opened << " " << it->opened;
+    if ((info.path == it->path) &&
+        (info.url == it->url) &&
+        (info.referrer_url == it->referrer_url) &&
+        (info.start_time == it->start_time) &&
+        (info.end_time == it->end_time) &&
+        (info.received_bytes == it->received_bytes) &&
+        (info.total_bytes == it->total_bytes) &&
+        ((it->state == DownloadItem::IN_PROGRESS) ||
+         (info.state == it->state)) &&
+        (info.opened == it->opened)) {
+      loaded_infos_.erase(it);
+      data->set_is_persisted(true);
+      data->set_db_handle(it->db_handle);
+      break;
+    }
   }
 
-  int32 id = download_item->GetId();
-  DownloadPersistentStoreInfo history_info =
-      download_item->GetPersistentStoreInfo();
-  hs->CreateDownload(id, history_info, &history_consumer_, callback);
+  MaybeAddToHistory(item);
+}
+
+void DownloadHistory::ManagerGoingDown(DownloadManager* manager) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(manager_, manager);
+  manager_->RemoveObserver(this);
+  manager_ = NULL;
+}
+
+void DownloadHistory::MaybeAddToHistory(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  int32 download_id = item->GetId();
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  bool removing = (removing_.find(data->db_handle()) != removing_.end());
+  bool already_adding = (adding_.find(download_id) != adding_.end());
+  LOG(INFO) << "occam " << __FUNCTION__ << " " << data->is_persisted() << " " << item->IsTemporary() << " " << removing << " " << already_adding;
+  if (!data->is_persisted() &&
+      !item->IsTemporary() &&
+      !removing &&
+      !already_adding) {
+    adding_.insert(download_id);
+    history_->CreateDownload(
+        download_id,
+        infos_[download_id],
+        base::Bind(&DownloadHistory::ItemAdded, AsWeakPtr()));
+  }
 }
 
-void DownloadHistory::UpdateEntry(DownloadItem* download_item) {
-  // Don't store info in the database if the download was initiated while in
-  // incognito mode or if it hasn't been initialized in our database table.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
+void DownloadHistory::ItemAdded(int32 download_id, int64 db_handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  adding_.erase(download_id);
+
+  if (!manager_)
     return;
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
+  DownloadItem* item = manager_->GetDownload(download_id);
+  LOG(INFO) << "occam " << __FUNCTION__ << " " << item << " " << db_handle;
+  if (!item) {
+    // We should have caught OnDownloadDestroyed().  If the item should have
+    // been removed from history, then OnDownloadRemoved() would have been
+    // called, so don't manually call it here.
+    DCHECK(infos_.find(download_id) == infos_.end());
     return;
-  hs->UpdateDownload(download_item->GetPersistentStoreInfo());
+  }
+
+  UMA_HISTOGRAM_CUSTOM_COUNTS("Download.HistorySize2",
+                              history_size_,
+                              0/*min*/,
+                              (1 << 23)/*max*/,
+                              (1 << 7)/*num_buckets*/);
+  ++history_size_;
+
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  data->set_is_persisted(true);
+  data->set_db_handle(db_handle);

[Randy Smith (Not in Mondays), 2012/08/02 22:47:56]

Maybe also store the last DownloadPersistentStateInfo in external data as well?

[benjhayden, 2012/08/14 21:31:47]

Done.

+
+  // In case the item changed or became temporary while it was being added.
+  // Don't just UpdateObservers() because we're the only observer that can also
+  // see is_persisted/db_handle, which is the only thing that we changed.
+  OnDownloadUpdated(item);
 }
 
-void DownloadHistory::UpdateDownloadPath(DownloadItem* download_item,
-                                         const FilePath& new_path) {
-  // No update necessary if the download was initiated while in incognito mode.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
+void DownloadHistory::OnDownloadUpdated(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!manager_)
     return;
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->UpdateDownloadPath(new_path, download_item->GetDbHandle());
-}
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  if ((data == NULL) ||
+      !data->is_persisted()) {
+    MaybeAddToHistory(item);
+    return;
+  }
 
-void DownloadHistory::RemoveEntry(DownloadItem* download_item) {
-  // No update necessary if the download was initiated while in incognito mode.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
+  if (item->IsTemporary()) {
+    OnDownloadRemoved(item);
     return;
+  }
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->RemoveDownload(download_item->GetDbHandle());
+  // TODO(asanka): Persist GetTargetFilePath() as well.
+  DownloadPersistentStoreInfo current_info = GetPersistentStoreInfo(item);
+  InfoMap::const_iterator previous_info = infos_.find(item->GetId());
+  if ((previous_info == infos_.end()) ||
+      (previous_info->second.end_time != current_info.end_time) ||
+      (previous_info->second.received_bytes != current_info.received_bytes) ||
+      (previous_info->second.state != current_info.state) ||
+      (previous_info->second.opened != current_info.opened)) {
+    history_->UpdateDownload(current_info);
+  }
+  infos_[item->GetId()] = current_info;
 }
 
-void DownloadHistory::RemoveEntriesBetween(const base::Time remove_begin,
-                                           const base::Time remove_end) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->RemoveDownloadsBetween(remove_begin, remove_end);
+void DownloadHistory::OnDownloadRemoved(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  if ((data == NULL) ||
+      !data->is_persisted() ||
+      (data->db_handle() <= DownloadHistoryData::kUninitializedHandle))
+    return;
+
+  // For database efficiency, batch removals together if they happen all at
+  // once.
+  if (removing_.empty()) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadHistory::RemoveDownloadsBatch, AsWeakPtr()));
+  }
+  removing_.insert(data->db_handle());
+  data->set_db_handle(DownloadHistoryData::kUninitializedHandle);
+  data->set_is_persisted(false);
 }
 
-int64 DownloadHistory::GetNextFakeDbHandle() {
-  return next_fake_db_handle_--;
+void DownloadHistory::RemoveDownloadsBatch() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  HandleSet remove_handles;
+  removing_.swap(remove_handles);
+  history_->RemoveDownloads(remove_handles);
+  --history_size_;
 }
 
-void DownloadHistory::OnGotVisitCountToHost(HistoryService::Handle handle,
-                                            bool found_visits,
-                                            int count,
-                                            base::Time first_visit) {
-  VisitedBeforeRequestsMap::iterator request =
-      visited_before_requests_.find(handle);
-  DCHECK(request != visited_before_requests_.end());
-  VisitedBeforeDoneCallback callback = request->second;
-  visited_before_requests_.erase(request);
-  callback.Run(found_visits && count &&
-      (first_visit.LocalMidnight() < base::Time::Now().LocalMidnight()));
+void DownloadHistory::OnDownloadDestroyed(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  infos_.erase(item->GetId());
+  item->RemoveObserver(this);
 }