Replace sets with vectors when storing file handlers.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/extensions/file_manager/file_handler_util.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/json/json_writer.h"
@@ skipping 47 matching lines @@
 const char kTaskDrive[] = "drive";
 const char kTaskApp[] = "app";
 
 namespace {
 
 // Legacy Drive task extension prefix, used by CrackTaskID.
 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 |
                                       base::PLATFORM_FILE_CREATE_ALWAYS |
                                       base::PLATFORM_FILE_OPEN_TRUNCATED |
                                       base::PLATFORM_FILE_READ |
                                       base::PLATFORM_FILE_WRITE |
                                       base::PLATFORM_FILE_EXCLUSIVE_READ |
                                       base::PLATFORM_FILE_EXCLUSIVE_WRITE |
                                       base::PLATFORM_FILE_ASYNC |
@@ skipping 73 matching lines @@
 std::string EscapedUtf8ToLower(const std::string& str) {
   string16 utf16 = UTF8ToUTF16(
       net::UnescapeURLComponent(str, net::UnescapeRule::NORMAL));
   return net::EscapeUrlEncodedData(
       UTF16ToUTF8(base::i18n::ToLower(utf16)),
       false /* do not replace space with plus */);
 }
 
 bool GetFileBrowserHandlers(Profile* profile,
                             const GURL& selected_file_url,
-                            FileBrowserHandlerSet* results) {
+                            FileBrowserHandlerList* results) {
   ExtensionService* service =
       extensions::ExtensionSystem::Get(profile)->extension_service();
   if (!service)
     return false;  // In unit-tests, we may not have an ExtensionService.
 
   // We need case-insensitive matching, and pattern in the handler is already
   // in lower case.
   const GURL lowercase_url(EscapedUtf8ToLower(selected_file_url.spec()));
 
   for (ExtensionSet::const_iterator iter = service->extensions()->begin();
        iter != service->extensions()->end();
        ++iter) {
     const Extension* extension = *iter;
     if (profile->IsOffTheRecord() &&
         !service->IsIncognitoEnabled(extension->id()))
       continue;
 
     FileBrowserHandler::List* handler_list =
         FileBrowserHandler::GetHandlers(extension);
     if (!handler_list)
       continue;
     for (FileBrowserHandler::List::const_iterator action_iter =
              handler_list->begin();
          action_iter != handler_list->end();
          ++action_iter) {
       const FileBrowserHandler* action = action_iter->get();
       if (!action->MatchesURL(lowercase_url))
         continue;
 
-      results->insert(action_iter->get());
+      results->push_back(action_iter->get());
     }
   }
   return true;
 }
 
 }  // namespace
 
 void UpdateDefaultTask(Profile* profile,
                        const std::string& task_id,
                        const std::set<std::string>& suffixes,
@@ skipping 119 matching lines @@
            *task_type == kTaskApp);
   }
 
   if (action_id)
     *action_id = result[2];
 
   return true;
 }
 
 // Find a specific handler in the handler list.
-FileBrowserHandlerSet::iterator FindHandler(
-    FileBrowserHandlerSet* handler_set,
+FileBrowserHandlerList::iterator FindHandler(
+    FileBrowserHandlerList* handler_list,

[hashimoto, 2013/05/24 08:38:57]

nit: Could you add const while you are here?

[mtomasz, 2013/05/24 10:37:59]

If I do that, then I have to return a const_iterator, and as a result I can't
remove in #420 and #422. This is a bug in C++'s STL, and it is fixed in some new
implementations.

[hashimoto, 2013/05/24 11:15:51]

Oops, I was too careless to forget to look at how the returned iterator is used.
Please do not mind.

     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++;

[hashimoto, 2013/05/24 08:38:57]

nit: Could you fix this to ++iter while you are here?

[mtomasz, 2013/05/24 10:37:59]

If I do that then it makes lots of stuff complicated. If I remove the iterator
and just return the handler then I will have to convert it back to an iterator
to remove from the collection (#420, #422).

If I used size_t for loop instead of iterating, but instead of returning the
pointer to the handler, returned an iterator, then it just looks like two
different approaches mixed in 5 lines. WDYT?

[hashimoto, 2013/05/24 11:15:51]

I just wanted you to use preincrement (++iter) instead of postincrement
(iter++).
Even though it doesn't make much difference with std::vector (vector::iterator
is a pointer, essentially), ++iter is the one preferred generally.
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...)

[mtomasz, 2013/05/27 01:20:20]

Done.

   }
   return iter;
 }
 
 // Given the list of selected files, returns array of file action tasks
 // 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();
 
   std::set<std::string> default_ids;
   for (std::vector<base::FilePath>::const_iterator it = files_list.begin();
        it != files_list.end(); ++it) {
     std::string task_id = file_handler_util::GetDefaultTaskIdFromPrefs(
         profile, "", it->Extension());
     if (!task_id.empty())
       default_ids.insert(task_id);
   }
 
   const FileBrowserHandler* builtin_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();

[hashimoto, 2013/05/24 08:38:57]

nit: Use size_t to iterate over a vector.

[mtomasz, 2013/05/24 10:37:59]

Why is it better? We work extensively on iterators here.

[hashimoto, 2013/05/24 11:15:51]

Fair enough.

        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;
   }
 
   // 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);
+    default_tasks->push_back(builtin_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
     // files will be empty at the end, and so will the default tasks.
     if (file_actions.empty())
       return true;
 
     // 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;
 }
 
 bool GetTaskForURLAndPath(Profile* profile,
                    const GURL& url,
                    const base::FilePath& file_path,
                    const FileBrowserHandler** handler) {
   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;
 
   if (common_tasks.empty())
     return false;
 
   std::vector<base::FilePath> file_paths;
   file_paths.push_back(file_path);
 
   FindDefaultTasks(profile, file_paths, common_tasks, &default_tasks);
@@ skipping 521 matching lines @@
     extensions::LaunchPlatformAppWithFileHandler(profile(), GetExtension(),
         action_id_, file_urls[i].path());
   }
 
   if (!done.is_null())
     done.Run(true);
   return true;
 }
 
 } // namespace file_handler_util