Kill DownloadManagerImpl::save_page_downloads_

content/browser/download/download_manager_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_manager_impl.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 32 matching lines @@
 
 using content::BrowserThread;
 using content::DownloadId;
 using content::DownloadItem;
 using content::DownloadPersistentStoreInfo;
 using content::ResourceDispatcherHostImpl;
 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
   // DownloadUrlParameters can-not include resource_dispatcher_host_impl.h, so
   // we must down cast. RDHI is the only subclass of RDH as of 2012 May 4.
   scoped_ptr<net::URLRequest> request(new net::URLRequest(
       params->url(),
       NULL,
       params->resource_context()->GetRequestContext()));
   request->set_referrer(params->referrer().url.spec());
@@ skipping 235 matching lines @@
   // in DownloadItem destruction.
   DownloadMap downloads_to_delete;
   downloads_to_delete.swap(downloads_);
 
   active_downloads_.clear();
   STLDeleteValues(&downloads_to_delete);
 
   // 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();
 }
 
 void DownloadManagerImpl::GetTemporaryDownloads(
     const FilePath& dir_path, DownloadVector* result) {
   DCHECK(result);
 
   for (DownloadMap::iterator it = downloads_.begin();
@@ skipping 212 matching lines @@
       page_url,
       is_otr,
       GetNextId(),
       mime_type,
       bound_net_log);
 
   download->AddObserver(observer);
 
   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_)
     delegate_->AddItemToPersistentStore(download);
 
   return download;
 }
 
 // The target path for the download item is now valid. We proceed with the
 // determination of an intermediate path.
@@ skipping 242 matching lines @@
     const DownloadVector& pending_deletes) {
   if (pending_deletes.empty())
     return 0;
 
   // Delete from internal maps.
   for (DownloadVector::const_iterator it = pending_deletes.begin();
       it != pending_deletes.end();
       ++it) {
     DownloadItem* download = *it;
     DCHECK(download);
-    save_page_downloads_.erase(download->GetId());
     downloads_.erase(download->GetId());
   }
 
   // Tell observers to refresh their views.
   NotifyModelChanged();
 
   // Delete the download items themselves.
   const int num_deleted = static_cast<int>(pending_deletes.size());
   STLDeleteContainerPointers(pending_deletes.begin(), pending_deletes.end());
   return num_deleted;
@@ skipping 150 matching lines @@
   // This includes buttons to save or cancel, for a dangerous download.
   ShowDownloadInBrowser(download);
 
   // Inform interested objects about the new download.
   NotifyModelChanged();
 }
 
 
 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,
 // associate it with its db_handle (TODO(benjhayden) merge db_handle with id),
 // show it in the browser (TODO(benjhayden) the ui should observe us instead),
 // and notify observers (TODO(benjhayden) observers should be able to see the
 // item when it's created so they can observe it directly. Are there any
 // clients that actually need to know when the item is added to the history?).
 void DownloadManagerImpl::OnDownloadItemAddedToPersistentStore(
     int32 download_id, int64 db_handle) {
@@ skipping 76 matching lines @@
   if (ContainsKey(active_downloads_, download_id))
     return active_downloads_[download_id];
   return NULL;
 }
 
 // Confirm that everything in all maps is also in |downloads_|, and that
 // everything in |downloads_| is also in some other map.
 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();
          it != input_maps[i]->end(); ++it) {
       all_sets[i]->insert(&*it->second);
     }
   }
 
   DownloadSet all_downloads;
   for (DownloadMap::const_iterator it = downloads_.begin();
@@ skipping 23 matching lines @@
 // Shutdown) the result will be that the SavePage system will not be able to
 // properly update the download item (which no longer exists) or the download
 // history, but the action will complete properly anyway.  This may lead to the
 // history entry being wrong on a reload of chrome (specifically in the case of
 // Initiation -> History Callback -> Removal -> Completion), but there's no way
 // to solve that without canceling on Remove (which would then update the DB).
 
 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);
 
   // Finalize this download if it finished before the history callback.
   if (!download->IsInProgress())
     SavePageDownloadFinished(download);
 }
 
 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,
           content::Source<DownloadManager>(this),
           content::Details<DownloadItem>(download));
   }
 }
 
 void DownloadManagerImpl::DownloadOpened(DownloadItem* download) {
   if (delegate_)
@@ skipping 21 matching lines @@
 void DownloadManagerImpl::DownloadRenamedToFinalName(
     DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // If the rename failed, we receive an OnDownloadInterrupted() call before we
   // receive the DownloadRenamedToFinalName() call.
   if (delegate_) {
     delegate_->UpdatePathForItemInPersistentStore(
         download, download->GetFullPath());
   }
 }