Kill DownloadManagerImpl::save_page_downloads_

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index a34d25ebcf1d3cc2e9b9b90a583a1261d063e82e..c8ed23c85521bb180d0d4e46320ae2464427888e 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -50,6 +50,28 @@ using content::WebContents;
 
 namespace {
 
+// This is just used to remember which DownloadItems come from SavePage.
+class SavePageExternalData : public DownloadItem::ExternalData {
+ public:
+  // A spoonful of syntactic sugar.
+  static SavePageExternalData* Get(DownloadItem* item) {

[Randy Smith (Not in Mondays), 2012/07/10 19:06:05]

Why not make this return bool?  That's what you're actually curious about, and
you don't ever do anything with this once you've created it.

[benjhayden, 2012/07/10 20:00:08]

Done.

+    DownloadItem::ExternalData* data = item->GetExternalData(Key());
+    return data ? static_cast<SavePageExternalData*>(data) : NULL;
+  }
+
+  explicit SavePageExternalData(DownloadItem* item) {
+    item->SetExternalData(Key(), this);
+  }
+  virtual ~SavePageExternalData() {}
+
+ private:
+  static const void* Key() {
+    return reinterpret_cast<const void*>(&SavePageExternalData::Get);

[asanka, 2012/07/10 18:43:31]

Multiple functions with identical code can be aliased in optimized builds. So if
there was, say, a FooExternalData class with a similar Get() method, then we
might end up with FooExternalData::Get() and SavePageExternalData::Get() being
at the same address.

[Randy Smith (Not in Mondays), 2012/07/10 19:06:05]

Agreed.  I also think it's useful for debugging (in the abstract--I've never
actually used it) if we maintain the pattern that the key is a pointer to a
const static char * which describes the subsystem using the key.

[benjhayden, 2012/07/10 20:00:08]

Done.

[benjhayden, 2012/07/10 20:00:08]

Done.

+  }
+
+  DISALLOW_COPY_AND_ASSIGN(SavePageExternalData);
+};
+
 void BeginDownload(content::DownloadUrlParameters* params) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // ResourceDispatcherHost{Base} is-not-a URLRequest::Delegate, and
@@ -305,8 +327,6 @@ void DownloadManagerImpl::Shutdown() {
   // We'll have nothing more to report to the observers after this point.
   observers_.Clear();
 
-  DCHECK(save_page_downloads_.empty());
-
   file_manager_ = NULL;
   if (delegate_)
     delegate_->Shutdown();
@@ -539,8 +559,9 @@ DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
 
   DCHECK(!ContainsKey(downloads_, download->GetId()));
   downloads_[download->GetId()] = download;
-  DCHECK(!ContainsKey(save_page_downloads_, download->GetId()));
-  save_page_downloads_[download->GetId()] = download;
+  DCHECK(!SavePageExternalData::Get(download));
+  new SavePageExternalData(download);
+  DCHECK(SavePageExternalData::Get(download));
 
   // Will notify the observer in the callback.
   if (delegate_)
@@ -803,7 +824,6 @@ int DownloadManagerImpl::RemoveDownloadItems(
       ++it) {
     DownloadItem* download = *it;
     DCHECK(download);
-    save_page_downloads_.erase(download->GetId());
     downloads_.erase(download->GetId());
   }
 
@@ -974,12 +994,15 @@ void DownloadManagerImpl::AddDownloadItemToHistory(DownloadItem* download,
 
 void DownloadManagerImpl::OnItemAddedToPersistentStore(int32 download_id,
                                                        int64 db_handle) {
-  if (save_page_downloads_.count(download_id)) {
+  DownloadItem* item = GetDownload(download_id);
+  // It's valid that we don't find a matching item, i.e. on shutdown.
+  if (!item)
+    return;
+  if (SavePageExternalData::Get(item)) {
     OnSavePageItemAddedToPersistentStore(download_id, db_handle);
-  } else if (active_downloads_.count(download_id)) {
+  } else {
     OnDownloadItemAddedToPersistentStore(download_id, db_handle);
   }
-  // It's valid that we don't find a matching item, i.e. on shutdown.
 }
 
 // Once the new DownloadItem has been committed to the persistent store,
@@ -1076,10 +1099,9 @@ DownloadItem* DownloadManagerImpl::GetActiveDownloadItem(int download_id) {
 void DownloadManagerImpl::AssertContainersConsistent() const {
 #if !defined(NDEBUG)
   // Turn everything into sets.
-  const DownloadMap* input_maps[] = {&active_downloads_,
-                                     &save_page_downloads_};
-  DownloadSet active_set, save_page_set;
-  DownloadSet* all_sets[] = {&active_set, &save_page_set};
+  const DownloadMap* input_maps[] = {&active_downloads_};
+  DownloadSet active_set;
+  DownloadSet* all_sets[] = {&active_set};
   DCHECK_EQ(ARRAYSIZE_UNSAFE(input_maps), ARRAYSIZE_UNSAFE(all_sets));
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(input_maps); i++) {
     for (DownloadMap::const_iterator it = input_maps[i]->begin();
@@ -1123,18 +1145,8 @@ void DownloadManagerImpl::AssertContainersConsistent() const {
 void DownloadManagerImpl::OnSavePageItemAddedToPersistentStore(
     int32 download_id, int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  DownloadMap::const_iterator it = save_page_downloads_.find(download_id);
-  // This can happen if the download manager is shutting down and all maps
-  // have been cleared.
-  if (it == save_page_downloads_.end())
-    return;
-
-  DownloadItem* download = it->second;
-  if (!download) {
-    NOTREACHED();
-    return;
-  }
+  DownloadItem* download = GetDownload(download_id);
+  DCHECK(download);  // TODO(benjhayden) Pass download in.

[Randy Smith (Not in Mondays), 2012/07/10 19:06:05]

I don't understand the TODO?

[benjhayden, 2012/07/10 20:00:08]

I realized that it would be easier to do the todo than to explain the todo, so I
did it.

 
   AddDownloadItemToHistory(download, db_handle);
 
@@ -1147,9 +1159,6 @@ void DownloadManagerImpl::SavePageDownloadFinished(DownloadItem* download) {
   if (download->IsPersisted()) {
     if (delegate_)
       delegate_->UpdateItemInPersistentStore(download);
-    DCHECK(ContainsKey(save_page_downloads_, download->GetId()));
-    save_page_downloads_.erase(download->GetId());
-
     if (download->IsComplete())
       content::NotificationService::current()->Notify(
           content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,