OnDownloadStarted takes DownloadItem* instead of DownloadId

content/browser/download/download_resource_handler.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_resource_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 26 matching lines @@
 using content::DownloadManager;
 using content::ResourceDispatcherHostImpl;
 using content::ResourceRequestInfoImpl;
 
 namespace {
 
 static const int kDownloadByteStreamSize = 100 * 1024;
 
 void CallStartedCBOnUIThread(
     const DownloadResourceHandler::OnStartedCallback& started_cb,
-    DownloadId id,
+    DownloadItem* item,
     net::Error error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (started_cb.is_null())
     return;
-  started_cb.Run(id, error);
+  started_cb.Run(item, error);
 }
 
 // Static function in order to prevent any accidental accesses to
 // DownloadResourceHandler members from the UI thread.
 static void StartOnUIThread(
     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<content::ByteStreamReader> stream,
     const DownloadResourceHandler::OnStartedCallback& started_cb) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadManager* download_manager = info->request_handle.GetDownloadManager();
   if (!download_manager) {
     // NULL in unittests or if the page closed right after starting the
     // download.
     if (!started_cb.is_null())
-      started_cb.Run(DownloadId(), net::ERR_ACCESS_DENIED);
+      started_cb.Run(NULL, net::ERR_ACCESS_DENIED);
     return;
   }
 
-  DownloadId download_id =
-      download_manager->StartDownload(info.Pass(), stream.Pass());
+  DownloadItem* item = download_manager->StartDownload(

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

What would you think of just passing the started_cb into DM::StartDownload so
that the DRH doesn't need to know about DownloadItems?  It requires the DM to
know about the callback type, but that doesn't seem like a horrible thing.

(Note that this would suggest some refactoring so that the default calling path
in the DRH is to pass a NULL DI and assert that the error is not net::OK. 
Specifically, I think that CallStartedCB is now *always* called with a null
download (and probably always was :-})).

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

Hm, maybe in another CL.
I have no strong objections to that, but I kind of like being able to see all of
the callers of started_cb in a single file, and DRH would still need to include
download_item.h because it uses DI::IN_PROGRESS further down.

+      info.Pass(), stream.Pass());
 
   if (!started_cb.is_null())
-    started_cb.Run(download_id, net::OK);
+    started_cb.Run(item, net::OK);
 }
 
 }  // namespace
 
 DownloadResourceHandler::DownloadResourceHandler(
     net::URLRequest* request,
     const DownloadResourceHandler::OnStartedCallback& started_cb,
     const content::DownloadSaveInfo& save_info)
     : render_view_id_(0),               // Actually initialized below.
       content_length_(0),
@@ skipping 120 matching lines @@
                  // Pass to StartOnUIThread so that variable
                  // access is always on IO thread but function
                  // is called on UI thread.
                  started_cb_));
   // Guaranteed to be called in StartOnUIThread
   started_cb_.Reset();
 
   return true;
 }
 
-void DownloadResourceHandler::CallStartedCB(DownloadId id, net::Error error) {
+void DownloadResourceHandler::CallStartedCB(
+    DownloadItem* item, net::Error error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (started_cb_.is_null())
     return;
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&CallStartedCBOnUIThread, started_cb_, id, error));
+      base::Bind(&CallStartedCBOnUIThread, started_cb_, item, error));
   started_cb_.Reset();
 }
 
 bool DownloadResourceHandler::OnWillStart(int request_id,
                                           const GURL& url,
                                           bool* defer) {
   return true;
 }
 
 // Create a new buffer, which will be handed to the download thread for file
@@ skipping 101 matching lines @@
           reason = content::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
           break;
       }
     }
   }
 
   download_stats::RecordAcceptsRanges(accept_ranges_, bytes_read_);
   download_stats::RecordNetworkBlockage(
       base::TimeTicks::Now() - download_start_time_, total_pause_time_);
 
-  CallStartedCB(DownloadId(), error_code);
+  CallStartedCB(NULL, error_code);
 
   // Send the info down the stream.  Conditional is in case we get
   // OnResponseCompleted without OnResponseStarted.
   if (stream_writer_.get())
     stream_writer_->Close(reason);
 
   stream_writer_.reset();  // We no longer need the stream.
   read_buffer_ = NULL;
 
   return true;
@@ skipping 68 matching lines @@
                             render_view_id_,
                             save_info_.file_path.value().c_str());
 }
 
 DownloadResourceHandler::~DownloadResourceHandler() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // This won't do anything if the callback was called before.
   // If it goes through, it will likely be because OnWillStart() returned
   // false somewhere in the chain of resource handlers.
-  CallStartedCB(DownloadId(), net::ERR_ACCESS_DENIED);
+  CallStartedCB(NULL, net::ERR_ACCESS_DENIED);
 
   // Remove output stream callback if a stream exists.
   if (stream_writer_.get())
     stream_writer_->RegisterCallback(base::Closure());
 
   UMA_HISTOGRAM_TIMES("SB2.DownloadDuration",
                       base::TimeTicks::Now() - download_start_time_);
 }