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 b4aa58279c12f1edceca6d6030693599df8c27eb..795d6e933e2dc9238c0cd64cfbccae3663423686 100644
--- a/chrome/browser/download/download_history.cc
+++ b/chrome/browser/download/download_history.cc
@@ -4,146 +4,397 @@
 

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Explanatory comment at top of file about basic architecture?  Interaction with
DM/DI via observers & UserData, interaction with DB thread via ?, any other high
level information that would help someone reading through this file?  

[benjhayden, 2012/08/17 18:20:45]

I had hoped that the code could speak for itself, but I wrote something that
you're welcome to rewrite or ask me to remove.

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

L G T M; thanks.

 #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() {}
+class DownloadHistoryData : public base::SupportsUserData::Data {

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Comment documenting purpose and use?

[benjhayden, 2012/08/17 18:20:45]

Done.

+ public:
+  static const int64 kUninitializedHandle = -1;

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Why here?  I would think this would be a DownloadHistory of DownloadDatabase
concept?

[benjhayden, 2012/08/17 18:20:45]

Moved it out into the anonymous namespace.

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

I think I was just confused in my earlier comment.  If no-one outside this file
every uses -1 for a db handle to indicate an uninitialized handle, and if the
mechanism for allocating db handles is in this file, it's appropriate for this
to be here (I don't have any strong feeling about current placement versus
inside the helper class; either's fine).  But if other files are structuring the
download handle namespace, I'd think this would belong in those other locations.
 I'll assume for now that all db_handle namespace manipulations are being done
in this file now (so please ignore my original comment and you can put the
constant wherever you'd like) but will mark up other files appropriately if I
see counter-examples.

 
-void DownloadHistory::GetNextId(
-    const HistoryService::DownloadNextIdCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
+  static DownloadHistoryData* Get(DownloadItem* item) {
+    base::SupportsUserData::Data* data = item->GetUserData(kKey);
+    return (data == NULL) ? NULL :
+      static_cast<DownloadHistoryData*>(data);
+  }
 
-  hs->GetNextDownloadId(&history_consumer_, callback);
+  explicit DownloadHistoryData(DownloadItem* item)
+    : is_adding_(false),
+      is_persisted_(false),
+      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; }
+
+  // Whether this item is already persisted in the database.
+  bool is_persisted() const { return is_persisted_; }
+  void set_is_persisted(bool p) { is_persisted_ = p; }
+
+  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_persisted_;
+  int64 db_handle_;
+  scoped_ptr<DownloadPersistentStoreInfo> info_;
+
+  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());
 }
 
-void DownloadHistory::Load(
+}  // anonymous namespace
+
+HistoryServiceDownloadAdapter::HistoryServiceDownloadAdapter(
+    HistoryService* history_service)
+  : history_service_(history_service) {
+}
+
+HistoryServiceDownloadAdapter::~HistoryServiceDownloadAdapter() {}
+
+void HistoryServiceDownloadAdapter::QueryDownloads(
     const HistoryService::DownloadQueryCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      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);
+}
+
+void HistoryServiceDownloadAdapter::RemoveDownloads(
+    const std::set<int64>& handles) {
+  history_service_->RemoveDownloads(handles);
+}
+
+void HistoryServiceDownloadAdapter::OnDownloadHistoryDestroyed() {
+  delete this;
+}
+
+DownloadHistory::DownloadHistory(
+    DownloadManager* manager,
+    HistoryServiceDownloadAdapter* history)
+  : manager_(manager),
+    history_(history),
+    loading_(false),
+    history_size_(0) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(manager_);
+  manager_->AddObserver(this);
+  history_->QueryDownloads(base::Bind(
+      &DownloadHistory::QueryCallback, AsWeakPtr()));
+}
+
+DownloadHistory::~DownloadHistory() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  history_->OnDownloadHistoryDestroyed();
+  if (manager_)
+    manager_->RemoveObserver(this);

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

DCHECK that the manager has no items on it?

[benjhayden, 2012/08/17 18:20:45]

Did the observing_items_ set trick instead.
I'll start writing AddObserver(WeakPtr<Observer>) by the time you read this.

