A few minor changes to the chrome.downloads extension API

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 b831d52d88f5323f54b336e07b2496bdea7c990a..b8a8ad3bb4d8ac15e2f18051e7d31b53f1c53a82 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -14,6 +14,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
@@ -29,9 +30,11 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_danger_prompt.h"
 #include "chrome/browser/download/download_file_icon_extractor.h"
+#include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_query.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
+#include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/extensions/event_names.h"
 #include "chrome/browser/extensions/event_router.h"
@@ -42,8 +45,10 @@
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/icon_loader.h"
 #include "chrome/browser/icon_manager.h"
+#include "chrome/browser/platform_util.h"
 #include "chrome/browser/renderer_host/chrome_render_message_filter.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/common/cancelable_task_tracker.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/api/downloads.h"
@@ -77,16 +82,18 @@ using content::DownloadManager;
 namespace download_extension_errors {
 
 // Error messages
-const char kGenericError[] = "I'm afraid I can't do that";
 const char kIconNotFoundError[] = "Icon not found";
 const char kInvalidDangerTypeError[] = "Invalid danger type";
 const char kInvalidFilenameError[] = "Invalid filename";
 const char kInvalidFilterError[] = "Invalid query filter";
+const char kInvalidHeaderError[] = "Invalid request header";
 const char kInvalidOperationError[] = "Invalid operation";
 const char kInvalidOrderByError[] = "Invalid orderBy field";
 const char kInvalidQueryLimit[] = "Invalid query limit";
 const char kInvalidStateError[] = "Invalid state";
 const char kInvalidURLError[] = "Invalid URL";
+const char kNotPermittedURLError[] = "In order to access that URL, this "
+  "extension must add the host to \"permissions\" in manifest.json";
 const char kNotImplementedError[] = "NotImplemented";
 const char kTooManyListenersError[] = "Each extension may have at most one "
   "onDeterminingFilename listener between all of its renderer execution "
@@ -101,19 +108,20 @@ const int  kDefaultIconSize = 32;
 
 // Parameter keys
 const char kBytesReceivedKey[] = "bytesReceived";
-const char kDangerAcceptedKey[] = "dangerAccepted";
+const char kCanResumeKey[] = "canResume";
+const char kDangerAccepted[] = "accepted";
 const char kDangerContent[] = "content";
 const char kDangerFile[] = "file";
+const char kDangerHost[] = "host";
 const char kDangerKey[] = "danger";
 const char kDangerSafe[] = "safe";
 const char kDangerUncommon[] = "uncommon";
-const char kDangerAccepted[] = "accepted";
-const char kDangerHost[] = "host";
 const char kDangerUrl[] = "url";
 const char kEndTimeKey[] = "endTime";
 const char kEndedAfterKey[] = "endedAfter";
 const char kEndedBeforeKey[] = "endedBefore";
 const char kErrorKey[] = "error";
+const char kEstimatedEndTimeKey[] = "estimatedEndTime";
 const char kExistsKey[] = "exists";
 const char kFileSizeKey[] = "fileSize";
 const char kFilenameKey[] = "filename";
@@ -123,6 +131,7 @@ const char kIncognito[] = "incognito";
 const char kMimeKey[] = "mime";
 const char kPausedKey[] = "paused";
 const char kQueryKey[] = "query";
+const char kReferrerUrlKey[] = "referrer";
 const char kStartTimeKey[] = "startTime";
 const char kStartedAfterKey[] = "startedAfter";
 const char kStartedBeforeKey[] = "startedBefore";
@@ -215,15 +224,14 @@ scoped_ptr<base::DictionaryValue> DownloadItemToJSON(
   json->SetInteger(kIdKey, download_item->GetId());
   const GURL& url = download_item->GetOriginalUrl();
   json->SetString(kUrlKey, (url.is_valid() ? url.spec() : std::string()));
+  const GURL& referrer = download_item->GetReferrerUrl();
+  json->SetString(kReferrerUrlKey, (referrer.is_valid() ? referrer.spec()
+                                                        : std::string()));
   json->SetString(kFilenameKey,
                   download_item->GetTargetFilePath().LossyDisplayName());
   json->SetString(kDangerKey, DangerString(download_item->GetDangerType()));
-  if (download_item->GetDangerType() !=
-      content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)
-    json->SetBoolean(kDangerAcceptedKey,
-                     download_item->GetDangerType() ==
-                     content::DOWNLOAD_DANGER_TYPE_USER_VALIDATED);
   json->SetString(kStateKey, StateString(download_item->GetState()));
+  json->SetBoolean(kCanResumeKey, download_item->CanResume());
   json->SetBoolean(kPausedKey, download_item->IsPaused());
   json->SetString(kMimeKey, download_item->GetMimeType());
   json->SetString(kStartTimeKey, TimeToISO8601(download_item->GetStartTime()));
@@ -231,14 +239,19 @@ scoped_ptr<base::DictionaryValue> DownloadItemToJSON(
   json->SetInteger(kTotalBytesKey, download_item->GetTotalBytes());
   json->SetBoolean(kIncognito, incognito);
   if (download_item->GetState() == DownloadItem::INTERRUPTED) {
-    json->SetInteger(kErrorKey, static_cast<int>(
+    json->SetString(kErrorKey, content::InterruptReasonDebugString(
         download_item->GetLastReason()));
   } else if (download_item->GetState() == DownloadItem::CANCELLED) {
-    json->SetInteger(kErrorKey, static_cast<int>(
+    json->SetString(kErrorKey, content::InterruptReasonDebugString(
         content::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED));
   }
   if (!download_item->GetEndTime().is_null())
     json->SetString(kEndTimeKey, TimeToISO8601(download_item->GetEndTime()));
+  base::TimeDelta time_remaining;
+  if (download_item->TimeRemaining(&time_remaining)) {
+    base::Time now = base::Time::Now();
+    json->SetString(kEstimatedEndTimeKey, TimeToISO8601(now + time_remaining));
+  }
   // TODO(benjhayden): Implement fileSize.
   json->SetInteger(kFileSizeKey, download_item->GetTotalBytes());
   return scoped_ptr<base::DictionaryValue>(json);
@@ -299,7 +312,6 @@ typedef base::hash_map<std::string, DownloadQuery::FilterType> FilterTypeMap;
 
 void InitFilterTypeMap(FilterTypeMap& filter_types) {
   filter_types[kBytesReceivedKey] = DownloadQuery::FILTER_BYTES_RECEIVED;
-  filter_types[kDangerAcceptedKey] = DownloadQuery::FILTER_DANGER_ACCEPTED;
   filter_types[kExistsKey] = DownloadQuery::FILTER_EXISTS;
   filter_types[kFilenameKey] = DownloadQuery::FILTER_FILENAME;
   filter_types[kFilenameRegexKey] = DownloadQuery::FILTER_FILENAME_REGEX;
@@ -325,7 +337,6 @@ typedef base::hash_map<std::string, DownloadQuery::SortType> SortTypeMap;
 void InitSortTypeMap(SortTypeMap& sorter_types) {
   sorter_types[kBytesReceivedKey] = DownloadQuery::SORT_BYTES_RECEIVED;
   sorter_types[kDangerKey] = DownloadQuery::SORT_DANGER;
-  sorter_types[kDangerAcceptedKey] = DownloadQuery::SORT_DANGER_ACCEPTED;
   sorter_types[kEndTimeKey] = DownloadQuery::SORT_END_TIME;
   sorter_types[kExistsKey] = DownloadQuery::SORT_EXISTS;
   sorter_types[kFilenameKey] = DownloadQuery::SORT_FILENAME;
@@ -394,6 +405,8 @@ enum DownloadsFunctionName {
   DOWNLOADS_FUNCTION_DRAG = 9,
   DOWNLOADS_FUNCTION_GET_FILE_ICON = 10,
   DOWNLOADS_FUNCTION_OPEN = 11,
+  DOWNLOADS_FUNCTION_DELETE_FILE = 12,
+  DOWNLOADS_FUNCTION_SET_SHELF_VISIBLE = 13,
   // Insert new values here, not at the beginning.
   DOWNLOADS_FUNCTION_LAST
 };
@@ -405,7 +418,9 @@ void RecordApiFunctions(DownloadsFunctionName function) {
 }
 
 void CompileDownloadQueryOrderBy(
-    const std::string& order_by_str, std::string* error, DownloadQuery* query) {
+    const std::vector<std::string>& order_by_strs,
+    std::string* error,
+    DownloadQuery* query) {
   // TODO(benjhayden): Consider switching from LazyInstance to explicit string
   // comparisons.
   static base::LazyInstance<SortTypeMap> sorter_types =
@@ -413,8 +428,6 @@ void CompileDownloadQueryOrderBy(
   if (sorter_types.Get().size() == 0)
     InitSortTypeMap(sorter_types.Get());
 
-  std::vector<std::string> order_by_strs;
-  base::SplitString(order_by_str, ' ', &order_by_strs);
   for (std::vector<std::string>::const_iterator iter = order_by_strs.begin();
        iter != order_by_strs.end(); ++iter) {
     std::string term_str = *iter;
@@ -450,13 +463,18 @@ void RunDownloadQuery(
 
   DownloadQuery query_out;
 
+  size_t limit = 1000;
   if (query_in.limit.get()) {
     if (*query_in.limit.get() < 0) {
       *error = download_extension_errors::kInvalidQueryLimit;
       return;
     }
-    query_out.Limit(*query_in.limit.get());
+    limit = *query_in.limit.get();
   }
+  if (limit > 0) {
+    query_out.Limit(limit);
+  }
+
   std::string state_string =
       extensions::api::downloads::ToString(query_in.state);
   if (!state_string.empty()) {
@@ -513,6 +531,21 @@ void RunDownloadQuery(
   query_out.Search(all_items.begin(), all_items.end(), results);
 }
 
+DownloadPathReservationTracker::FilenameConflictAction ConvertConflictAction(
+    extensions::api::downloads::FilenameConflictAction action) {
+  switch (action) {
+    case extensions::api::downloads::FILENAME_CONFLICT_ACTION_NONE:
+    case extensions::api::downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY:
+      return DownloadPathReservationTracker::UNIQUIFY;
+    case extensions::api::downloads::FILENAME_CONFLICT_ACTION_OVERWRITE:
+      return DownloadPathReservationTracker::OVERWRITE;
+    case extensions::api::downloads::FILENAME_CONFLICT_ACTION_PROMPT:
+      return DownloadPathReservationTracker::PROMPT;
+  }
+  NOTREACHED();
+  return DownloadPathReservationTracker::UNIQUIFY;
+}
+
 class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
  public:
   static ExtensionDownloadsEventRouterData* Get(DownloadItem* download_item) {
@@ -531,6 +564,8 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
       : updated_(0),
         changed_fired_(0),
         json_(json_item.Pass()),
+        creator_conflict_action_(
+            extensions::api::downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY),
         determined_conflict_action_(
             extensions::api::downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY) {
     download_item->SetUserData(kKey, this);
@@ -556,6 +591,10 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
       const ExtensionDownloadsEventRouter::FilenameChangedCallback& change) {
     filename_no_change_ = no_change;
     filename_change_ = change;
+    determined_filename_ = creator_suggested_filename_;
+    determined_conflict_action_ = creator_conflict_action_;
+    // determiner_.install_time should default to 0 so that creator suggestions
+    // should be lower priority than any actual onDeterminingFilename listeners.
   }
 
   void ClearPendingDeterminers() {
@@ -603,6 +642,28 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
     return false;
   }
 
+  void CreatorSuggestedFilename(
+      const base::FilePath& filename,
+      extensions::api::downloads::FilenameConflictAction conflict_action) {
+    creator_suggested_filename_ = filename;
+    creator_conflict_action_ = conflict_action;
+  }
+
+  base::FilePath creator_suggested_filename() const {
+    return creator_suggested_filename_;
+  }
+
+  extensions::api::downloads::FilenameConflictAction
+  creator_conflict_action() const {
+    return creator_conflict_action_;
+  }
+
+  void ResetCreatorSuggestion() {
+    creator_suggested_filename_.clear();
+    creator_conflict_action_ =
+      extensions::api::downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY;
+  }
+
   // Returns false if this |extension_id| was not expected or if this
   // |extension_id| has already reported. The caller is responsible for
   // validating |filename|.
@@ -664,15 +725,8 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
         filename_no_change_.Run();
     } else {
       if (!filename_change_.is_null()) {
-        DownloadPathReservationTracker::FilenameConflictAction conflict_action =
-          DownloadPathReservationTracker::UNIQUIFY;
-        if (determined_conflict_action_ ==
-            extensions::api::downloads::FILENAME_CONFLICT_ACTION_OVERWRITE)
-          conflict_action = DownloadPathReservationTracker::OVERWRITE;
-        if (determined_conflict_action_ ==
-            extensions::api::downloads::FILENAME_CONFLICT_ACTION_PROMPT)
-          conflict_action = DownloadPathReservationTracker::PROMPT;
-        filename_change_.Run(determined_filename_, conflict_action);
+        filename_change_.Run(determined_filename_, ConvertConflictAction(
+            determined_conflict_action_));
       }
     }
     // Don't clear determiners_ immediately in case there's a second listener
@@ -698,6 +752,9 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
 
   DeterminerInfoVector determiners_;
 
+  base::FilePath creator_suggested_filename_;
+  extensions::api::downloads::FilenameConflictAction
+    creator_conflict_action_;
   base::FilePath determined_filename_;
   extensions::api::downloads::FilenameConflictAction
     determined_conflict_action_;
@@ -807,13 +864,15 @@ bool DownloadsDownloadFunction::RunImpl() {
   EXTENSION_FUNCTION_VALIDATE(params.get());
   const extensions::api::downloads::DownloadOptions& options = params->options;
   GURL download_url(options.url);
-  if (!download_url.is_valid() ||
-      (!download_url.SchemeIs("data") &&
-       download_url.GetOrigin() != GetExtension()->url().GetOrigin() &&
-       !extensions::PermissionsData::HasHostPermission(GetExtension(),
-                                                       download_url))) {
+  if (!download_url.is_valid()) {
     error_ = download_extension_errors::kInvalidURLError;
     return false;
+  } else if (!download_url.SchemeIs("data") &&
+             download_url.GetOrigin() != GetExtension()->url().GetOrigin() &&
+             !extensions::PermissionsData::HasHostPermission(GetExtension(),
+                                                             download_url)) {
+    error_ = download_extension_errors::kNotPermittedURLError;
+    return false;
   }
 
   Profile* current_profile = profile();
@@ -827,28 +886,24 @@ bool DownloadsDownloadFunction::RunImpl() {
           render_view_host()->GetRoutingID(),
           current_profile->GetResourceContext()));
 
+  base::FilePath creator_suggested_filename;
   if (options.filename.get()) {
-    // TODO(benjhayden): Make json_schema_compiler generate string16s instead of
-    // std::strings. Can't get filename16 from options.ToValue() because that
-    // converts it from std::string.
+#if defined(OS_WIN)
+    // Can't get filename16 from options.ToValue() because that converts it from
+    // std::string.
     base::DictionaryValue* options_value = NULL;
     EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &options_value));
-    string16 filename16;
+    base::string16 filename16;
     EXTENSION_FUNCTION_VALIDATE(options_value->GetString(
         kFilenameKey, &filename16));
-#if defined(OS_WIN)
-    base::FilePath file_path(filename16);
+    creator_suggested_filename = base::FilePath(filename16);
 #elif defined(OS_POSIX)
-    base::FilePath file_path(*options.filename.get());
+    creator_suggested_filename = base::FilePath(*options.filename.get());
 #endif
-    if (!net::IsSafePortableBasename(file_path) ||
-        (file_path.DirName().value() != base::FilePath::kCurrentDirectory)) {
+    if (!net::IsSafePortableRelativePath(creator_suggested_filename)) {
       error_ = download_extension_errors::kInvalidFilenameError;
       return false;
     }
-    // TODO(benjhayden) Ensure that this filename is interpreted as a path
-    // relative to the default downloads directory without allowing '..'.
-    download_params->set_suggested_name(filename16);
   }
 
   if (options.save_as.get())
@@ -862,7 +917,7 @@ bool DownloadsDownloadFunction::RunImpl() {
          ++iter) {
       const HeaderNameValuePair& name_value = **iter;
       if (!net::HttpUtil::IsSafeHeader(name_value.name)) {
-        error_ = download_extension_errors::kGenericError;
+        error_ = download_extension_errors::kInvalidHeaderError;
         return false;
       }
       download_params->add_request_header(name_value.name, name_value.value);
@@ -876,7 +931,8 @@ bool DownloadsDownloadFunction::RunImpl() {
   if (options.body.get())
     download_params->set_post_body(*options.body.get());
   download_params->set_callback(base::Bind(
-      &DownloadsDownloadFunction::OnStarted, this));
+      &DownloadsDownloadFunction::OnStarted, this,
+      creator_suggested_filename, options.conflict_action));
   // Prevent login prompts for 401/407 responses.
   download_params->set_load_flags(net::LOAD_DO_NOT_PROMPT_FOR_LOGIN);
 
@@ -888,12 +944,26 @@ bool DownloadsDownloadFunction::RunImpl() {
 }
 
 void DownloadsDownloadFunction::OnStarted(
-    DownloadItem* item, net::Error error) {
+    const base::FilePath& creator_suggested_filename,
+    extensions::api::downloads::FilenameConflictAction creator_conflict_action,
+    DownloadItem* item,
+    net::Error error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(1) << __FUNCTION__ << " " << item << " " << error;
   if (item) {
     DCHECK_EQ(net::OK, error);
     SetResult(base::Value::CreateIntegerValue(item->GetId()));
+    if (!creator_suggested_filename.empty()) {
+      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);
+    }
   } else {
     DCHECK_NE(net::OK, error);
     error_ = net::ErrorToString(error);
@@ -1035,6 +1105,53 @@ bool DownloadsEraseFunction::RunImpl() {
   return true;
 }
 
+DownloadsDeleteFileFunction::DownloadsDeleteFileFunction() {}
+
+DownloadsDeleteFileFunction::~DownloadsDeleteFileFunction() {}
+
+bool DownloadsDeleteFileFunction::RunImpl() {
+  scoped_ptr<extensions::api::downloads::DeleteFile::Params> params(
+      extensions::api::downloads::DeleteFile::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  DownloadItem* download_item = GetDownload(
+      profile(), include_incognito(), params->download_id);
+  if (!download_item ||
+      (download_item->GetState() == DownloadItem::IN_PROGRESS) ||
+      download_item->GetFileExternallyRemoved()) {
+    error_ = download_extension_errors::kInvalidOperationError;
+    return false;
+  }
+
+  RecordApiFunctions(DOWNLOADS_FUNCTION_DELETE_FILE);
+
+  DownloadManager* manager = NULL;
+  DownloadManager* incognito_manager = NULL;
+  GetManagers(profile(), include_incognito(), &manager, &incognito_manager);
+  ManagerDestructionObserver::CheckForHistoryFilesRemoval(manager);
+  ManagerDestructionObserver::CheckForHistoryFilesRemoval(incognito_manager);
+
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadsDeleteFileFunction::DeleteOnFileThread, this,
+                 download_item->GetFullPath()),

[asanka, 2013/07/16 19:57:52]

I'd avoid deleting files unless the download is COMPLETE.

If the download is CANCELLED, and there's a file at download->GetFullPath(),
that file doesn't belong to the download.

If the download is INTERRUPTED, it's a bit unclear. For resumption, the
intermediate file is kept around if GetFullPath() is not empty. Deleting this
file isn't fatal and won't prevent a resumption (the download will just restart
from the beginning). Arguably, the correct way to "give up" on an INTERRUPTED
download is to call Cancel(). However deleting the intermediate file could be a
way to free up disk space, while still allowing resumption if desired at a later
point in time. If you want to allow deleting the intermediate file for an
INTERRUPTED download, check if GetFullPath() is empty.

[benjhayden, 2013/07/19 15:53:55]

Done.

+      base::Bind(&DownloadsDeleteFileFunction::RespondOnUIThread, this));
+  return true;
+}
+
+void DownloadsDeleteFileFunction::DeleteOnFileThread(
+    const base::FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (file_util::DirectoryExists(path)) {
+    error_ = download_extension_errors::kInvalidOperationError;
+    return;
+  }
+  base::Delete(path, false);
+}
+
+void DownloadsDeleteFileFunction::RespondOnUIThread() {
+  SendResponse(error_.empty());
+}
+
 DownloadsAcceptDangerFunction::DownloadsAcceptDangerFunction() {}
 
 DownloadsAcceptDangerFunction::~DownloadsAcceptDangerFunction() {}
@@ -1061,20 +1178,29 @@ bool DownloadsAcceptDangerFunction::RunImpl() {
       web_contents,
       true,
       base::Bind(&DownloadsAcceptDangerFunction::DangerPromptCallback,
-                 this, true, params->download_id),
-      base::Bind(&DownloadsAcceptDangerFunction::DangerPromptCallback,
-                 this, false, params->download_id));
+                 this, params->download_id));
   // DownloadDangerPrompt deletes itself
   return true;
 }
 
 void DownloadsAcceptDangerFunction::DangerPromptCallback(
-    bool accept, int download_id) {
-  if (accept) {
-    DownloadItem* download_item = GetDownloadIfInProgress(
-        profile(), include_incognito(), download_id);
-    if (download_item)
+    int download_id, DownloadDangerPrompt::Action action) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadItem* download_item = GetDownloadIfInProgress(
+      profile(), include_incognito(), download_id);
+  if (!download_item) {
+    error_ = download_extension_errors::kInvalidOperationError;
+    return;
+  }
+  switch (action) {
+    case DownloadDangerPrompt::ACCEPT:
       download_item->ValidateDangerousDownload();
+      break;
+    case DownloadDangerPrompt::CANCEL:
+      download_item->Remove();
+      break;
+    case DownloadDangerPrompt::DISMISS:
+      break;
   }
   SendResponse(error_.empty());
 }
@@ -1087,13 +1213,21 @@ bool DownloadsShowFunction::RunImpl() {
   scoped_ptr<extensions::api::downloads::Show::Params> params(
       extensions::api::downloads::Show::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
-  DownloadItem* download_item = GetDownload(
-      profile(), include_incognito(), params->download_id);
-  if (!download_item) {
-    error_ = download_extension_errors::kInvalidOperationError;
-    return false;
+  if (params->download_id) {
+    DownloadItem* download_item = GetDownload(
+        profile(), include_incognito(), *params->download_id);
+    if (!download_item) {
+      error_ = download_extension_errors::kInvalidOperationError;
+      return false;
+    }
+    download_item->ShowDownloadInShell();
+  } else {
+    DownloadManager* manager = NULL;
+    DownloadManager* incognito_manager = NULL;
+    GetManagers(profile(), include_incognito(), &manager, &incognito_manager);
+    platform_util::OpenItem(DownloadPrefs::FromDownloadManager(
+        manager)->DownloadPath());
   }
-  download_item->ShowDownloadInShell();
   RecordApiFunctions(DOWNLOADS_FUNCTION_SHOW);
   return true;
 }
@@ -1146,6 +1280,26 @@ bool DownloadsDragFunction::RunImpl() {
   return true;
 }
 
+DownloadsSetShelfVisibleFunction::DownloadsSetShelfVisibleFunction() {}
+
+DownloadsSetShelfVisibleFunction::~DownloadsSetShelfVisibleFunction() {}
+
+bool DownloadsSetShelfVisibleFunction::RunImpl() {
+  scoped_ptr<extensions::api::downloads::SetShelfVisible::Params> params(
+      extensions::api::downloads::SetShelfVisible::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  DownloadShelf* shelf = GetCurrentBrowser()->window()->GetDownloadShelf();

[asanka, 2013/07/16 19:57:52]

Is it expected that this will operate on the foremost browser if this is called
from a background page? This seems like a function that's specific to a browser
window.

[benjhayden, 2013/07/19 15:53:55]

Yep, it will probably primarily be called from background event pages, which are
often the only javascript context that an extension has.

Should it take a window id from the chrome.windows API? Or a flag to operate on
all windows for the profile versus the current window?

[asanka, 2013/07/23 21:13:48]

The browser window for a download is determined by the WebContents that
initiated the download. So if an extension were to listen for an onCreate and
wants to hide the shelf, then it would have to wait for the associated browser
window to come into the foreground so that it can hide the shelf.

Even then, hiding the shelf only works if the shelf is visible at the time the
extension API call makes it to the browser process. The visibility of the shelf
is a bit complicated since we delay opening it for downloads that are temporary
or are marked for opening automatically. To test this, you can start a slow
download of a type that you've set to auto-open.

In the browser process, calling DownloadItemModel.SetShouldShowInShelf() during
an OnStarted callback for a download prevents it from showing up in the shelf.
However, this doesn't scale to extensions since the roundtrip to the extension
process could be too late.

I'm not sure what a reliable way to implement this is short of having an API
that disables all future downloads from showing up on the shelf and also closing
any shelf that's visible.

[asargent_no_longer_on_chrome, 2013/07/23 21:36:57]

Note that chrome can be running but with no current browser windows (eg on Mac,
or win/linux if you have extensions/apps with the "background" permission
installed). Also the comments above ExtensionFunction::GetCurrentBrowser mention
that during parts of startup/shutdown the result can be null. Anyhow, you should
check for null return from GetCurrentBrowser here. :)

+  if (params->visible) {
+    if (!shelf->IsShowing())
+      shelf->Show();

[asanka, 2013/07/16 19:57:52]

What's supposed to happen if there are no downloads on the shelf?
Hide()/Unhide() will take the intended visual state of the shelf into
consideration.

[benjhayden, 2013/07/19 15:53:55]

Done.

+  } else {
+    if (shelf->IsShowing())
+      shelf->Close(DownloadShelf::USER_ACTION);
+  }
+  RecordApiFunctions(DOWNLOADS_FUNCTION_SET_SHELF_VISIBLE);
+  return true;
+}
+
 DownloadsGetFileIconFunction::DownloadsGetFileIconFunction()
     : icon_extractor_(new DownloadFileIconExtractorImpl()) {
 }
@@ -1268,7 +1422,14 @@ void ExtensionDownloadsEventRouter::OnDeterminingFilename(
                 json);
   if (!any_determiners) {
     data->ClearPendingDeterminers();
-    no_change.Run();
+    if (!data->creator_suggested_filename().empty()) {
+      change.Run(data->creator_suggested_filename(),
+                 ConvertConflictAction(data->creator_conflict_action()));
+      // If all listeners are removed, don't keep |data| around.
+      data->ResetCreatorSuggestion();
+    } else {
+      no_change.Run();
+    }
   }
 }
 
@@ -1358,7 +1519,8 @@ void ExtensionDownloadsEventRouter::OnListenerRemoved(
       // should proceed.
       data->DeterminerRemoved(details.extension_id);
     }
-    if (!any_listeners) {
+    if (!any_listeners &&
+        data->creator_suggested_filename().empty()) {
       ExtensionDownloadsEventRouterData::Remove(*iter);
     }
   }
@@ -1428,7 +1590,9 @@ void ExtensionDownloadsEventRouter::OnDownloadUpdated(
   for (base::DictionaryValue::Iterator iter(*new_json.get());
        !iter.IsAtEnd(); iter.Advance()) {
     new_fields.insert(iter.key());
-    if (iter.key() != kBytesReceivedKey) {
+    if ((iter.key() != kBytesReceivedKey) &&
+        (iter.key() != kIdKey) &&
+        (iter.key() != kEstimatedEndTimeKey)) {
       const base::Value* old_value = NULL;
       if (!data->json().HasKey(iter.key()) ||
           (data->json().Get(iter.key(), &old_value) &&
@@ -1445,7 +1609,9 @@ void ExtensionDownloadsEventRouter::OnDownloadUpdated(
   // difference in |delta|.
   for (base::DictionaryValue::Iterator iter(data->json());
        !iter.IsAtEnd(); iter.Advance()) {
-    if (new_fields.find(iter.key()) == new_fields.end()) {
+    if ((new_fields.find(iter.key()) == new_fields.end()) &&
+        (iter.key() != kEstimatedEndTimeKey)) {

[asanka, 2013/07/16 19:57:52]

Why exclude kEstimatedEndTimeKey? It can also disappear if the transfer drops
for a substantial amount of time  (~10s currently) or if the download is paused.

[benjhayden, 2013/07/19 15:53:55]

estimatedEndTime is not in DownloadDelta because it is as volatile as
bytesReceived.
DownloadDelta is restricted to less-volatile fields in order to reduce IPCs.

+      // estimatedEndTime disappears after completion, but bytesReceived stays.
       delta->Set(iter.key() + ".previous", iter.value().DeepCopy());
       changed = true;
     }