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 9d701950dcb5375eba799c55229a3e71a0588c42..959fe98c0c64675fb6fb6ed19433df48a0e832fd 100644
--- a/chrome/browser/download/download_history.cc
+++ b/chrome/browser/download/download_history.cc
@@ -2,148 +2,463 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// DownloadHistory observes a single DownloadManager and all its DownloadItems.

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

Very good; thanks.  Minor tweak: Can you start with "DownloadHistory manages
persisting DownloadItems to the history service by observing ...".  I think of
starting with the highest level view and drilling down being the best form of
documentation (gives people context) and the purpose of the class is the highest
level view.

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

Done.

+// DownloadHistory decides whether and when to add items to, remove items from,
+// and update items in the database. DownloadHistory uses DownloadHistoryData to
+// store per-DownloadItem data such as its db_handle, whether the item is being
+// added and waiting for its db_handle, and the last DownloadPersistentStoreInfo
+// that was passed to the database.  When the DownloadManager and its delegate
+// (ChromeDownloadManagerDelegate) are initialized, DownloadHistory is created
+// and queries the HistoryService. When the HistoryService calls back from
+// QueryDownloads() to QueryCallback(), DownloadHistory uses
+// DownloadManager::CreateDownloadItem() to inform DownloadManager of these
+// persisted DownloadItems. CreateDownloadItem() internally calls
+// OnDownloadCreated(), which normally adds items to the database, so
+// QueryCallback() uses |loading_| to disable adding items to the database.
+// If a download is removed via OnDownloadRemoved() while the item is still
+// being added to the database, DownloadHistory uses |removed_while_adding_| to
+// remember to remove the item when its ItemAdded() callback is called.

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

Destruction context so that we can reason about relative lifetimes?

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

Done.

+// All callbacks are bound with a weak pointer to DownloadHistory to prevent
+// use-after-free bugs, even though CancelableRequestConsumer should guarantee
+// the same thing.

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

This probably should be done in another CL, but if we've got another mechanism
than CancelableRequestConsumer to guarantee lifetime issues, can we eliminate
CancelableRequestConsumer?  I think you and I are both more comfortable with the
weak pointer guarantees.

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

It required changing our HistoryService/HistoryBackend methods, so I don't know
how the owner of those classes will feel, but I like it.

[Randy Smith (Not in Mondays), 2012/08/27 19:28:19]

I didn't follow what you meant here when I first read it, but now I do, and I
wince a bit.  We're effectively driving a separate functionality stake through
HistoryService in order not to use CancelableRequestConsumer.  But it would be
nice to avoid using CancelableRequestConsumer.  So yes, see what the owner says.
 I'd recommend Brett--I think he wrote that code and knows it best.

+
 #include "chrome/browser/download/download_history.h"
 
-#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "chrome/browser/cancelable_request.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 "chrome/browser/download/download_persistent_store_info.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item.h"
-#include "content/public/browser/download_persistent_store_info.h"
+#include "content/public/browser/download_manager.h"
 
+using content::BrowserThread;
 using content::DownloadItem;
-using content::DownloadPersistentStoreInfo;
+using content::DownloadManager;
 
