Implement chrome.downloads.onDeterminingFilename()

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 92d09b2b8abe8916cd9a9f3b35f9f82db70ad575..7e4bef85447f3b4f4ba168b049f353c5c07bd2ea 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_path.h"
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ -180,6 +181,8 @@ DownloadItem::DownloadState StateEnumFromString(const std::string& state) {
 
 bool ValidateFilename(const string16& filename) {
   // TODO(benjhayden): More robust validation of filename.
+  if (filename.size() < 1)

[battre, 2013/01/11 14:29:37]

nit: why not
if (filename.empty())
?

[benjhayden, 2013/01/11 21:21:27]

Done.

+    return false;
   if ((filename.find('/') != string16::npos) ||
       (filename.find('\\') != string16::npos))
     return false;
@@ -256,8 +259,7 @@ bool DownloadFileIconExtractorImpl::ExtractIconURLForPath(
   IconManager* im = g_browser_process->icon_manager();
   // The contents of the file at |path| may have changed since a previous
   // request, in which case the associated icon may also have changed.
-  // Therefore, for the moment we always call LoadIcon instead of attempting
-  // a LookupIcon.
+  // Therefore, always call LoadIcon instead of attempting a LookupIcon.
   im->LoadIcon(path,
                icon_size,
                base::Bind(&DownloadFileIconExtractorImpl::OnIconLoadComplete,
@@ -536,7 +538,8 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
       scoped_ptr<base::DictionaryValue> json_item)
       : updated_(0),
         changed_fired_(0),
-        json_(json_item.Pass()) {
+        json_(json_item.Pass()),
+        determined_overwrite_(false) {
     download_item->SetUserData(kKey, this);
   }
 
@@ -555,12 +558,52 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
   void OnItemUpdated() { ++updated_; }
   void OnChangedFired() { ++changed_fired_; }
 
+  void set_filename_change_callbacks(
+      const base::Closure& no_change,
+      const ExtensionDownloadsEventRouter::FilenameChangedCallback& change) {
+    filename_no_change_ = no_change;
+    filename_change_ = change;
+  }
+
+  void AddDeterminer(const std::string& determiner_id) {

[battre, 2013/01/11 14:29:37]

How about "AddPendingDeterminer"?

[benjhayden, 2013/01/11 21:21:27]

Done.

+    DCHECK(determiners_.insert(determiner_id).second);
+  }
+
+  bool DetermineFilename(

[battre, 2013/01/11 14:29:37]

How about "DeterminerCallback"

[benjhayden, 2013/01/11 21:21:27]

Done.

+      const std::string& determiner_id,
+      const FilePath& filename,
+      bool overwrite) {
+    DeterminerSet::iterator iter = determiners_.find(determiner_id);
+    if (iter == determiners_.end())
+      return false;
+    determiners_.erase(iter);

[battre, 2013/01/11 14:29:37]

This looks strange. DetermineFilename should not modify the object |this| based
on my judgement of the filename. As mentioned in download_api_internal.cc I
think that "DetermineFilename" is not a good name.

[benjhayden, 2013/01/11 21:21:27]

This is the method that runs when any onDeterminingFilename listener returns.
When called in response to the *last* listener returning, it needs to call
either filename_no_change_ or filename_change_.
I changed this method to detect the last listener using a flag for each
listener.
Is there a better way to detect when the last listener has returned?

+    if (!filename.empty() && !filename.IsAbsolute()) {
+      determined_filename_ = filename;
+      determined_overwrite_ = overwrite;
+    }
+    if (determiners_.empty()) {
+      if (determined_filename_.empty()) {
+        filename_no_change_.Run();
+      } else {
+        filename_change_.Run(determined_filename_, determined_overwrite_);
+      }
+    }

[battre, 2013/01/11 14:29:37]

This is "Last callback wins" semantic. In the WebRequest API we tried to be sure
to be deterministic by enforcing that the last installed extension that wants to
modify the request takes precedence.

[benjhayden, 2013/01/11 21:21:27]

Done.

+    return true;
+  }
+
  private:
+  typedef std::set<std::string> DeterminerSet;
+
   static const char kKey[];
 
   int updated_;
   int changed_fired_;
   scoped_ptr<base::DictionaryValue> json_;
+  base::Closure filename_no_change_;
+  ExtensionDownloadsEventRouter::FilenameChangedCallback filename_change_;
+  FilePath determined_filename_;
+  bool determined_overwrite_;
+  DeterminerSet determiners_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionDownloadsEventRouterData);
 };
@@ -739,7 +782,7 @@ bool DownloadsResumeFunction::RunImpl() {
     error_ = download_extension_errors::kInvalidOperationError;
   } else {
     // Note that if the item isn't paused, this will be a no-op, and
-    // we will silently treat the extension call as a success.
+    // the extension call will seem successful.
     download_item->Resume();
   }
   if (error_.empty())
@@ -759,8 +802,7 @@ bool DownloadsCancelFunction::RunImpl() {
   if (download_item != NULL)
     download_item->Cancel(true);
   // |download_item| can be NULL if the download ID was invalid or if the
-  // download is not currently active.  Either way, we don't consider it a
-  // failure.
+  // download is not currently active.  Either way, it's not a failure.
   RecordApiFunctions(DOWNLOADS_FUNCTION_CANCEL);
   return true;
 }
@@ -794,19 +836,6 @@ bool DownloadsEraseFunction::RunImpl() {
   return true;
 }
 
-DownloadsSetDestinationFunction::DownloadsSetDestinationFunction() {}
-DownloadsSetDestinationFunction::~DownloadsSetDestinationFunction() {}
-
-bool DownloadsSetDestinationFunction::RunImpl() {
-  scoped_ptr<extensions::api::downloads::SetDestination::Params> params(
-      extensions::api::downloads::SetDestination::Params::Create(*args_));
-  EXTENSION_FUNCTION_VALIDATE(params.get());
-  error_ = download_extension_errors::kNotImplementedError;
-  if (error_.empty())
-    RecordApiFunctions(DOWNLOADS_FUNCTION_SET_DESTINATION);
-  return error_.empty();
-}
-
 DownloadsAcceptDangerFunction::DownloadsAcceptDangerFunction() {}
 DownloadsAcceptDangerFunction::~DownloadsAcceptDangerFunction() {}
 
@@ -911,8 +940,8 @@ bool DownloadsGetFileIconFunction::RunImpl() {
     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.
+  // which doesn't have the final extension. Therefore a good file icon can't be
+  // found, so use GetTargetFilePath() instead.
   DCHECK(icon_extractor_.get());
   DCHECK(icon_size == 16 || icon_size == 32);
   EXTENSION_FUNCTION_VALIDATE(icon_extractor_->ExtractIconURLForPath(
@@ -933,6 +962,94 @@ void DownloadsGetFileIconFunction::OnIconURLExtracted(const std::string& url) {
   SendResponse(error_.empty());
 }
 
+// The method by which extensions hook into the filename determination process
+// is based on the method by which the webRequest API allows extensions to hook
+// into the resource loading process.  Extensions that wish to play a part in
+// the filename determination process call
+// chrome.downloads.onDeterminingFilename.addListener, which secretly (see
+// chrome/renderer/resources/extensions/downloads_custom_bindings.js) calls
+// DownloadsInternalAddFilenameDeterminerFunction (see ../downloads_internal/),
+// which adds the extension's ID to the profile's ExtensionDownloadsEventRouter
+// (EDER).  When a download's filename is being determined,
+// ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone (CVRBD) passes
+// 2 callbacks to EDER::OnDownloadFilenameDetermined (ODFD), which stores the
+// callbacks in the item's ExtensionDownloadsEventRouterData (EDERD) along with
+// all of the extension IDs that are listening for onDeterminingFilename events.
+// ODFD dispatches chrome.downloads.onDeterminingFilename. When the extension's
+// event handler returns, downloads_custom_bindings.js calls
+// DownloadsInternalDetermineFilenameFunction::RunImpl, which notifies the
+// item's EDERD.  When the last extension's event handler returns, EDERD calls
+// one of the two callbacks that CVRBD passed to ODFD, allowing CDMD to complete
+// the filename determination process. If multiple extensions wish to override
+// the filename, then the last one wins. However, there's no deterministic way
+// to determine which extension will finish last, so users are advised to try to

[vabr (Chromium), 2013/01/11 12:43:40]

How precisely are the users advised? Using the ExtensionWarning class?

[benjhayden, 2013/01/11 21:21:27]

Oh, I was just going to make a note in downloads.idl, the developer
documentation.
ExtensionWarning says it puts a badge on the hotdog menu. I don't know if this
situation is severe enough to bug users like that.

[vabr (Chromium), 2013/01/15 08:19:22]

The analogy in WebRequest is, e.g., when two extension give conflicting redirect
instructions (see
https://cs.corp.google.com/#chrome/src/chrome/browser/extensions/api/web_requ...).
There the ExtensionWarning is used, and I think that might justify using it for
clashing download renaming. From the user's point of view, the results of two
conflicting extensions doing download renaming might be rather confusing, and we
can't really expect them to read the developers' docs.
Also, if it was fairly improbable for two extensions to clash on download
renaming, then that would be one more reason to use ExtensionWarning and not to
worry about its severity.

However, I don't feel strongly about this. If Dominic, or another reviewer
knowledgeable in extensions, is OK with emitting no warning, then I'm fine with
that as well.

[battre, 2013/01/15 09:00:40]

Please don't add a warning badge for this. I think that is too intrusive. We
introduced the warning badge because the WebRequest API commands can be critical
for privacy/security reasons (e.g. deleting headers that disguise your
identity).

+// use extensions that won't conflict.
+
+bool ExtensionDownloadsEventRouter::AddFilenameDeterminer(
+    Profile* profile,
+    bool include_incognito,
+    const std::string& ext_id,
+    const std::string& determiner_id) {
+  if (include_incognito && profile->HasOffTheRecordProfile()) {
+    AddFilenameDeterminer(profile->GetOffTheRecordProfile(),
+                          false,
+                          ext_id,
+                          determiner_id);
+  }
+  ExtensionDownloadsEventRouter* router =
+    DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter();

[vabr (Chromium), 2013/01/11 12:43:40]

+2 spaces of indentation

[benjhayden, 2013/01/11 21:21:27]

Done.

+  return router->filename_determiners_[determiner_id].insert(ext_id).second;
+}
+
+bool ExtensionDownloadsEventRouter::RemoveFilenameDeterminer(
+    Profile* profile,
+    bool include_incognito,
+    const std::string& ext_id,
+    const std::string& determiner_id) {
+  if (include_incognito && profile->HasOffTheRecordProfile()) {
+    RemoveFilenameDeterminer(profile->GetOffTheRecordProfile(),
+                             false,
+                             ext_id,
+                             determiner_id);
+  }
+  ExtensionDownloadsEventRouter* router =
+    DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter();

[vabr (Chromium), 2013/01/11 12:43:40]

+2 spaces of indentation

[benjhayden, 2013/01/11 21:21:27]

Done.

+  DeterminerMap::iterator determiner_entry(
+      router->filename_determiners_.find(determiner_id));
+  DCHECK(determiner_entry != router->filename_determiners_.end());
+  if (determiner_entry == router->filename_determiners_.end())
+    return false;
+  ExtensionIdSet::iterator extension_entry(
+      router->filename_determiners_[determiner_id].find(ext_id));
+  DCHECK(extension_entry != router->filename_determiners_[determiner_id].end());
+  if (extension_entry == router->filename_determiners_[determiner_id].end())
+    return false;
+  router->filename_determiners_[determiner_id].erase(extension_entry);
+  if (router->filename_determiners_[determiner_id].empty())
+    router->filename_determiners_.erase(determiner_entry);
+  return true;
+}
+
+bool ExtensionDownloadsEventRouter::DetermineFilename(
+    Profile* profile,
+    bool include_incognito,
+    const std::string& ext_id,
+    const std::string& determiner_id,
+    int download_id,
+    const FilePath& filename,
+    bool overwrite) {
+  DownloadItem* item = GetDownloadIfInProgress(
+      profile, include_incognito, download_id);
+  if (!item)
+    return false;
+  ExtensionDownloadsEventRouterData* data =
+    ExtensionDownloadsEventRouterData::Get(item);

[vabr (Chromium), 2013/01/11 12:43:40]

+2 spaces of indentation

[benjhayden, 2013/01/11 21:21:27]

Done.

+  if (!data)
+    return false;
+  return data->DetermineFilename(
+      ext_id + "-" + determiner_id, filename, overwrite);
+}
+
 ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(
     Profile* profile,
     DownloadManager* manager)
@@ -940,6 +1057,45 @@ ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(
       ALLOW_THIS_IN_INITIALIZER_LIST(notifier_(manager, this)) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile_);
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
+                 content::Source<Profile>(profile_));
+}
+
+void ExtensionDownloadsEventRouter::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_UNLOADED: {
+      const extensions::Extension* extension = content::Details<
+        extensions::UnloadedExtensionInfo>(details)->extension;

[vabr (Chromium), 2013/01/11 12:43:40]

+2 spaces of indent

[benjhayden, 2013/01/11 21:21:27]

Done.

+      std::set<std::string> empty_determiners;
+      for (DeterminerMap::iterator
+           determiner_iter = filename_determiners_.begin();
+           determiner_iter != filename_determiners_.end();
+           ++determiner_iter) {
+        ExtensionIdSet::iterator found(determiner_iter->second.find(
+            extension->id()));
+        if (found != determiner_iter->second.end()) {
+          determiner_iter->second.erase(found);
+          if (determiner_iter->second.empty())
+            empty_determiners.insert(determiner_iter->first);
+        }
+      }
+      for (std::set<std::string>::const_iterator
+           iter = empty_determiners.begin();

[vabr (Chromium), 2013/01/11 12:43:40]

+4 spaces of indent
nit: please break after "=", not before "iter"

[benjhayden, 2013/01/11 21:21:27]

Done.

+           iter != empty_determiners.end();
+           ++iter) {
+        DeterminerMap::iterator found = filename_determiners_.find(*iter);
+        CHECK(found != filename_determiners_.end());
+        filename_determiners_.erase(found);
+      }
+      break;
+    }
+    default:
+      NOTREACHED();
+      break;
+  }
 }
 
 ExtensionDownloadsEventRouter::~ExtensionDownloadsEventRouter() {
@@ -960,6 +1116,44 @@ void ExtensionDownloadsEventRouter::OnDownloadCreated(
     new ExtensionDownloadsEventRouterData(download_item, json_item.Pass());
 }
 
+void ExtensionDownloadsEventRouter::OnDownloadFilenameDetermined(
+    DownloadItem* item,
+    const FilePath& suggested_path,
+    const base::Closure& no_change,
+    const FilenameChangedCallback& change) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (filename_determiners_.empty()) {
+    no_change.Run();
+    return;
+  }
+
+  ExtensionDownloadsEventRouterData* data =
+    ExtensionDownloadsEventRouterData::Get(item);

[vabr (Chromium), 2013/01/11 12:43:40]

+2 spaces of indent

[benjhayden, 2013/01/11 21:21:27]

Done.

+  data->set_filename_change_callbacks(no_change, change);
+  std::string event_name(
+      extensions::event_names::kOnDownloadDeterminingFilename);
+  event_name += "/";
+  for (DeterminerMap::const_iterator
+       determiner_iter = filename_determiners_.begin();

[vabr (Chromium), 2013/01/11 12:43:40]

+4 spaces
nit: please break after "="

[benjhayden, 2013/01/11 21:21:27]

Done.

+       determiner_iter != filename_determiners_.end();
+       ++determiner_iter) {
+    const std::string& determiner_id = determiner_iter->first;
+    for (ExtensionIdSet::const_iterator
+         ext_iter = determiner_iter->second.begin();
+         ext_iter != determiner_iter->second.end();
+         ++ext_iter) {
+      data->AddDeterminer((*ext_iter) + "-" + determiner_id);
+    }
+    base::DictionaryValue* json = DownloadItemToJSON(
+        item, profile_->IsOffTheRecord()).release();
+    json->SetString(
+        kFilenameKey, suggested_path.LossyDisplayName());
+    DispatchEvent(
+        (event_name + determiner_id).c_str(),
+        json);
+  }
+}
+
 void ExtensionDownloadsEventRouter::OnDownloadUpdated(
     DownloadManager* manager, DownloadItem* download_item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -1039,13 +1233,13 @@ void ExtensionDownloadsEventRouter::DispatchEvent(
   // There is a one EDER for each on-record Profile, and a separate EDER for
   // each off-record Profile, so there is exactly one EDER for each
   // DownloadManager.  EDER only watches its own DM, so all the items that an
-  // EDER sees are either all on-record or all off-record. However, we want
-  // extensions in off-record contexts to see on-record items. So, if this EDER
-  // is watching an on-record DM, and there is a corresponding off-record
-  // Profile, then dispatch this event to both the on-record Profile and the
-  // off-record Profile.  There may or may not be an off-record Profile, so send
-  // a *copy* of |args| to the off-record Profile, and Pass() |args|
-  // to the Profile that we know is there.
+  // EDER sees are either all on-record or all off-record. However, extensions
+  // in off-record contexts should see on-record items. So, if this EDER is
+  // watching an on-record DM, and there is a corresponding off-record Profile,
+  // then dispatch this event to both the on-record Profile and the off-record
+  // Profile.  There may or may not be an off-record Profile, so send a *copy*
+  // of |args| to the off-record Profile, and Pass() |args| to the Profile that
+  // is certainly there.
   if (profile_->HasOffTheRecordProfile() &&
       !profile_->IsOffTheRecord()) {
     DispatchEventInternal(