[Downloads] Always call DM::StartDownload() for explicit downloads.

chrome/browser/extensions/api/downloads/downloads_api.cc

Index: chrome/browser/extensions/api/downloads/downloads_api.cc
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc
index 0a297d2116bd4a67bd2bed82ea1b9fbfa9a1556f..c8fc9921f1a7da08effe28cd23b0bc691ee5a2cd 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -1038,7 +1038,6 @@ void DownloadsDownloadFunction::OnStarted(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   VLOG(1) << __FUNCTION__ << " " << item << " " << interrupt_reason;
   if (item) {
-    DCHECK_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
     SetResult(new base::FundamentalValue(static_cast<int>(item->GetId())));
     if (!creator_suggested_filename.empty() ||
         (creator_conflict_action !=
@@ -1056,7 +1055,6 @@ void DownloadsDownloadFunction::OnStarted(
     new DownloadedByExtension(item, extension()->id(), extension()->name());
     item->UpdateObservers();
   } else {
-    DCHECK_NE(content::DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
     error_ = content::DownloadInterruptReasonToString(interrupt_reason);

[benjhayden, 2016/02/13 00:07:32]

Let me see if I'm reading this right.

The OnStarted callback used to guarantee that, if a DownloadItem had been
created, then the interrupt_reason would be NONE, otherwise it would not be
NONE. (Right? Or was this always broken? :-)

Now, a DownloadItem may still be created or not be created, and that has no
relation to the interrupt_reason.

The CL description seems to imply that a DownloadItem would always be non-NULL,
but perhaps I skimmed it too quickly?

When might this |item| be NULL?
Why would there be an item with a non-NONE interrupt reason?
Why would there not be an item but a NONE interrupt reason?

I'm trying to figure out how to explain to extension authors how to make sense
of a callback receiving {error: 'NONE'} without a download id, and whether the
if(item) branch needs to set error_ as well, but then how to make sense of
{error: 'something'} with a download id.

[Randy Smith (Not in Mondays), 2016/02/13 00:19:29]

FWIW, my understanding of the code is that we may sometimes have a DownloadItem
and a non-NONE error code, but we will never have no DownloadItem and a NONE
error code,  Which I think means the DCHECK directly above this one should
actually have been left in.  But you should believe Asanka over me (though if
I'm wrong about that invariant I feel like I was pretty clueless in the
review--I hope I'm right).

[asanka, 2016/02/13 00:57:36]

You're right that the above DCHECK should'be been left in. If the download is
nullptr, then the interrupt reason should be something other than NONE. I'll
re-instate it.

However, the interrupt reasons were always supposed to indicate the state of a
download. Not explain its absence. That said, we can set it to NONE when
returning a valid download item, and then the extension authors can inspect the
download item on their own.

   }
   SendResponse(error_.empty());