Add extension permissions for Media Gallery API.

Add extension permissions for Media Gallery API.

BUG=NONE
TEST=NONE

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

[vandebo (ex-Chrome), 2012-07-17 23:36:56]

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
chrome/common/extensions/permissions/api_permission.cc:172: {
kMediaGalleriesDefaultAll, "mediaGalleriesDefaultAll",
I'm not sure what to call this permission. Regular extensions won't get access
to any media galleries by default, the user will have to configure/approve
access to them in the constrained dialog.  However, platform apps can specify
this permission to get access to all configured galleries by default, hence
"DefaultAll."

[Evan Stade, 2012-07-18 05:11:00]

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/app/generated_r...
chrome/app/generated_resources.grd:4412: +        Access all of your media files
without further prompts
I don't like the "without further prompts part", that's implicit in all
permissions. How about "Access all known media files on your device".

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:37:
SetResult(list);
I think the typical thing is SetResult(false). Also you want to set error_ to
something useful.

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
chrome/common/extensions/permissions/api_permission.cc:101: { kMediaGalleries,
"mediaGalleries" },
no messages?

also is it documented somewhere what these mean?

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
chrome/common/extensions/permissions/api_permission.cc:172: {
kMediaGalleriesDefaultAll, "mediaGalleriesDefaultAll",
On 2012/07/17 23:36:56, vandebo wrote:
> I'm not sure what to call this permission. Regular extensions won't get access
> to any media galleries by default, the user will have to configure/approve
> access to them in the constrained dialog.  However, platform apps can specify
> this permission to get access to all configured galleries by default, hence
> "DefaultAll."

maybe platform apps shouldn't have to specify this permission at all, but just
get it by virtue of being a platform app that has mediaGalleriesRead. I mean
people just click through the permissions prompt anyway. Perhaps the onus of
acting responsibly should be on the app's shoulders.

but setting that aside I would call this mediaGalleriesAccessEverything

[vandebo (ex-Chrome), 2012-07-18 22:43:32]

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/app/generated_r...
chrome/app/generated_resources.grd:4412: +        Access all of your media files
without further prompts
On 2012/07/18 05:11:00, Evan Stade wrote:
> I don't like the "without further prompts part", that's implicit in all
> permissions. How about "Access all known media files on your device".

Done.

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:37:
SetResult(list);
On 2012/07/18 05:11:00, Evan Stade wrote:
> I think the typical thing is SetResult(false). Also you want to set error_ to
> something useful.

Done.

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
chrome/common/extensions/permissions/api_permission.cc:101: { kMediaGalleries,
"mediaGalleries" },
On 2012/07/18 05:11:00, Evan Stade wrote:
> no messages?

Specifying the mediaGalleries permission only makes it possible to call the
functions, you won't get any access unless you specify either read or write. 
Then, even in that case, it still requires a runtime permission grant to get any
access.  I initially had messages but decided that the runtime request made them
unnecessary. i.e. geo permission is runtime and there's not install time message
 (an imperfect analogy since it's not an extension API).
 
> also is it documented somewhere what these mean?

Not yet.  It will go into the API docs.

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
chrome/common/extensions/permissions/api_permission.cc:172: {
kMediaGalleriesDefaultAll, "mediaGalleriesDefaultAll",
On 2012/07/18 05:11:00, Evan Stade wrote:
> On 2012/07/17 23:36:56, vandebo wrote:
> > I'm not sure what to call this permission. Regular extensions won't get
access
> > to any media galleries by default, the user will have to configure/approve
> > access to them in the constrained dialog.  However, platform apps can
specify
> > this permission to get access to all configured galleries by default, hence
> > "DefaultAll."
> 
> maybe platform apps shouldn't have to specify this permission at all, but just
> get it by virtue of being a platform app that has mediaGalleriesRead. I mean
> people just click through the permissions prompt anyway. Perhaps the onus of
> acting responsibly should be on the app's shoulders.

Maybe I could be convinced of this concept, but most of the platform app only
permissions have messages so it sounds like a question to bring up in a larger
context.
 
> but setting that aside I would call this mediaGalleriesAccessEverything

Done.

[Evan Stade, 2012-07-19 00:33:38]

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
chrome/common/extensions/permissions/api_permission.cc:101: { kMediaGalleries,
"mediaGalleries" },
On 2012/07/18 22:43:32, vandebo wrote:
> On 2012/07/18 05:11:00, Evan Stade wrote:
> > no messages?
> 
> Specifying the mediaGalleries permission only makes it possible to call the
> functions, you won't get any access unless you specify either read or write. 

I think that you only want read and write and not to have mediaGalleries.
PermissionSet::HasAccessToFunction seems like the place to do hack this in.

> Then, even in that case, it still requires a runtime permission grant to get
any
> access.  I initially had messages but decided that the runtime request made
them
> unnecessary. i.e. geo permission is runtime and there's not install time
message
>  (an imperfect analogy since it's not an extension API).

yea, for extensions we want install-time messages rather than use-time requests.
(I had to add this for WebRTC for example, another html5 api that was popping up
an infobar.) However the configuration UI is unavoidable in this case so what
you have here is ok. We just need to make sure that the UI has different text
for read and write.

>  
> > also is it documented somewhere what these mean?
> 
> Not yet.  It will go into the API docs.

[vandebo (ex-Chrome), 2012-07-19 04:51:53]

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/1/chrome/common/extensio...
chrome/common/extensions/permissions/api_permission.cc:101: { kMediaGalleries,
"mediaGalleries" },
On 2012/07/19 00:33:38, Evan Stade wrote:
> I think that you only want read and write and not to have mediaGalleries.
> PermissionSet::HasAccessToFunction seems like the place to do hack this in.

I was going to update the extension permission system i.e. PermissionSet to make
the permissions more direct, but aa@ suggested doing this instead.  He just
forwarded you that thread for other reasons.
 
> yea, for extensions we want install-time messages rather than use-time
requests.
> (I had to add this for WebRTC for example, another html5 api that was popping
up
> an infobar.) However the configuration UI is unavoidable in this case so what
> you have here is ok. We just need to make sure that the UI has different text
> for read and write.

Agreed.

[vandebo (ex-Chrome), 2012-07-23 23:12:38]

Updated the CL to:
- imply the mediaGallery permission from the access permissions (read, write,
append).
- remove permissions we don't support yet (write, append)
- constrain the permissions as much as possible for M22.

[Evan Stade, 2012-07-23 23:51:06]

lgtm

http://codereview.chromium.org/10704258/diff/3002/chrome/renderer/resources/e...
File
chrome/renderer/resources/extensions/experimental.media_galleries_custom_bindings.js
(right):

http://codereview.chromium.org/10704258/diff/3002/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/experimental.media_galleries_custom_bindings.js:20:
if (response != undefined) {
I think if (response) is fine. If you have to check specifically against
undefined for some reason, use typeof response !== 'undefined'

[vandebo (ex-Chrome), 2012-07-24 00:00:38]

Matt, can you take a look?  Sorry for adding you at the start instead of the
end.

Trying to get this in today, for M22.

https://chromiumcodereview.appspot.com/10704258/diff/3002/chrome/renderer/res...
File
chrome/renderer/resources/extensions/experimental.media_galleries_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10704258/diff/3002/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.media_galleries_custom_bindings.js:20:
if (response != undefined) {
On 2012/07/23 23:51:06, Evan Stade wrote:
> I think if (response) is fine. If you have to check specifically against
> undefined for some reason, use typeof response !== 'undefined'

Done.

[Matt Perry, 2012-07-24 00:21:07]

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:63: }
What happens if the extension doesn't have read permission? Should we return an
error?

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
chrome/common/extensions/permissions/api_permission.cc:103: {
kMediaGalleriesRead, "mediaGalleriesRead" },
Does the read permission have no warning message?

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
chrome/common/extensions/permissions/api_permission.cc:172: {
kMediaGalleriesAccessEverything, "mediaGalleriesAccessEverything",
I don't see this actually used in this CL. What does it do? The name is a bit
confusing. Does it just combine read/write?

[vandebo (ex-Chrome), 2012-07-24 00:32:57]

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:63: }
On 2012/07/24 00:21:07, Matt Perry wrote:
> What happens if the extension doesn't have read permission? Should we return
an
> error?

It's an implementation detail that we could get here with the MediaGalleries
permission.  Only the MediaGalleriesRead/Write/Append permission should be
specified by extensions. If there's a cleaner way to do that then the implied
permission I've used here, I'd be happy to hear about it.  But given all that,
I'm happy to add an error case if only mediaGalleries is specified.

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
chrome/common/extensions/permissions/api_permission.cc:103: {
kMediaGalleriesRead, "mediaGalleriesRead" },
On 2012/07/24 00:21:07, Matt Perry wrote:
> Does the read permission have no warning message?

No - By itself it only conveys permission to request permission for galleries at
runtime (instead of install time).  (aa@ et al are aware of this concept and
signed off on it.)

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
chrome/common/extensions/permissions/api_permission.cc:172: {
kMediaGalleriesAccessEverything, "mediaGalleriesAccessEverything",
On 2012/07/24 00:21:07, Matt Perry wrote:
> I don't see this actually used in this CL. What does it do? The name is a bit
> confusing. Does it just combine read/write?

Sorry, working on another CL to hook this up.  In the long run this will
probably remain a platform app only permission (whereas read, write, append will
be available for regular extensions). This permission will grant access to all
of the users media galleries without further prompting.  It will be
read/write/append access depending on the the other media gallery permissions.

Commentary: I'd be happy to add comments in the code explaining how/what these
permissions do, but it doesn't seem to be the norm.

[Matt Perry, 2012-07-24 00:51:20]

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:63: }
On 2012/07/24 00:32:57, vandebo wrote:
> On 2012/07/24 00:21:07, Matt Perry wrote:
> > What happens if the extension doesn't have read permission? Should we return
> an
> > error?
> 
> It's an implementation detail that we could get here with the MediaGalleries
> permission.  Only the MediaGalleriesRead/Write/Append permission should be
> specified by extensions. If there's a cleaner way to do that then the implied
> permission I've used here, I'd be happy to hear about it.  But given all that,
> I'm happy to add an error case if only mediaGalleries is specified.

But an extension could get here by only requesting the mediaGalleriesWrite
permission? Is calling this method an error in that case?

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
chrome/common/extensions/permissions/api_permission.cc:172: {
kMediaGalleriesAccessEverything, "mediaGalleriesAccessEverything",
On 2012/07/24 00:32:57, vandebo wrote:
> On 2012/07/24 00:21:07, Matt Perry wrote:
> > I don't see this actually used in this CL. What does it do? The name is a
bit
> > confusing. Does it just combine read/write?
> 
> Sorry, working on another CL to hook this up.  In the long run this will
> probably remain a platform app only permission (whereas read, write, append
will
> be available for regular extensions). This permission will grant access to all
> of the users media galleries without further prompting.  It will be
> read/write/append access depending on the the other media gallery permissions.
> 
> Commentary: I'd be happy to add comments in the code explaining how/what these
> permissions do, but it doesn't seem to be the norm.

maybe mediaGalleriesAllGalleries then? Agreed that adding a comment here is
unusual.

[vandebo (ex-Chrome), 2012-07-24 01:07:59]

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/browser/exte...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:63: }
On 2012/07/24 00:51:20, Matt Perry wrote:
> On 2012/07/24 00:32:57, vandebo wrote:
> > On 2012/07/24 00:21:07, Matt Perry wrote:
> > > What happens if the extension doesn't have read permission? Should we
return
> > an
> > > error?
> > 
> > It's an implementation detail that we could get here with the MediaGalleries
> > permission.  Only the MediaGalleriesRead/Write/Append permission should be
> > specified by extensions. If there's a cleaner way to do that then the
implied
> > permission I've used here, I'd be happy to hear about it.  But given all
that,
> > I'm happy to add an error case if only mediaGalleries is specified.
> 
> But an extension could get here by only requesting the mediaGalleriesWrite
> permission? Is calling this method an error in that case?

Correct.  That's not an error.  But right now, only Read is defined, Write, and
Append will get defined later.

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://chromiumcodereview.appspot.com/10704258/diff/7005/chrome/common/exten...
chrome/common/extensions/permissions/api_permission.cc:172: {
kMediaGalleriesAccessEverything, "mediaGalleriesAccessEverything",
On 2012/07/24 00:51:20, Matt Perry wrote:
> On 2012/07/24 00:32:57, vandebo wrote:
> > On 2012/07/24 00:21:07, Matt Perry wrote:
> > > I don't see this actually used in this CL. What does it do? The name is a
> bit
> > > confusing. Does it just combine read/write?
> > 
> > Sorry, working on another CL to hook this up.  In the long run this will
> > probably remain a platform app only permission (whereas read, write, append
> will
> > be available for regular extensions). This permission will grant access to
all
> > of the users media galleries without further prompting.  It will be
> > read/write/append access depending on the the other media gallery
permissions.
> > 
> > Commentary: I'd be happy to add comments in the code explaining how/what
these
> > permissions do, but it doesn't seem to be the norm.
> 
> maybe mediaGalleriesAllGalleries then? Agreed that adding a comment here is
> unusual.

Done.

[Matt Perry, 2012-07-24 01:13:29]

lgtm