Initial extension bindings for Media Gallery API.

Initial extension bindings for Media Gallery API.

Previous review: http://codereview.chromium.org/9358076/

BUG=NONE
TEST=NONE

[vandebo (ex-Chrome), 2012-03-23 21:28:07]

Patch set one is what was previously committed.  Patch set two makes the Blobs
optional for now.  Happy to pause this until everyone is satisfied with the
approach, but maybe the discussion should move to this new CL.

[vandebo (ex-Chrome), 2012-03-26 21:21:24]

Aaron,

This doc is 95% done (a few details to hammer out here and there)
https://docs.google.com/a/google.com/document/d/1dWrdskIUA-8z7vfHY4LmXq_VMoZy...

Can you take a a look and give your thoughts on moving forward?

[Aaron Boodman, 2012-03-29 05:02:51]

Hi Steve,

Sorry this took a few days, and thanks for the document. Overall, I like this a
lot. Do it.

I only have a few broad pieces of feedback / thoughts:

1. I'd like to be involved in the initial reviews - particularly the IPC and
bits that connect to the extension bindings. You are blazing new ground here and
I want to make sure what happens here is a good base for future work.

2. The current system has the property that all IPCs to the browser are
automatically privilege-checked. This relies on metadata declared with the APIs
that we lose with the raw IPC approach. It would be nice, though probably not
immediately required, to have some similar automated way of declaring the
extension permissions required for IPC messages.

