Declare providing extension capabilities in the manifest.

Declare providing extension capabilities in the manifest.

Instead of mount options and runtime checks for event handlers, this CL moves
those capabilities to the manifest.

TEST=browser_tests: *FileSystemProvider*{Mount|Configure}
BUG=None

Committed: https://crrev.com/09db9160ab8676c5683383c5f39b59fe877a4e32
Cr-Commit-Position: refs/heads/master@{#328932}

[mtomasz, 2015-04-23 16:34:07]

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

[mtomasz, 2015-04-23 16:34:17]

@kalman: PTAL. Thanks.

[not at google - send to devlin, 2015-04-23 23:16:35]

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

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_manager_private.idl:178: enum Source { file,
device, network };
When https://codereview.chromium.org/1100333006/ goes in you'll be able to
directly reference the other file and not need to declare this enum at all,
i.e., below where you have:

// Source of file systems' data.
Source source;

you should be able to have:

// Source of the file system's data.
fileSystemProvider.Source source;

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

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:68: boolean
multiple_mounts;
multipleMounts would be more consistent.

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:71: Source source;
This is now required; was that an intentional change? should there be, or does
there need to be, an "other" in the source?

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:53:
return FileSystemProviderCapabilities();
Rather than copying the FileSystemProviderCapabilities could you return a
pointer?

Obviously, in the default case - no you couldn't - but is there a reason callers
couldn't deal with a null result? You could also lazily create the default
capabilities and return a pointer to that, if you prefer.

Hopefully this will mean you don't need to define a default constructor, too.

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h:21:
class FileSystemProviderCapabilities : public Extension::ManifestData {
Consider declaring this here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

that will
- let you automatically generate much of the code here (in the CC file at least)
- let you automatically generate docs
- easily update as you go
- maybe even let you reference the enum sources from the APIs directly, rather
than defining them yet-again?

https://codereview.chromium.org/1077823005/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/mount/test.js
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/mount/test.js:158:
chrome.test.assertEq('network', extensions[0].source);
you should be able to use the symbolic constants here, rather than the strings

[mtomasz, 2015-04-24 12:16:11]

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

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_manager_private.idl:178: enum Source { file,
device, network };
On 2015/04/23 23:16:35, kalman wrote:
> When https://codereview.chromium.org/1100333006/ goes in you'll be able to
> directly reference the other file and not need to declare this enum at all,
> i.e., below where you have:
> 
> // Source of file systems' data.
> Source source;
> 
> you should be able to have:
> 
> // Source of the file system's data.
> fileSystemProvider.Source source;

Done.

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

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:68: boolean
multiple_mounts;
On 2015/04/23 23:16:35, kalman wrote:
> multipleMounts would be more consistent.

In manifest we always use underscored_keys. So we either have consistency within
manifest, or consistency with results of
fileBrowserPrivate.getProvidingExtensions(). Which one do you prefer?

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:71: Source source;
On 2015/04/23 23:16:35, kalman wrote:
> This is now required; was that an intentional change? should there be, or does
> there need to be, an "other" in the source?

I found the "OTHER" an overkill. Instead I decided to use a default "FILE" if
the entire "file_system_provider" section is not available. The manifest section
is optional, but all keys required if the section is declared.

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2015/04/23 23:16:35, kalman wrote:
> 2015

Done.

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:53:
return FileSystemProviderCapabilities();
On 2015/04/23 23:16:35, kalman wrote:
> Rather than copying the FileSystemProviderCapabilities could you return a
> pointer?
> 
> Obviously, in the default case - no you couldn't - but is there a reason
callers
> couldn't deal with a null result? You could also lazily create the default
> capabilities and return a pointer to that, if you prefer.
> 
> Hopefully this will mean you don't need to define a default constructor, too.

Good idea. Done.

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h:21:
class FileSystemProviderCapabilities : public Extension::ManifestData {
On 2015/04/23 23:16:35, kalman wrote:
> Consider declaring this here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> 
> that will
> - let you automatically generate much of the code here (in the CC file at
least)
> - let you automatically generate docs
> - easily update as you go
> - maybe even let you reference the enum sources from the APIs directly, rather
> than defining them yet-again?

I often feel reluctant to use generated types widely. I usually convert them to
manually written types as soon as possible. The reason is that the auto
generated classes are not very neat: not modifiable, in inconvenient namespaces,
often break style guides, and enums have some extra values like X_NONE, which
forces to handle them in each switch case (or use default: which disables
compile checks for unhandled values) even though the enum value is always
mandatory.

I'm not sure about moving "Capabilities" dictionary (aka file_system_provider
manifest section) from file_system_provider.idl to manifest_types.json, as we
want to generate the "Capabilities" documentation in File System Provider API
documentation page, instead of the manifest one. In the manifest one, we'd
simply like to mention that such section exists with a link to File System
Provider API documentation with more details. Similarly to File Browser Handler
API, see:

https://developer.chrome.com/extensions/manifest.

WDYT?

https://codereview.chromium.org/1077823005/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/mount/test.js
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/mount/test.js:158:
chrome.test.assertEq('network', extensions[0].source);
On 2015/04/23 23:16:35, kalman wrote:
> you should be able to use the symbolic constants here, rather than the strings

Done.

[not at google - send to devlin, 2015-04-24 16:18:32]

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

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:68: boolean
multiple_mounts;
On 2015/04/24 12:16:11, mtomasz wrote:
> On 2015/04/23 23:16:35, kalman wrote:
> > multipleMounts would be more consistent.
> 
> In manifest we always use underscored_keys. So we either have consistency
within
> manifest, or consistency with results of
> fileBrowserPrivate.getProvidingExtensions(). Which one do you prefer?

There are plenty of counterexamples to this rule, but for better or worse, we
typically use underscores for manifest keys and camelcase for APIs. It should
really be camelcase for everything.

For an example of something where we differ in manifest and the API, see the
browser action API; the manifest keys are browser_action,
browser_action.default_title, etc; but the API uses browserAction,
browserAction.setTitle, browserAction.setTitle({imageData}), etc.

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h:21:
class FileSystemProviderCapabilities : public Extension::ManifestData {
On 2015/04/24 12:16:11, mtomasz wrote:
> On 2015/04/23 23:16:35, kalman wrote:
> > Consider declaring this here:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> > 
> > that will
> > - let you automatically generate much of the code here (in the CC file at
> least)
> > - let you automatically generate docs
> > - easily update as you go
> > - maybe even let you reference the enum sources from the APIs directly,
rather
> > than defining them yet-again?
> 
> I often feel reluctant to use generated types widely. I usually convert them
to
> manually written types as soon as possible. The reason is that the auto
> generated classes are not very neat: not modifiable, in inconvenient
namespaces,
> often break style guides, and enums have some extra values like X_NONE, which
> forces to handle them in each switch case (or use default: which disables
> compile checks for unhandled values) even though the enum value is always
> mandatory.

I agree. My other issue with using generated types directly is that they're
harder to find if you're browsing the documentation using grep; only codesearch
will find things properly.

You can still many of the benefits using the generate types, though; generating
the parsing logic (even if you just chuck that straight into a new struct),
compile safety now and as you change things, documentation generation, and
hopefully cross-referencing the shared data, like enums. I did something similar
for ExternallyConnectable; declared the type in the JSON file, parsed it, then
put it in a new struct that I defined in code.

> 
> I'm not sure about moving "Capabilities" dictionary (aka file_system_provider
> manifest section) from file_system_provider.idl to manifest_types.json, as we
> want to generate the "Capabilities" documentation in File System Provider API
> documentation page, instead of the manifest one. In the manifest one, we'd
> simply like to mention that such section exists with a link to File System
> Provider API documentation with more details. Similarly to File Browser
Handler
> API, see:
> 
> https://developer.chrome.com/extensions/manifest.
> 
> WDYT?

Does this come back to the question about consistency within the API (reusing
the same structure), or consistency with the manifest style (underscores in
manifest, camelcase in API)? Either way, I think it's a good idea to declare the
type in file_system_provider.idl rather than manifest_types.json - and that
doesn't stop you from using it here in the same way. And like I said in the
other comment, my preference *is* for manifest consistency rather than API
consistency.

https://codereview.chromium.org/1077823005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
(right):

https://codereview.chromium.org/1077823005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc:496:
providing_extension->multiple_mounts = info.capabilities.multiple_mounts();
assigning this twice?

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:48:
return capabilities;
just return the expression from lines 45-47, without the intermediate
assignment?

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h
(right):

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h:23:
FileSystemProviderCapabilities();
do you still need to define the default constructor?

[mtomasz, 2015-04-24 17:12:10]

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

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider.idl:68: boolean
multiple_mounts;
On 2015/04/24 16:18:32, kalman wrote:
> On 2015/04/24 12:16:11, mtomasz wrote:
> > On 2015/04/23 23:16:35, kalman wrote:
> > > multipleMounts would be more consistent.
> > 
> > In manifest we always use underscored_keys. So we either have consistency
> within
> > manifest, or consistency with results of
> > fileBrowserPrivate.getProvidingExtensions(). Which one do you prefer?
> 
> There are plenty of counterexamples to this rule, but for better or worse, we
> typically use underscores for manifest keys and camelcase for APIs. It should
> really be camelcase for everything.
> 
> For an example of something where we differ in manifest and the API, see the
> browser action API; the manifest keys are browser_action,
> browser_action.default_title, etc; but the API uses browserAction,
> browserAction.setTitle, browserAction.setTitle({imageData}), etc.

Well, this is exactly what I'm doing. This dictionary is used for manifest
parsing (see comment). For extensions, on JS side, camelCase is used instead,
see file_manager_private.idl.

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h
(right):

https://codereview.chromium.org/1077823005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h:21:
class FileSystemProviderCapabilities : public Extension::ManifestData {
On 2015/04/24 16:18:32, kalman wrote:
> On 2015/04/24 12:16:11, mtomasz wrote:
> > On 2015/04/23 23:16:35, kalman wrote:
> > > Consider declaring this here:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> > > 
> > > that will
> > > - let you automatically generate much of the code here (in the CC file at
> > least)
> > > - let you automatically generate docs
> > > - easily update as you go
> > > - maybe even let you reference the enum sources from the APIs directly,
> rather
> > > than defining them yet-again?
> > 
> > I often feel reluctant to use generated types widely. I usually convert them
> to
> > manually written types as soon as possible. The reason is that the auto
> > generated classes are not very neat: not modifiable, in inconvenient
> namespaces,
> > often break style guides, and enums have some extra values like X_NONE,
which
> > forces to handle them in each switch case (or use default: which disables
> > compile checks for unhandled values) even though the enum value is always
> > mandatory.
> 
> I agree. My other issue with using generated types directly is that they're
> harder to find if you're browsing the documentation using grep; only
codesearch
> will find things properly.
> 
> You can still many of the benefits using the generate types, though;
generating
> the parsing logic (even if you just chuck that straight into a new struct),
> compile safety now and as you change things, documentation generation, and
> hopefully cross-referencing the shared data, like enums. I did something
similar
> for ExternallyConnectable; declared the type in the JSON file, parsed it, then
> put it in a new struct that I defined in code.

I may be missing sth here, but it's exactly what I'm doing here. I'm using
Capabilities dictionary from file_system_provider.idl to parse the manifest
"file_system_provider" section.

> 
> > 
> > I'm not sure about moving "Capabilities" dictionary (aka
file_system_provider
> > manifest section) from file_system_provider.idl to manifest_types.json, as
we
> > want to generate the "Capabilities" documentation in File System Provider
API
> > documentation page, instead of the manifest one. In the manifest one, we'd
> > simply like to mention that such section exists with a link to File System
> > Provider API documentation with more details. Similarly to File Browser
> Handler
> > API, see:
> > 
> > https://developer.chrome.com/extensions/manifest.
> > 
> > WDYT?
> 
> Does this come back to the question about consistency within the API (reusing
> the same structure), or consistency with the manifest style (underscores in
> manifest, camelcase in API)? Either way, I think it's a good idea to declare
the
> type in file_system_provider.idl rather than manifest_types.json - and that
> doesn't stop you from using it here in the same way. And like I said in the
> other comment, my preference *is* for manifest consistency rather than API
> consistency.

I think it's exactly what I'm doing.

https://codereview.chromium.org/1077823005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
(right):

https://codereview.chromium.org/1077823005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc:496:
providing_extension->multiple_mounts = info.capabilities.multiple_mounts();
On 2015/04/24 16:18:32, kalman wrote:
> assigning this twice?

Done.

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:48:
return capabilities;
On 2015/04/24 16:18:32, kalman wrote:
> just return the expression from lines 45-47, without the intermediate
> assignment?

Done.

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h
(right):

https://codereview.chromium.org/1077823005/diff/60001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.h:23:
FileSystemProviderCapabilities();
On 2015/04/24 16:18:32, kalman wrote:
> do you still need to define the default constructor?

Yes. I store capabilities as a member in ProvidingExtensionInfo (service.cc). I
also find it useful, as I can define default capabilities with the default
constructor.

[not at google - send to devlin, 2015-04-24 17:49:31]

sorry yes, I didn't read carefully enough that you were already doing that. lgtm
but I have some other comments to consider, loop back if you want to discuss
more.

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
(right):

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc:496: switch
(info.capabilities.source()) {
providing_extension->source and info.capabilities.source are now the same type
right, so you can directly assign them?

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:56: typedef
extensions::FileSystemProviderCapabilities Capabilities;
prefer not to typedef things in the header file, though if you really need to
for some reason, IIRC it should be "using Capabilities =
extensions::FileSystemProviderCapabilities";

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:60:
&idl_capabilities)) {
Btw, the nice thing about using manifest_types.json is that it has:

   "compiler_options": {
      "generate_error_messages": true
    },

which means that instead of having a single "failed" you get "failed
because...", and it also warns if there was something malformed that it managed
to parse anyway (e.g. an extra field, or an optional value that didn't parse".
Generally speaking I think it's nicer to gracefully recover from failing to
parse a manifest key, rather than entirely failing the installation of an
extension. In this case, it means writing out to the warnings, rather than
populating an error.

with that in mind, it would be good to make that a per-type annotation, so that
we can avoid adding "generate_error_messages" to the entire API.  I think that
would actually be tricky because you'd need to know how to transitively apply
the warning messages across other types.

Therefore, perhaps try just putting the "generate_error_messages" annotation on
the file system providers API and see if it (a) compiles, (b) doesn't add a
significant binary size increase. If not, consider moving the type into
manifest_types.json after all, if you think that the advantages there outweigh
the cost here. Ultimately the API and the documentation will be the same, with
better messages/recovery on failure - it's just some extra complexity on the
implementation side - but not too much, I think.

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:67:
case ::extensions::api::file_system_provider::SOURCE_FILE:
you're already in the extensions namespace, do you really need the ::extensions
out the front of these?

[mtomasz, 2015-04-24 17:54:01]

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:60:
&idl_capabilities)) {
On 2015/04/24 17:49:31, kalman wrote:
> Btw, the nice thing about using manifest_types.json is that it has:
> 
>    "compiler_options": {
>       "generate_error_messages": true
>     },
> 
> which means that instead of having a single "failed" you get "failed
> because...", and it also warns if there was something malformed that it
managed
> to parse anyway (e.g. an extra field, or an optional value that didn't parse".
> Generally speaking I think it's nicer to gracefully recover from failing to
> parse a manifest key, rather than entirely failing the installation of an
> extension. In this case, it means writing out to the warnings, rather than
> populating an error.
> 
> with that in mind, it would be good to make that a per-type annotation, so
that
> we can avoid adding "generate_error_messages" to the entire API.  I think that
> would actually be tricky because you'd need to know how to transitively apply
> the warning messages across other types.
> 
> Therefore, perhaps try just putting the "generate_error_messages" annotation
on
> the file system providers API and see if it (a) compiles, (b) doesn't add a
> significant binary size increase. If not, consider moving the type into
> manifest_types.json after all, if you think that the advantages there outweigh
> the cost here. Ultimately the API and the documentation will be the same, with
> better messages/recovery on failure - it's just some extra complexity on the
> implementation side - but not too much, I think.

Wow. I didn't know about "generate_error_messages". I was even thinking about
adding such feature. So good it's already there.

I have to run from the office. I'll address other comments once I'm back to the
office (next week).

[mtomasz, 2015-05-07 03:33:59]

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
(right):

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc:496: switch
(info.capabilities.source()) {
On 2015/04/24 17:49:30, kalman wrote:
> providing_extension->source and info.capabilities.source are now the same type
> right, so you can directly assign them?

They are not the same. I convert the generated type to a custom enum. I usually
try to avoid using generated types for more than parsing.

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/1077823005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:56: typedef
extensions::FileSystemProviderCapabilities Capabilities;
On 2015/04/24 17:49:30, kalman wrote:
> prefer not to typedef things in the header file, though if you really need to
> for some reason, IIRC it should be "using Capabilities =
> extensions::FileSystemProviderCapabilities";

Done.

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:60:
&idl_capabilities)) {
On 2015/04/24 17:54:01, mtomasz wrote:
> On 2015/04/24 17:49:31, kalman wrote:
> > Btw, the nice thing about using manifest_types.json is that it has:
> > 
> >    "compiler_options": {
> >       "generate_error_messages": true
> >     },
> > 
> > which means that instead of having a single "failed" you get "failed
> > because...", and it also warns if there was something malformed that it
> managed
> > to parse anyway (e.g. an extra field, or an optional value that didn't
parse".
> > Generally speaking I think it's nicer to gracefully recover from failing to
> > parse a manifest key, rather than entirely failing the installation of an
> > extension. In this case, it means writing out to the warnings, rather than
> > populating an error.
> > 
> > with that in mind, it would be good to make that a per-type annotation, so
> that
> > we can avoid adding "generate_error_messages" to the entire API.  I think
that
> > would actually be tricky because you'd need to know how to transitively
apply
> > the warning messages across other types.
> > 
> > Therefore, perhaps try just putting the "generate_error_messages" annotation
> on
> > the file system providers API and see if it (a) compiles, (b) doesn't add a
> > significant binary size increase. If not, consider moving the type into
> > manifest_types.json after all, if you think that the advantages there
outweigh
> > the cost here. Ultimately the API and the documentation will be the same,
with
> > better messages/recovery on failure - it's just some extra complexity on the
> > implementation side - but not too much, I think.
> 
> Wow. I didn't know about "generate_error_messages". I was even thinking about
> adding such feature. So good it's already there.
> 
> I have to run from the office. I'll address other comments once I'm back to
the
> office (next week).

Done. Moved to manifest_types.json.

https://codereview.chromium.org/1077823005/diff/80001/chrome/common/extension...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:67:
case ::extensions::api::file_system_provider::SOURCE_FILE:
On 2015/04/24 17:49:31, kalman wrote:
> you're already in the extensions namespace, do you really need the
::extensions
> out the front of these?

Done.

[mtomasz, 2015-05-07 11:25:26]

@kalman: Done. PTAL one more time, as there is quite a lot of changes.

[not at google - send to devlin, 2015-05-07 22:57:57]

lgtm

https://codereview.chromium.org/1077823005/diff/160001/chrome/common/extensio...
File
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc
(right):

https://codereview.chromium.org/1077823005/diff/160001/chrome/common/extensio...
chrome/common/extensions/api/file_system_provider/file_system_provider_handler.cc:51:
const base::DictionaryValue* section = NULL;
You should just read in |section| as a base::Value.

Populate (as does FromValue) takes a base::Value and will generate an error
message for you if it's not a dictionary. Then, you should be able to
CHECK(extension->manifest()->Get(mkeys::kFileSystemProvider), &section))
(otherwise Keys() doesn't work).

And then you can delete merrors::kInvalidFileSystemProvider.

[mtomasz, 2015-05-08 01:21:40]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-05-08 01:21:41]

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

[commit-bot: I haz the power, 2015-05-08 01:22:46]

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

[commit-bot: I haz the power, 2015-05-08 02:26:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-08 02:26:15]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mtomasz, 2015-05-08 04:16:50]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-05-08 04:16:51]

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

[commit-bot: I haz the power, 2015-05-08 04:17:05]

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

[commit-bot: I haz the power, 2015-05-08 05:48:18]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-08 05:49:06]

Patchset 11 (id:??) landed as
https://crrev.com/09db9160ab8676c5683383c5f39b59fe877a4e32
Cr-Commit-Position: refs/heads/master@{#328932}