Change DownloadProcessHandle to be more of an encapsulated class.

chrome/browser/download/download_file_manager.cc

 // Copyright (c) 2011 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 "chrome/browser/download/download_file_manager.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/stl_util-inl.h"
 #include "base/task.h"
@@ skipping 50 matching lines @@
   DCHECK(info);
   VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString();
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Life of |info| ends here. No more references to it after this method.
   scoped_ptr<DownloadCreateInfo> infop(info);
 
   scoped_ptr<DownloadFile>
       download_file(new DownloadFile(info, download_manager));
   if (!download_file->Initialize(get_hash)) {
-    download_util::CancelDownloadRequest(resource_dispatcher_host_,
-                                         info->process_handle);
+    info->process_handle.CancelRequest();
     return;
   }
 
   int32 id = info->download_id;
   DCHECK(GetDownloadFile(id) == NULL);
   downloads_[id] = download_file.release();
 
   // The file is now ready, we can un-pause the request and start saving data.
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       NewRunnableMethod(this, &DownloadFileManager::ResumeDownloadRequest,
                         info->process_handle));
 
   StartUpdateTimer();
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
                         &DownloadManager::StartDownload, id));
 }
 
 void DownloadFileManager::ResumeDownloadRequest(
     DownloadProcessHandle process) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // This balances the pause in DownloadResourceHandler::OnResponseStarted.
-  resource_dispatcher_host_->PauseRequest(process.child_id(),
-                                          process.request_id(),
-                                          false);
+  process.PauseRequest(false);
 }
 
 DownloadFile* DownloadFileManager::GetDownloadFile(int id) {
   DownloadFileMap::iterator it = downloads_.find(id);
   return it == downloads_.end() ? NULL : it->second;
 }
 
 void DownloadFileManager::StartUpdateTimer() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (!update_timer_.IsRunning()) {
@@ skipping 28 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   return next_id_++;
 }
 
 void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(info);
 
   DownloadManager* manager = info->process_handle.GetDownloadManager();
   if (!manager) {
-    download_util::CancelDownloadRequest(resource_dispatcher_host_,
-                                         info->process_handle);
+    info->process_handle.CancelRequest();
     delete info;
     return;
   }
 
   // TODO(phajdan.jr): fix the duplication of path info below.
   info->path = info->save_info.file_path;
 
   manager->CreateDownloadItem(info);
 
   bool hash_needed = resource_dispatcher_host_->safe_browsing_service()->

[Paweł Hajdan Jr., 2011/06/04 08:58:51]

It seems that this is now the only place where resource_dispatcher_host_ is used
in DownloadFileManager. How about adding this accessor also to
DownloadProcessHandle, and removing resource_dispatcher_host_ from
DownloadFileManager?

[Randy Smith (Not in Mondays), 2011/06/07 22:41:11]

I'm reluctant to do that, as it doesn't feel like it's conceptually unified with
the DownloadProcessHandle to me.  That class/objects of that class are about
having a handle to the URLRequest and doing things with it (and I'm going to
change the name to DownloadRequestHandle for this reason).  What we're doing
here is getting some global state that isn't per-request.  

I thought about a couple of other alternatives:
* Switch this over to using the BrowserProcess global for
ResourceDispatcherHost; we're on the UI thread, so that's safe.  I'm a bit
reluctant, as it reduces possible mocking opportunities in the future, but we'd
probably have to do some careful thinking about interfaces to be able to mock
download_file_manager.h with other structures, and it's a pretty small bit of
state to want to mock, so I could be ok with this.
* Switching the constructor to DownloadFileManager to take a hash needed
boolean.  This means we can't change that value during a running session of the
browser.
* Take the safe browsing service in the constructor rather than the resource
dispatcher host.  This is a minor plus, but doesn't reduce the member
variables/dependencies in the same way.

WDYT?

       DownloadBinHashNeeded();
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(this, &DownloadFileManager::CreateDownloadFile,
                         info, make_scoped_refptr(manager), hash_needed));
 }
 
 // We don't forward an update to the UI thread here, since we want to throttle
 // the UI update rate via a periodic timer. If the user has cancelled the
 // download (in the UI thread), we may receive a few more updates before the IO
@@ skipping 90 matching lines @@
 void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(manager);
 
   std::set<DownloadFile*> to_remove;
 
   for (DownloadFileMap::iterator i = downloads_.begin();
        i != downloads_.end(); ++i) {
     DownloadFile* download_file = i->second;
     if (download_file->GetDownloadManager() == manager) {
-      download_file->CancelDownloadRequest(resource_dispatcher_host_);
+      download_file->CancelDownloadRequest();
       to_remove.insert(download_file);
     }
   }
 
   for (std::set<DownloadFile*>::iterator i = to_remove.begin();
        i != to_remove.end(); ++i) {
     downloads_.erase((*i)->id());
     delete *i;
   }
 }
@@ skipping 94 matching lines @@
 
   DownloadFile* download_file = GetDownloadFile(id);
   if (!download_file)
     return;
 
   DownloadManager* download_manager = download_file->GetDownloadManager();
   if (!download_manager) {
     // Without a download manager, we can't cancel the request normally, so we
     // need to do it here.  The normal path will also update the download
     // history before cancelling the request.
-    download_file->CancelDownloadRequest(resource_dispatcher_host_);
+    download_file->CancelDownloadRequest();
     return;
   }
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
                         &DownloadManager::DownloadCancelled, id));
 }
 
 void DownloadFileManager::EraseDownload(int id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   if (!ContainsKey(downloads_, id))
     return;
 
   DownloadFile* download_file = downloads_[id];
 
   VLOG(20) << " " << __FUNCTION__ << "()"
            << " id = " << id
            << " download_file = " << download_file->DebugString();
 
   downloads_.erase(id);
 
   delete download_file;
 
   if (downloads_.empty())
     StopUpdateTimer();
 }