fileSystemProvider: First cut at implementing fileSystemProvider API

fileSystemProvider: First cut at implementing fileSystemProvider API

Implement fileSystemProvider.mount() that does not do anything meaningful
yet. The purpose of this patch is to add boilerplate files so adding new
code is easier.

Design doc of the API: http://goo.gl/lLXJYQ

BUG=248427
TEST=none
R=benwells@chromium.org, kinuko@chromium.org
TBR=thestig@chromium.org
# TBR thestig@ for chrome/renderer/resources/renderer_resources.grd

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

[satorux1, 2013-10-30 07:59:24]

I plan to send this to benwells@ but, kinuko@, could you take a look first?

[kinuko, 2013-10-30 10:45:04]

The code lgtm, I think it's ready to ask extension owners for real review.

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc:38:
error.name = kSecurityError;
nit: should we give some descriptive message in error.message as well?

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/file_system_provider/file_system_provider_apitest.cc
(right):

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/file_system_provider/file_system_provider_apitest.cc:7:
namespace chrome {
chrome -> extensions ?

https://codereview.chromium.org/50703013/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/50703013/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_permission_features.json:339: "extension_types":
["platform_app"]
So are we going to implement this as a platform_app API or it's tentative and
we'll revisit this later?

[satorux1, 2013-10-31 01:51:26]

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc:38:
error.name = kSecurityError;
On 2013/10/30 10:45:05, kinuko wrote:
> nit: should we give some descriptive message in error.message as well?

Done.

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/file_system_provider/file_system_provider_apitest.cc
(right):

https://codereview.chromium.org/50703013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/file_system_provider/file_system_provider_apitest.cc:7:
namespace chrome {
On 2013/10/30 10:45:05, kinuko wrote:
> chrome -> extensions ?

oops. done.

https://codereview.chromium.org/50703013/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/50703013/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_permission_features.json:339: "extension_types":
["platform_app"]
On 2013/10/30 10:45:05, kinuko wrote:
> So are we going to implement this as a platform_app API or it's tentative and
> we'll revisit this later?

It's tentative. At this moment, I'm thinking about platform apps without icons
in the app launcher (you can do this with "display_in_launcher": false in
manifest.json).

[satorux1, 2013-10-31 01:59:12]

+benwells

[benwells, 2013-10-31 06:06:11]

On 2013/10/31 01:59:12, satorux1 wrote:
> +benwells

First question - has the API gone through API review? Can you send me a link to
the design docs or relevant things like that?

[satorux1, 2013-10-31 06:18:27]

On 2013/10/31 06:06:11, benwells wrote:
> On 2013/10/31 01:59:12, satorux1 wrote:
> > +benwells
> 
> First question - has the API gone through API review? Can you send me a link
to
> the design docs or relevant things like that?

Yes. IIRC, you were there at the API review meeting. :) Maybe you left earlier?

https://docs.google.com/a/chromium.org/document/d/1VkaRYDP3RMESygl6Po0ajimXqi...

[benwells, 2013-10-31 23:58:37]

+kalman for error handling opinions

[not at google - send to devlin, 2013-11-01 00:18:09]

https://codereview.chromium.org/50703013/diff/110001/chrome/common/extensions...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/50703013/diff/110001/chrome/common/extensions...
chrome/common/extensions/api/file_system_provider.idl:28: DOMString
fileSystemId);
I understand why you want to do this - setting chrome.runtime.lastError to a
DOMError would be a confusing API, and I agree that you'll want to use a
DOMError to match the file system API.

If you want consistency with DOM, how about using 2 callbacks? A success
callback and a failure callback. That model will be easier to move to a
Promise-based API when we do that, compared to including the error inline.

[satorux1, 2013-11-01 01:02:03]

