<webview>: Implement ExecuteScript

<webview>: Implement ExecuteScript

This patch implements executeScript for <webview> by using extensions bindings for forwarding requests to the browser process from the app process.

The <webview> shim passes the ProcessId and the RouteID of the guest process to the ExecuteScriptFunction object in the browser process. From there, ExecuteScriptFunction grabs the guest web contents, and creates a ScriptExecutor object attached to the guest WebContents.

The callback is supported trivially through the extension bindings.

When a new guest web contents is created, we inject the Chrome App's extension information into the guest process, so that executeScript knows to bypass permission requests when attempting to execute script within the guest (as suggested by mpcomplete@).

BUG=153530
Test=WebViewTest.Shim, webViewExecuteScript

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

[Fady Samuel, 2013-01-21 18:08:28]

Hi Matt,

Could you please take a look at the changes I've made in web_view.js, and
ExecuteScriptFunction, as well as the injection of the Chrome App's extension
object into the guest web contents in
ChromeContentBrowserClient::GuestWebContentsCreated. Also, please take a look at
the bypassing of permissions for executeScript in UserScriptScheduler.

I believe what I've done matches our discussions. Could you please provide some
feedback? Does this approach look reasonable to you?

Thanks,

Fady

[Fady Samuel, 2013-01-23 01:16:55]

+lazyboy@ for browser_plugin changes.

[Matt Perry, 2013-01-23 02:50:49]

General approach looks good. One concern is the amount of code duplication in
ExecuteScript. I imagine you already tried to share that with
ExecuteCodeInTabFunction.

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:879: const Extension* extension
= service->extensions()->GetByID(url.host());
Use GetExtensionOrAppByURL - the URL here is not necessarily a chrome-extension
URL, correct?

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:884:
guest_web_contents->Send(new ExtensionMsg_Loaded(extensions));
Normally this is sent by
ChromeRenderViewHostObserver::InitRenderViewForExtensions. Does that not get
called for <webview>s?

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/api/webview/execute_script_function.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/execute_script_function.cc:36:
EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(2, &details_value));
You can clean up some of this parsing logic using the JSON schema bindings. See
alarms_api.cc for an example. You'll do something like
webview::ExecuteScript::Params::Create(*args_).

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/execute_script_function.cc:52:
script_observers_;
no trailing _

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/execute_script_function.cc:80:
*details->code.get(),
There's a version of executeScript that uses a file. Do you plan not to support
that one?

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_registry.cc:63: // Browser Plugin.
We're trying to decentralize API registration. See 
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions...
for an example (search for RegisterFunction).

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/user_script_scheduler.cc:178: if
(!extension->is_app() &&
This is not quite the right check.

Are we sure we want to let an embedder call webview.executeScript() no matter
what URL is loaded into that webview? What if we required the webview app to
list its host permissions before it can execute script on those hosts?

If we really need to bypass this check, we should do it in a manner more
specific to webview. That might mean modifying ScriptExecutor to pass down an
extra is_webview param.

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:83: { "type": "function",
"name": "callback", "optional": true }]);
If you need to expose more APIs to webview, I recommend moving this to a .json
file and parsing it like the other APIs.

https://codereview.chromium.org/11968054/diff/13001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11968054/diff/13001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:84: int guest_route_id() const
{ return guest_route_id_; }
seems inconsistent to prefix this with guest_ when process_id() isn't.

[Fady Samuel, 2013-01-23 20:12:03]

PTAL

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:879: const Extension* extension
= service->extensions()->GetByID(url.host());
On 2013/01/23 02:50:49, Matt Perry wrote:
> Use GetExtensionOrAppByURL - the URL here is not necessarily a
chrome-extension
> URL, correct?

Done.

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:884:
guest_web_contents->Send(new ExtensionMsg_Loaded(extensions));
On 2013/01/23 02:50:49, Matt Perry wrote:
> Normally this is sent by
> ChromeRenderViewHostObserver::InitRenderViewForExtensions. Does that not get
> called for <webview>s?