-DownloadHistory::DownloadHistory(Profile* profile)
-    : profile_(profile),
-      next_fake_db_handle_(DownloadItem::kUninitializedHandle - 1) {
-  DCHECK(profile);
-}
+namespace {
 
-DownloadHistory::~DownloadHistory() {}
+// The value of |db_handle| indicating that the associated DownloadItem is not
+// yet persisted.
+static const int64 kUninitializedHandle = -1;
 
-void DownloadHistory::GetNextId(
-    const HistoryService::DownloadNextIdCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
+// Per-DownloadItem data. This information does not belong inside DownloadItem,
+// and keeping maps in DownloadHistory from DownloadItem to this information is
+// error-prone and complicated. Unfortunately, DownloadHistory::removing_ and
+// removed_while_adding_ cannot be moved into this class partly because
+// DownloadHistoryData is destroyed when DownloadItems are destroyed, and we
+// have no control over when DownloadItems are destroyed.
+class DownloadHistoryData : public base::SupportsUserData::Data {
+ public:
+
+  static DownloadHistoryData* Get(DownloadItem* item) {
+    base::SupportsUserData::Data* data = item->GetUserData(kKey);
+    return (data == NULL) ? NULL :
+      static_cast<DownloadHistoryData*>(data);
+  }
+
+  DownloadHistoryData(
+      DownloadItem* item,
+      const base::WeakPtr<DownloadHistory>& history)
+    : is_adding_(false),
+      is_disabled_(false),
+      history_(history),
+      db_handle_(kUninitializedHandle),
+      info_(NULL) {
+    item->SetUserData(kKey, this);
+  }
+
+  virtual ~DownloadHistoryData() {
+  }
+
+  // Whether this item is currently being added to the database.
+  bool is_adding() const { return is_adding_; }
+  void set_is_adding(bool a) { is_adding_ = a; }
+
+  bool is_disabled() const { return is_disabled_; }
+  void set_is_disabled(bool d, DownloadItem* item) {
+    if (is_disabled_ == d)
+      return;
+    is_disabled_ = d;
+    // May either add item to db or remove it.
+    // Could also call item->NotifyObservers(), but that interface should not be
+    // public, and DH is the only observer that knows and cares about
+    // is_disabled().
+    if (history_.get())

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

Can you think of any contexts in which a DownloadItem could outlive the
DownloadHistory?  The DownloadHistory is tied to the lifetime of the manager,
and by the time the manager is destroyed, all of the DownloadItems on it should
already have been destroyed.

I'm happy with this being a DCHECK, but relative lifetimes strike me as part of
the knowledge this code should have (i.e. not an implementation detail of other
pieces of code) and if those lifetimes say that all the DIs should be destroyed
before the DH is destroyed, let's DCHECK that.

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

This was part of DownloadHistory::Disable, which was speculative and so removed.

+      history_->OnDownloadUpdated(item);
+  }
+
+  // Whether this item is already persisted in the database.
+  bool is_persisted() const { return db_handle_ > kUninitializedHandle; }
+
+  int64 db_handle() const { return db_handle_; }
+  void set_db_handle(int64 h) { db_handle_ = h; }
+
+  // This allows OnDownloadUpdated() to see what changed in a DownloadItem if
+  // anything, in order to prevent writing to the database unnecessarily.  It is
+  // nullified when the item is no longer in progress in order to save memory.
+  DownloadPersistentStoreInfo* info() { return info_.get(); }
+  void set_info(const DownloadPersistentStoreInfo& i) {
+    info_.reset(new DownloadPersistentStoreInfo(i));
+  }
+  void clear_info() {
+    info_.reset();
+  }
+
+ private:
+  static const char kKey[];
+
+  bool is_adding_;
+  bool is_disabled_;
+  base::WeakPtr<DownloadHistory> history_;
+  int64 db_handle_;
+  scoped_ptr<DownloadPersistentStoreInfo> info_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadHistoryData);
+};
+
+const char DownloadHistoryData::kKey[] =
+  "DownloadItem DownloadHistoryData";
+
+DownloadPersistentStoreInfo GetPersistentStoreInfo(DownloadItem* item) {
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  return DownloadPersistentStoreInfo(
+      item->GetFullPath(),
+      item->GetURL(),
+      item->GetReferrerUrl(),
+      item->GetStartTime(),
+      item->GetEndTime(),
+      item->GetReceivedBytes(),
+      item->GetTotalBytes(),
+      item->GetState(),
+      ((dhd != NULL) ? dhd->db_handle() : kUninitializedHandle),
+      item->GetOpened());
+}
 
-  hs->GetNextDownloadId(&history_consumer_, callback);
+}  // anonymous namespace
+
+HistoryServiceDownloadAdapter::HistoryServiceDownloadAdapter(
+    HistoryService* history_service)
+  : history_service_(history_service) {
 }
 