https://codereview.chromium.org/50703013/diff/110001/chrome/common/extensions...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/50703013/diff/110001/chrome/common/extensions...
chrome/common/extensions/api/file_system_provider.idl:28: DOMString
fileSystemId);
On 2013/11/01 00:18:10, kalman wrote:
> I understand why you want to do this - setting chrome.runtime.lastError to a
> DOMError would be a confusing API, and I agree that you'll want to use a
> DOMError to match the file system API.
> 
> If you want consistency with DOM, how about using 2 callbacks? A success
> callback and a failure callback. That model will be easier to move to a
> Promise-based API when we do that, compared to including the error inline.

That would be great but can we do that? Actually, kinuko@ also suggested that
way, and the API would look like:

  mount(DOMString displayName,
        MountCallback callback,
        ErrorCallback errorCallback);

However, seems to me that extension function can only take one callback. My
understanding is that "result_ = Results::Create(...); SendResponse(true)" will
run the callback passed as the last parameter of an API function).

Maybe we can emulate the two-callback API with custom bindings?

[satorux1, 2013-11-01 08:21:52]

Tried the two-callback approach, but didn't work. Here's what I did:

1) changed mount() in IDL to take two callbacks, like mount(name, callback,
errorCallback)
2) changed callers of mount() in test.js and pass two callbacks
3) in custom_bindings.js, tried to handle the two callbacks but was stuck.

 apiFunctions.setCustomCallback(
    'mount',
    function(name, request, response) {
       ... how to handle two callbacks here??


request.callback gives access to the last callback passed to mount(), but there
seems to be no way to access the one before the last callback (in this case,
callback for the success case).

I'm willing to switch to the two-callback approach, but if it's difficult, can
we proceed with the current approach?

BTW, in Patch 5, I changed the code to use the real DOMError object rather thank
a fake Dictionary.

[not at google - send to devlin, 2013-11-02 00:46:35]

On 2013/11/01 08:21:52, satorux1 wrote:
> Tried the two-callback approach, but didn't work. Here's what I did:
> 
> 1) changed mount() in IDL to take two callbacks, like mount(name, callback,
> errorCallback)
> 2) changed callers of mount() in test.js and pass two callbacks
> 3) in custom_bindings.js, tried to handle the two callbacks but was stuck.
> 
>  apiFunctions.setCustomCallback(
>     'mount',
>     function(name, request, response) {
>        ... how to handle two callbacks here??
> 
> 
> request.callback gives access to the last callback passed to mount(), but
there
> seems to be no way to access the one before the last callback (in this case,
> callback for the success case).
> 
> I'm willing to switch to the two-callback approach, but if it's difficult, can
> we proceed with the current approach?
> 
> BTW, in Patch 5, I changed the code to use the real DOMError object rather
thank
> a fake Dictionary.

I didn't have time to properly look into this today, so I did it all in a bit of
a rush, but you should be able to do something like this:

https://codereview.chromium.org/55903005

you might have to do something in the updateArgumentsPostValidate method, I'm
not sure.

[satorux1, 2013-11-05 05:35:24]

On 2013/11/02 00:46:35, kalman wrote:
> On 2013/11/01 08:21:52, satorux1 wrote:
> > Tried the two-callback approach, but didn't work. Here's what I did:
> > 
> > 1) changed mount() in IDL to take two callbacks, like mount(name, callback,
> > errorCallback)
> > 2) changed callers of mount() in test.js and pass two callbacks
> > 3) in custom_bindings.js, tried to handle the two callbacks but was stuck.
> > 
> >  apiFunctions.setCustomCallback(
> >     'mount',
> >     function(name, request, response) {
> >        ... how to handle two callbacks here??
> > 
> > 
> > request.callback gives access to the last callback passed to mount(), but
> there
> > seems to be no way to access the one before the last callback (in this case,
> > callback for the success case).
> > 
> > I'm willing to switch to the two-callback approach, but if it's difficult,
can
> > we proceed with the current approach?
> > 
> > BTW, in Patch 5, I changed the code to use the real DOMError object rather
> thank
> > a fake Dictionary.
> 
> I didn't have time to properly look into this today, so I did it all in a bit
of
> a rush, but you should be able to do something like this:
> 
> https://codereview.chromium.org/55903005
> 
> you might have to do something in the updateArgumentsPostValidate method, I'm
> not sure.

Thank you for providing the rough sketch. I was able to switch to the
two-callback API based on your idea. Please take a look. Diff from Patch Set 6
to 7 contains the changes.

[satorux1, 2013-11-06 23:02:31]

friendly ping.

[satorux1, 2013-11-07 07:37:01]

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:18:
// so we can use the error callback later.
kalman: I took another look at other APIs, but could not find a better idea for
holding the error callback. I'm willing to switch to a cleaner solution, so
please give your suggestions. If there is no other easy way, the piggybacking
trick may be acceptable. :P

[not at google - send to devlin, 2013-11-07 15:58:04]

CC+jyasskin since he might be interested in the Promise point.

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:18:
// so we can use the error callback later.
On 2013/11/07 07:37:02, satorux1 wrote:
> kalman: I took another look at other APIs, but could not find a better idea
for
> holding the error callback. I'm willing to switch to a cleaner solution, so
> please give your suggestions. If there is no other easy way, the piggybacking
> trick may be acceptable. :P

I think the piggybacking trick is fine for now. When we switch over to using
Promise-based APIs we'll get a new error handling mechanism which this will fit
perfectly into.

As a nit, I'd put it on a variable errorCallback_ though (trailing _).

[satorux1, 2013-11-08 02:25:53]

In this patch, I also

- moved file_system_provider_api* to c/b/chromeos/extensions using
"implemented_in" directive in the IDL
- did namespace migration from WebKit to blink
- added c/b/chromeos/extensions/file_system_provider/OWNERS

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:18:
// so we can use the error callback later.
On 2013/11/07 15:58:05, kalman wrote:
> On 2013/11/07 07:37:02, satorux1 wrote:
> > kalman: I took another look at other APIs, but could not find a better idea
> for
> > holding the error callback. I'm willing to switch to a cleaner solution, so
> > please give your suggestions. If there is no other easy way, the
piggybacking
> > trick may be acceptable. :P
> 
> I think the piggybacking trick is fine for now. When we switch over to using
> Promise-based APIs we'll get a new error handling mechanism which this will
fit
> perfectly into.
> 
> As a nit, I'd put it on a variable errorCallback_ though (trailing _).

Thanks. Promise-based APIs sound good. Added trailing _ here.

[benwells, 2013-11-08 03:34:30]

Just some little questions... apart from that lg.

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:51: #include
"chrome/renderer/extensions/file_system_provider_natives.h"
nit: sorting

https://codereview.chromium.org/50703013/diff/500001/chrome/browser/extension...
File
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/50703013/diff/500001/chrome/browser/extension...
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc:13:
const char kOK[] = "OK";
nit: no longer used.

https://codereview.chromium.org/50703013/diff/500001/chrome/browser/extension...
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc:52:
"Empty display name is not allowed"));
Please use a constant for this too.

