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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202384 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/extensions/file_manager/file_browser_handler.cc

@@ -182,12 +182,11 @@ FileBrowserHandler* LoadFileBrowserHandler(
   }
 
   // Initialize file filters (mandatory, unless "create" access is specified,
-  // in which case is ignored).
+  // in which case is ignored). The list can be empty.
   if (!result->HasCreateAccessPermission()) {
     const ListValue* file_filters = NULL;
     if (!file_browser_handler->HasKey(keys::kFileFilters) ||
-        !file_browser_handler->GetList(keys::kFileFilters, &file_filters) ||
-        file_filters->empty()) {
+        !file_browser_handler->GetList(keys::kFileFilters, &file_filters)) {
       *error = ASCIIToUTF16(errors::kInvalidFileFiltersList);
       return NULL;
     }

chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc

@@ -1041,8 +1041,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,
@@ -1050,7 +1050,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) {
@@ -1074,7 +1074,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 {

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 |
@@ -100,7 +98,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() == kFileBrowserDomain ||
           task->extension_id() ==
               extension_misc::kQuickOfficeComponentExtensionId ||
@@ -108,18 +108,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 =
@@ -160,7 +148,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)
@@ -190,7 +178,7 @@ bool GetFileBrowserHandlers(Profile* profile,
       if (!action->MatchesURL(lowercase_url))
         continue;
 
-      results->insert(action_iter->get());
+      results->push_back(action_iter->get());
     }
   }
   return true;
@@ -330,15 +318,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;
 }
@@ -347,8 +335,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();
 
@@ -361,46 +349,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
@@ -410,38 +394,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;
 }
 
@@ -452,8 +435,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;
 

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

chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc

@@ -776,9 +776,7 @@ IN_PROC_BROWSER_TEST_P(FileManagerBrowserLocalTest, TestGalleryOpen) {
   DoTestGalleryOpen(&volume_);
 }
 
-// Disabled temporarily since fails on Linux Chromium OS ASAN Tests (2).
-// TODO(mtomasz): crbug.com/243611.
-IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, DISABLED_TestGalleryOpen) {
+IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, TestGalleryOpen) {
   DoTestGalleryOpen(&volume_);
 }
 

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": {

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": {