-void DownloadHistory::Load(
+HistoryServiceDownloadAdapter::~HistoryServiceDownloadAdapter() {}
+
+void HistoryServiceDownloadAdapter::QueryDownloads(
     const HistoryService::DownloadQueryCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
+  history_service_->QueryDownloads(&history_consumer_, callback);
+  history_service_->CleanUpInProgressEntries();
+}
+
+HistoryService::Handle
+HistoryServiceDownloadAdapter::GetVisibleVisitCountToHost(
+    const GURL& referrer_url,
+    const HistoryService::GetVisibleVisitCountToHostCallback&
+      callback) {
+  return history_service_->GetVisibleVisitCountToHost(
+      referrer_url, &history_consumer_, callback);
+}
+
+void HistoryServiceDownloadAdapter::CreateDownload(
+    int32 id,
+    const DownloadPersistentStoreInfo& info,
+    const HistoryService::DownloadCreateCallback& callback) {
+  history_service_->CreateDownload(id, info, &history_consumer_, callback);
+}
+
+void HistoryServiceDownloadAdapter::UpdateDownload(
+    const DownloadPersistentStoreInfo& info) {
+  history_service_->UpdateDownload(info);

[Randy Smith (Not in Mondays), 2012/08/27 19:28:19]

So the fact that this does not take a db handle indicates that we duplicate the
db handle between DownloadHistoryData and DownloadPersistentStateInfo.  My
initial reaction was that we could remove it from DHD, but that obviously won't
work because we don't always have DPSI.  But can we remove it from DPSI?  That'd
require a bit of re-plumbing, but the db handle is arguably a control parameter
rather than actually part of the actual persistent storage info.

I don't feel strongly about this, I just think it would be cleaner without the
duplicate info.  If you don't dedup it, could you put a comment next to
db_handle in DHD to explain why it's duplicated?

+}
+
+void HistoryServiceDownloadAdapter::RemoveDownloads(
+    const std::set<int64>& handles) {
+  history_service_->RemoveDownloads(handles);
+}
+
+void HistoryServiceDownloadAdapter::OnDownloadHistoryDestroyed() {
+  delete this;
+}
+
+// static
+void DownloadHistory::Disable(content::DownloadItem* download_item) {
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(download_item);
+  // If the item has been created, then it has a DHD.
+  dhd->set_is_disabled(true, download_item);
+}
+
+DownloadHistory::DownloadHistory(
+    DownloadManager* manager,
+    HistoryServiceDownloadAdapter* history)
+  : manager_(manager),
+    history_(history),
+    loading_(false),
+    history_size_(0),
+    ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(manager_);
+  manager_->AddObserver(this);
+  history_->QueryDownloads(base::Bind(
+      &DownloadHistory::QueryCallback, weak_ptr_factory_.GetWeakPtr()));
+}
+
+DownloadHistory::~DownloadHistory() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  history_->OnDownloadHistoryDestroyed();

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

Why isn't this just a scoped_ptr<>?

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

Done.

+  for (ItemSet::const_iterator iter = observing_items_.begin();

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

Shouldn't this list always be null at this point, because we've already torn
down the DownloadManager?  What's the destruction order?

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

Yes, DownloadManager::Shutdown() deletes the items before calling
delegate->Shutdown(), which is ChromeDownloadManagerDelegate::Shutdown(), which
deletes the DownloadHistory, but this seems like an implementation detail.
Also, this code will evaporate when HyperbolicDownloadItemObserver (or
WeakObserverList, whichever y'all like) is committed.

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

Hmmm.  Ok.  What I hear you saying here is that DownloadHistory implemented as
an observer should be independent of the lifetime of the class it observers.  If
that's a consistent design principle in this code, I'm good with it.  I'm
tempted to ask you to test it, but that's probably overkill :-}.

+       iter != observing_items_.end(); ++iter) {
+    (*iter)->RemoveObserver(this);
+  }
+  observing_items_.clear();
+  if (manager_)
+    manager_->RemoveObserver(this);
+}
+
+void DownloadHistory::QueryCallback(
+    DownloadHistory::InfoVector* infos) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // We may have caught ManagerGoingDown() before the history loaded.
+  if (!manager_)
     return;
+  // OnDownloadCreated() is called inside DownloadManager::CreateDownloadItem(),
+  // before we have a chance to attach a DownloadHistoryData, so temporarily
+  // disable adding new unpersisted items to the history.  All methods run on
+  // the UI thread and CreateDownloadItem() is synchronous, so it is impossible
+  // for an OnDownloadCreated() to come in for another DownloadItem while we are
+  // processing the database.
+  loading_ = true;
+  for (InfoVector::const_iterator it = infos->begin();
+       it != infos->end(); ++it) {
+    DownloadItem* download_item = manager_->CreateDownloadItem(
+        it->path,
+        it->url,
+        it->referrer_url,
+        it->start_time,
+        it->end_time,
+        it->received_bytes,
+        it->total_bytes,
+        it->state,
+        it->opened);
+    DownloadHistoryData* dhd = DownloadHistoryData::Get(download_item);
+    DCHECK(it->db_handle > kUninitializedHandle);
+    dhd->set_db_handle(it->db_handle);
+    ++history_size_;
+  }
+  manager_->CheckForHistoryFilesRemoval();
+  loading_ = false;
+}
 