https://codereview.chromium.org/50703013/diff/670001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/50703013/diff/670001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:16:
const char kEncodingError[] = "EncodingError";
Do you need to declare all of these? Only one is used, so you could just add
them as they are needed and prevent the dead code.

Or, as their generic file system api errors, can they go somewhere more generic?

https://codereview.chromium.org/50703013/diff/670001/chrome/renderer/extensio...
File chrome/renderer/extensions/file_system_provider_natives.cc (right):

https://codereview.chromium.org/50703013/diff/670001/chrome/renderer/extensio...
chrome/renderer/extensions/file_system_provider_natives.cc:24: void
fileSystemProviderNatives::GetDOMError(
This seems like it would be useful to other APIs. Do any other APIs use it?
Maybe you could put it in the same natives file as the file system.

We seem to have a lot of potentially duplicated file system native api
implementation code in the renderer (e.g. file_system, media_galleries...),
maybe we could have file_api_helper_natives or something.

[satorux1, 2013-11-08 03:57:04]

Thank you for the feedback.

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/50703013/diff/360001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:51: #include
"chrome/renderer/extensions/file_system_provider_natives.h"
On 2013/11/08 03:34:31, benwells wrote:
> nit: sorting

Done.

https://codereview.chromium.org/50703013/diff/500001/chrome/browser/extension...
File
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/50703013/diff/500001/chrome/browser/extension...
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc:13:
const char kOK[] = "OK";
On 2013/11/08 03:34:31, benwells wrote:
> nit: no longer used.

Done. Good catch!

https://codereview.chromium.org/50703013/diff/500001/chrome/browser/extension...
chrome/browser/extensions/api/file_system_provider/file_system_provider_api.cc:52:
"Empty display name is not allowed"));
On 2013/11/08 03:34:31, benwells wrote:
> Please use a constant for this too.