+}
+
+void DownloadHistory::QueryCallback(
+    DownloadHistory::InfoVector* infos) {

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Who owns the storage in the vector pointed to by Infos?  Who releases it?

[benjhayden, 2012/08/17 18:20:45]

HistoryService::QueryDownloads() creates a history::DownloadQueryRequest(),
which contains this vector, and passes it to HistoryBackend::QueryDownloads(),
which stuffs it in a scoped_refptr. There's some callback magic in
DownloadQueryRequest's base class, but that's pretty much the whole story.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // We may have caught ManagerGoingDown() before the history loaded.
+  if (manager_ == NULL)
     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* data = DownloadHistoryData::Get(download_item);
+    data->set_is_persisted(true);
+    data->set_db_handle(it->db_handle);
+    ++history_size_;
+  }
+  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);
+  // All downloads should pass through OnDownloadCreated exactly once.
+  CHECK(!DownloadHistoryData::Get(item));
+  DownloadHistoryData* data = new DownloadHistoryData(item);
+  if (item->GetState() == DownloadItem::IN_PROGRESS) {
+    data->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* data = DownloadHistoryData::Get(item);
+  bool removing = (removing_.find(data->db_handle()) != removing_.end());

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Should this be a DCHECK?

[benjhayden, 2012/08/17 18:20:45]

I could imagine DownloadItem sending one or two final OnDownloadUpdated calls
between OnDownloadRemoved and OnDownloadDestroyed at some point. It seems like
an implementation detail that that currently can't happen.

+  if (!loading_ &&
+      !download_crx_util::IsExtensionDownload(*item) &&
+      !data->is_persisted() &&
+      !item->IsTemporary() &&
+      !removing &&
+      !data->is_adding()) {
+    data->set_is_adding(true);
+    if (data->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.
+      data->set_info(GetPersistentStoreInfo(item));
     }
+    history_->CreateDownload(
+        download_id,
+        *data->info(),
+        base::Bind(&DownloadHistory::ItemAdded, AsWeakPtr()));
   }
-  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;
+
+  DownloadItem* item = manager_->GetDownload(download_id);
+  if (!item) {
+    // We should have caught OnDownloadDestroyed().  If the item should have

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Does this mean we could have a DCHECK() here?

[benjhayden, 2012/08/17 18:20:45]

Nope. There's no mechanism that cancels this particular ItemAdded() callback
just because OnDownloadDestroyed() is called.
Thanks for making me take another look at this code. The removed_while_adding_
stuff did not depend on !item, so I moved it up a couple lines.

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

Hmmm.  Then I think the comment's a little confusing; when I read "We should
have caught OnDownloadDestroyed()" I read that as "but we didn't".  Is what
you're trying to say "The item was destroyed before the callback from the DB
signaling successful addition arrived.  We should leave it in the DB unless it
was also removed, in which case it will be in removed_while_adding_."?  Do you
consider that clearer?  If not, I'm happy with something indicating that our
catching OnDownloadDestroyed() is what really happened.

, though I'm having a hard enough time writing a less confusing comment that
maybe I should leave well enough alone.  

+    // been removed from history, then OnDownloadRemoved() put |download_id| in
+    // removed_while_adding_.
+    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, AsWeakPtr()));
+      }
+      removing_.insert(db_handle);
+    }
     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* data = DownloadHistoryData::Get(item);
+  data->set_is_adding(false);
+  data->set_is_persisted(true);
+  data->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 is_persisted/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* data = DownloadHistoryData::Get(item);
+  if (!data->is_persisted()) {
+    MaybeAddToHistory(item);
+    return;
+  }
+  if (item->IsTemporary()) {
+    OnDownloadRemoved(item);
     return;
-  hs->UpdateDownload(download_item->GetPersistentStoreInfo());
+  }
+
+  // TODO(asanka): Persist GetTargetFilePath() as well.
+  DownloadPersistentStoreInfo current_info(GetPersistentStoreInfo(item));
+  DownloadPersistentStoreInfo* previous_info = data->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) {
+    data->set_info(current_info);
+  } else {
+    data->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* data = DownloadHistoryData::Get(item);
+  if (!data->is_persisted()) {
+    MaybeAddToHistory(item);
     return;
+  }
+  if (item->IsTemporary()) {
+    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) {

[Randy Smith (Not in Mondays), 2012/08/15 20:47:44]

Can they be opened before they're completed?  I.e. is this test necessary?

[benjhayden, 2012/08/17 18:20:45]

Wasn't there some chatter about streaming downloads to applications just the
other day?
Either way, feels like an implementation detail.

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

Fair enough.  We're well short of streaming data to applications, though.

+    data->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* data = DownloadHistoryData::Get(item);
+  if ((data == NULL) ||
+      !data->is_persisted() ||
+      (data->db_handle() <= DownloadHistoryData::kUninitializedHandle)) {
+    if (data->is_adding()) {
+      removed_while_adding_.insert(item->GetId());
+    }
     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);
+  --history_size_;
+}
+
+void DownloadHistory::RemoveDownloadsBatch() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  HandleSet remove_handles;
+  removing_.swap(remove_handles);
+  history_->RemoveDownloads(remove_handles);
+}
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->RemoveDownload(download_item->GetDbHandle());
+void DownloadHistory::ManagerGoingDown(DownloadManager* manager) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(manager_, manager);
+  manager_->RemoveObserver(this);
+  manager_ = NULL;
 }
 
-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::OnDownloadDestroyed(DownloadItem* item) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  item->RemoveObserver(this);
 }
 
-int64 DownloadHistory::GetNextFakeDbHandle() {
-  return next_fake_db_handle_--;
+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,
+                   AsWeakPtr()));
+    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());