-  hs->QueryDownloads(&history_consumer_, callback);
+void DownloadHistory::OnDownloadCreated(
+    DownloadManager* manager, DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // This is the initial load, so do a cleanup of corrupt in-progress entries.
-  hs->CleanUpInProgressEntries();
+  // Observe even temporary downloads in case they are marked not temporary.
+  item->AddObserver(this);
+  observing_items_.insert(item);
+  // All downloads should pass through OnDownloadCreated exactly once.
+  CHECK(!DownloadHistoryData::Get(item));
+  DownloadHistoryData* dhd = new DownloadHistoryData(
+      item, weak_ptr_factory_.GetWeakPtr());
+  if (item->GetState() == DownloadItem::IN_PROGRESS) {
+    dhd->set_info(GetPersistentStoreInfo(item));
+  }
+  MaybeAddToHistory(item);
 }
 
-void DownloadHistory::CheckVisitedReferrerBefore(
-    int32 download_id,
-    const GURL& referrer_url,
-    const VisitedBeforeDoneCallback& callback) {
-  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;
+void DownloadHistory::MaybeAddToHistory(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  int32 download_id = item->GetId();
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  bool removing = (removing_.find(dhd->db_handle()) != removing_.end());
+  // TODO(benjhayden): Remove IsTemporary().
+  if (!loading_ &&

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

Suggestion: Reverse the sense of the tests and return?

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

Done.

+      !download_crx_util::IsExtensionDownload(*item) &&
+      !dhd->is_persisted() &&
+      !dhd->is_disabled() &&
+      !item->IsTemporary() &&
+      !removing &&
+      !dhd->is_adding()) {
+    dhd->set_is_adding(true);
+    if (dhd->info() == NULL) {
+      // Keep the info here regardless of whether the item is in progress so
+      // that, when ItemAdded() calls OnDownloadUpdated(), it can choose more
+      // intelligently whether to Update the db and/or discard the info.
+      dhd->set_info(GetPersistentStoreInfo(item));
     }
+    HistoryService::DownloadCreateCallback callback = base::Bind(
+        &DownloadHistory::ItemAdded, weak_ptr_factory_.GetWeakPtr());
+    history_->CreateDownload(download_id, *dhd->info(), callback);
   }
-  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_crx_util::IsExtensionDownload(*download_item) ||
-      download_item->IsTemporary() || !hs) {
-    callback.Run(download_item->GetId(), GetNextFakeDbHandle());
+void DownloadHistory::ItemAdded(int32 download_id, int64 db_handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (!manager_)
+    return;
+
+  if (removed_while_adding_.find(download_id) !=
+      removed_while_adding_.end()) {
+    removed_while_adding_.erase(download_id);
+    if (removing_.empty()) {
+      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+          base::Bind(&DownloadHistory::RemoveDownloadsBatch,
+                     weak_ptr_factory_.GetWeakPtr()));
+    }
+    removing_.insert(db_handle);
+    return;
+  }
+
+  DownloadItem* item = manager_->GetDownload(download_id);
+  if (!item) {
+    // We should have caught OnDownloadDestroyed().  If the item should have
+    // been removed from history, then OnDownloadRemoved() put |download_id| in
+    // removed_while_adding_.
     return;
   }
 
-  int32 id = download_item->GetId();
-  DownloadPersistentStoreInfo history_info =
-      download_item->GetPersistentStoreInfo();
-  hs->CreateDownload(id, history_info, &history_consumer_, callback);
+  UMA_HISTOGRAM_CUSTOM_COUNTS("Download.HistorySize2",
+                              history_size_,
+                              0/*min*/,
+                              (1 << 23)/*max*/,
+                              (1 << 7)/*num_buckets*/);
+  ++history_size_;
+
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  dhd->set_is_adding(false);
+  DCHECK(db_handle > kUninitializedHandle);
+  dhd->set_db_handle(db_handle);
+
+  // 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 db_handle, which is the only thing that we changed.
+  OnDownloadUpdated(item);
 }
 
-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)
-    return;
+void DownloadHistory::OnDownloadUpdated(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  if (!dhd->is_persisted()) {
+    MaybeAddToHistory(item);

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

Under what circumstances would we actually want to add a download item to the
history through OnDownloadUpdated()?  It seems like the download would always be
created first, and hence always be tossed at the history through
OnDownloadCreated().  (Same comment for OnDownloadOpened()).

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

Transition from Temporary to not Temporary. (In the future, read Temporary as
"disabled".)

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

Yuck; I had adding state transitions for transition options I don't expect to
ever happen.  But I guess it's not particularly clean at the history level to
specify this as something set at the beginning of the download item and not
changing; since it's in SupportsUserData::Data, it can't be a constructor
argument.  Ok.  

If the code's in here, you should probably have a test for it, otherwise it'll
rot.  (Double yuck--testing code that I don't expect to ever used.)

[Randy Smith (Not in Mondays), 2012/08/27 19:28:19]

Also: You have the same call to MaybeAddToHistory() in OnDownloadOpened(). 
Shouldn't we always get an OnDownloadUpdated() before an OnDownloadOpened(),
even in the ->!Temporary case?  Heck, the transition to disabled happens through
us--I hope when we get there that we can remove all of these, and just persist
it when we get the call to make it non-disabled.

+    return;
+  }
+  if (item->IsTemporary() ||
+      dhd->is_disabled()) {
+    OnDownloadRemoved(item);
     return;
-  hs->UpdateDownload(download_item->GetPersistentStoreInfo());
+  }
+
+  // TODO(asanka): Persist GetTargetFilePath() as well.
+  DownloadPersistentStoreInfo current_info(GetPersistentStoreInfo(item));
+  DownloadPersistentStoreInfo* previous_info = dhd->info();
+  bool do_update = (
+      (previous_info == NULL) ||
+      (previous_info->path != current_info.path) ||
+      (previous_info->end_time != current_info.end_time) ||
+      (previous_info->received_bytes != current_info.received_bytes) ||
+      (previous_info->total_bytes != current_info.total_bytes) ||
+      (previous_info->state != current_info.state) ||
+      (previous_info->opened != current_info.opened));
+  UMA_HISTOGRAM_ENUMERATION("Download.HistoryPropagatedUpdate", do_update, 2);
+  if (do_update) {
+    history_->UpdateDownload(current_info);
+  }
+  if (item->GetState() == DownloadItem::IN_PROGRESS) {
+    dhd->set_info(current_info);
+  } else {
+    dhd->clear_info();
+  }
 }
 
-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)
+// Downloads may be opened after they are completed.
+void DownloadHistory::OnDownloadOpened(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  if (!dhd->is_persisted()) {
+    MaybeAddToHistory(item);
+    return;
+  }
+  if (item->IsTemporary() ||
+      dhd->is_disabled()) {
+    OnDownloadRemoved(item);
     return;
+  }
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->UpdateDownloadPath(new_path, download_item->GetDbHandle());
+  DownloadPersistentStoreInfo current_info(GetPersistentStoreInfo(item));
+  history_->UpdateDownload(current_info);
+  if (item->GetState() == DownloadItem::IN_PROGRESS) {
+    dhd->set_info(current_info);
+  }
 }
 