ChromeRenderViewHostObserver::GetExtension() returns NULL there for guests.

We don't have access to the embedder WebContents in
ChromeRenderViewHostObserver.

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/api/webview/execute_script_function.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/execute_script_function.cc:36:
EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(2, &details_value));
On 2013/01/23 02:50:49, Matt Perry wrote:
> You can clean up some of this parsing logic using the JSON schema bindings.
See
> alarms_api.cc for an example. You'll do something like
> webview::ExecuteScript::Params::Create(*args_).

Done.

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/execute_script_function.cc:52:
script_observers_;
On 2013/01/23 02:50:49, Matt Perry wrote:
> no trailing _

Done.

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/execute_script_function.cc:80:
*details->code.get(),
On 2013/01/23 02:50:49, Matt Perry wrote:
> There's a version of executeScript that uses a file. Do you plan not to
support
> that one?

I'll have to chat with creis@ and others and perhaps add this if we decide to
support it, in another patch.

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_registry.cc:63: // Browser Plugin.
On 2013/01/23 02:50:49, Matt Perry wrote:
> We're trying to decentralize API registration. See 
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions...
> for an example (search for RegisterFunction).

Done.

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/user_script_scheduler.cc:178: if
(!extension->is_app() &&
On 2013/01/23 02:50:49, Matt Perry wrote:
> This is not quite the right check.
> 
> Are we sure we want to let an embedder call webview.executeScript() no matter
> what URL is loaded into that webview? What if we required the webview app to
> list its host permissions before it can execute script on those hosts?
> 
> If we really need to bypass this check, we should do it in a manner more
> specific to webview. That might mean modifying ScriptExecutor to pass down an
> extra is_webview param.

Done.

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11968054/diff/13001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:83: { "type": "function",
"name": "callback", "optional": true }]);
On 2013/01/23 02:50:49, Matt Perry wrote:
> If you need to expose more APIs to webview, I recommend moving this to a .json
> file and parsing it like the other APIs.

Done.

https://codereview.chromium.org/11968054/diff/13001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11968054/diff/13001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:84: int guest_route_id() const
{ return guest_route_id_; }
On 2013/01/23 02:50:49, Matt Perry wrote:
> seems inconsistent to prefix this with guest_ when process_id() isn't.

Renamed process_id() to guest_process_id(). There's another
render_view_routing_id(), and that's the routing ID of the embedder.

[Matt Perry, 2013-01-23 20:23:58]

lgtm

https://codereview.chromium.org/11968054/diff/12034/chrome/browser/extensions...
File chrome/browser/extensions/api/webview/webview_api.h (right):

https://codereview.chromium.org/11968054/diff/12034/chrome/browser/extensions...
chrome/browser/extensions/api/webview/webview_api.h:13: struct InjectDetails;
Do you need this forward decl?

https://codereview.chromium.org/11968054/diff/12034/chrome/common/extensions/...
File chrome/common/extensions/api/webview.json (right):

https://codereview.chromium.org/11968054/diff/12034/chrome/common/extensions/...
chrome/common/extensions/api/webview.json:7: "namespace": "webview",
add "nodoc": true here. Otherwise we'll generate extension docs for this, which
is really just a private implementation detail. I assume the webview API is
documented elsewhere?

[lazyboy, 2013-01-23 20:42:55]

browser_plugin changes LG.

https://codereview.chromium.org/11968054/diff/12034/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/11968054/diff/12034/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:306: // This should
not be exposed to developers.
I wish we could ensure that these types of methods could be only called from
shim code.

[Fady Samuel, 2013-01-23 20:54:32]

Fixed nits.

+jam@ for chrome/content API OWNER approval.

https://codereview.chromium.org/11968054/diff/12034/chrome/browser/extensions...
File chrome/browser/extensions/api/webview/webview_api.h (right):

https://codereview.chromium.org/11968054/diff/12034/chrome/browser/extensions...
chrome/browser/extensions/api/webview/webview_api.h:13: struct InjectDetails;
On 2013/01/23 20:23:58, Matt Perry wrote:
> Do you need this forward decl?

