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 m
benjhayden
2013/01/11 21:21:27
This is the method that runs when any onDeterminin
|
+ 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 WebR
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 Ext
benjhayden
2013/01/11 21:21:27
Oh, I was just going to make a note in downloads.i
vabr (Chromium)
2013/01/15 08:19:22
The analogy in WebRequest is, e.g., when two exten
battre
2013/01/15 09:00:40
Please don't add a warning badge for this. I think
|
+// 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 "=", n
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( |