Initial extension bindings for Media Gallery API.

Initial extension bindings for Media Gallery API.

BUG=NONE
TEST=NONE

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

[vandebo (ex-Chrome), 2012-02-15 19:00:33]

Jon, I picked an extension reviewer at random, please redirect if appropriate.

[jstritar, 2012-02-16 21:34:18]

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:12: 
Can you add an apitest for these methods? It will be pretty simple since the
methods don't actually do anything, but it'll verify that you've done all the
wiring correct.

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:18:
SendResponse(true);
You won't need these SendResponses once they're SyncExtensionFunctions.

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.h (right):

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_gallery/media_gallery_api.h:17: class
GetMediaGalleriesFunction : public AsyncExtensionFunction {
These should just be SyncExtensionFunctions until you need the
asynchronous-ness.

http://codereview.chromium.org/9358076/diff/1001/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/1001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.mediaGalleries.json:9: "description":
"A class representing a media gallery on the user's machines."
do you need to define any properties on these?

[vandebo (ex-Chrome), 2012-03-19 23:09:52]

Sorry for the long delay.  We weren't sure if this approach was going to work
out.

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:12: 
On 2012/02/16 21:34:18, jstritar wrote:
> Can you add an apitest for these methods? It will be pretty simple since the
> methods don't actually do anything, but it'll verify that you've done all the
> wiring correct.

Done. But I had to change the "Blob" type arguments to "object" type to make the
validator happy.

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:18:
SendResponse(true);
On 2012/02/16 21:34:18, jstritar wrote:
> You won't need these SendResponses once they're SyncExtensionFunctions.

Done.

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.h (right):

http://codereview.chromium.org/9358076/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_gallery/media_gallery_api.h:17: class
GetMediaGalleriesFunction : public AsyncExtensionFunction {
On 2012/02/16 21:34:18, jstritar wrote:
> These should just be SyncExtensionFunctions until you need the
> asynchronous-ness.

Done.

http://codereview.chromium.org/9358076/diff/1001/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/1001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.mediaGalleries.json:9: "description":
"A class representing a media gallery on the user's machines."
On 2012/02/16 21:34:18, jstritar wrote:
> do you need to define any properties on these?

The full API is at go/galleryapi .  As you can see, we actually want to create
instance of this object.  After some investigation (the cause of the delay in
the CL), we'll use custom callbacks and Webkit objects defined by IDL to
implement this.  I'll put the full definition of this and other objects here for
the generated documentation in a later CL.

[vandebo (ex-Chrome), 2012-03-20 23:15:00]

Calendar says your in MTV this week; I understand travel time is best spent
meeting with people in MTV.  Should I grab a different reviewer?

[jstritar, 2012-03-21 00:02:02]

It's no problem... sorry for taking a little while.

What do you need the custom bindings for? Do you really need them?

http://codereview.chromium.org/9358076/diff/15001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.h (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_gallery/media_gallery_api.h:16: //
Implements the mediaGalleries.GetMediaGalleries() extension function.
nit: don't really need these comments since the DECLARE_EXTENSION_FUNCTION_NAME
macros are pretty self documenting below (same for others)

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:8: "type":
"object",
you can add properties to the object if you know what they'll be. here's an
example for Tab:

http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/ap...

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:21: "description":
"Get the media galleries configured in this user agent. Calling
getMediaGalleries will trigger a permission prompt for any origin that has not
yet been granted permission.",
Do you mean these will just be protected by host permissions? e.g., they need to
have permissions: ["http://google.com/*"] for google to call these methods? In
that case, you can probably rely on existing ways of acquiring permissions
(manifest.json and optional permissions), rather than prompting yourself?

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:48: "type":
"object",
should this be type Blob?

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:78: "description":
"A Blob",
ditto

http://codereview.chromium.org/9358076/diff/15001/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:495:
IDR_MEDIA_GALLERY_CUSTOM_BINDINGS_JS);
I think the rest of these are ordered alphabetically

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_gallery/manifest.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_gallery/manifest.json:7: "page":
"test.html"
if you only have <script/>'s in test.html, you can get rid of test.html and do
this:

"background": {
  "scripts": ["test.js"] 
}

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_gallery/test.js (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_gallery/test.js:11:
chrome.test.assertEq(results, []);
nit: for each of these, I think the body of callbackPass should be indented 2
spaces from the original indentation, rather than from the arguments:

     mediaGalleries.getMediaGalleries(
         chrome.test.callbackPass(function(results) {
       chrome.test.assertEq(results, []);
     }));

[jstritar, 2012-03-21 00:02:24]

Also, you can update the TEST= field with your added test

[jstritar, 2012-03-21 00:02:25]

Also, you can update the TEST= field with your added test

[vandebo (ex-Chrome), 2012-03-21 16:17:57]

On 2012/03/21 00:02:02, jstritar wrote:
> What do you need the custom bindings for? Do you really need them?

If you look at go/galleryapi, you'll see that this API is rather extensive and
actually uses instances of objects and not just static functions in a nested
namespaces.  As far as I understand, extensions can't do that (or at least
couldn't until the module system was landed recently?).  Furthermore, we need
the MediaGallery object to be serializable (like Blob is), so that web intents
can delivery a media gallery when a media device is attached.  Because of these
constraints, only the top level of the API (as stubbed out in the CL) will be
implemented with the extension bindings.  The other objects will use the IDL
definition in Webkit. (You can look at the experimentation patch in this CL if
you like: https://chromiumcodereview.appspot.com/9621021/ .)  I understand this
is unusual.  If you can suggest a better way to do this, I'm happy to listen,
but as far as I've been able to determine so far this is the cleanest way to
accomplish the goals.

i.e. we need the custom bindings to create objects defined by IDL in webkit.

On 2012/03/21 00:02:25, jstritar wrote:
> Also, you can update the TEST= field with your added test

As I understand it, the TEST= field is for manual testing instructions to QA.
See
https://groups.google.com/a/chromium.org/group/chromium-dev/msg/96e7247c533e8f1c

http://codereview.chromium.org/9358076/diff/15001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.h (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_gallery/media_gallery_api.h:16: //
Implements the mediaGalleries.GetMediaGalleries() extension function.
On 2012/03/21 00:02:02, jstritar wrote:
> nit: don't really need these comments since the
DECLARE_EXTENSION_FUNCTION_NAME
> macros are pretty self documenting below (same for others)

Done.  I had just copied another file (don't remember which one).

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:8: "type":
"object",
On 2012/03/21 00:02:02, jstritar wrote:
> you can add properties to the object if you know what they'll be. here's an
> example for Tab:
> 
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/ap...

I'll put the full defincition of this and other objects here  (for documentation
only) in a later CL.

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:21: "description":
"Get the media galleries configured in this user agent. Calling
getMediaGalleries will trigger a permission prompt for any origin that has not
yet been granted permission.",
On 2012/03/21 00:02:02, jstritar wrote:
> Do you mean these will just be protected by host permissions? e.g., they need
to
> have permissions: ["http://google.com/*"] for google to call these methods? In
> that case, you can probably rely on existing ways of acquiring permissions
> (manifest.json and optional permissions), rather than prompting yourself?

Sorry this comment is forward looking to a web standard API implementation. 
Changed to be accurate for the current, extension only, implementation.

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:48: "type":
"object",
On 2012/03/21 00:02:02, jstritar wrote:
> should this be type Blob?

I had it as a blob but when I added the API test it wouldn't validate.  I
searched the other extension description json - the only other place blob/Blob
is used is as a callback argument (which I think isn't validated).

http://codereview.chromium.org/9358076/diff/15001/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:446:
module_system->RegisterNativeHandler("experimental_mediaGalleries",
Alphabetization fixed here too.

http://codereview.chromium.org/9358076/diff/15001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:495:
IDR_MEDIA_GALLERY_CUSTOM_BINDINGS_JS);
On 2012/03/21 00:02:02, jstritar wrote:
> I think the rest of these are ordered alphabetically

Oops, fixed.  The first argument was introduced recently.

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_gallery/manifest.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_gallery/manifest.json:7: "page":
"test.html"
On 2012/03/21 00:02:02, jstritar wrote:
> if you only have <script/>'s in test.html, you can get rid of test.html and do
> this:
> 
> "background": {
>   "scripts": ["test.js"] 
> }

I tried to do this, but it didn't work.

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_gallery/test.js (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_gallery/test.js:11:
chrome.test.assertEq(results, []);
On 2012/03/21 00:02:02, jstritar wrote:
> nit: for each of these, I think the body of callbackPass should be indented 2
> spaces from the original indentation, rather than from the arguments:
> 
>      mediaGalleries.getMediaGalleries(
>          chrome.test.callbackPass(function(results) {
>        chrome.test.assertEq(results, []);
>      }));

Hmm, that looks a bit strange to me... instead I pulled the callbacks out to
explicit functions.

[jstritar, 2012-03-21 18:10:47]

LGTM.

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:48: "type":
"object",
On 2012/03/21 16:17:57, vandebo wrote:
>
> I had it as a blob but when I added the API test it wouldn't validate.  I
> searched the other extension description json - the only other place blob/Blob
> is used is as a callback argument (which I think isn't validated). 

If you think there's a bug in there, can you file it?

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_gallery/manifest.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_gallery/manifest.json:7: "page":
"test.html"
On 2012/03/21 16:17:57, vandebo wrote:
>
> I tried to do this, but it didn't work.

What didn't work about it?

[commit-bot: I haz the power, 2012-03-21 19:53:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9358076/24002

[commit-bot: I haz the power, 2012-03-21 20:57:43]

Can't apply patch for file
chrome/browser/extensions/extension_function_registry.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_function_registry.cc
Hunk #1 FAILED at 13.
Hunk #2 succeeded at 477 with fuzz 2 (offset 11 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_registry.cc.rej

[commit-bot: I haz the power, 2012-03-21 21:29:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9358076/26009

[commit-bot: I haz the power, 2012-03-21 21:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9358076/26010

[vandebo (ex-Chrome), 2012-03-21 22:08:07]

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:48: "type":
"object",
On 2012/03/21 18:10:47, jstritar wrote:
> On 2012/03/21 16:17:57, vandebo wrote:
> >
> > I had it as a blob but when I added the API test it wouldn't validate.  I
> > searched the other extension description json - the only other place
blob/Blob
> > is used is as a callback argument (which I think isn't validated). 
> 
> If you think there's a bug in there, can you file it?

Filed http://crbug.com/119372

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_gallery/manifest.json (right):

http://codereview.chromium.org/9358076/diff/15001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_gallery/manifest.json:7: "page":
"test.html"
On 2012/03/21 18:10:47, jstritar wrote:
> On 2012/03/21 16:17:57, vandebo wrote:
> >
> > I tried to do this, but it didn't work.
> 
> What didn't work about it?

[3608:3608:0321/143215:443953231440:ERROR:extension_error_reporter.cc(56)]
Extension error: Could not load extension from
'/usr/local/google/home/vandebo/chrome/src/chrome/test/data/extensions/api_test/media_gallery'.
Invalid value for 'background.scripts'.

I looked closer and I wasn't putting the value in an array - I had "scripts":
"test.js".  An opportunity for improvement?

Fixed.

[commit-bot: I haz the power, 2012-03-21 23:15:01]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[Aaron Boodman, 2012-03-22 21:58:14]

http://codereview.chromium.org/9358076/diff/26010/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/26010/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:62: "choices": [
You can just mark the param optional - you don't need to use choices. Null is
counted as 'not specified'.

[Aaron Boodman, 2012-03-22 22:13:00]

On Wed, Mar 21, 2012 at 9:17 AM,  <vandebo@chromium.org> wrote:
> On 2012/03/21 00:02:02, jstritar wrote:
>>
>> What do you need the custom bindings for? Do you really need them?
>
>
> If you look at go/galleryapi, you'll see that this API is rather extensive
> and
> actually uses instances of objects and not just static functions in a nested
> namespaces.  As far as I understand, extensions can't do that (or at least
> couldn't until the module system was landed recently?).  Furthermore, we
> need
> the MediaGallery object to be serializable (like Blob is), so that web
> intents
> can delivery a media gallery when a media device is attached.  Because of
> these
> constraints, only the top level of the API (as stubbed out in the CL) will
> be
> implemented with the extension bindings.  The other objects will use the IDL
> definition in Webkit. (You can look at the experimentation patch in this CL
> if
> you like: https://chromiumcodereview.appspot.com/9621021/ .)  I understand
> this
> is unusual.  If you can suggest a better way to do this, I'm happy to
> listen,
> but as far as I've been able to determine so far this is the cleanest way to
> accomplish the goals.
>
> i.e. we need the custom bindings to create objects defined by IDL in webkit.

Yes, this is very unusual. Can you please pause this review and send a
short design document (or a pointer to an existing one if I've missed
it) on how the entire media gallery API will fit together?

I'm concerned that if the meat of the API is implemented WebKit style,
that there will be a lot of opportunity for us to accidentally bypass
important parts of the extension system (i.e., the ones that implement
security).

- a

[vandebo (ex-Chrome), 2012-03-23 01:03:24]

On 2012/03/22 22:13:00, Aaron Boodman wrote:
> On Wed, Mar 21, 2012 at 9:17 AM,  <mailto:vandebo@chromium.org> wrote:
> >
> > If you look at go/galleryapi, you'll see that this API is rather extensive
> > and
> > actually uses instances of objects and not just static functions in a nested
> > namespaces. &nbsp;As far as I understand, extensions can't do that (or at
least
> > couldn't until the module system was landed recently?). &nbsp;Furthermore,
we
> > need
> > the MediaGallery object to be serializable (like Blob is), so that web
> > intents
> > can delivery a media gallery when a media device is attached. &nbsp;Because
of
> > these
> > constraints, only the top level of the API (as stubbed out in the CL) will
> > be
> > implemented with the extension bindings. &nbsp;The other objects will use
the IDL
> > definition in Webkit. (You can look at the experimentation patch in this CL
> > if
> > you like: https://chromiumcodereview.appspot.com/9621021/ .) &nbsp;I
understand
> > this
> > is unusual. &nbsp;If you can suggest a better way to do this, I'm happy to
> > listen,
> > but as far as I've been able to determine so far this is the cleanest way to
> > accomplish the goals.
> >
> > i.e. we need the custom bindings to create objects defined by IDL in webkit.
> 
> Yes, this is very unusual. Can you please pause this review and send a
> short design document (or a pointer to an existing one if I've missed
> it) on how the entire media gallery API will fit together?
> 
> I'm concerned that if the meat of the API is implemented WebKit style,
> that there will be a lot of opportunity for us to accidentally bypass
> important parts of the extension system (i.e., the ones that implement
> security).
> 
> - a

As mentioned above, the entire API is described in this document:
https://docs.google.com/a/google.com/document/pub?id=1RsLin39Zmun9kaxNUpChL55...
There is a high level design document at goto/mediagallerydesign , but I'm not
sure that's what you're asking for.

My (limited) understanding of the security mechanisms that the extension system
enforces is that it ensures that only contexts which should be able to call the
extension interface are able to do so and that it does type enforcement for the
extension methods.

I think the way to think of this (wrt permissions) is that the top level of the
API (those methods in the CL) are implemented in a mostly normal extension way .
 However, the results of those functions are javascript objects implemented with
IDL in webkit.  The IDL implementation takes care of the type checking and only
code that is permitted by the extension system can request the IDL objects.

I'm not sure what would be in a mini design doc for this portion of the media
gallery implementation because there isn't a principled design - this is what
we've figured out will work/satisfy the requirements.  (When I was in MTV a few
weeks ago, I talked with asargent@ and this is the direction we came up with,
not to say he approved this, but it seemed to be a reasonable way to get the job
done.)  But I'm happy to figure out answers for your questions.

http://codereview.chromium.org/9358076/diff/26010/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.mediaGalleries.json (right):

http://codereview.chromium.org/9358076/diff/26010/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:48: "type":
"object",
This is actually the argument that's giving me trouble.

http://codereview.chromium.org/9358076/diff/26010/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.mediaGalleries.json:62: "choices": [
On 2012/03/22 21:58:14, Aaron Boodman wrote:
> You can just mark the param optional - you don't need to use choices. Null is
> counted as 'not specified'.

Since this is a callback, i.e. not the user's option, I thought it worthwhile to
call out all the possibilities - either it's a blob, or it's null.  Given that,
you prefer to make it an optional Blob, will do.

[Aaron Boodman, 2012-03-23 02:07:08]

On Thu, Mar 22, 2012 at 6:03 PM,  <vandebo@chromium.org> wrote:
> On 2012/03/22 22:13:00, Aaron Boodman wrote:
>>
>> On Wed, Mar 21, 2012 at 9:17 AM,  <mailto:vandebo@chromium.org> wrote:
>> >
>> > If you look at go/galleryapi, you'll see that this API is rather
>> > extensive
>> > and
>> > actually uses instances of objects and not just static functions in a
>> > nested
>> > namespaces. &nbsp;As far as I understand, extensions can't do that (or
>> > at
>
> least
>>
>> > couldn't until the module system was landed recently?).
>> > &nbsp;Furthermore,
>
> we
>>
>> > need
>> > the MediaGallery object to be serializable (like Blob is), so that web
>> > intents
>> > can delivery a media gallery when a media device is attached.
>> > &nbsp;Because
>
> of
>>
>> > these
>> > constraints, only the top level of the API (as stubbed out in the CL)
>> > will
>> > be
>> > implemented with the extension bindings. &nbsp;The other objects will
>> > use
>
> the IDL
>>
>> > definition in Webkit. (You can look at the experimentation patch in this
>> > CL
>> > if
>> > you like: https://chromiumcodereview.appspot.com/9621021/ .) &nbsp;I
>
> understand
>>
>> > this
>> > is unusual. &nbsp;If you can suggest a better way to do this, I'm happy
>> > to
>>
>> > listen,
>> > but as far as I've been able to determine so far this is the cleanest
>> > way to
>> > accomplish the goals.
>> >
>> > i.e. we need the custom bindings to create objects defined by IDL in
>> > webkit.
>
>
>> Yes, this is very unusual. Can you please pause this review and send a
>> short design document (or a pointer to an existing one if I've missed
>> it) on how the entire media gallery API will fit together?
>
>
>> I'm concerned that if the meat of the API is implemented WebKit style,
>> that there will be a lot of opportunity for us to accidentally bypass
>> important parts of the extension system (i.e., the ones that implement
>> security).
>
>
>> - a
>
>
> As mentioned above, the entire API is described in this document:
>
https://docs.google.com/a/google.com/document/pub?id=1RsLin39Zmun9kaxNUpChL55...
> There is a high level design document at goto/mediagallerydesign , but I'm
> not
> sure that's what you're asking for.

I was looking for more detail on the class-level design than either of
those documents provides.

> My (limited) understanding of the security mechanisms that the extension
> system
> enforces is that it ensures that only contexts which should be able to call
> the
> extension interface are able to do so and that it does type enforcement for
> the
> extension methods.

One other bit:

- Extension-related IPC messages are rejected if they aren't coming
from a renderer that is known to be running an extension with
permission to the related feature.

> I think the way to think of this (wrt permissions) is that the top level of
> the
> API (those methods in the CL) are implemented in a mostly normal extension
> way .
>  However, the results of those functions are javascript objects implemented
> with
> IDL in webkit.  The IDL implementation takes care of the type checking and
> only
> code that is permitted by the extension system can request the IDL objects.

Right, but those objects expose further methods that end up sending
additional messages to the browser. It would be easy to end up having
those messages circumvent the security checks that are usually applied
to extension-related messages.

- a

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

On 2012/03/23 02:07:08, Aaron Boodman wrote:
> On Thu, Mar 22, 2012 at 6:03 PM,  <mailto:vandebo@chromium.org> wrote:
> > As mentioned above, the entire API is described in this document:
> >
>
https://docs.google.com/a/google.com/document/pub?id=1RsLin39Zmun9kaxNUpChL55...
> > There is a high level design document at goto/mediagallerydesign , but I'm
> > not
> > sure that's what you're asking for.
> 
> I was looking for more detail on the class-level design than either of
> those documents provides.

This document is still in progress put probably contains what you want:
https://docs.google.com/a/google.com/document/d/1dWrdskIUA-8z7vfHY4LmXq_VMoZy...

> > My (limited) understanding of the security mechanisms that the extension
> > system
> > enforces is that it ensures that only contexts which should be able to call
> > the
> > extension interface are able to do so and that it does type enforcement for
> > the
> > extension methods.
> 
> One other bit:
> 
> - Extension-related IPC messages are rejected if they aren't coming
> from a renderer that is known to be running an extension with
> permission to the related feature.

Indeed, it was already part of the plan to do this as well.  There are multiple
gallery providers, so as requests come into the browser we have to dispatch to
the correct provider.  At this point we will also validate
requests wrt sanity of request from a particular renderer.