ChromeOS: Use file extensions in Files app to decide which apps to use.

ChromeOS: Use file extensions in Files app to decide which apps to use.

The file browser now decides which apps can open a file based on the
file extensions an app can handle, as well as MIME type.

BUG=167950
TEST=browser_tests LocalFileSystemExtensionApiTest.AppFileHanlder [sic]

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198748

[Matt Giuca, 2013-05-07 05:39:58]

Please fix the change description.

ChromeOS: The file browser now decides which apps can open a file based on the
file extensions an app can handle, as well as MIME type.

or similar.

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc
(right):

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:396:
void GetMimeTypeAndPathsForFileURLs(
I wouldn't change the name here, because it isn't really getting paths as well,
it's just adding mime type info to an existing list of paths.

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.h
(right):

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.h:23: typedef
std::set<std::pair<std::string, base::FilePath> > MimeTypeAndPathSet;
Document this type.

Also I would swap the order of the pair, because then it's like a key:value
mapping where the filepath is the key, and mime type is the value. (It seems
more natural to me to have the key on the left side of the pair.)

[Sam McNally, 2013-05-07 06:15:26]

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc
(right):

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_browser_private_api.cc:396:
void GetMimeTypeAndPathsForFileURLs(
On 2013/05/07 05:39:59, Matt Giuca wrote:
> I wouldn't change the name here, because it isn't really getting paths as
well,
> it's just adding mime type info to an existing list of paths.

Done.

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.h
(right):

https://codereview.chromium.org/14790018/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.h:23: typedef
std::set<std::pair<std::string, base::FilePath> > MimeTypeAndPathSet;
On 2013/05/07 05:39:59, Matt Giuca wrote:
> Document this type.
> 
> Also I would swap the order of the pair, because then it's like a key:value
> mapping where the filepath is the key, and mime type is the value. (It seems
> more natural to me to have the key on the left side of the pair.)

Done.

[Matt Giuca, 2013-05-07 06:33:50]

Good. It would be a sin to not LGTM this.

Re the change description: What is "Files.app"? Is that what it's called
internally? I've never heard of .app (in fact, isn't that a Mac thing?)

Also, you don't need TEST= if it's just the name of a test case (that's for
manual testing). But if you are going to keep it, fix the spelling of
"AppFileHanlder".

[Matt Giuca, 2013-05-07 06:37:45]

LGTM

[benwells, 2013-05-07 08:05:17]

lgtm. we should get a review from whoever owns the file browser private api, i
think tbarzik has reviewed this stuff in the past so adding him.

https://codereview.chromium.org/14790018/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.h
(right):

https://codereview.chromium.org/14790018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.h:39:
FindFileHandlers(const Extension& extension,
Nit: please rename to just FileHandlersForFiles and add a comment explaining the
logic (i.e. that file handlers matching all files are found).

Also explain that files must be populated with paths but that the mime type is
optional.

https://codereview.chromium.org/14790018/diff/24001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_browser/app_file_handler/manifest.json
(right):

https://codereview.chromium.org/14790018/diff/24001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_browser/app_file_handler/manifest.json:6:
"description": "Tests of chrome.fileSystem.* methods for handling Web Intents",
yak: Please update this description while here to ... for handling files, and
the name above to ChroemOS File handler extension.

[benwells, 2013-05-07 08:09:52]

-tbarzic who apparently isn't working on files app anymore
+mtomasz instead.

[Sam McNally, 2013-05-07 08:25:53]

https://codereview.chromium.org/14790018/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.h
(right):

https://codereview.chromium.org/14790018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.h:39:
FindFileHandlers(const Extension& extension,
On 2013/05/07 08:05:17, benwells wrote:
> Nit: please rename to just FileHandlersForFiles and add a comment explaining
the
> logic (i.e. that file handlers matching all files are found).
> 
> Also explain that files must be populated with paths but that the mime type is
> optional.

Done.

https://codereview.chromium.org/14790018/diff/24001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_browser/app_file_handler/manifest.json
(right):

https://codereview.chromium.org/14790018/diff/24001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_browser/app_file_handler/manifest.json:6:
"description": "Tests of chrome.fileSystem.* methods for handling Web Intents",
On 2013/05/07 08:05:17, benwells wrote:
> yak: Please update this description while here to ... for handling files, and
> the name above to ChroemOS File handler extension.

Done.

[mtomasz, 2013-05-07 10:10:55]

lgtm

[commit-bot: I haz the power, 2013-05-07 10:18:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/14790018/31001

[commit-bot: I haz the power, 2013-05-07 17:01:31]

Change committed as 198748