[Extensions] Make chrome://extensions use developerPrivate for packing crxs

[Extensions] Make chrome://extensions use developerPrivate for packing crxs

Make the chrome://extensions page use chrome.developerPrivate API to
select a file path for a crx and to perform the pack job itself.
Also add tests for the api function, and convert it to a
UIThreadExtensionFunction.

BUG=461039
Committed: https://crrev.com/83c39e683f9f50bb07e065e3a89a0259abf44664
Cr-Commit-Position: refs/heads/master@{#318754}

[Devlin, 2015-02-27 02:11:35]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-02-27 02:11:39]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-02-27 02:12:07]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-02-27 02:16:04]

rdevlin.cronin@chromium.org changed reviewers:
+ dbeam@chromium.org

[Devlin, 2015-02-27 02:16:04]

Hey, here's the simplification we're looking for!

Ben, can you take a look at the api and webui?
Dan, mind taking the JS?

https://codereview.chromium.org/958803004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (left):

https://codereview.chromium.org/958803004/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:18: dictionary ReloadOptions
{
ignore this (rebasing issue).

[not at google - send to devlin, 2015-02-27 17:58:11]

Lg but I want to test it when I get to work before I look into the JS too much
(and I am assuming Dan will look at that as well).

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(left):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1282:
AddRef();
So this was a leak?

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:759:
bool fail_quietly = params->options && params->options->fail_quietly;
Urk, fail_quietly should be optional.

  dictionary ReloadOptions {
    // If false, an alert dialog will show in the event of a reload error.
    // Defaults to false.
    boolean failQuietly;
  };

is funny - "default to false" is meaningless because you're forced to provide a
value!

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:879:
0)) {
Comment what 0 is.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:908:
content::WebContents* web_contents = GetSenderWebContents();
Thank you.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:936:
Respond(OneArgument(response.ToValue().release()));
Should be:

Respond(ArgumentList(
    developer::PackDirectory::Results::Create(response)));

to make use of the safe generated code.

This comment applies to several more instances below.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1237:
ExtensionFunction::ResponseAction DeveloperPrivateChoosePathFunction::Run() {
Hm I get that load unpacked is safer to be implemented in trusted UI ... but
choosePath? By the name it seems like it could absolutely be a standard file
picker? Oh well :-( there's code out there that uses it, so be it.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:65: if
(!chrome.runtime.lastError && path)
Add note, that this will happen if the file chooser is cancelled.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:99:
onPackResponse_: function(response) {
I will give this a spin when I get to work.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:101:
console.log(JSON.stringify(response));
Remove logging?

[not at google - send to devlin, 2015-02-27 19:23:59]

lgtm

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:117: if
(response.status == 'SUCCESS') {
Can you switch?

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:136:
assert(response.status == 'ERROR');
I'd rather cover this in a switch case then in a default case assert(false).

[not at google - send to devlin, 2015-02-27 19:24:08]

(Devlin just demoed to me and it works)

[Dan Beam, 2015-02-27 19:57:51]

looks pretty good to me

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:65: if
(!chrome.runtime.lastError && path)
i find it a little odd that we rely on a global here instead of passing back an
error to the callback.

if something else goes wrong before this dialog is shown, will
chrome.runtime.lastError still be set?  is it only set while inside this
function?  if this function is async and the global operates similarly to any
other global: this sounds super racey.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:101:
console.log(JSON.stringify(response));
On 2015/02/27 17:58:11, kalman wrote:
> Remove logging?

+1

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:117: if
(response.status == 'SUCCESS') {
On 2015/02/27 19:23:58, kalman wrote:
> Can you switch?

yes, switch (stringValue) {...} works in js, and if we could create an @enum
{string} for the potential responses that'd be cool as well (I think it'd
integrate well with the closure compiler)

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:145:
alertOverlay.setValues(alertTitle,
where does this variable come from?  is alertOverlay == $('alertOverlay')?

https://codereview.chromium.org/958803004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (left):

https://codereview.chromium.org/958803004/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:18: dictionary ReloadOptions
{
On 2015/02/27 02:16:04, Devlin wrote:
> ignore this (rebasing issue).

fix before you CQ...

[Devlin, 2015-02-27 21:37:03]

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(left):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1282:
AddRef();
On 2015/02/27 17:58:10, kalman wrote:
> So this was a leak?

Yes. Scary, huh?

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:759:
bool fail_quietly = params->options && params->options->fail_quietly;
On 2015/02/27 17:58:10, kalman wrote:
> Urk, fail_quietly should be optional.
> 
>   dictionary ReloadOptions {
>     // If false, an alert dialog will show in the event of a reload error.
>     // Defaults to false.
>     boolean failQuietly;
>   };
> 
> is funny - "default to false" is meaningless because you're forced to provide
a
> value!

Well, two things:
- The fact it shows up here is a rebase error; my bad.
- I was a little torn on this.  ReloadOptions itself is actually optional, and
that's the only field.  I guess we could allow people to pass in a {} for
options if they so desired, but it seems kinda... silly.  I figured we'd just
update it to be optional if/when we added more than that for options.  (Note
that this means that default to false still has meaning, because you can omit
the options param).

In either case, it's a discussion for a separate CL :)

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:879:
0)) {
On 2015/02/27 17:58:10, kalman wrote:
> Comment what 0 is.

Done.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:936:
Respond(OneArgument(response.ToValue().release()));
On 2015/02/27 17:58:11, kalman wrote:
> Should be:
> 
> Respond(ArgumentList(
>     developer::PackDirectory::Results::Create(response)));
> 
> to make use of the safe generated code.
> 
> This comment applies to several more instances below.

Does it make a difference?  developer::PackDirectory::Results::Create(response)
just creates a list value and appends response.ToValue() on it; isn't that the
same thing as this?

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:1237:
ExtensionFunction::ResponseAction DeveloperPrivateChoosePathFunction::Run() {
On 2015/02/27 17:58:10, kalman wrote:
> Hm I get that load unpacked is safer to be implemented in trusted UI ... but
> choosePath? By the name it seems like it could absolutely be a standard file
> picker? Oh well :-( there's code out there that uses it, so be it.

I know; I thought of that too :(  Maybe one day we can update all the code that
uses it.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:65: if
(!chrome.runtime.lastError && path)
On 2015/02/27 17:58:11, kalman wrote:
> Add note, that this will happen if the file chooser is cancelled.

Done.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:65: if
(!chrome.runtime.lastError && path)
On 2015/02/27 19:57:51, Dan Beam wrote:
> i find it a little odd that we rely on a global here instead of passing back
an
> error to the callback.
> 
> if something else goes wrong before this dialog is shown, will
> chrome.runtime.lastError still be set?  is it only set while inside this
> function?  if this function is async and the global operates similarly to any
> other global: this sounds super racey.

TL;DR: Part of the extensions system.  Should be okay.

Kalman can confirm this, but I believe that runtime.lastError is actually only
set for the lifespan of the callback.  That is, before we call the callback in
extension bindings, we set lastError, and then we unset it afterwards.  It's
done this way for every extension api, so I don't think it's really in the scope
of this CL to change it. :)

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:99:
onPackResponse_: function(response) {
On 2015/02/27 17:58:11, kalman wrote:
> I will give this a spin when I get to work.

Spun.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:101:
console.log(JSON.stringify(response));
On 2015/02/27 19:57:51, Dan Beam wrote:
> On 2015/02/27 17:58:11, kalman wrote:
> > Remove logging?
> 
> +1

Done.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:117: if
(response.status == 'SUCCESS') {
On 2015/02/27 19:57:51, Dan Beam wrote:
> On 2015/02/27 19:23:58, kalman wrote:
> > Can you switch?
> 
> yes, switch (stringValue) {...} works in js, and if we could create an @enum
> {string} for the potential responses that'd be cool as well (I think it'd
> integrate well with the closure compiler)

switch done.

Dan, what would it look like to use the closure compiler syntax for an enum
here?  (Bearing in mind that this "enum" is actually just a string, not a real
enum, and is part of the extension system).  Would we create a separate enum,
and assign response.status into it?  Or...?

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:136:
assert(response.status == 'ERROR');
On 2015/02/27 19:23:58, kalman wrote:
> I'd rather cover this in a switch case then in a default case assert(false).

Done.

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:145:
alertOverlay.setValues(alertTitle,
On 2015/02/27 19:57:50, Dan Beam wrote:
> where does this variable come from?  is alertOverlay == $('alertOverlay')?

Took me forever to find it, to, when I was trying to figure out what these calls
were doing.  It's an object defined here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
(and is separate, but related, to $('alertOverlay'))

https://codereview.chromium.org/958803004/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (left):

https://codereview.chromium.org/958803004/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:18: dictionary ReloadOptions
{
On 2015/02/27 19:57:51, Dan Beam wrote:
> On 2015/02/27 02:16:04, Devlin wrote:
> > ignore this (rebasing issue).
> 
> fix before you CQ...

Done.

[Dan Beam, 2015-02-27 22:57:18]

lgtm w/nits

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:65: if
(!chrome.runtime.lastError && path)
On 2015/02/27 21:37:02, Devlin wrote:
> On 2015/02/27 19:57:51, Dan Beam wrote:
> > i find it a little odd that we rely on a global here instead of passing back
> an
> > error to the callback.
> > 
> > if something else goes wrong before this dialog is shown, will
> > chrome.runtime.lastError still be set?  is it only set while inside this
> > function?  if this function is async and the global operates similarly to
any
> > other global: this sounds super racey.
> 
> TL;DR: Part of the extensions system.  Should be okay.
> 
> Kalman can confirm this, but I believe that runtime.lastError is actually only
> set for the lifespan of the callback.  That is, before we call the callback in
> extension bindings, we set lastError, and then we unset it afterwards.  It's
> done this way for every extension api, so I don't think it's really in the
scope
> of this CL to change it. :)

lame-ish

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:117: if
(response.status == 'SUCCESS') {
On 2015/02/27 21:37:02, Devlin wrote:
> On 2015/02/27 19:57:51, Dan Beam wrote:
> > On 2015/02/27 19:23:58, kalman wrote:
> > > Can you switch?
> > 
> > yes, switch (stringValue) {...} works in js, and if we could create an @enum
> > {string} for the potential responses that'd be cool as well (I think it'd
> > integrate well with the closure compiler)
> 
> switch done.
> 
> Dan, what would it look like to use the closure compiler syntax for an enum
> here?  (Bearing in mind that this "enum" is actually just a string, not a real
> enum, and is part of the extension system).  Would we create a separate enum,
> and assign response.status into it?  Or...?

http://stackoverflow.com/questions/18186309/is-there-a-way-to-use-an-enum-as-...

/** @enum {string} */
var PackDirectoryResponse = {
  SUCCESS: 'SUCCESS',
  WARNING: 'WARNING',
  ERROR: 'ERROR',
};

... later on  ...
@typedef PackDirectoryResponse {{
  ...
  status: PackDirecotyResponse,
  ...
}}

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:126: alertCancel =
loadTimeData.getString('cancel'),
, -> ;

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:139: alertOk =
loadTimeData.getString('ok'),
, -> ;

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:147:
assert(false);
  assertNotReached();

also maybe

  return;

?

[not at google - send to devlin, 2015-02-27 22:58:45]

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:65: if
(!chrome.runtime.lastError && path)
On 2015/02/27 22:57:17, Dan Beam wrote:
> On 2015/02/27 21:37:02, Devlin wrote:
> > On 2015/02/27 19:57:51, Dan Beam wrote:
> > > i find it a little odd that we rely on a global here instead of passing
back
> > an
> > > error to the callback.
> > > 
> > > if something else goes wrong before this dialog is shown, will
> > > chrome.runtime.lastError still be set?  is it only set while inside this
> > > function?  if this function is async and the global operates similarly to
> any
> > > other global: this sounds super racey.
> > 
> > TL;DR: Part of the extensions system.  Should be okay.
> > 
> > Kalman can confirm this, but I believe that runtime.lastError is actually
only
> > set for the lifespan of the callback.  That is, before we call the callback
in
> > extension bindings, we set lastError, and then we unset it afterwards.  It's
> > done this way for every extension api, so I don't think it's really in the
> scope
> > of this CL to change it. :)
> 
> lame-ish

Indeed. It's weighing down my TODO list to fix it, but it's an epic project.

[Devlin, 2015-02-27 23:38:38]

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:117: if
(response.status == 'SUCCESS') {
On 2015/02/27 22:57:17, Dan Beam wrote:
> On 2015/02/27 21:37:02, Devlin wrote:
> > On 2015/02/27 19:57:51, Dan Beam wrote:
> > > On 2015/02/27 19:23:58, kalman wrote:
> > > > Can you switch?
> > > 
> > > yes, switch (stringValue) {...} works in js, and if we could create an
@enum
> > > {string} for the potential responses that'd be cool as well (I think it'd
> > > integrate well with the closure compiler)
> > 
> > switch done.
> > 
> > Dan, what would it look like to use the closure compiler syntax for an enum
> > here?  (Bearing in mind that this "enum" is actually just a string, not a
real
> > enum, and is part of the extension system).  Would we create a separate
enum,
> > and assign response.status into it?  Or...?
> 
>
http://stackoverflow.com/questions/18186309/is-there-a-way-to-use-an-enum-as-...
> 
> /** @enum {string} */
> var PackDirectoryResponse = {
>   SUCCESS: 'SUCCESS',
>   WARNING: 'WARNING',
>   ERROR: 'ERROR',
> };
> 
> ... later on  ...
> @typedef PackDirectoryResponse {{
>   ...
>   status: PackDirecotyResponse,
>   ...
> }}

I was more curious if there was a way of doing it without adjusting the closure
compiler extern definition since, technically, even though we say these are
enums in the idl, they're really just strings we pass.  What's more, we
*shouldn't* have the practice of declaring enums as such in the extern in
general, because then we can't call the function.  For instance, if we use an
enum for the "enum" of SelectType (FILE or FOLDER), then closure compiler breaks
when we try to call it with 'FOLDER', since it's a string. And, of course, we
don't (as part of the extension api) expose enum definitions on the api
anywhere, so we can't pass a real enum FileType.  Because of this, I don't
really think declaring PackStatus to be an @enum in the externs is really the
right thing to do... WDYT?

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:126: alertCancel =
loadTimeData.getString('cancel'),
On 2015/02/27 22:57:17, Dan Beam wrote:
> , -> ;

Done.

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:139: alertOk =
loadTimeData.getString('ok'),
On 2015/02/27 22:57:17, Dan Beam wrote:
> , -> ;

Done.

https://codereview.chromium.org/958803004/diff/60001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:147:
assert(false);
On 2015/02/27 22:57:17, Dan Beam wrote:
>   assertNotReached();
> 
> also maybe
> 
>   return;
> 
> ?

Done.

[Dan Beam, 2015-02-28 00:17:57]

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/extensions/pack_extension_overlay.js (right):

https://codereview.chromium.org/958803004/diff/40001/chrome/browser/resources...
chrome/browser/resources/extensions/pack_extension_overlay.js:117: if
(response.status == 'SUCCESS') {
On 2015/02/27 23:38:37, Devlin wrote:
> On 2015/02/27 22:57:17, Dan Beam wrote:
> > On 2015/02/27 21:37:02, Devlin wrote:
> > > On 2015/02/27 19:57:51, Dan Beam wrote:
> > > > On 2015/02/27 19:23:58, kalman wrote:
> > > > > Can you switch?
> > > > 
> > > > yes, switch (stringValue) {...} works in js, and if we could create an
> @enum
> > > > {string} for the potential responses that'd be cool as well (I think
it'd
> > > > integrate well with the closure compiler)
> > > 
> > > switch done.
> > > 
> > > Dan, what would it look like to use the closure compiler syntax for an
enum
> > > here?  (Bearing in mind that this "enum" is actually just a string, not a
> real
> > > enum, and is part of the extension system).  Would we create a separate
> enum,
> > > and assign response.status into it?  Or...?
> > 
> >
>
http://stackoverflow.com/questions/18186309/is-there-a-way-to-use-an-enum-as-...
> > 
> > /** @enum {string} */
> > var PackDirectoryResponse = {
> >   SUCCESS: 'SUCCESS',
> >   WARNING: 'WARNING',
> >   ERROR: 'ERROR',
> > };
> > 
> > ... later on  ...
> > @typedef PackDirectoryResponse {{
> >   ...
> >   status: PackDirecotyResponse,
> >   ...
> > }}
> 
> I was more curious if there was a way of doing it without adjusting the
closure
> compiler extern definition since, technically, even though we say these are
> enums in the idl, they're really just strings we pass.

JavaScript has no enums natively, so using some existing type is necessary if we
want similar functionality now.  And even in C++, enums are backed by integers.

> What's more, we *shouldn't* have the practice of declaring enums as such in
the extern in general, because then we can't call the function. For instance, if
we use an enum for the "enum" of SelectType (FILE or FOLDER), then closure
compiler breaks when we try to call it with 'FOLDER', since it's a string.

why can't you call the function with SelectedType.FOLDER instead of 'FOLDER',
even in tests?  declare the @enum at runtime instead of in @externs.

> And, of course, we don't (as part of the extension api) expose enum
definitions on the api anywhere, so we can't pass a real enum FileType.

why don't you?  the web does this sometimes:
https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType

[Devlin, 2015-03-02 16:54:13]

> JavaScript has no enums natively, so using some existing type is necessary if
we
> want similar functionality now.  And even in C++, enums are backed by
integers.
> 
> > What's more, we *shouldn't* have the practice of declaring enums as such in
> the extern in general, because then we can't call the function. For instance,
if
> we use an enum for the "enum" of SelectType (FILE or FOLDER), then closure
> compiler breaks when we try to call it with 'FOLDER', since it's a string.
> 
> why can't you call the function with SelectedType.FOLDER instead of 'FOLDER',
> even in tests?  declare the @enum at runtime instead of in @externs.
> 

We could do this, but it seems odd to have enum declarations for the extension
API in extensions_list.js; should we do this for all enums we use?  What about
other webui that uses extension apis; should they all declare the enums that are
specified in the API?  And then, how do we keep them up to date with the API
(when, e.g., a new enum value is added)?  In short, it's doable, but it seems
like the wrong solution to be declaring extension api details in a webui js
file... Of course, just my opinion.

> > And, of course, we don't (as part of the extension api) expose enum
> definitions on the api anywhere, so we can't pass a real enum FileType.
> 
> why don't you?  the web does this sometimes:
> https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType

We absolutely *should* expose enums on the api, and have things like
developerPrivate.SelectType.FOLDER, which would of course solve this entire
problem (then it makes sense to declare them in the externs, and we can use them
correctly).  I was just saying that, as of now, we don't, and I don't know off
the top of my head how much work it would be to make that happen (Ben might have
a better guestimate).  I don't think it belongs in this CL, though. ;)  The goal
of these is to make the webui use extension apis, rather than to rework the
extension api system itself.  Not saying that's not worth doing, just maybe not
the place.

[not at google - send to devlin, 2015-03-02 17:05:22]

> I don't know off
> the top of my head how much work it would be to make that happen (Ben might
have
> a better guestimate).  I don't think it belongs in this CL, though. ;)

I wrote this ages ago but didn't have time to submit it:

https://codereview.chromium.org/305643011/

[Dan Beam, 2015-03-02 18:42:31]

On 2015/03/02 16:54:13, Devlin wrote:
> > JavaScript has no enums natively, so using some existing type is necessary
if
> we
> > want similar functionality now.  And even in C++, enums are backed by
> integers.
> > 
> > > What's more, we *shouldn't* have the practice of declaring enums as such
in
> > the extern in general, because then we can't call the function. For
instance,
> if
> > we use an enum for the "enum" of SelectType (FILE or FOLDER), then closure
> > compiler breaks when we try to call it with 'FOLDER', since it's a string.
> > 
> > why can't you call the function with SelectedType.FOLDER instead of
'FOLDER',
> > even in tests?  declare the @enum at runtime instead of in @externs.
> > 
> 
> We could do this, but it seems odd to have enum declarations for the extension
> API in extensions_list.js; should we do this for all enums we use?  What about
> other webui that uses extension apis; should they all declare the enums that
are
> specified in the API?  And then, how do we keep them up to date with the API
> (when, e.g., a new enum value is added)?  In short, it's doable, but it seems
> like the wrong solution to be declaring extension api details in a webui js
> file... Of course, just my opinion.
> 
> > > And, of course, we don't (as part of the extension api) expose enum
> > definitions on the api anywhere, so we can't pass a real enum FileType.
> > 
> > why don't you?  the web does this sometimes:
> > https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType
> 
> We absolutely *should* expose enums on the api, and have things like
> developerPrivate.SelectType.FOLDER, which would of course solve this entire
> problem (then it makes sense to declare them in the externs, and we can use
them
> correctly).  I was just saying that, as of now, we don't, and I don't know off
> the top of my head how much work it would be to make that happen (Ben might
have
> a better guestimate).  I don't think it belongs in this CL, though. ;)  The
goal
> of these is to make the webui use extension apis, rather than to rework the
> extension api system itself.  Not saying that's not worth doing, just maybe
not
> the place.

lgtm with any proof of intention to do this (bug? todo? both?)

[Devlin, 2015-03-02 19:03:34]

> lgtm with any proof of intention to do this (bug? todo? both?)

Done.

[Devlin, 2015-03-02 20:28:17]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-03-02 20:28:18]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/958803004/#ps120001
(title: "Win fix")

[commit-bot: I haz the power, 2015-03-02 20:29:41]

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

[commit-bot: I haz the power, 2015-03-02 21:07:39]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-03-02 21:08:44]

Patchset 5 (id:??) landed as
https://crrev.com/83c39e683f9f50bb07e065e3a89a0259abf44664
Cr-Commit-Position: refs/heads/master@{#318754}