switch mediaGalleries to .idl api definition

switch mediaGalleries to .idl api definition

BUG=none
TEST=manual

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

[Evan Stade, 2012-07-19 00:10:55]

http://codereview.chromium.org/10806023/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/10806023/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:47:
interactivity = *params->details->interactivity;
don't know why git failed to notice the move. The main difference in this file
is the |interactivity| variable and the removal of the other function.

[Evan Stade, 2012-07-19 01:38:24]

(git try is choking but this passes the api test locally and my manual tests)

[vandebo (ex-Chrome), 2012-07-19 19:58:31]

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
This file didn't get detected as a move.

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:83: } else
if (interactivity == "prompt") {
For prompt and prompt_if_needed, we should still return the list of file
systems, so these should be above the if (silent) and the if (silent) body
should unconditional after them.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:27: //
TODO(estade): the type of the argument should be LocalFileSystem[].
Do you want to include bug numbers here?

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:28: callback
MediaFileSystemsCallback = void (object mediaFileSystems);
mediaFileSystems => arrayOfMediaFileSystems ?

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:35: static void
getMediaFileSystems(optional MediaFileSystemsDetails details,
Does this mean that the interactivity argument is optional? If so, optional
arguments should only go at the end... I don't think it needs to be optional.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:50: [nocompile]
static object extractEmbeddedThumbnails(object mediaFile);
Does nocompile mean that it's currently not implemented?  If so, we can do that
for assembleMediaFile too.

http://codereview.chromium.org/10806023/diff/4001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_galleries/test.js (right):

http://codereview.chromium.org/10806023/diff/4001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_galleries/test.js:34:
chrome.test.assertTrue(mediaGalleries.openMediaGalleryManager !== null);
this test needs to be updated

[asargent_no_longer_on_chrome, 2012-07-19 20:40:12]

LGTM

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:50: [nocompile]
static object extractEmbeddedThumbnails(object mediaFile);
On 2012/07/19 19:58:31, vandebo wrote:
> Does nocompile mean that it's currently not implemented?  If so, we can do
that
> for assembleMediaFile too.

nocompile means we don't autogenerate C++ code for method dispatch between the
renderer and browser processes, but the method signature is still exposed to JS.
Typically this is used for methods that have some or all of their implementation
in JS (typically in a file in chrome/renderer/resources/extensions/..).

[Evan Stade, 2012-07-19 20:55:27]

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2012/07/19 19:58:31, vandebo wrote:
> This file didn't get detected as a move.

yes, I know. What would you like me to do about it?

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:83: } else
if (interactivity == "prompt") {
On 2012/07/19 19:58:31, vandebo wrote:
> For prompt and prompt_if_needed, we should still return the list of file
> systems, so these should be above the if (silent) and the if (silent) body
> should unconditional after them.

not exactly, showing the prompt is asynchronous. When we get to that, we'll have
to factor the if (silent) stuff into a separate function that can be called
after the prompt returns.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:27: //
TODO(estade): the type of the argument should be LocalFileSystem[].
On 2012/07/19 19:58:31, vandebo wrote:
> Do you want to include bug numbers here?

are there bug numbers for this? I'm not even sure there's any bug preventing it.
[instanceOf] works, I just don't know if it supports arrays, I figured I'd look
into that later.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:28: callback
MediaFileSystemsCallback = void (object mediaFileSystems);
On 2012/07/19 19:58:31, vandebo wrote:
> mediaFileSystems => arrayOfMediaFileSystems ?

the type tells you "arrayOf", and the trailing "s" also implies it.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:35: static void
getMediaFileSystems(optional MediaFileSystemsDetails details,
On 2012/07/19 19:58:31, vandebo wrote:
> Does this mean that the interactivity argument is optional? If so, optional
> arguments should only go at the end... I don't think it needs to be optional.

yes, it is optional, as I think it should be. Optional arguments need not go at
the end.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:49: // should be
[instanceOf=FileEntry].
the reason I didn't include this instanceOf is not that instanceOf doesn't work,
but because it does work, and it beraks the tests (they just use dummy objects).

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:50: [nocompile]
static object extractEmbeddedThumbnails(object mediaFile);
On 2012/07/19 20:40:12, Antony Sargent wrote:
> On 2012/07/19 19:58:31, vandebo wrote:
> > Does nocompile mean that it's currently not implemented?  If so, we can do
> that
> > for assembleMediaFile too.
> 
> nocompile means we don't autogenerate C++ code for method dispatch between the
> renderer and browser processes, but the method signature is still exposed to
JS.
> Typically this is used for methods that have some or all of their
implementation
> in JS (typically in a file in chrome/renderer/resources/extensions/..).
>  
> 

what Antony said, but as you know it's not implemented in js, it's implemented
in c++ in the renderer.

http://codereview.chromium.org/10806023/diff/4001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/media_galleries/test.js (right):

http://codereview.chromium.org/10806023/diff/4001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/media_galleries/test.js:34:
chrome.test.assertTrue(mediaGalleries.openMediaGalleryManager !== null);
On 2012/07/19 19:58:31, vandebo wrote:
> this test needs to be updated

yea, it does. The reason it still passed is because undefined !== null.

[Evan Stade, 2012-07-19 21:44:37]

got git to recognize the move.

[vandebo (ex-Chrome), 2012-07-19 22:45:22]

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/10806023/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:83: } else
if (interactivity == "prompt") {
On 2012/07/19 20:55:28, Evan Stade wrote:
> On 2012/07/19 19:58:31, vandebo wrote:
> > For prompt and prompt_if_needed, we should still return the list of file
> > systems, so these should be above the if (silent) and the if (silent) body
> > should unconditional after them.
> 
> not exactly, showing the prompt is asynchronous. When we get to that, we'll
have
> to factor the if (silent) stuff into a separate function that can be called
> after the prompt returns.

Sure... but this structure will break expectations until things get fixed.  Can
you just call the silent body until the prompt modes actually do something.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:27: //
TODO(estade): the type of the argument should be LocalFileSystem[].
On 2012/07/19 20:55:28, Evan Stade wrote:
> On 2012/07/19 19:58:31, vandebo wrote:
> > Do you want to include bug numbers here?
> 
> are there bug numbers for this? I'm not even sure there's any bug preventing
it.
> [instanceOf] works, I just don't know if it supports arrays, I figured I'd
look
> into that later.

Sorry, not sure.  I initially assumed it was the same issue as the bug comments
in the .json.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:28: callback
MediaFileSystemsCallback = void (object mediaFileSystems);
On 2012/07/19 20:55:28, Evan Stade wrote:
> On 2012/07/19 19:58:31, vandebo wrote:
> > mediaFileSystems => arrayOfMediaFileSystems ?
> 
> the type tells you "arrayOf", and the trailing "s" also implies it.

What do you mean "the type" tells you...?  Not sure why you want to fight about
a clarifying name change.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:35: static void
getMediaFileSystems(optional MediaFileSystemsDetails details,
On 2012/07/19 20:55:28, Evan Stade wrote:
> On 2012/07/19 19:58:31, vandebo wrote:
> > Does this mean that the interactivity argument is optional? If so, optional
> > arguments should only go at the end... I don't think it needs to be
optional.
> 
> yes, it is optional, as I think it should be. Optional arguments need not go
at
> the end.

Optional args not at the end of the arg list weirds me out, but if that's common
and acceptable in js, then so be it.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:49: // should be
[instanceOf=FileEntry].
On 2012/07/19 20:55:28, Evan Stade wrote:
> the reason I didn't include this instanceOf is not that instanceOf doesn't
work,
> but because it does work, and it beraks the tests (they just use dummy
objects).

Does it work properly?  I had locally set up the tests to use the right object
types but there were bugs that made that not work, so I pulled to get back to
simpler test code.

[Evan Stade, 2012-07-19 23:06:33]

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:83: } else
if (interactivity == "prompt") {
On 2012/07/19 22:45:22, vandebo wrote:
> On 2012/07/19 20:55:28, Evan Stade wrote:
> > On 2012/07/19 19:58:31, vandebo wrote:
> > > For prompt and prompt_if_needed, we should still return the list of file
> > > systems, so these should be above the if (silent) and the if (silent) body
> > > should unconditional after them.
> > 
> > not exactly, showing the prompt is asynchronous. When we get to that, we'll
> have
> > to factor the if (silent) stuff into a separate function that can be called
> > after the prompt returns.
> 
> Sure... but this structure will break expectations until things get fixed. 
Can
> you just call the silent body until the prompt modes actually do something.

ok

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/common/exten...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/common/exten...
chrome/common/extensions/api/experimental_media_galleries.idl:27: //
TODO(estade): the type of the argument should be LocalFileSystem[].
On 2012/07/19 22:45:22, vandebo wrote:
> On 2012/07/19 20:55:28, Evan Stade wrote:
> > On 2012/07/19 19:58:31, vandebo wrote:
> > > Do you want to include bug numbers here?
> > 
> > are there bug numbers for this? I'm not even sure there's any bug preventing
> it.
> > [instanceOf] works, I just don't know if it supports arrays, I figured I'd
> look
> > into that later.
> 
> Sorry, not sure.  I initially assumed it was the same issue as the bug
comments
> in the .json.

no, i believe that bug's resolved because instanceOf does work.

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/common/exten...
chrome/common/extensions/api/experimental_media_galleries.idl:28: callback
MediaFileSystemsCallback = void (object mediaFileSystems);
On 2012/07/19 22:45:22, vandebo wrote:
> On 2012/07/19 20:55:28, Evan Stade wrote:
> > On 2012/07/19 19:58:31, vandebo wrote:
> > > mediaFileSystems => arrayOfMediaFileSystems ?
> > 
> > the type tells you "arrayOf", and the trailing "s" also implies it.
> 
> What do you mean "the type" tells you...?  Not sure why you want to fight
about
> a clarifying name change.

By "the type" I mean once the TODO is fixed, there will be a type which tells
you the parameter is an array. I believe variable names should make sense, but
should not include types. Hungarian notation is explicitly disallowed by the C++
style guide.

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/common/exten...
chrome/common/extensions/api/experimental_media_galleries.idl:49: // should be
[instanceOf=FileEntry].
On 2012/07/19 22:45:22, vandebo wrote:
> On 2012/07/19 20:55:28, Evan Stade wrote:
> > the reason I didn't include this instanceOf is not that instanceOf doesn't
> work,
> > but because it does work, and it beraks the tests (they just use dummy
> objects).
> 
> Does it work properly? 

yes, I had this and other types specified, but then the tests failed because
they were using {}.

> I had locally set up the tests to use the right object
> types but there were bugs that made that not work, so I pulled to get back to
> simpler test code.
> 

I had hoped we'd be able to fix the tests and then define the types.

[vandebo (ex-Chrome), 2012-07-20 18:30:26]

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/common/exten...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/common/exten...
chrome/common/extensions/api/experimental_media_galleries.idl:28: callback
MediaFileSystemsCallback = void (object mediaFileSystems);
On 2012/07/19 23:06:33, Evan Stade wrote:
> On 2012/07/19 22:45:22, vandebo wrote:
> > On 2012/07/19 20:55:28, Evan Stade wrote:
> > > On 2012/07/19 19:58:31, vandebo wrote:
> > > > mediaFileSystems => arrayOfMediaFileSystems ?
> > > 
> > > the type tells you "arrayOf", and the trailing "s" also implies it.
> > 
> > What do you mean "the type" tells you...?  Not sure why you want to fight
> about
> > a clarifying name change.
> 
> By "the type" I mean once the TODO is fixed, there will be a type which tells
> you the parameter is an array. I believe variable names should make sense, but
> should not include types. Hungarian notation is explicitly disallowed by the
C++
> style guide.

I see... Is there something hard about fixing this todo (something more than
changing the text here?).  If so, can you do array of object in the meantime?

https://chromiumcodereview.appspot.com/10806023/diff/4001/chrome/common/exten...
chrome/common/extensions/api/experimental_media_galleries.idl:49: // should be
[instanceOf=FileEntry].
On 2012/07/19 23:06:33, Evan Stade wrote:
> On 2012/07/19 22:45:22, vandebo wrote:
> > On 2012/07/19 20:55:28, Evan Stade wrote:
> > > the reason I didn't include this instanceOf is not that instanceOf doesn't
> > work,
> > > but because it does work, and it beraks the tests (they just use dummy
> > objects).
> > 
> > Does it work properly? 
> 
> yes, I had this and other types specified, but then the tests failed because
> they were using {}.
> 
> > I had locally set up the tests to use the right object
> > types but there were bugs that made that not work, so I pulled to get back
to
> > simpler test code.
> > 
> 
> I had hoped we'd be able to fix the tests and then define the types.

You can use WebKitBlobBuilder to create a Blob test object.

https://chromiumcodereview.appspot.com/10806023/diff/22/chrome/test/data/exte...
File chrome/test/data/extensions/api_test/media_galleries/test.js (right):

https://chromiumcodereview.appspot.com/10806023/diff/22/chrome/test/data/exte...
chrome/test/data/extensions/api_test/media_galleries/test.js:30: },
Shouldn't we add tests here for the interactivity modes?

[Evan Stade, 2012-07-20 19:48:12]

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:28: callback
MediaFileSystemsCallback = void (object mediaFileSystems);
On 2012/07/20 18:30:26, vandebo wrote:
> On 2012/07/19 23:06:33, Evan Stade wrote:
> > On 2012/07/19 22:45:22, vandebo wrote:
> > > On 2012/07/19 20:55:28, Evan Stade wrote:
> > > > On 2012/07/19 19:58:31, vandebo wrote:
> > > > > mediaFileSystems => arrayOfMediaFileSystems ?
> > > > 
> > > > the type tells you "arrayOf", and the trailing "s" also implies it.
> > > 
> > > What do you mean "the type" tells you...?  Not sure why you want to fight
> > about
> > > a clarifying name change.
> > 
> > By "the type" I mean once the TODO is fixed, there will be a type which
tells
> > you the parameter is an array. I believe variable names should make sense,
but
> > should not include types. Hungarian notation is explicitly disallowed by the
> C++
> > style guide.
> 
> I see... Is there something hard about fixing this todo (something more than
> changing the text here?).  If so, can you do array of object in the meantime?

yes, the hard thing is figuring out the syntax. After reading through the idl
parser I think I got the correct thing now though.

http://codereview.chromium.org/10806023/diff/4001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_media_galleries.idl:49: // should be
[instanceOf=FileEntry].
On 2012/07/20 18:30:26, vandebo wrote:
> On 2012/07/19 23:06:33, Evan Stade wrote:
> > On 2012/07/19 22:45:22, vandebo wrote:
> > > On 2012/07/19 20:55:28, Evan Stade wrote:
> > > > the reason I didn't include this instanceOf is not that instanceOf
doesn't
> > > work,
> > > > but because it does work, and it beraks the tests (they just use dummy
> > > objects).
> > > 
> > > Does it work properly? 
> > 
> > yes, I had this and other types specified, but then the tests failed because
> > they were using {}.
> > 
> > > I had locally set up the tests to use the right object
> > > types but there were bugs that made that not work, so I pulled to get back
> to
> > > simpler test code.
> > > 
> > 
> > I had hoped we'd be able to fix the tests and then define the types.
> 
> You can use WebKitBlobBuilder to create a Blob test object.

Done.

http://codereview.chromium.org/10806023/diff/22/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/media_galleries/test.js (right):

http://codereview.chromium.org/10806023/diff/22/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/media_galleries/test.js:30: },
On 2012/07/20 18:30:26, vandebo wrote:
> Shouldn't we add tests here for the interactivity modes?

it would currently fail any test because the prompt's not implemented, and when
it is implemented it's hard to test things that require user interaction. The
only thing we could test is the types, which I can add if you want but that
style of test doesn't seem to exist in this file.

http://codereview.chromium.org/10806023/diff/16025/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

http://codereview.chromium.org/10806023/diff/16025/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_media_galleries.idl:27: void
([instanceOf=LocalFileSystem] optional object[] mediaFileSystems);
Antony, does this look correct? it seems to work.

[asargent_no_longer_on_chrome, 2012-07-20 19:55:07]

http://codereview.chromium.org/10806023/diff/16025/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_media_galleries.idl (right):

http://codereview.chromium.org/10806023/diff/16025/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_media_galleries.idl:27: void
([instanceOf=LocalFileSystem] optional object[] mediaFileSystems);
On 2012/07/20 19:48:13, Evan Stade wrote:
> Antony, does this look correct? it seems to work.

Yep, that looks correct.

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

LGTM for now.

[Evan Stade, 2012-07-23 19:12:08]

On 2012/07/23 18:18:07, vandebo wrote:
> LGTM for now.

nothing is set in stone.