chrome.downloads.open()

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 <algorithm>
 #include <cctype>
 #include <iterator>
 #include <set>
@@ skipping 319 matching lines @@
   if (profile->HasOffTheRecordProfile() &&
       (include_incognito ||
        profile->IsOffTheRecord())) {
     *incognito_manager = BrowserContext::GetDownloadManager(
         profile->GetOffTheRecordProfile());
   } else {
     *incognito_manager = NULL;
   }
 }
 
-DownloadItem* GetActiveItem(Profile* profile, bool include_incognito, int id) {
+DownloadItem* GetDownload(Profile* profile, bool include_incognito, int id) {
   DownloadManager* manager = NULL;
   DownloadManager* incognito_manager = NULL;
   GetManagers(profile, include_incognito, &manager, &incognito_manager);
   DownloadItem* download_item = manager->GetDownload(id);
   if (!download_item && incognito_manager)
     download_item = incognito_manager->GetDownload(id);
+  return download_item;
+}
+
+DownloadItem* GetActiveItem(Profile* profile, bool include_incognito, int id) {

[Randy Smith (Not in Mondays), 2012/12/05 14:17:42]

Maybe regularize the names of these two functions?  (E.g. GetActiveDownload()
instead of GetDownload())?

[benjhayden, 2012/12/05 18:41:02]

Is GetDownloadIfInProgress ok?

[Randy Smith (Not in Mondays), 2012/12/05 19:12:05]

Sure.

+  DownloadItem* download_item = GetDownload(profile, include_incognito, id);
   return download_item && download_item->IsInProgress() ? download_item : NULL;
 }
 
 enum DownloadsFunctionName {
   DOWNLOADS_FUNCTION_DOWNLOAD = 0,
   DOWNLOADS_FUNCTION_SEARCH = 1,
   DOWNLOADS_FUNCTION_PAUSE = 2,
   DOWNLOADS_FUNCTION_RESUME = 3,
   DOWNLOADS_FUNCTION_CANCEL = 4,
   DOWNLOADS_FUNCTION_ERASE = 5,
@@ skipping 438 matching lines @@
   return error_.empty();
 }
 
 DownloadsOpenFunction::DownloadsOpenFunction() {}
 DownloadsOpenFunction::~DownloadsOpenFunction() {}
 
 bool DownloadsOpenFunction::RunImpl() {
   scoped_ptr<extensions::api::downloads::Open::Params> params(
       extensions::api::downloads::Open::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
-  error_ = download_extension_errors::kNotImplementedError;
-  if (error_.empty())
-    RecordApiFunctions(DOWNLOADS_FUNCTION_OPEN);
-  return error_.empty();
+  DownloadItem* download_item = GetDownload(
+      profile(), include_incognito(), params->download_id);
+  if (!download_item || !download_item->IsComplete()) {

[Randy Smith (Not in Mondays), 2012/12/05 14:17:42]

I think I agree with the behavioral choice to not have "open before complete"
set auto-open, but be aware that since this isn't the way the UI behaves, people
might be surprised by it, which means you should make sure that it's mentioned
in the docs (I haven't checked the docs).

[benjhayden, 2012/12/05 18:41:02]

Decided to use SetOpenWhenComplete. PTAL.

[Randy Smith (Not in Mondays), 2012/12/05 19:12:05]

*wince*  I guess that's fine.  I'm uncomfortable with it because I think
long-term we want to remove the auto-open behavior from DownloadItem completely,
and make it the responsibility of the callers (e.g. they should observe the item
and call Open when it completes.)  But if auto-opening is the behavior you want
at the extension level, that objection certainly shouldn't stop you
(functionality should drive mechanism, not the other way around :-}), and that
is the way to accomplish what you want in the current scheme.  We'll just need
to port it if/when we move auto-open out of content.

Separate from the above, could you say a little bit about why you want to
present the auto-open interface to extension authors rather than making them
wait for completion?  Because it's one less event they have to handle, which'll
result in simpler code?

[benjhayden, 2012/12/05 19:29:48]

Actually, the primary reason that I'd like for open() to set open_when_complete
is what you said originally: it's the least surprising because it's in keeping
with the current UI. I don't think it'll be a terrible burden to implement it as
an observer if/when SetOpenWhenComplete is removed.
Are there plans to remove the open_when_complete feature from the UI at some
point?

+    error_ = download_extension_errors::kInvalidOperationError;
+    return false;
+  }
+  download_item->OpenDownload();
+  RecordApiFunctions(DOWNLOADS_FUNCTION_OPEN);
+  return true;
 }
 
 DownloadsDragFunction::DownloadsDragFunction() {}
 DownloadsDragFunction::~DownloadsDragFunction() {}
 
 bool DownloadsDragFunction::RunImpl() {
   scoped_ptr<extensions::api::downloads::Drag::Params> params(
       extensions::api::downloads::Drag::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
   error_ = download_extension_errors::kNotImplementedError;
@@ skipping 16 matching lines @@
 
 bool DownloadsGetFileIconFunction::RunImpl() {
   scoped_ptr<extensions::api::downloads::GetFileIcon::Params> params(
       extensions::api::downloads::GetFileIcon::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
   const extensions::api::downloads::GetFileIconOptions* options =
       params->options.get();
   int icon_size = kDefaultIconSize;
   if (options && options->size.get())
     icon_size = *options->size.get();
-  DownloadManager* manager = NULL;
-  DownloadManager* incognito_manager = NULL;
-  GetManagers(profile(), include_incognito(), &manager, &incognito_manager);
-  DownloadItem* download_item = manager->GetDownload(params->download_id);
-  if (!download_item && incognito_manager)
-    download_item = incognito_manager->GetDownload(params->download_id);
+  DownloadItem* download_item = GetDownload(
+      profile(), include_incognito(), params->download_id);
   if (!download_item || download_item->GetTargetFilePath().empty()) {
     error_ = download_extension_errors::kInvalidOperationError;
     return false;
   }
   // In-progress downloads return the intermediate filename for GetFullPath()
   // which doesn't have the final extension. Therefore we won't be able to
   // derive a good file icon for it. So we use GetTargetFilePath() instead.
   DCHECK(icon_extractor_.get());
   DCHECK(icon_size == 16 || icon_size == 32);
   EXTENSION_FUNCTION_VALIDATE(icon_extractor_->ExtractIconURLForPath(
@@ skipping 130 matching lines @@
   if (profile_->HasOffTheRecordProfile() &&
       !profile_->IsOffTheRecord()) {
     DispatchEventInternal(
         profile_->GetOffTheRecordProfile(),
         event_name,
         json_args,
         scoped_ptr<base::ListValue>(args->DeepCopy()));
   }
   DispatchEventInternal(profile_, event_name, json_args, args.Pass());
 }