-void DownloadHistory::RemoveEntry(DownloadItem* download_item) {
-  // No update necessary if the download was initiated while in incognito mode.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
+void DownloadHistory::OnDownloadRemoved(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  if (dhd == NULL)

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

How could this happen?

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

Done.

+    return;
+  if (!dhd->is_persisted()) {
+    if (dhd->is_adding()) {
+      removed_while_adding_.insert(item->GetId());
+    }
     return;
+  }
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->RemoveDownload(download_item->GetDbHandle());
+  // 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,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+  removing_.insert(dhd->db_handle());
+  dhd->set_db_handle(kUninitializedHandle);
+  --history_size_;
 }
 
-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::RemoveDownloadsBatch() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  HandleSet remove_handles;
+  removing_.swap(remove_handles);
+  history_->RemoveDownloads(remove_handles);
 }
 
-int64 DownloadHistory::GetNextFakeDbHandle() {
-  return next_fake_db_handle_--;
+void DownloadHistory::ManagerGoingDown(DownloadManager* manager) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(manager_, manager);
+  manager_->RemoveObserver(this);
+  manager_ = NULL;
+}
+
+void DownloadHistory::OnDownloadDestroyed(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  item->RemoveObserver(this);
+  observing_items_.erase(item);
+}
+
+void DownloadHistory::CheckVisitedReferrerBefore(
+    int32 download_id,
+    const GURL& referrer_url,
+    const VisitedBeforeDoneCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (referrer_url.is_valid()) {
+    HistoryService::Handle handle = history_->GetVisibleVisitCountToHost(
+        referrer_url,
+        base::Bind(&DownloadHistory::OnGotVisitCountToHost,
+                   weak_ptr_factory_.GetWeakPtr()));
+    visited_before_requests_[handle] = callback;
+    return;
+  }
+  callback.Run(false);
 }
 
-void DownloadHistory::OnGotVisitCountToHost(HistoryService::Handle handle,
-                                            bool found_visits,
-                                            int count,
-                                            base::Time first_visit) {
+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());