Add support for directory access to the file system API.

Add support for directory access to the file system API.

This adds 'openDirectory' as a type option for
chrome.fileSystem.chooseEntry to allow access to directories. This is
restricted to apps with the fileSystem.directory permission.

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

[Matt Giuca, 2013-08-27 04:30:52]

Some initial comments after a quick read through.

https://codereview.chromium.org/23146016/diff/44001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23146016/diff/44001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4151: +        Access directories that you
have opened in the application
What does "Access" mean? (I see you use similar wording to "Write to files"
below.) Is this for both read and write access at the same time? If it's just
read access, you should say "Read". If it's read and write, you should say "Read
and write". Also change the desc string.

Edit: Oh, I get it: it will be writable if you also have the write permission?

Well ideally I would want this to correctly state "read" or "read and write" but
that is some amount of work. (It means modifying the system that prints out
these messages to dynamically choose a version of this message, depending on
whether there is the separate write permission.) I think it's worth the effort,
so that our users are clear on what can be done with the directories. But not in
this CL.

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
(right):

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:68: return
base::PathIsWritable(path);
Do you handle the case of trying to write to a file that is actually a
directory, and vice versa?

(More generally, do you handle the case where you retain access to a file or
directory, then in between sessions, the file changes to a directory or vice
versa?)

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:174: // Whether
access to a directory is being retained.
// Whether the file being retained is a directory.

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:78: callback EntryCallback = void
([instanceOf=Entry] object entry);
Are you sure these names are not used anywhere in the user's code? (I can't
think why they would be, but just checking.)

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
chrome/common/extensions/permissions/chrome_api_permissions.cc:271: // The
permission string for "fileSystem" is only shown when "write" is
Update this comment. ""write" or "directory" is present."

And add "or directory" after "file" in several places.

[Sam McNally, 2013-08-27 05:16:36]

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
(right):

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:68: return
base::PathIsWritable(path);
On 2013/08/27 04:30:53, Matt Giuca wrote:
> Do you handle the case of trying to write to a file that is actually a
> directory, and vice versa?
> 
> (More generally, do you handle the case where you retain access to a file or
> directory, then in between sessions, the file changes to a directory or vice
> versa?)

That would cause getFile/getDirectory to fail in the custom bindings.

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/23146016/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:174: // Whether
access to a directory is being retained.
On 2013/08/27 04:30:53, Matt Giuca wrote:
> // Whether the file being retained is a directory.

Done.

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:78: callback EntryCallback = void
([instanceOf=Entry] object entry);
On 2013/08/27 04:30:53, Matt Giuca wrote:
> Are you sure these names are not used anywhere in the user's code? (I can't
> think why they would be, but just checking.)

Yes.

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/23146016/diff/44001/chrome/common/extensions/...
chrome/common/extensions/permissions/chrome_api_permissions.cc:271: // The
permission string for "fileSystem" is only shown when "write" is
On 2013/08/27 04:30:53, Matt Giuca wrote:
> Update this comment. ""write" or "directory" is present."
> 
> And add "or directory" after "file" in several places.

Done.

[benwells, 2013-08-27 09:05:05]