No. Removed.

https://codereview.chromium.org/11968054/diff/12034/chrome/common/extensions/...
File chrome/common/extensions/api/webview.json (right):

https://codereview.chromium.org/11968054/diff/12034/chrome/common/extensions/...
chrome/common/extensions/api/webview.json:7: "namespace": "webview",
On 2013/01/23 20:23:58, Matt Perry wrote:
> add "nodoc": true here. Otherwise we'll generate extension docs for this,
which
> is really just a private implementation detail. I assume the webview API is
> documented elsewhere?

Done. Yes, <webview> is documented elsewhere.

https://codereview.chromium.org/11968054/diff/12034/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/11968054/diff/12034/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:306: // This should
not be exposed to developers.
On 2013/01/23 20:42:55, lazyboy wrote:
> I wish we could ensure that these types of methods could be only called from
> shim code.

As long as it's not exposed in the shim, it cannot be accessed externally unless
the developer uses BrowserPlugin directly (which isn't supported).

[jam, 2013-01-23 22:16:03]

content lgtm

[jam, 2013-01-23 22:16:09]

https://codereview.chromium.org/11968054/diff/11021/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11968054/diff/11021/content/common/browser_pl...
content/common/browser_plugin_messages.h:72: // Chrome's routing ID for the
guest's RenderView.
nit: here and above, it's more generic to say "the browser" instead of "Chrome"

https://codereview.chromium.org/11968054/diff/11021/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11968054/diff/11021/content/public/browser/co...
content/public/browser/content_browser_client.h:170: virtual void
GuestWebContentsCreated(WebContents* guest_web_contents,
nit: put this method after RenderViewHostCreated above

[Fady Samuel, 2013-01-23 22:38:01]

Fixed nits.

+cdn@ for browser_plugin_messages.h IPC audit.

https://codereview.chromium.org/11968054/diff/11021/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11968054/diff/11021/content/common/browser_pl...
content/common/browser_plugin_messages.h:72: // Chrome's routing ID for the
guest's RenderView.
On 2013/01/23 22:16:09, John Abd-El-Malek wrote:
> nit: here and above, it's more generic to say "the browser" instead of
"Chrome"

Done.

https://codereview.chromium.org/11968054/diff/11021/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11968054/diff/11021/content/public/browser/co...
content/public/browser/content_browser_client.h:170: virtual void
GuestWebContentsCreated(WebContents* guest_web_contents,
On 2013/01/23 22:16:09, John Abd-El-Malek wrote:
> nit: put this method after RenderViewHostCreated above

Done.

[Cris Neckar, 2013-01-23 23:07:52]

IPC security audit LGTM

[commit-bot: I haz the power, 2013-01-23 23:51:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11968054/3019

[commit-bot: I haz the power, 2013-01-24 03:17:43]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[not at google - send to devlin, 2013-01-25 22:04:13]

and yes I am aware that that this patch has already been committed.

https://codereview.chromium.org/11968054/diff/3019/chrome/renderer/extensions...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/11968054/diff/3019/chrome/renderer/extensions...
chrome/renderer/extensions/user_script_scheduler.cc:178: if (!params.is_web_view
&&
I'm trying to understand the code here. Why no check for webview?

[Fady Samuel, 2013-01-25 22:06:09]

On 2013/01/25 22:04:13, kalman wrote:
> and yes I am aware that that this patch has already been committed.
> 
>
https://codereview.chromium.org/11968054/diff/3019/chrome/renderer/extensions...
> File chrome/renderer/extensions/user_script_scheduler.cc (right):
> 
>
https://codereview.chromium.org/11968054/diff/3019/chrome/renderer/extensions...
> chrome/renderer/extensions/user_script_scheduler.cc:178: if
(!params.is_web_view
> &&
> I'm trying to understand the code here. Why no check for webview?

<webview> has unrestricted access to its guest. It doesn't need additional
permissions to access the guest.