Add the boilerplate for actions to File System Provider API.

Add the boilerplate for actions to File System Provider API.

The new chrome.fileSystemProvider.onGetActionsRequested method is designed
to provide special actions per files and directories, such as sharing, pinnig
as well as custom ones such as eg. opening in a browser.

TEST=unit_tests: *FileSystemProviderOperations*Actions*
BUG=449525
Committed: https://crrev.com/2dad8077d9346332cf9853710b08223d6f7de19c
Cr-Commit-Position: refs/heads/master@{#335641}

[mtomasz, 2015-05-27 08:07:46]

mtomasz@chromium.org changed reviewers:
+ benwells@chromium.org, fukino@chromium.org, isherman@chromium.org

[mtomasz, 2015-05-27 08:08:16]

@isherman: PTAL at histograms.
@benwells: PTAL at IDLs.
@fukino: PTAL and everything else.

Thanks.

[benwells, 2015-05-27 08:13:35]

On 2015/05/27 08:08:16, mtomasz wrote:
> @isherman: PTAL at histograms.
> @benwells: PTAL at IDLs.
> @fukino: PTAL and everything else.
> 
> Thanks.

Has this gone through API review?

[mtomasz, 2015-05-27 08:20:12]

On 2015/05/27 08:13:35, benwells wrote:
> On 2015/05/27 08:08:16, mtomasz wrote:
> > @isherman: PTAL at histograms.
> > @benwells: PTAL at IDLs.
> > @fukino: PTAL and everything else.
> > 
> > Thanks.
> 
> Has this gone through API review?

It didn't. Do we need it for each API change, even though it's small?

[mtomasz, 2015-05-27 08:34:40]

On 2015/05/27 08:20:12, mtomasz wrote:
> On 2015/05/27 08:13:35, benwells wrote:
> > On 2015/05/27 08:08:16, mtomasz wrote:
> > > @isherman: PTAL at histograms.
> > > @benwells: PTAL at IDLs.
> > > @fukino: PTAL and everything else.
> > > 
> > > Thanks.
> > 
> > Has this gone through API review?
> 
> It didn't. Do we need it for each API change, even though it's small?

@benwells: I just created a one pager for this CL.
https://docs.google.com/document/d/1S2ZbJF87TGWrBDGp5_Y0IfusWbIT06Z5vkgY9Hmoc...

Let me know if this is enough, or we need a full API review process.

[Ilya Sherman, 2015-05-27 18:17:16]

histograms lgtm

[benwells, 2015-05-27 22:51:02]

On 2015/05/27 08:34:40, mtomasz wrote:
> On 2015/05/27 08:20:12, mtomasz wrote:
> > On 2015/05/27 08:13:35, benwells wrote:
> > > On 2015/05/27 08:08:16, mtomasz wrote:
> > > > @isherman: PTAL at histograms.
> > > > @benwells: PTAL at IDLs.
> > > > @fukino: PTAL and everything else.
> > > > 
> > > > Thanks.
> > > 
> > > Has this gone through API review?
> > 
> > It didn't. Do we need it for each API change, even though it's small?

All public API changes should go through API approval.

> 
> @benwells: I just created a one pager for this CL.
>
https://docs.google.com/document/d/1S2ZbJF87TGWrBDGp5_Y0IfusWbIT06Z5vkgY9Hmoc...
> 
> Let me know if this is enough, or we need a full API review process.

We need the full review process for all public API changes, no matter how small.
I actually think this change is significant as well.

[not at google - send to devlin, 2015-05-27 23:37:57]

kalman@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2015-05-27 23:37:58]

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:45: //
<code>"PIN_TOGGLE"</code> for pinning (saving for offline access) and
could you call it like SAVE_OFFLINE then? "pin" seems UI dependent.

[benwells, 2015-05-28 00:17:37]

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:45: //
<code>"PIN_TOGGLE"</code> for pinning (saving for offline access) and
On 2015/05/27 23:37:58, kalman wrote:
> could you call it like SAVE_OFFLINE then? "pin" seems UI dependent.

Yep it also feels like it should be two actions as well instead of a toggle
action.

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:410: ActionType type;
Why have a type and an ID? Can we just have an ID with some known IDs?

[mtomasz, 2015-05-28 04:12:15]

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:45: //
<code>"PIN_TOGGLE"</code> for pinning (saving for offline access) and
On 2015/05/28 00:17:36, benwells wrote:
> On 2015/05/27 23:37:58, kalman wrote:
> > could you call it like SAVE_OFFLINE then? "pin" seems UI dependent.
> 
> Yep it also feels like it should be two actions as well instead of a toggle
> action.

Good idea. How to name opposite? UNSAVE_OFFLINE? How about:
MARK_FOR_OFFLINE + UNMARK_FOR_OFFLINE?

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:410: ActionType type;
On 2015/05/28 00:17:36, benwells wrote:
> Why have a type and an ID? Can we just have an ID with some known IDs?

I'm fine to combine. How about:

enum CommonActionId { SHARE, TOGGLE_PINNED };

Do we support IDL unions? If so, then we could use:

dictionary Action {
  (DOMString or CommonActionId) id;
}

[not at google - send to devlin, 2015-05-28 16:02:39]

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:410: ActionType type;
On 2015/05/28 04:12:15, mtomasz wrote:
> On 2015/05/28 00:17:36, benwells wrote:
> > Why have a type and an ID? Can we just have an ID with some known IDs?
> 
> I'm fine to combine. How about:
> 
> enum CommonActionId { SHARE, TOGGLE_PINNED };
> 
> Do we support IDL unions? If so, then we could use:
> 
> dictionary Action {
>   (DOMString or CommonActionId) id;
> }

