diff options
author | mtomasz@chromium.org <mtomasz@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-11 02:03:27 +0000 |
---|---|---|
committer | mtomasz@chromium.org <mtomasz@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-11 02:03:27 +0000 |
commit | c95b60ffe59f1e5427b5359581766033436b8597 (patch) | |
tree | 887875f4dc1636e4e9d02efef3d4787df48898db | |
parent | 82d853071581f40c48f22c99e5819ab2cfae05c0 (diff) | |
download | chromium_src-c95b60ffe59f1e5427b5359581766033436b8597.zip chromium_src-c95b60ffe59f1e5427b5359581766033436b8597.tar.gz chromium_src-c95b60ffe59f1e5427b5359581766033436b8597.tar.bz2 |
Merge 202384 "Replace sets with vectors when storing file handlers."
> Replace sets with vectors when storing file handlers.
>
> This caused undeterministic behavior in setting the default task, especially on ASAN bots. Moreover, it sounds reasonable to use order of handlers as priorities.
>
> TEST=browser_tests on Linux ChromeOS ASAN pass.
> BUG=243611, 242615
>
> Review URL: https://chromiumcodereview.appspot.com/15975004
TBR=mtomasz@chromium.org
Review URL: https://codereview.chromium.org/16024012
git-svn-id: svn://svn.chromium.org/chrome/branches/1500/src@205380 0039d316-1c4b-4281-b951-d872f2087c98
5 files changed, 98 insertions, 123 deletions
diff --git a/chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc b/chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc index 9a0175d..509c120 100644 --- a/chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc +++ b/chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc @@ -1017,8 +1017,8 @@ bool GetFileTasksFileBrowserFunction::RunImpl() { // the extension tasks to the Drive task list. We know there aren't duplicates // because they're entirely different kinds of tasks, but there could be both // kinds of tasks for a file type (an image file, for instance). - std::set<const FileBrowserHandler*> common_tasks; - std::set<const FileBrowserHandler*> default_tasks; + file_handler_util::FileBrowserHandlerList common_tasks; + file_handler_util::FileBrowserHandlerList default_tasks; if (!file_handler_util::FindCommonTasks(profile_, file_urls, &common_tasks)) return false; file_handler_util::FindDefaultTasks(profile_, file_paths, @@ -1026,7 +1026,7 @@ bool GetFileTasksFileBrowserFunction::RunImpl() { ExtensionService* service = extensions::ExtensionSystem::Get(profile_)->extension_service(); - for (std::set<const FileBrowserHandler*>::const_iterator iter = + for (file_handler_util::FileBrowserHandlerList::const_iterator iter = common_tasks.begin(); iter != common_tasks.end(); ++iter) { @@ -1050,7 +1050,8 @@ bool GetFileTasksFileBrowserFunction::RunImpl() { // Only set the default if there isn't already a default set. if (!default_already_set && - default_tasks.find(*iter) != default_tasks.end()) { + std::find(default_tasks.begin(), default_tasks.end(), *iter) != + default_tasks.end()) { task->SetBoolean("isDefault", true); default_already_set = true; } else { diff --git a/chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc b/chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc index b4d4121..f83f918 100644 --- a/chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc +++ b/chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc @@ -65,8 +65,6 @@ const char kDriveTaskExtensionPrefix[] = "drive-app:"; const size_t kDriveTaskExtensionPrefixLength = arraysize(kDriveTaskExtensionPrefix) - 1; -typedef std::set<const FileBrowserHandler*> FileBrowserHandlerSet; - const int kReadWriteFilePermissions = base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_CREATE | base::PLATFORM_FILE_OPEN_ALWAYS | @@ -102,7 +100,9 @@ int ExtractProcessFromExtensionId(Profile* profile, return process->GetID(); } -bool IsBuiltinTask(const FileBrowserHandler* task) { +// Returns true if the task should be used as a fallback. Such tasks are +// Files.app's internal handlers as well as quick office extensions. +bool IsFallbackTask(const FileBrowserHandler* task) { return (task->extension_id() == kFileBrowserExtensionId || task->extension_id() == extension_misc::kQuickOfficeComponentExtensionId || @@ -110,18 +110,6 @@ bool IsBuiltinTask(const FileBrowserHandler* task) { task->extension_id() == extension_misc::kQuickOfficeExtensionId); } -bool MatchesAllURLs(const FileBrowserHandler* handler) { - const std::set<URLPattern>& patterns = - handler->file_url_patterns().patterns(); - for (std::set<URLPattern>::const_iterator it = patterns.begin(); - it != patterns.end(); - ++it) { - if (it->match_all_urls()) - return true; - } - return false; -} - const FileBrowserHandler* FindFileBrowserHandler(const Extension* extension, const std::string& action_id) { FileBrowserHandler::List* handler_list = @@ -162,7 +150,7 @@ std::string EscapedUtf8ToLower(const std::string& str) { bool GetFileBrowserHandlers(Profile* profile, const GURL& selected_file_url, - FileBrowserHandlerSet* results) { + FileBrowserHandlerList* results) { ExtensionService* service = extensions::ExtensionSystem::Get(profile)->extension_service(); if (!service) @@ -192,7 +180,7 @@ bool GetFileBrowserHandlers(Profile* profile, if (!action->MatchesURL(lowercase_url)) continue; - results->insert(action_iter->get()); + results->push_back(action_iter->get()); } } return true; @@ -332,15 +320,15 @@ bool CrackTaskID(const std::string& task_id, } // Find a specific handler in the handler list. -FileBrowserHandlerSet::iterator FindHandler( - FileBrowserHandlerSet* handler_set, +FileBrowserHandlerList::iterator FindHandler( + FileBrowserHandlerList* handler_list, const std::string& extension_id, const std::string& id) { - FileBrowserHandlerSet::iterator iter = handler_set->begin(); - while (iter != handler_set->end() && + FileBrowserHandlerList::iterator iter = handler_list->begin(); + while (iter != handler_list->end() && !((*iter)->extension_id() == extension_id && (*iter)->id() == id)) { - iter++; + ++iter; } return iter; } @@ -349,8 +337,8 @@ FileBrowserHandlerSet::iterator FindHandler( // that are shared between them. void FindDefaultTasks(Profile* profile, const std::vector<base::FilePath>& files_list, - const FileBrowserHandlerSet& common_tasks, - FileBrowserHandlerSet* default_tasks) { + const FileBrowserHandlerList& common_tasks, + FileBrowserHandlerList* default_tasks) { DCHECK(default_tasks); default_tasks->clear(); @@ -363,46 +351,42 @@ void FindDefaultTasks(Profile* profile, default_ids.insert(task_id); } - const FileBrowserHandler* builtin_task = NULL; + const FileBrowserHandler* fallback_task = NULL; // Convert the default task IDs collected above to one of the handler pointers // from common_tasks. - for (FileBrowserHandlerSet::const_iterator task_iter = common_tasks.begin(); + for (FileBrowserHandlerList::const_iterator task_iter = common_tasks.begin(); task_iter != common_tasks.end(); ++task_iter) { std::string task_id = MakeTaskID((*task_iter)->extension_id(), kTaskFile, (*task_iter)->id()); std::set<std::string>::iterator default_iter = default_ids.find(task_id); if (default_iter != default_ids.end()) { - default_tasks->insert(*task_iter); + default_tasks->push_back(*task_iter); continue; } - // If it's a built in task, remember it. If there are no default tasks among - // common tasks, builtin task will be used as a fallback. - // Note that builtin tasks are not overlapping, so there can be at most one - // builtin tasks for each set of files. - if (IsBuiltinTask(*task_iter)) - builtin_task = *task_iter; + // Remember the first fallback task. + if (!fallback_task && IsFallbackTask(*task_iter)) + fallback_task = *task_iter; } - // If there are no default tasks found, use builtin task (if found) as a - // default. - if (builtin_task && default_tasks->empty()) - default_tasks->insert(builtin_task); + // If there are no default tasks found, use fallback as default. + if (fallback_task && default_tasks->empty()) + default_tasks->push_back(fallback_task); } // Given the list of selected files, returns array of context menu tasks // that are shared bool FindCommonTasks(Profile* profile, const std::vector<GURL>& files_list, - FileBrowserHandlerSet* common_tasks) { + FileBrowserHandlerList* common_tasks) { DCHECK(common_tasks); common_tasks->clear(); - FileBrowserHandlerSet common_task_set; + FileBrowserHandlerList common_task_list; std::set<std::string> default_task_ids; for (std::vector<GURL>::const_iterator it = files_list.begin(); it != files_list.end(); ++it) { - FileBrowserHandlerSet file_actions; + FileBrowserHandlerList file_actions; if (!GetFileBrowserHandlers(profile, *it, &file_actions)) return false; // If there is nothing to do for one file, the intersection of tasks for all @@ -412,38 +396,37 @@ bool FindCommonTasks(Profile* profile, // For the very first file, just copy all the elements. if (it == files_list.begin()) { - common_task_set = file_actions; + common_task_list = file_actions; } else { // For all additional files, find intersection between the accumulated and // file specific set. - FileBrowserHandlerSet intersection; - std::set_intersection(common_task_set.begin(), common_task_set.end(), + FileBrowserHandlerList intersection; + std::set_intersection(common_task_list.begin(), common_task_list.end(), file_actions.begin(), file_actions.end(), - std::inserter(intersection, - intersection.begin())); - common_task_set = intersection; - if (common_task_set.empty()) + std::back_inserter(intersection)); + common_task_list = intersection; + if (common_task_list.empty()) return true; } } - FileBrowserHandlerSet::iterator watch_iter = FindHandler( - &common_task_set, kFileBrowserDomain, kFileBrowserWatchTaskId); - FileBrowserHandlerSet::iterator gallery_iter = FindHandler( - &common_task_set, kFileBrowserDomain, kFileBrowserGalleryTaskId); - if (watch_iter != common_task_set.end() && - gallery_iter != common_task_set.end()) { + FileBrowserHandlerList::iterator watch_iter = FindHandler( + &common_task_list, kFileBrowserDomain, kFileBrowserWatchTaskId); + FileBrowserHandlerList::iterator gallery_iter = FindHandler( + &common_task_list, kFileBrowserDomain, kFileBrowserGalleryTaskId); + if (watch_iter != common_task_list.end() && + gallery_iter != common_task_list.end()) { // Both "watch" and "gallery" actions are applicable which means that the // selection is all videos. Showing them both is confusing, so we only keep // the one that makes more sense ("watch" for single selection, "gallery" // for multiple selection). if (files_list.size() == 1) - common_task_set.erase(gallery_iter); + common_task_list.erase(gallery_iter); else - common_task_set.erase(watch_iter); + common_task_list.erase(watch_iter); } - common_tasks->swap(common_task_set); + common_tasks->swap(common_task_list); return true; } @@ -454,8 +437,8 @@ bool GetTaskForURLAndPath(Profile* profile, std::vector<GURL> file_urls; file_urls.push_back(url); - FileBrowserHandlerSet default_tasks; - FileBrowserHandlerSet common_tasks; + FileBrowserHandlerList default_tasks; + FileBrowserHandlerList common_tasks; if (!FindCommonTasks(profile, file_urls, &common_tasks)) return false; diff --git a/chrome/browser/chromeos/extensions/file_manager/file_handler_util.h b/chrome/browser/chromeos/extensions/file_manager/file_handler_util.h index dbab083..beb0a3f 100644 --- a/chrome/browser/chromeos/extensions/file_manager/file_handler_util.h +++ b/chrome/browser/chromeos/extensions/file_manager/file_handler_util.h @@ -28,6 +28,9 @@ class FileSystemURL; namespace file_handler_util { +// Tasks are stored as a vector in order of priorities. +typedef std::vector<const FileBrowserHandler*> FileBrowserHandlerList; + // Specifies the task type for a task id that represents some file action, Drive // action, or Web Intent action. extern const char kTaskFile[]; @@ -74,13 +77,13 @@ bool CrackTaskID(const std::string& task_id, // prefs) from the |common_tasks|. void FindDefaultTasks(Profile* profile, const std::vector<base::FilePath>& files_list, - const std::set<const FileBrowserHandler*>& common_tasks, - std::set<const FileBrowserHandler*>* default_tasks); + const FileBrowserHandlerList& common_tasks, + FileBrowserHandlerList* default_tasks); // This generates list of tasks common for all files in |file_list|. bool FindCommonTasks(Profile* profile, const std::vector<GURL>& files_list, - std::set<const FileBrowserHandler*>* common_tasks); + FileBrowserHandlerList* common_tasks); // Finds a task for a file whose URL is |url| and whose path is |path|. // Returns default task if one is defined (The default task is the task that is diff --git a/chrome/browser/resources/file_manager/manifest.json b/chrome/browser/resources/file_manager/manifest.json index 49bde4f..190a9d1 100644 --- a/chrome/browser/resources/file_manager/manifest.json +++ b/chrome/browser/resources/file_manager/manifest.json @@ -31,35 +31,6 @@ "https://drive.google.com/" ], "file_browser_handlers": [ - // Automatically opens a volume and later close Files.app when unmounted. - // TODO(mtomasz): Implement a better filtering than using file_filters. - { - "id": "auto-open", - "default_title": "__MSG_OPEN_ACTION__", - "default_icon": "images/filetype_generic.png", - "file_filters": [ - "filesystem:*" - ] - }, - // Selects the passed file after launching Files.app. - { - "id": "select", - "default_title": "__MSG_OPEN_ACTION__", - "default_icon": "images/filetype_generic.png", - "file_filters": [ - "filesystem:*" - ] - }, - // Opens the passed directory after launching Files.app. - // TODO(mtomasz): Implement a directories filtering instead of files. - { - "id": "open", - "default_title": "__MSG_OPEN_ACTION__", - "default_icon": "images/filetype_generic.png", - "file_filters": [ - "filesystem:*" - ] - }, { "id": "play", "default_title": "__MSG_PLAY_MEDIA__", @@ -207,6 +178,29 @@ "file_filters": [ "filesystem:*.gslides" ] + }, + // The following handlers are used only internally, therefore they do not + // have any file filter. + // Automatically opens a volume and later close Files.app when unmounted. + { + "id": "auto-open", + "default_title": "__MSG_OPEN_ACTION__", + "default_icon": "images/filetype_generic.png", + "file_filters": [] + }, + // Selects the passed file after launching Files.app. + { + "id": "select", + "default_title": "__MSG_OPEN_ACTION__", + "default_icon": "images/filetype_generic.png", + "file_filters": [] + }, + // Opens the passed directory after launching Files.app. + { + "id": "open", + "default_title": "__MSG_OPEN_ACTION__", + "default_icon": "images/filetype_generic.png", + "file_filters": [] } ], "chrome_url_overrides": { diff --git a/chrome/browser/resources/file_manager/manifest_new_ui.json b/chrome/browser/resources/file_manager/manifest_new_ui.json index cc306eb..e86db3e 100644 --- a/chrome/browser/resources/file_manager/manifest_new_ui.json +++ b/chrome/browser/resources/file_manager/manifest_new_ui.json @@ -32,35 +32,6 @@ "https://drive.google.com/" ], "file_browser_handlers": [ - // Automatically opens a volume and later close Files.app when unmounted. - // TODO(mtomasz): Implement a better filtering than using file_filters. - { - "id": "auto-open", - "default_title": "__MSG_OPEN_ACTION__", - "default_icon": "images/filetype_generic.png", - "file_filters": [ - "filesystem:*" - ] - }, - // Selects the passed file after launching Files.app. - { - "id": "select", - "default_title": "__MSG_OPEN_ACTION__", - "default_icon": "images/filetype_generic.png", - "file_filters": [ - "filesystem:*" - ] - }, - // Opens the passed directory after launching Files.app. - // TODO(mtomasz): Implement a directories filtering instead of files. - { - "id": "open", - "default_title": "__MSG_OPEN_ACTION__", - "default_icon": "images/filetype_generic.png", - "file_filters": [ - "filesystem:*" - ] - }, { "id": "play", "default_title": "__MSG_PLAY_MEDIA__", @@ -208,6 +179,29 @@ "file_filters": [ "filesystem:*.gslides" ] + }, + // The following handlers are used only internally, therefore they do not + // have any file filter. + // Automatically opens a volume and later close Files.app when unmounted. + { + "id": "auto-open", + "default_title": "__MSG_OPEN_ACTION__", + "default_icon": "images/filetype_generic.png", + "file_filters": [] + }, + // Selects the passed file after launching Files.app. + { + "id": "select", + "default_title": "__MSG_OPEN_ACTION__", + "default_icon": "images/filetype_generic.png", + "file_filters": [] + }, + // Opens the passed directory after launching Files.app. + { + "id": "open", + "default_title": "__MSG_OPEN_ACTION__", + "default_icon": "images/filetype_generic.png", + "file_filters": [] } ], "chrome_url_overrides": { |