still looking, this is what i have so far

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4151: Write to files that you have opened in
the application
Hmm - we should merge these warnings together. Is that possible with the current
system?

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4151: +        Access directories that you
have opened in the application
This doesn't seem strong enough. For now let's include words to the effect of
'recursively', such as 'Access directories that you open, and all files and
folders those directories contain.'

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
(right):

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:62: bool
DoCheckWritableFile(const base::FilePath& path) {
oh noes! all these names are now wrong!!

Can you log a bug and add TODOs to fix them? I would prefer that change in a
separate CL for reviewer sanity but don't want it forgotten.

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:64: if
(base::PathExists(path) && file_util::IsLink(path))
We need to check what happens to symbolic links contained in folders. Can they
be followed to other folders?

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:68: return
base::PathIsWritable(path);
Hmmm. This isn't right. Apps with write permission should still be able to read
read-only directories.

There doesn't appear to be a corresponding check for files (which is good).

I'm not sure if we should auto-create folders like we do files. We shouldn't
auto-create a file if they want a directory which I think this code will do.  
Maybe this will never get reached for directories as the picker ensures the
directory exists, if that's the case I'd still prefer this to handle directories
not existing to avoid it knowing about its callers.

I think that means this needs an is_directory parameter.

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:330:
GrantedFileEntry::GrantedFileEntry() {}
I'm sure there is an intelligent reason, but why did you add this?

[Matt Giuca, 2013-08-28 00:37:42]

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4151: Write to files that you have opened in
the application
On 2013/08/27 09:05:06, benwells wrote:
> Hmm - we should merge these warnings together. Is that possible with the
current
> system?

I think it will take some work (but note that my suggestion also takes work).

I think we should either merge them, or do my suggestion of making the directory
warning message different depending on whether files are writable. My concern
with merging them is that the directory access one won't look much scarier than
the normal file one. If there are two warnings then maybe you are more likely to
notice the directory one.

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
(right):

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:330:
GrantedFileEntry::GrantedFileEntry() {}
It's an unrelated change, but I believe it's in the style guide that you should
always put an explicit empty constructor in the .cc file instead of leaving it
implicit, for non-POD classes, to avoid the binary code duplication of having an
implicit constructor generated in every module that constructs an object of that
type.

[Sam McNally, 2013-08-28 05:44:25]

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/23146016/diff/59001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4151: +        Access directories that you
have opened in the application
On 2013/08/27 09:05:06, benwells wrote:
> This doesn't seem strong enough. For now let's include words to the effect of
> 'recursively', such as 'Access directories that you open, and all files and
> folders those directories contain.'

Done.

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
(right):

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:62: bool
DoCheckWritableFile(const base::FilePath& path) {
On 2013/08/27 09:05:06, benwells wrote:
> oh noes! all these names are now wrong!!
> 
> Can you log a bug and add TODOs to fix them? I would prefer that change in a
> separate CL for reviewer sanity but don't want it forgotten.

I think it's OK. Directories are files.

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:64: if
(base::PathExists(path) && file_util::IsLink(path))
On 2013/08/27 09:05:06, benwells wrote:
> We need to check what happens to symbolic links contained in folders. Can they
> be followed to other folders?

The HTML5 filesystem API returns NOT_FOUND_ERR for symlinks, to either files or
directories.

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:68: return
base::PathIsWritable(path);
On 2013/08/27 09:05:06, benwells wrote:
> Hmmm. This isn't right. Apps with write permission should still be able to
read
> read-only directories.
> 
> There doesn't appear to be a corresponding check for files (which is good).
> 
> I'm not sure if we should auto-create folders like we do files. We shouldn't
> auto-create a file if they want a directory which I think this code will do.  
> Maybe this will never get reached for directories as the picker ensures the
> directory exists, if that's the case I'd still prefer this to handle
directories
> not existing to avoid it knowing about its callers.
> 
> I think that means this needs an is_directory parameter.

Done.

https://codereview.chromium.org/23146016/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:330:
GrantedFileEntry::GrantedFileEntry() {}
On 2013/08/28 00:37:42, Matt Giuca wrote:
> It's an unrelated change, but I believe it's in the style guide that you
should
> always put an explicit empty constructor in the .cc file instead of leaving it
> implicit, for non-POD classes, to avoid the binary code duplication of having
an
> implicit constructor generated in every module that constructs an object of
that
> type.

What Matt said. Specifically,
http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors...
started complaining about it.

[benwells, 2013-08-29 00:04:23]

Looked at everything except tests now...

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
(right):

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:69: return
file_util::CreateDirectory(path);
I'm not sure about this. Can we return file_util::DirectoryExists()?

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:408: if
(is_directory_ &&
Nit: add DCHECK for correct thread

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:419: if
(base::DirectoryExists(path_)) {
Nit: add DCHECK for correct thread

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:827: }
Nit: blank line after early return.

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:835: return true;
Nit: blank line after early return.

[Sam McNally, 2013-08-29 00:16:08]

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc
(right):

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc:69: return
file_util::CreateDirectory(path);
On 2013/08/29 00:04:24, benwells wrote:
> I'm not sure about this. Can we return file_util::DirectoryExists()?

Done.

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:408: if
(is_directory_ &&
On 2013/08/29 00:04:24, benwells wrote:
> Nit: add DCHECK for correct thread

Done.

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:419: if
(base::DirectoryExists(path_)) {
On 2013/08/29 00:04:24, benwells wrote:
> Nit: add DCHECK for correct thread

Done.

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:827: }
On 2013/08/29 00:04:24, benwells wrote:
> Nit: blank line after early return.

Done.

https://codereview.chromium.org/23146016/diff/66001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:835: return true;
On 2013/08/29 00:04:24, benwells wrote:
> Nit: blank line after early return.

Done.

[benwells, 2013-08-29 04:10:50]

Super close now!

https://codereview.chromium.org/23146016/diff/66001/apps/app_restore_service_...
File apps/app_restore_service_browsertest.cc (right):

https://codereview.chromium.org/23146016/diff/66001/apps/app_restore_service_...
apps/app_restore_service_browsertest.cc:102:
IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MAYBE_FileAccessIsRestored) {
Is there value in this test? Isn't this already tested in the api test?

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:303:
FileSystemApiOpenDirectoryWithoutPermissionTest) {
I'd also like to see an app with write but not directory get tested.

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:549: {
Why is this block here?

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test_util.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test_util.js:5:
// This is a modified duplicate of the file test_util in
Can you think of a way not to modify this (i.e. update all the others)? This
system of duplicated files is already a bit of a nightmare, having some slightly
different feels even worse.

i.e. we could have checkFileEntry and checkDirectoryEntry.

I'd be happy for you to add a TODO to do this with a logged crbug; it doesn't
need to block directory access.

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test_util.js:41:
0);
super nit: indent.

[Sam McNally, 2013-08-29 05:31:03]

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:303:
FileSystemApiOpenDirectoryWithoutPermissionTest) {
On 2013/08/29 04:10:50, benwells wrote:
> I'd also like to see an app with write but not directory get tested.

Done.

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:549: {
On 2013/08/29 04:10:50, benwells wrote:
> Why is this block here?

To set up the retained directory when the app is installed.

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test_util.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test_util.js:5:
// This is a modified duplicate of the file test_util in
On 2013/08/29 04:10:50, benwells wrote:
> Can you think of a way not to modify this (i.e. update all the others)? This
> system of duplicated files is already a bit of a nightmare, having some
slightly
> different feels even worse.
> 
> i.e. we could have checkFileEntry and checkDirectoryEntry.
> 
> I'd be happy for you to add a TODO to do this with a logged crbug; it doesn't
> need to block directory access.

Done.

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test_util.js:41:
0);
On 2013/08/29 04:10:50, benwells wrote:
> super nit: indent.

Done.

[benwells, 2013-08-29 05:43:08]

lgtm. LGTM. LLLGGTTTMMM!!!!!!!!

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:549: {
On 2013/08/29 05:31:04, Sam McNally wrote:
> On 2013/08/29 04:10:50, benwells wrote:
> > Why is this block here?
> 
> To set up the retained directory when the app is installed.

As discussed I meant the block itself, not its contents. Lets see if it works
without it.

[Sam McNally, 2013-08-29 05:51:25]

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/23146016/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:549: {
On 2013/08/29 05:43:09, benwells wrote:
> On 2013/08/29 05:31:04, Sam McNally wrote:
> > On 2013/08/29 04:10:50, benwells wrote:
> > > Why is this block here?
> > 
> > To set up the retained directory when the app is installed.
> 
> As discussed I meant the block itself, not its contents. Lets see if it works
> without it.

It works!

[Matt Giuca, 2013-08-29 09:34:44]

lgtm with nits

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_system/open_directory/test.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory/test.js:5:
chrome.test.runTests([
// Testing with directory access, but no write permission.

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test.js:5:
chrome.test.runTests([
// Testing with directory access and write permission.

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_system/restore_directory/test.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/restore_directory/test.js:22:
}));
Why did you remove the isRestorable tests?

[Sam McNally, 2013-08-29 11:08:08]

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_system/open_directory/test.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory/test.js:5:
chrome.test.runTests([
On 2013/08/29 09:34:44, Matt Giuca wrote:
> // Testing with directory access, but no write permission.

Done.

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_directory_with_write/test.js:5:
chrome.test.runTests([
On 2013/08/29 09:34:44, Matt Giuca wrote:
> // Testing with directory access and write permission.

Done.

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/file_system/restore_directory/test.js
(right):

https://codereview.chromium.org/23146016/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/restore_directory/test.js:22:
}));
On 2013/08/29 09:34:44, Matt Giuca wrote:
> Why did you remove the isRestorable tests?

These are testing with an invalid ID. That's already tested in the existing
restore_entry test.

[commit-bot: I haz the power, 2013-08-29 11:09:28]

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

[commit-bot: I haz the power, 2013-08-29 14:16:28]

Change committed as 220287