chrome.downloads.open()

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 9ee1954712d33bd84591c6b7b6eedbf8b7b80e12..db69dc564864d9486a746208a5e89505003ecbe4 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -337,13 +337,18 @@ void GetManagers(
   }
 }
 
-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;
 }
 
@@ -802,10 +807,15 @@ 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() {}
@@ -842,12 +852,8 @@ bool DownloadsGetFileIconFunction::RunImpl() {
   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;