[Aaron Boodman, 2012-03-29 05:28:30]

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/browser/exten...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.h (right):

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/browser/exten...
chrome/browser/extensions/api/media_gallery/media_gallery_api.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
I thought that you were not going to use ExtensionFunctions. From the doc, I was
expecting it to just be a custom bindings javascript file, a NativeHandler, and
then that NativeHandler using normal IPC to talk to its own collection of
objects in the browser.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:1: [
license header

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:4: "nodoc": true,
Not necessary.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:9: "description":
"A class representing a media gallery on the user's machines."
You can say: isInstanceOf: "MediaGallery" to declare that the object returned is
of that DOM type.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:39: "description":
"Open the user agent’s UI to configure galleries. Calling
openMediaGalleryManager will trigger a permission prompt for any origin that has
not yet been granted permission.",
Is this comment right? Extension and App APIs don't typically work this way
(with runtime prompting).

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:50: "description":
"A Blob - marked as optional object until bug 119372 is resolved.",
These files support comments, so you can just have a regular TODO here.

[vandebo (ex-Chrome), 2012-03-29 18:45:02]

On 2012/03/29 05:02:51, Aaron Boodman wrote:
> Hi Steve,
> 
> Sorry this took a few days, and thanks for the document. Overall, I like this
a
> lot. Do it.

No worries.  I only pinged you to make sure it was on your todo list.

> I only have a few broad pieces of feedback / thoughts:
> 
> 1. I'd like to be involved in the initial reviews - particularly the IPC and
> bits that connect to the extension bindings. You are blazing new ground here
and
> I want to make sure what happens here is a good base for future work.

SG.
 
> 2. The current system has the property that all IPCs to the browser are
> automatically privilege-checked. This relies on metadata declared with the
APIs
> that we lose with the raw IPC approach. It would be nice, though probably not
> immediately required, to have some similar automated way of declaring the
> extension permissions required for IPC messages.

That sounds good.  Maybe we can do a specialization of RenderViewHostObserver,
or maybe add IPC_BEGIN_MESSAGE_MAP_WITH_SECURITY_CHECK... It seems like it will
still need to be opt it, but we can make it non-brittle once the code has opted
in.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/browser/exten...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.h (right):

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/browser/exten...
chrome/browser/extensions/api/media_gallery/media_gallery_api.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2012/03/29 05:28:30, Aaron Boodman wrote:
> I thought that you were not going to use ExtensionFunctions. From the doc, I
was
> expecting it to just be a custom bindings javascript file, a NativeHandler,
and
> then that NativeHandler using normal IPC to talk to its own collection of
> objects in the browser.

All of these top level APIs are static, so will work with ExtensionFunctions and
most require information from the browser process. Using ExtensionFunctions
gives two things - normal extension security mechanism at the top of the API and
an existing mechanism to connect a reply with an initial request.  It doesn't
seem to cost anything extra over a different approach.

These methods are declared as SyncExtensionFunction because they are no-ops
right now and Jon requested I change from from Async to Sync.  Once implemented
they will be Async.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:1: [
On 2012/03/29 05:28:30, Aaron Boodman wrote:
> license header

Done.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:4: "nodoc": true,
On 2012/03/29 05:28:30, Aaron Boodman wrote:
> Not necessary.

Done.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:9: "description":
"A class representing a media gallery on the user's machines."
On 2012/03/29 05:28:30, Aaron Boodman wrote:
> You can say: isInstanceOf: "MediaGallery" to declare that the object returned
is
> of that DOM type.

Not sure what you're getting at here.  Eventually I'll fill out all the types
here so that the extension documentation system will describe the entire API -
even those bits which we implement with custom IPCs and IDL.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:39: "description":
"Open the user agent’s UI to configure galleries. Calling
openMediaGalleryManager will trigger a permission prompt for any origin that has
not yet been granted permission.",
On 2012/03/29 05:28:30, Aaron Boodman wrote:
> Is this comment right? Extension and App APIs don't typically work this way
> (with runtime prompting).

No, I copy and pasted from my initial description of the API.  As an OWP
implementation there would be a permission prompt for this call, but not as an
extension.  Removed that part.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:50: "description":
"A Blob - marked as optional object until bug 119372 is resolved.",
On 2012/03/29 05:28:30, Aaron Boodman wrote:
> These files support comments, so you can just have a regular TODO here.

Done.

[Aaron Boodman, 2012-03-30 22:14:56]

LGTM

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/browser/exten...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.h (right):

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/browser/exten...
chrome/browser/extensions/api/media_gallery/media_gallery_api.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2012/03/29 18:45:02, vandebo wrote:
> On 2012/03/29 05:28:30, Aaron Boodman wrote:
> > I thought that you were not going to use ExtensionFunctions. From the doc, I
> was
> > expecting it to just be a custom bindings javascript file, a NativeHandler,
> and
> > then that NativeHandler using normal IPC to talk to its own collection of
> > objects in the browser.
> 
> All of these top level APIs are static, so will work with ExtensionFunctions
and
> most require information from the browser process. Using ExtensionFunctions
> gives two things - normal extension security mechanism at the top of the API
and
> an existing mechanism to connect a reply with an initial request.  It doesn't
> seem to cost anything extra over a different approach.
> 
> These methods are declared as SyncExtensionFunction because they are no-ops
> right now and Jon requested I change from from Async to Sync.  Once
implemented
> they will be Async.

OK, I understand now. These are the special top-level entry points. Initially, I
thought there was only one of those in your design, for some reason.

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

https://chromiumcodereview.appspot.com/9837012/diff/6001/chrome/common/extens...
chrome/common/extensions/api/experimental.mediaGalleries.json:9: "description":
"A class representing a media gallery on the user's machines."
On 2012/03/29 18:45:02, vandebo wrote:
> On 2012/03/29 05:28:30, Aaron Boodman wrote:
> > You can say: isInstanceOf: "MediaGallery" to declare that the object
returned
> is
> > of that DOM type.
> 
> Not sure what you're getting at here.  Eventually I'll fill out all the types
> here so that the extension documentation system will describe the entire API -
> even those bits which we implement with custom IPCs and IDL.

It doesn't seem like a great idea to me to duplicate all that information. I
think it is OK for the docs to simply refer to the webplatform-style
documentation elsewhere.

[vandebo (ex-Chrome), 2012-03-31 00:40:02]

Thanks for the review and LG. We got some feedback about the API today that is
making us take a step back and reevaluate things.  I'll likely end up making
some changes to the API, but not the approach and I'll let you know when I'm
ready for you to take another look.