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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202384

[kinaba, 2013-05-24 07:34:13]

Adding +hashimoto from the OWNERs.

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:418:
std::inserter(intersection,
std::back_inserter(intersection)

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.h (left):

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.h:73: // This
generates a list of default tasks (tasks set as default by the user in
nit: probably this "set" is a past participle of the verb "set" so it should be
kept as is.

[mtomasz, 2013-05-24 07:49:45]

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:418:
std::inserter(intersection,
On 2013/05/24 07:34:13, kinaba wrote:
> std::back_inserter(intersection)

Done.

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.h (left):

https://codereview.chromium.org/15975004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.h:73: // This
generates a list of default tasks (tasks set as default by the user in
On 2013/05/24 07:34:13, kinaba wrote:
> nit: probably this "set" is a past participle of the verb "set" so it should
be
> kept as is.

Done.

[kinaba, 2013-05-24 08:08:52]

lgtm

[mtomasz, 2013-05-24 08:27:44]

On 2013/05/24 08:08:52, kinaba wrote:
> lgtm

I think this patch doesn't solve the root cause, yet. I'll investigate deeper.

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

lgtm

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc
(right):

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:1053:
for (file_handler_util::FileBrowserHandlerList::const_iterator iter =
nit: Use size_t to iterate over a vector.

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:332:
FileBrowserHandlerList* handler_list,
nit: Could you add const while you are here?

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:339:
iter++;
nit: Could you fix this to ++iter while you are here?

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:365: for
(FileBrowserHandlerList::const_iterator task_iter = common_tasks.begin();
nit: Use size_t to iterate over a vector.

[mtomasz, 2013-05-24 10:37:52]

On 2013/05/24 08:38:57, hashimoto wrote:
> lgtm
> 
>
https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
> File
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
>
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:1053:
> for (file_handler_util::FileBrowserHandlerList::const_iterator iter =
> nit: Use size_t to iterate over a vector.
> 
>
https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
> File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
> (right):
> 
>
https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
> chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:332:
> FileBrowserHandlerList* handler_list,
> nit: Could you add const while you are here?
> 
>
https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
> chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:339:
> iter++;
> nit: Could you fix this to ++iter while you are here?
> 
>
https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
> chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:365: for
> (FileBrowserHandlerList::const_iterator task_iter = common_tasks.begin();
> nit: Use size_t to iterate over a vector.

I've addressed some comments. Moreover, I changed a little to fix another bug
(see BUG=), which is very tightly related.

PTAL one more time. Thanks.

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

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc
(right):

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:1053:
for (file_handler_util::FileBrowserHandlerList::const_iterator iter =
On 2013/05/24 08:38:57, hashimoto wrote:
> nit: Use size_t to iterate over a vector.

Done.

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:332:
FileBrowserHandlerList* handler_list,
On 2013/05/24 08:38:57, hashimoto wrote:
> nit: Could you add const while you are here?

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.

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:339:
iter++;
On 2013/05/24 08:38:57, hashimoto wrote:
> nit: Could you fix this to ++iter while you are here?

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?

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:365: for
(FileBrowserHandlerList::const_iterator task_iter = common_tasks.begin();
On 2013/05/24 08:38:57, hashimoto wrote:
> nit: Use size_t to iterate over a vector.

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

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

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:332:
FileBrowserHandlerList* handler_list,
On 2013/05/24 10:37:59, mtomasz wrote:
> On 2013/05/24 08:38:57, hashimoto wrote:
> > nit: Could you add const while you are here?
> 
> 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.

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

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:339:
iter++;
On 2013/05/24 10:37:59, mtomasz wrote:
> On 2013/05/24 08:38:57, hashimoto wrote:
> > nit: Could you fix this to ++iter while you are here?
> 
> 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?

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...)

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:365: for
(FileBrowserHandlerList::const_iterator task_iter = common_tasks.begin();
On 2013/05/24 10:37:59, mtomasz wrote:
> On 2013/05/24 08:38:57, hashimoto wrote:
> > nit: Use size_t to iterate over a vector.
> 
> Why is it better? We work extensively on iterators here.

Fair enough.

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/file_browser_handler.cc
(left):

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_browser_handler.cc:190:
file_filters->empty()) {
You are going to change the behavior of a public API.
(http://developer.chrome.com/extensions/fileBrowserHandler.html)
I don't think allowing empty file filters as an alternative to "filesystem:*" is
a good idea, can't we go as strict as possible?

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:101: bool
IsFallbackTask(const FileBrowserHandler* task) {
Please add a function comment.
It's not clear from the new code why the task can be used as the fallback if it
matches the conditions below.

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/manifest_new_ui.json (left):

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/file_manager/manifest_new_ui.json:35: // Automatically
opens a volume and later close Files.app when unmounted.
How about having a comment about how the order of handlers affect the behavior
in practice?

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

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/6/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:339:
iter++;
On 2013/05/24 11:15:51, hashimoto wrote:
> On 2013/05/24 10:37:59, mtomasz wrote:
> > On 2013/05/24 08:38:57, hashimoto wrote:
> > > nit: Could you fix this to ++iter while you are here?
> > 
> > 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?
> 
> 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...)

Done.

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/file_browser_handler.cc
(left):

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_browser_handler.cc:190:
file_filters->empty()) {
On 2013/05/24 11:15:51, hashimoto wrote:
> You are going to change the behavior of a public API.
> (http://developer.chrome.com/extensions/fileBrowserHandler.html)
> I don't think allowing empty file filters as an alternative to "filesystem:*"
is
> a good idea, can't we go as strict as possible?

I don't think it changes the api. This change allows an empty list, which is not
forbidden in the provided link.

Empty filter list means that this handler does handle any specific files. It is
used internally only.

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:101: bool
IsFallbackTask(const FileBrowserHandler* task) {
On 2013/05/24 11:15:51, hashimoto wrote:
> Please add a function comment.
> It's not clear from the new code why the task can be used as the fallback if
it
> matches the conditions below.

I didn't change anything in this class but set -> vector + code style. Added a
comment. Done.

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/manifest_new_ui.json (left):

https://codereview.chromium.org/15975004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/file_manager/manifest_new_ui.json:35: // Automatically
opens a volume and later close Files.app when unmounted.
On 2013/05/24 11:15:51, hashimoto wrote:
> How about having a comment about how the order of handlers affect the behavior
> in practice?

Here the order doesn't matter at all. I just changed it for better readability.

[mtomasz, 2013-05-27 01:20:31]

@hashimoto: PTAL one more time. Thanks.

[hashimoto, 2013-05-27 03:55:38]

lgtm

https://codereview.chromium.org/15975004/diff/29001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/manifest.json (right):

https://codereview.chromium.org/15975004/diff/29001/chrome/browser/resources/...
chrome/browser/resources/file_manager/manifest.json:187: "file_filters": []
nit: Could you add a comment to clarify that these handlers are just used
internally, not to be matched to file names?

[kinaba, 2013-05-27 04:01:45]

lgtm

[mtomasz, 2013-05-27 05:00:10]

https://codereview.chromium.org/15975004/diff/29001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/manifest.json (right):

https://codereview.chromium.org/15975004/diff/29001/chrome/browser/resources/...
chrome/browser/resources/file_manager/manifest.json:187: "file_filters": []
On 2013/05/27 03:55:39, hashimoto wrote:
> nit: Could you add a comment to clarify that these handlers are just used
> internally, not to be matched to file names?

Done.

[commit-bot: I haz the power, 2013-05-27 05:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/15975004/35001

[commit-bot: I haz the power, 2013-05-27 05:00:33]

Failed to apply patch for
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
  Hunk #1 FAILED at 793.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc.rej

Patch:      
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
Index:
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
diff --git
a/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
b/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
index
eee5d2d8e878d45c1d045c2cf0b910517a7b8501..dfd33869358b5ccd860ee4f5c71c7333098c7ead
100644
---
a/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
+++
b/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
@@ -793,9 +793,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) {
   PrepareVolume();
   DoTestGalleryOpen(&volume_);
 }

[commit-bot: I haz the power, 2013-05-27 05:53:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/15975004/41001

[commit-bot: I haz the power, 2013-05-27 08:26:46]

Change committed as 202384