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

chrome/browser/extensions/api/downloads/downloads_api.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 "chrome/browser/extensions/api/downloads/downloads_api.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <set>
 #include <string>
@@ skipping 1020 matching lines @@
 }
 
 void DownloadsDownloadFunction::OnStarted(
     const base::FilePath& creator_suggested_filename,
     downloads::FilenameConflictAction creator_conflict_action,
     DownloadItem* item,
     content::DownloadInterruptReason interrupt_reason) {
   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 !=
          downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY)) {
       ExtensionDownloadsEventRouterData* data =
           ExtensionDownloadsEventRouterData::Get(item);
       if (!data) {
         data = new ExtensionDownloadsEventRouterData(
             item,
             scoped_ptr<base::DictionaryValue>(new base::DictionaryValue()));
       }
       data->CreatorSuggestedFilename(
           creator_suggested_filename, creator_conflict_action);
     }
     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());
 }
 
 DownloadsSearchFunction::DownloadsSearchFunction() {}
 
 DownloadsSearchFunction::~DownloadsSearchFunction() {}
 
 bool DownloadsSearchFunction::RunSync() {
   scoped_ptr<downloads::Search::Params> params(
@@ skipping 832 matching lines @@
     return;
   base::Time now(base::Time::Now());
   int delta = now.ToTimeT() - last_checked_removal_.ToTimeT();
   if (delta <= kFileExistenceRateLimitSeconds)
     return;
   last_checked_removal_ = now;
   manager->CheckForHistoryFilesRemoval();
 }
 
 }  // namespace extensions