Replace NewRunnable with Callback in DownloadManager.

content/browser/download/download_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 "content/browser/download/download_manager.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/synchronization/lock.h"
 #include "base/sys_string_conversions.h"
-#include "base/task.h"
+#include "base/tuple.h"
 #include "build/build_config.h"
 #include "content/browser/browser_context.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_persistent_store_info.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/download/download_status_updater.h"
 #include "content/browser/download/interrupt_reasons.h"
 #include "content/browser/renderer_host/render_process_host.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 
 namespace {
 
-void BeginDownload(
-    const GURL& url,
-    const GURL& referrer,
-    const DownloadSaveInfo& save_info,
-    ResourceDispatcherHost* resource_dispatcher_host,
-    int render_process_id,
-    int render_view_id,
-    const content::ResourceContext* context) {
-  net::URLRequest* request = new net::URLRequest(url, resource_dispatcher_host);
-  request->set_referrer(referrer.spec());
-  resource_dispatcher_host->BeginDownload(
-      request,
-      save_info,
-      true,
+typedef Tuple7<GURL,                      // url
+               GURL,                      // referrer
+               DownloadSaveInfo,
+               ResourceDispatcherHost*,
+               int,                       // render_process_id
+               int,                       // render_view_id
+               const content::ResourceContext*> BeginDownloadParams;

[Randy Smith (Not in Mondays), 2011/10/28 17:37:53]

This is very sledgehammery, and the fact that we're accessing it with
params.a/b/c/d/e/f/g tends to reinforce that.  Could you possibly instead create
new structures to get us down below the limit that have some limited conceptual
integrity?  E.g. Combine the url and referred, and combine the render_process_id
and render_view_id?

[achuithb, 2011/10/28 19:51:05]

Done.

+
+void BeginDownload(const BeginDownloadParams& params) {
+  net::URLRequest* request = new net::URLRequest(params.a, params.d);
+  request->set_referrer(params.b.spec());
+  params.d->BeginDownload(request, params.c, true,
       DownloadResourceHandler::OnStartedCallback(),
-      render_process_id,
-      render_view_id,
-      *context);
+      params.e, params.f, *params.g);
 }
 
 }  // namespace
 
 DownloadManager::DownloadManager(content::DownloadManagerDelegate* delegate,
                                  DownloadStatusUpdater* status_updater)
     : shutdown_needed_(false),
       browser_context_(NULL),
       next_id_(0),
       file_manager_(NULL),
@@ skipping 19 matching lines @@
            << " shutdown_needed_ = " << shutdown_needed_;
   if (!shutdown_needed_)
     return;
   shutdown_needed_ = false;
 
   FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown());
   // TODO(benjhayden): Consider clearing observers_.
 
   if (file_manager_) {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(file_manager_,
-                          &DownloadFileManager::OnDownloadManagerShutdown,
-                          make_scoped_refptr(this)));
+        base::Bind(&DownloadFileManager::OnDownloadManagerShutdown,
+                   file_manager_, make_scoped_refptr(this)));
   }
 
   AssertContainersConsistent();
 
   // Go through all downloads in downloads_.  Dangerous ones we need to
   // remove on disk, and in progress ones we need to cancel.
   for (DownloadSet::iterator it = downloads_.begin(); it != downloads_.end();) {
     DownloadItem* download = *it;
 
     // Save iterator from potential erases in this set done by called code.
@@ skipping 146 matching lines @@
     CheckForFileRemoval(it->second);
   }
 }
 
 void DownloadManager::CheckForFileRemoval(DownloadItem* download_item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (download_item->IsComplete() &&
       !download_item->file_externally_removed()) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(this,
-                          &DownloadManager::CheckForFileRemovalOnFileThread,
-                          download_item->db_handle(),
-                          download_item->GetTargetFilePath()));
+        base::Bind(&DownloadManager::CheckForFileRemovalOnFileThread,
+                   this, download_item->db_handle(),
+                   download_item->GetTargetFilePath()));
   }
 }
 
 void DownloadManager::CheckForFileRemovalOnFileThread(
     int64 db_handle, const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (!file_util::PathExists(path)) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        NewRunnableMethod(this,
-                          &DownloadManager::OnFileRemovalDetected,
-                          db_handle));
+        base::Bind(&DownloadManager::OnFileRemovalDetected, this, db_handle));
   }
 }
 
 void DownloadManager::OnFileRemovalDetected(int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = history_downloads_.find(db_handle);
   if (it != history_downloads_.end()) {
     DownloadItem* download_item = it->second;
     download_item->OnDownloadedFileRemoved();
   }
@@ skipping 72 matching lines @@
   in_progress_[download_id] = download;
   UpdateDownloadProgress();  // Reflect entry into in_progress_.
 
   // Rename to intermediate name.
   FilePath download_path;
   if (!delegate_->OverrideIntermediatePath(download, &download_path))
     download_path = download->full_path();
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::RenameInProgressDownloadFile,
-          download->global_id(), download_path));
+      base::Bind(&DownloadFileManager::RenameInProgressDownloadFile,
+                 file_manager_, download->global_id(), download_path));
 
   download->Rename(download_path);
 
   delegate_->AddItemToPersistentStore(download);
 }
 
 void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = active_downloads_.find(download_id);
   if (it != active_downloads_.end()) {
@@ skipping 140 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* item = GetDownloadItem(download_id);
   if (!item)
     return;
 
   if (item->safety_state() == DownloadItem::SAFE) {
     DCHECK_EQ(0, uniquifier) << "We should not uniquify SAFE downloads twice";
   }
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, NewRunnableMethod(
-      file_manager_,
-      &DownloadFileManager::CompleteDownload,
-      item->global_id()));
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::CompleteDownload,
+                 file_manager_, item->global_id()));
 
   if (uniquifier)
     item->set_path_uniquifier(uniquifier);
 
   item->OnDownloadRenamedToFinalName(full_path);
   delegate_->UpdatePathForItemInPersistentStore(item, full_path);
 }
 
 void DownloadManager::CancelDownload(int32 download_id) {
   DownloadItem* download = GetActiveDownload(download_id);
@@ skipping 160 matching lines @@
 void DownloadManager::DownloadUrlToFile(const GURL& url,
                                         const GURL& referrer,
                                         const std::string& referrer_charset,
                                         const DownloadSaveInfo& save_info,
                                         TabContents* tab_contents) {
   DCHECK(tab_contents);
   ResourceDispatcherHost* resource_dispatcher_host =
       content::GetContentClient()->browser()->GetResourceDispatcherHost();
   // We send a pointer to content::ResourceContext, instead of the usual
   // reference, so that a copy of the object isn't made.
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-      NewRunnableFunction(&BeginDownload,
-                          url,
-                          referrer,
-                          save_info,
-                          resource_dispatcher_host,
-                          tab_contents->GetRenderProcessHost()->id(),
-                          tab_contents->render_view_host()->routing_id(),
-                          &tab_contents->browser_context()->
-                              GetResourceContext()));
+  // We use Tuple7 because base::Bind can't handle 7 arguments.
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&BeginDownload, MakeTuple(
+          url, referrer, save_info, resource_dispatcher_host,
+          tab_contents->GetRenderProcessHost()->id(),
+          tab_contents->render_view_host()->routing_id(),
+          &tab_contents->browser_context()->GetResourceContext())));
 }
 
 void DownloadManager::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
   observer->ModelChanged();
 }
 
 void DownloadManager::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
@@ skipping 344 matching lines @@
 void DownloadManager::MarkDownloadOpened(DownloadItem* download) {
   delegate_->UpdateItemInPersistentStore(download);
   int num_unopened = 0;
   for (DownloadMap::iterator it = history_downloads_.begin();
        it != history_downloads_.end(); ++it) {
     if (it->second->IsComplete() && !it->second->opened())
       ++num_unopened;
   }
   download_stats::RecordOpensOutstanding(num_unopened);
 }