Allow extension APIs to be called from WebUI.

Allow extension APIs to be called from WebUI. The only API currently
available is chrome.test, but the internal APIs used for <webview>
and <extensionoptions> will be next. It also provides an alternative to
chrome.send in theory.

BUG=391944
R=rockot@chromium.org, jam@chromium.org

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

[not at google - send to devlin, 2014-07-18 19:03:23]

rockot - please review all the extension things.

jam - please review my content API changes. It's the only way I could see for
sanity checking WebUI calls but if there's a better way than changing the
content API then please let me know.

some people CCed here may be interested.

[not at google - send to devlin, 2014-07-18 19:05:33]

On 2014/07/18 19:03:23, kalman wrote:
> rockot - please review all the extension things.
> 
> jam - please review my content API changes. It's the only way I could see for
> sanity checking WebUI calls but if there's a better way than changing the
> content API then please let me know.
> 
> some people CCed here may be interested.

and of course, rockot/jam/other please sanity check this. it is a little scary.

[Ken Rockot(use gerrit already), 2014-07-18 22:33:40]

extensions lgtm

https://codereview.chromium.org/404883002/diff/1/extensions/browser/event_rou...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/404883002/diff/1/extensions/browser/event_rou...
extensions/browser/event_router.h:326: // NOTE: the Extension argument to this
may be NULL if this event if it's
nit: in/on/for* (not if) this event?

https://codereview.chromium.org/404883002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/binding.js (right):

https://codereview.chromium.org/404883002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/binding.js:431:
GetAvailability('runtime.lastError').is_available)) {
I'm not sure how to make this less confusing, but it took me a few reads to
understand it.

[not at google - send to devlin, 2014-07-18 23:28:35]

https://codereview.chromium.org/404883002/diff/1/extensions/browser/event_rou...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/404883002/diff/1/extensions/browser/event_rou...
extensions/browser/event_router.h:326: // NOTE: the Extension argument to this
may be NULL if this event if it's
On 2014/07/18 22:33:40, Ken Rockot wrote:
> nit: in/on/for* (not if) this event?

Done. I had way too many words in here.

https://codereview.chromium.org/404883002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/binding.js (right):

https://codereview.chromium.org/404883002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/binding.js:431:
GetAvailability('runtime.lastError').is_available)) {
On 2014/07/18 22:33:40, Ken Rockot wrote:
> I'm not sure how to make this less confusing, but it took me a few reads to
> understand it.

I was confused when I re-read my own CL. How does this look?

[Ken Rockot(use gerrit already), 2014-07-18 23:39:24]

GREAT.

[not at google - send to devlin, 2014-07-21 16:52:52]

New week --> inbox bubble.

Charlie since you're back you might want to have a glance at this, if you have
time.

[jam, 2014-07-21 17:25:17]

someone from security should look at this, randomly picking Justin

[jschuh, 2014-07-21 23:56:48]

On 2014/07/21 17:25:17, jam wrote:
> someone from security should look at this, randomly picking Justin

Chatted with kalman@, and I'm cool if we require path matching on top of the
current logic.

[Charlie Reis, 2014-07-22 20:43:51]

On 2014/07/21 16:52:52, kalman wrote:
> New week --> inbox bubble.
> 
> Charlie since you're back you might want to have a glance at this, if you have
> time.

Taking a look now.

[not at google - send to devlin, 2014-07-22 21:01:09]

Thanks.

I've implemented the URL restriction, but it turned out to be a very large
change in itself because the infrastructure wasn't in place for events. I need
to clean it up before being able to send out for review, and add tests, but here
it is:

https://codereview.chromium.org/411733002 (a diff off this patch)

Perhaps I can submit it as a follow-up to this patch? It would save a lot of the
churn which would result from either including that epic patch in this (it's
another 500+ line diff), *or* waiting several days for a pre-CL to be reviewed
and go in before coming back here.

Justin/John/Charlie - does that sound ok?

[Charlie Reis, 2014-07-22 21:10:24]

The general plan sounds good, since WebUI processes are already more privileged
than extensions.  I missed the discussion on the URL path restriction, but it
sounds like a good idea.  I'll defer to Justin on whether it needs to be
included in this patch or can be added as followup.

The rest LGTM with nits.  (Caveat: I don't have a deep familiarity with the
extension function dispatching so I doubt I'd spot any missing logic.)

https://codereview.chromium.org/404883002/diff/20001/content/public/browser/c...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/404883002/diff/20001/content/public/browser/c...
content/public/browser/child_process_security_policy.h:149: // Returns true if
the specified child_id has been granted WebUIBindings.
nit: WebUI bindings (same below)

(I realize it's just moving an old comment, but we may as well fix it while
we're here.)

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
extensions/browser/event_router.cc:558: if
(!ExtensionAPI::GetSharedInstance()->IsAvailableToWebUI(
Good, I'm glad we have a whitelist of available APIs.

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
extensions/browser/event_router.cc:564: // messaging) don't go through
EventRouter so this should be impossible.
"should be impossible:" Should this be a NOTREACHED (debug-only) or CHECK (all
builds) so that we notice if it's happening?

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
extensions/browser/event_router.h:326: // NOTE: the Extension argument to this
may be NULL if it's possible for this
nit: if -> because

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function_dispatcher.cc:403:
function->Run()->Execute();
Minor nit: Maybe this would be easier to read if we just put it in a "if
(!extension) { Execute; return; }" block on line 368, leaving the rest of the
block as it was?

[jschuh, 2014-07-22 23:09:51]

I'm fine with the URL matching in a follow-up, so lgtm in principle. I defer to
creis@ on the details of anything navigation related.

[not at google - send to devlin, 2014-07-22 23:42:33]

Thanks all.

jam@ - are the content changes reasonable?

https://codereview.chromium.org/404883002/diff/20001/content/public/browser/c...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/404883002/diff/20001/content/public/browser/c...
content/public/browser/child_process_security_policy.h:149: // Returns true if
the specified child_id has been granted WebUIBindings.
On 2014/07/22 21:10:24, Charlie Reis wrote:
> nit: WebUI bindings (same below)
> 
> (I realize it's just moving an old comment, but we may as well fix it while
> we're here.)

Done.

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
extensions/browser/event_router.cc:564: // messaging) don't go through
EventRouter so this should be impossible.
On 2014/07/22 21:10:24, Charlie Reis wrote:
> "should be impossible:" Should this be a NOTREACHED (debug-only) or CHECK (all
> builds) so that we notice if it's happening?

NOTREACHED sgtm.

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/event...
extensions/browser/event_router.h:326: // NOTE: the Extension argument to this
may be NULL if it's possible for this
On 2014/07/22 21:10:24, Charlie Reis wrote:
> nit: if -> because

Done.

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/404883002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function_dispatcher.cc:403:
function->Run()->Execute();
On 2014/07/22 21:10:24, Charlie Reis wrote:
> Minor nit: Maybe this would be easier to read if we just put it in a "if
> (!extension) { Execute; return; }" block on line 368, leaving the rest of the
> block as it was?

Done.

[not at google - send to devlin, 2014-07-24 00:49:02]

ping @jam

[jam, 2014-07-24 01:23:38]

content lgtm, sorry for the delay

[not at google - send to devlin, 2014-07-24 01:26:24]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-07-24 01:28:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/404883002/120001

[commit-bot: I haz the power, 2014-07-24 12:48:14]

Change committed as 285217