Add isWritableFileEntry to the fileSystem API

Add isWritableFileEntry to the fileSystem API

isWritableFileEntry determines whether an existing FileEntry, obtained via getWritableFileEntry or chooseFile, is writable (otherwise, we assume it is readable).

BUG=135689
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147399

[benwells, 2012-07-10 06:40:23]

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:138: return false;
Nit: add blank line after early return.

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:243: if
(!HasFileSystemWritePermission(GetExtension())) {
I'm not sure about this. Calls to IsWritableFileEntry should not fail just
because you don't have this permission.

However, unless there is a bug, they should always return false.

We should take out this check, and just ask the security policy what rights
there are.

Maybe we should also add this call to all the file system API tests for
chooseFile and getWritableFileEntry.

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/common/exten...
File chrome/common/extensions/api/file_system.idl (right):

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/common/exten...
chrome/common/extensions/api/file_system.idl:38: // Gets whether this FileEntry
is readable or writable.
Nit: ...is writable or not.

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/test/data/ex...
File
chrome/test/data/extensions/api_test/file_system/get_writable_file_entry/test.js
(right):

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/test/data/ex...
chrome/test/data/extensions/api_test/file_system/get_writable_file_entry/test.js:9:
// Test that we can have permission to get the entry as writable.
Nice catch!
Nit: ... that we cannot get a writable entry when we don't have permission to.

[thorogood, 2012-07-10 06:49:22]

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:138: return false;
On 2012/07/10 06:40:23, benwells wrote:
> Nit: add blank line after early return.

Done.

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:243: if
(!HasFileSystemWritePermission(GetExtension())) {
On 2012/07/10 06:40:23, benwells wrote:
> I'm not sure about this. Calls to IsWritableFileEntry should not fail just
> because you don't have this permission.
> 
> However, unless there is a bug, they should always return false.

True. We would never grant the policy if they do not have this extension
permission.

> 
> We should take out this check, and just ask the security policy what rights
> there are.

I've removed the check.

> 
> Maybe we should also add this call to all the file system API tests for
> chooseFile and getWritableFileEntry.

So, we make a call to HasFileSystemWritePermission inside chooseFile and
getWritableFileEntry already. I'm not sure what you mean by add a call to their
tests.

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/common/exten...
File chrome/common/extensions/api/file_system.idl (right):

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/common/exten...
chrome/common/extensions/api/file_system.idl:38: // Gets whether this FileEntry
is readable or writable.
On 2012/07/10 06:40:23, benwells wrote:
> Nit: ...is writable or not.

Done.

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/test/data/ex...
File
chrome/test/data/extensions/api_test/file_system/get_writable_file_entry/test.js
(right):

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/test/data/ex...
chrome/test/data/extensions/api_test/file_system/get_writable_file_entry/test.js:9:
// Test that we can have permission to get the entry as writable.
On 2012/07/10 06:40:23, benwells wrote:
> Nice catch!
> Nit: ... that we cannot get a writable entry when we don't have permission to.

Done.

[thorogood, 2012-07-10 08:06:16]

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10692104/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:243: if
(!HasFileSystemWritePermission(GetExtension())) {
On 2012/07/10 06:49:22, thorogood wrote:
> On 2012/07/10 06:40:23, benwells wrote:
> > I'm not sure about this. Calls to IsWritableFileEntry should not fail just
> > because you don't have this permission.
> > 
> > However, unless there is a bug, they should always return false.
> 
> True. We would never grant the policy if they do not have this extension
> permission.
> 
> > 
> > We should take out this check, and just ask the security policy what rights
> > there are.
> 
> I've removed the check.
> 
> > 
> > Maybe we should also add this call to all the file system API tests for
> > chooseFile and getWritableFileEntry.
> 
> So, we make a call to HasFileSystemWritePermission inside chooseFile and
> getWritableFileEntry already. I'm not sure what you mean by add a call to
their
> tests.

I've added a call to isWritableFileEntry to checkEntry, which is used by many of
the fileSystem tests (it's inside test_util.js). PTAL!

[benwells, 2012-07-11 04:49:18]

lgtm
+kinuko for file system perspective
+mihai for api
+avi as content owner

[kinuko, 2012-07-11 05:16:40]

On 2012/07/11 04:49:18, benwells wrote:
> lgtm
> +kinuko for file system perspective
> +mihai for api
> +avi as content owner

file system related code lgtm

[Mihai Parparita -not on Chrome, 2012-07-11 05:35:51]

LGTM

http://codereview.chromium.org/10692104/diff/4002/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/file_system/is_writable_file_entry/background.js
(right):

http://codereview.chromium.org/10692104/diff/4002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/file_system/is_writable_file_entry/background.js:6:
chrome.appWindow.create('test.html');
Do you actually need to create the window and run the test from there, or can
you do it directly from the background page?

Normally the file picker would not work from the background page, but since
you're overriding it anyway...

[thorogood, 2012-07-12 01:21:37]

http://codereview.chromium.org/10692104/diff/4002/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/file_system/is_writable_file_entry/background.js
(right):

http://codereview.chromium.org/10692104/diff/4002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/file_system/is_writable_file_entry/background.js:6:
chrome.appWindow.create('test.html');
On 2012/07/11 05:35:51, Mihai Parparita wrote:
> Do you actually need to create the window and run the test from there, or can
> you do it directly from the background page?
> 
> Normally the file picker would not work from the background page, but since
> you're overriding it anyway...

So amusingly the way the override was written actually loads the picker, but
then forces a selection. The picker still needs a valid calling page to be open
(although this feels like a bug - why do I need an open window to choose a
file)?

I'll leave this for now.

[benwells, 2012-07-12 01:40:13]

On 2012/07/12 01:21:37, thorogood wrote:
>
http://codereview.chromium.org/10692104/diff/4002/chrome/test/data/extensions...
> File
>
chrome/test/data/extensions/api_test/file_system/is_writable_file_entry/background.js
> (right):
> 
>
http://codereview.chromium.org/10692104/diff/4002/chrome/test/data/extensions...
>
chrome/test/data/extensions/api_test/file_system/is_writable_file_entry/background.js:6:
> chrome.appWindow.create('test.html');
> On 2012/07/11 05:35:51, Mihai Parparita wrote:
> > Do you actually need to create the window and run the test from there, or
can
> > you do it directly from the background page?
> > 
> > Normally the file picker would not work from the background page, but since
> > you're overriding it anyway...
> 
> So amusingly the way the override was written actually loads the picker, but
> then forces a selection. The picker still needs a valid calling page to be
open
> (although this feels like a bug - why do I need an open window to choose a
> file)?
> 
> I'll leave this for now.

Leaving for now sounds good. Personally I like the override stuff having as
minimal impact as possible, and the test stayign as it is, so we test as close
to the normal system as possible.

Also I like needing to have an open window to choose a file as it would feel
weird and confusing for file pickers to appear out of nowhere, but maybe I'm not
being imaginative enough :)

[thorogood, 2012-07-13 00:23:39]

-avi, who is on holiday
+brettw, for content

[brettw, 2012-07-17 20:07:41]

owners lgtm for content.

[thorogood, 2012-07-18 03:33:44]

On 2012/07/17 20:07:41, brettw wrote:
> owners lgtm for content.

[commit-bot: I haz the power, 2012-07-18 03:33:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692104/4002

[commit-bot: I haz the power, 2012-07-18 03:52:42]

Try job failure for 10692104-4002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-07-18 04:57:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692104/24004

[thorogood, 2012-07-18 04:57:34]

On 2012/07/18 03:52:42, I haz the power (commit-bot) wrote:
> Try job failure for 10692104-4002 (retry) on linux_clang for step "compile"
> (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

Trying again now with the CL merged/fixed at head. :)

[thorogood, 2012-07-19 00:01:05]

On 2012/07/18 04:57:34, thorogood wrote:
> On 2012/07/18 03:52:42, I haz the power (commit-bot) wrote:
> > Try job failure for 10692104-4002 (retry) on linux_clang for step "compile"
> > (clobber build).
> > It's a second try, previously, step "compile" failed.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
> 
> Trying again now with the CL merged/fixed at head. :)

The CQ seems to have given up. Let's try again.

[commit-bot: I haz the power, 2012-07-19 00:01:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692104/27002

[commit-bot: I haz the power, 2012-07-19 01:11:32]

Try job failure for 10692104-27002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-19 03:34:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10692104/27002

[thorogood, 2012-07-19 03:34:17]

On 2012/07/19 01:11:32, I haz the power (commit-bot) wrote:
> Try job failure for 10692104-27002 (retry) on mac_rel for step
"browser_tests".
> It's a second try, previously, step "browser_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Trying submit again. The test that failed on Mac runs fine on my local Mac Pro.
Presuming flaky at head.

[commit-bot: I haz the power, 2012-07-19 05:32:12]

Change committed as 147399