That's precisely what we support, and it was even Ben that added it.

[mtomasz, 2015-06-23 02:05:36]

@benwells, @fukino: PTAL again. Thanks.

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:45: //
<code>"PIN_TOGGLE"</code> for pinning (saving for offline access) and
On 2015/05/28 04:12:15, mtomasz wrote:
> On 2015/05/28 00:17:36, benwells wrote:
> > On 2015/05/27 23:37:58, kalman wrote:
> > > could you call it like SAVE_OFFLINE then? "pin" seems UI dependent.
> > 
> > Yep it also feels like it should be two actions as well instead of a toggle
> > action.
> 
> Good idea. How to name opposite? UNSAVE_OFFLINE? How about:
> MARK_FOR_OFFLINE + UNMARK_FOR_OFFLINE?

Done.

https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:410: ActionType type;
On 2015/05/28 16:02:39, kalman wrote:
> On 2015/05/28 04:12:15, mtomasz wrote:
> > On 2015/05/28 00:17:36, benwells wrote:
> > > Why have a type and an ID? Can we just have an ID with some known IDs?
> > 
> > I'm fine to combine. How about:
> > 
> > enum CommonActionId { SHARE, TOGGLE_PINNED };
> > 
> > Do we support IDL unions? If so, then we could use:
> > 
> > dictionary Action {
> >   (DOMString or CommonActionId) id;
> > }
> 
> That's precisely what we support, and it was even Ben that added it.

Sadly unions do not work when both strings and enums are used (as both are
implemented as strings). Let me use DOMString only here. Reworking the IDL code
generator would take significant time, which I believe shouldn't block this CL.

[mtomasz, 2015-06-23 02:06:50]

On 2015/06/23 02:05:36, mtomasz wrote:
> @benwells, @fukino: PTAL again. Thanks.
> 
>
https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
> File chrome/common/extensions/api/file_system_provider.idl (right):
> 
>
https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
> chrome/common/extensions/api/file_system_provider.idl:45: //
> <code>"PIN_TOGGLE"</code> for pinning (saving for offline access) and
> On 2015/05/28 04:12:15, mtomasz wrote:
> > On 2015/05/28 00:17:36, benwells wrote:
> > > On 2015/05/27 23:37:58, kalman wrote:
> > > > could you call it like SAVE_OFFLINE then? "pin" seems UI dependent.
> > > 
> > > Yep it also feels like it should be two actions as well instead of a
toggle
> > > action.
> > 
> > Good idea. How to name opposite? UNSAVE_OFFLINE? How about:
> > MARK_FOR_OFFLINE + UNMARK_FOR_OFFLINE?
> 
> Done.
> 
>
https://codereview.chromium.org/1151763007/diff/20001/chrome/common/extension...
> chrome/common/extensions/api/file_system_provider.idl:410: ActionType type;
> On 2015/05/28 16:02:39, kalman wrote:
> > On 2015/05/28 04:12:15, mtomasz wrote:
> > > On 2015/05/28 00:17:36, benwells wrote:
> > > > Why have a type and an ID? Can we just have an ID with some known IDs?
> > > 
> > > I'm fine to combine. How about:
> > > 
> > > enum CommonActionId { SHARE, TOGGLE_PINNED };
> > > 
> > > Do we support IDL unions? If so, then we could use:
> > > 
> > > dictionary Action {
> > >   (DOMString or CommonActionId) id;
> > > }
> > 
> > That's precisely what we support, and it was even Ben that added it.
> 
> Sadly unions do not work when both strings and enums are used (as both are
> implemented as strings). Let me use DOMString only here. Reworking the IDL
code
> generator would take significant time, which I believe shouldn't block this
CL.

Sorry for ugly diff, let me try to fix it.

[mtomasz, 2015-06-23 02:07:48]

Patchset #3 (id:40001) has been deleted

[mtomasz, 2015-06-23 02:19:59]

Fixed diffs. PTAL.

[fukino, 2015-06-23 05:58:51]

LGTM for files except for idls and histograms.

[benwells, 2015-06-23 06:29:39]

https://codereview.chromium.org/1151763007/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:189: long requestId;
Why is there a requestId?

[mtomasz, 2015-06-23 06:34:25]

https://codereview.chromium.org/1151763007/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:189: long requestId;
On 2015/06/23 06:29:38, benwells wrote:
> Why is there a requestId?

All FSP requests (file system related events wired to File API and/or Files app)
are abortable with the onAbortRequested event. The requestId field is used to
identify which operation to abort.

[benwells, 2015-06-23 06:51:06]

lgtm

https://codereview.chromium.org/1151763007/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/file_system_provider.idl (right):

https://codereview.chromium.org/1151763007/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:189: long requestId;
On 2015/06/23 06:34:25, mtomasz wrote:
> On 2015/06/23 06:29:38, benwells wrote:
> > Why is there a requestId?
> 
> All FSP requests (file system related events wired to File API and/or Files
app)
> are abortable with the onAbortRequested event. The requestId field is used to
> identify which operation to abort.

ok makes sense.

[mtomasz, 2015-06-23 07:09:52]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-06-23 07:09:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1151763007/#ps80001
(title: "Fixed.")

[commit-bot: I haz the power, 2015-06-23 07:10:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151763007/80001

[commit-bot: I haz the power, 2015-06-23 07:15:46]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-06-23 07:16:51]

Patchset 4 (id:??) landed as
https://crrev.com/2dad8077d9346332cf9853710b08223d6f7de19c
Cr-Commit-Position: refs/heads/master@{#335641}