OnDownloadStarted takes DownloadItem* instead of DownloadId

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 326 matching lines @@
   DCHECK(browser_context);
   DCHECK(!shutdown_needed_)  << "DownloadManager already initialized.";
   shutdown_needed_ = true;
 
   browser_context_ = browser_context;
 
   return true;
 }
 
 // We have received a message from DownloadFileManager about a new download.
-content::DownloadId DownloadManagerImpl::StartDownload(
+DownloadItem* DownloadManagerImpl::StartDownload(
     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<content::ByteStreamReader> stream) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // |bound_net_log| will be used for logging both the download item's and
   // the download file's events.
   net::BoundNetLog bound_net_log = CreateDownloadItem(info.get());
 
   // If info->download_id was unknown on entry to this function, it was
   // assigned in CreateDownloadItem.
@@ skipping 11 matching lines @@
                  this, download_id.local()));
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFileManager::CreateDownloadFile,
                  file_manager_, base::Passed(info.Pass()),
                  base::Passed(stream.Pass()), make_scoped_refptr(this),
                  (delegate_ && delegate_->GenerateFileHash()), bound_net_log,
                  callback));
 
-  return download_id;
+  return GetDownload(download_id.local());

[Randy Smith (Not in Mondays), 2012/10/08 19:20:52]

I think that it would be a better design to refactor this so that
CreateDownloadItem() returns the DownloadItem so you don't need to do
GetDownload().  OTOH, I'm doing exactly that in the DFM nuke CL, and there's
really no point in duplicating the work.  So this is fine, but I wanted to call
out the code preference.

[benjhayden, 2012/10/08 19:49:45]

Ya, I thought about making CreateDownloadItem return the item, but noticed that
that would require some surgery around bound_net_log, and decided to let you
handle it. :-)

 }
 
 void DownloadManagerImpl::OnDownloadFileCreated(
     int32 download_id, content::DownloadInterruptReason reason) {
   if (reason != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
     OnDownloadInterrupted(download_id, reason);
     // TODO(rdsmith): It makes no sense to continue along the
     // regular download path after we've gotten an error.  But it's
     // the way the code has historically worked, and this allows us
     // to get the download persisted and observers of the download manager
@@ skipping 645 matching lines @@
 void DownloadManagerImpl::DownloadRenamedToFinalName(
     DownloadItemImpl* 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());
   }
 }