[Extensions] Make chrome://extensions use developerPrivate for inspect

[Extensions] Make chrome://extensions use developerPrivate for inspect

Make the chrome://extensions page use chrome.developerPrivate API for
'inspect' calls.
Also convert the api function to a UIThreadExtensionFunction.

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

[Devlin, 2015-03-07 06:59:57]

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

[Devlin, 2015-03-07 06:59:58]

Easy one. :)

[not at google - send to devlin, 2015-03-09 18:26:38]

lgtm assuming you can figure it all out.

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:104:
const char kInvalidRenderProcessIdError[] = "Invalid render process id.";
These say the same thing, though I don't know if I'd bother having 2 errors
anyway.

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:848: //
TODO(devlin): These should clearly not be strings.
Hmm well what you could do is change the IDL to be a choice[*]. That will
generate options.render_process_id. as_string and
options.render_process_id.as_integer. The code could then be:

if (options.render_process_id.as_string &&
    !base::StringToInt(*options.render_process_id.as_string,
                       options.render_process_id.as_integer) {
  return RespondNow(Error(...));
}

or, whatever you desire - but then the chrome://extensions code can actually
make sense.

[*] I think the syntax is:
(DOMString | integer) render_process_id;

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:859:
browser_context())->enabled_extensions().GetByID(options.extension_id);
GetExtensionById, or a version of that which only looks at enabled extensions?

https://codereview.chromium.org/989843002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989843002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_list.js:734:
chrome.developerPrivate.inspect(
I very much prefer a style like

chrome.developerProvider.inspect({
  extension_id: ...
  ...
});

similar to functions, and how I usually see objects declared. Though, how
clang-format insists on NOT doing unfortunately.

[Devlin, 2015-03-09 19:13:58]

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:104:
const char kInvalidRenderProcessIdError[] = "Invalid render process id.";
On 2015/03/09 18:26:38, kalman wrote:
> These say the same thing, though I don't know if I'd bother having 2 errors
> anyway.

Done.

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:848: //
TODO(devlin): These should clearly not be strings.
On 2015/03/09 18:26:38, kalman wrote:
> Hmm well what you could do is change the IDL to be a choice[*]. That will
> generate options.render_process_id. as_string and
> options.render_process_id.as_integer. The code could then be:
> 
> if (options.render_process_id.as_string &&
>     !base::StringToInt(*options.render_process_id.as_string,
>                        options.render_process_id.as_integer) {
>   return RespondNow(Error(...));
> }
> 
> or, whatever you desire - but then the chrome://extensions code can actually
> make sense.
> 
> [*] I think the syntax is:
> (DOMString | integer) render_process_id;

Nifty. Done.

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:859:
browser_context())->enabled_extensions().GetByID(options.extension_id);
On 2015/03/09 18:26:38, kalman wrote:
> GetExtensionById, or a version of that which only looks at enabled extensions?

Hmm... the point is to be briefer than ExtensionRegistry::GetExtensionById.  If
we start adding bools and masks, doesn't that kinda, well, eliminate that point?
 Is it really worth it for one line?

https://codereview.chromium.org/989843002/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989843002/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_list.js:734:
chrome.developerPrivate.inspect(
On 2015/03/09 18:26:38, kalman wrote:
> I very much prefer a style like
> 
> chrome.developerProvider.inspect({
>   extension_id: ...
>   ...
> });
> 
> similar to functions, and how I usually see objects declared. Though, how
> clang-format insists on NOT doing unfortunately.

I tend to prefer it, too, but not sure how common it is.  Changed for now; dbeam
can decide  if it should change back.

[Devlin, 2015-03-09 19:14:17]

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

[Devlin, 2015-03-09 19:14:17]

+Dan for *.js.

[Dan Beam, 2015-03-09 20:26:25]

lgtm w/nit

fwiw: JavaScript uses doubles for all numbers, so it can't represent signed
int64, which is why we often send around string-ified numbers.

HOWEVER, PIDs currently seem restricted to 32 bits (pid_t or DWORD) so any PIDs
should be fine a doubles in JS.

https://codereview.chromium.org/989843002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989843002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:737: incognito:
view.incognito
nit: 2\s less indent

[not at google - send to devlin, 2015-03-09 21:13:17]

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/989843002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:859:
browser_context())->enabled_extensions().GetByID(options.extension_id);
On 2015/03/09 19:13:57, Devlin wrote:
> On 2015/03/09 18:26:38, kalman wrote:
> > GetExtensionById, or a version of that which only looks at enabled
extensions?
> 
> Hmm... the point is to be briefer than ExtensionRegistry::GetExtensionById. 
If
> we start adding bools and masks, doesn't that kinda, well, eliminate that
point?
>  Is it really worth it for one line?

to be consistent with ExtensionRegistry, which takes a flag:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

but whatevs.

[Devlin, 2015-03-09 22:08:28]

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

[Devlin, 2015-03-09 22:08:28]

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/989843002/#ps40001 (title:
" ")

[commit-bot: I haz the power, 2015-03-09 22:09:00]

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

[Devlin, 2015-03-09 22:10:27]

https://codereview.chromium.org/989843002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989843002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:737: incognito:
view.incognito
On 2015/03/09 20:26:25, Dan Beam wrote:
> nit: 2\s less indent

Done.

[commit-bot: I haz the power, 2015-03-09 22:21:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-09 22:22:32]

Patchset 3 (id:??) landed as
https://crrev.com/ea4d3ab414ee122ae86f268ecc92e0428454a52a
Cr-Commit-Position: refs/heads/master@{#319748}

[Dan Beam, 2015-03-09 22:23:51]

https://codereview.chromium.org/989843002/diff/40001/third_party/closure_comp...
File third_party/closure_compiler/externs/developer_private.js (right):

https://codereview.chromium.org/989843002/diff/40001/third_party/closure_comp...
third_party/closure_compiler/externs/developer_private.js:71: *  
render_view_id: (string|number),
btw, we should probably update the internal copy as well.  didn't know it
existed till right now :/

[Devlin, 2015-03-09 22:27:00]

On 2015/03/09 22:23:51, Dan Beam wrote:
>
https://codereview.chromium.org/989843002/diff/40001/third_party/closure_comp...
> File third_party/closure_compiler/externs/developer_private.js (right):
> 
>
https://codereview.chromium.org/989843002/diff/40001/third_party/closure_comp...
> third_party/closure_compiler/externs/developer_private.js:71: *  
> render_view_id: (string|number),
> btw, we should probably update the internal copy as well.  didn't know it
> existed till right now :/

Hmm... I wonder if it's okay for it to be a couple versions behind?  There are
gonna be some more changes to it soon, and it'd be nice to just do one big
update.