Done.

https://codereview.chromium.org/50703013/diff/670001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/50703013/diff/670001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:16:
const char kEncodingError[] = "EncodingError";
On 2013/11/08 03:34:31, benwells wrote:
> Do you need to declare all of these? Only one is used, so you could just add
> them as they are needed and prevent the dead code.
> 
> Or, as their generic file system api errors, can they go somewhere more
generic?

Done. Removed unused errors. Clang raised errors too. :)

https://codereview.chromium.org/50703013/diff/670001/chrome/renderer/extensio...
File chrome/renderer/extensions/file_system_provider_natives.cc (right):

https://codereview.chromium.org/50703013/diff/670001/chrome/renderer/extensio...
chrome/renderer/extensions/file_system_provider_natives.cc:24: void
fileSystemProviderNatives::GetDOMError(
On 2013/11/08 03:34:31, benwells wrote:
> This seems like it would be useful to other APIs. Do any other APIs use it?
> Maybe you could put it in the same natives file as the file system.
> 
> We seem to have a lot of potentially duplicated file system native api
> implementation code in the renderer (e.g. file_system, media_galleries...),
> maybe we could have file_api_helper_natives or something.

Good idea. Moved to file_system_natives.cc.

[benwells, 2013-11-08 04:48:53]

lgtm

[satorux1, 2013-11-08 06:15:22]

Committed patchset #10 manually as r233799 (presubmit successful).

[Lei Zhang, 2013-11-08 07:04:40]

.grd lgtm stamp

[Nico, 2013-11-09 01:52:02]

This added logspam to the build output:

ninja: Entering directory `out/Release'
[9-4.4-10804] RULE Generating C++ code from file_system_provider.idl IDL files
fileSystemProvider must have a namespace-level comment. This will appear on the
API summary page.
[9-1.7-10760] ACTION Generating C++ API bundle code
fileSystemProvider must have a namespace-level comment. This will appear on the
API summary page.

Is it possible to fix this?

[satorux1, 2013-11-09 11:16:59]

On 2013/11/09 01:52:02, Nico (ooo until Nov 12) wrote:
> This added logspam to the build output:
> 
> ninja: Entering directory `out/Release'
> [9-4.4-10804] RULE Generating C++ code from file_system_provider.idl IDL files
> fileSystemProvider must have a namespace-level comment. This will appear on
the
> API summary page.
> [9-1.7-10760] ACTION Generating C++ API bundle code
> fileSystemProvider must have a namespace-level comment. This will appear on
the
> API summary page.
> 
> Is it possible to fix this?

Oops. I wasn't aware of this. Let me fix this as the first thing on Monday JST
(I don't have access to code right now). 

A quick fix would be to add "// TODO(satorux): Write this" before the line of
"[platforms=("chromeos"),", if anyone wants to suppress the spam before Monday.