Enable <webview>.executeScript outside of Apps and Extensions [1]

Enable <webview>.executeScript outside of Apps and Extensions [1]

This patch enables javascript code injection like <webview>.executeScript({code: ...}), but does not include file injection like <webview>.executeScript({file: ...}). File injection will be in another patch.

BUG=434081
Committed: https://crrev.com/79f7a57302cae3f7afcd53c6dff542e32bada999
Cr-Commit-Position: refs/heads/master@{#319727}

[Xi Han, 2015-02-19 19:44:08]

Patchset #1 (id:1) has been deleted

[Xi Han, 2015-02-19 19:46:06]

Patchset #1 (id:20001) has been deleted

[Xi Han, 2015-02-19 19:49:06]

hanxi@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-02-19 19:52:55]

https://codereview.chromium.org/942533003/diff/40001/extensions/renderer/prog...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/942533003/diff/40001/extensions/renderer/prog...
extensions/renderer/programmatic_script_injector.cc:87: return
PermissionsData::ACCESS_DENIED;
I'm confused by this, doesn't this disable executeScript for WebUI?

[Xi Han, 2015-02-19 19:56:04]

https://codereview.chromium.org/942533003/diff/40001/extensions/renderer/prog...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/942533003/diff/40001/extensions/renderer/prog...
extensions/renderer/programmatic_script_injector.cc:87: return
PermissionsData::ACCESS_DENIED;
On 2015/02/19 19:52:54, Fady Samuel wrote:
> I'm confused by this, doesn't this disable executeScript for WebUI?

Yes, this disable executeScript for WebUI, but not <webview>.executeCode on
WebUI, which has been handled in line 75. Right?

[Fady Samuel, 2015-02-23 17:03:23]

Ohh I see. lgtm, but I'm not really the right person to review this. Please ask
devlin or kalman. Thanks!

[Xi Han, 2015-02-23 17:17:34]

New patchsets have been uploaded after l-g-t-m from fsamuel@chromium.org

[Xi Han, 2015-02-23 17:22:36]

Patchset #2 (id:60001) has been deleted

[Xi Han, 2015-02-23 17:26:36]

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

[Xi Han, 2015-02-23 17:26:36]

Fady: I remove the check and add a DCHECK there, hopefully the code looks better
now:)

rdevlin.cronin@chromium.org: Please review all changes. Thanks!

[Devlin, 2015-02-25 17:22:36]

If this patch is all it takes to make webui able to use executeScript, then it
needs tests.

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
extensions/browser/api/execute_code_function.cc:39: : host_id_(new HostID()) {
Why is this is a scoped ptr if it's never null?

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/w...
File extensions/browser/api/web_view/web_view_internal_api.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/w...
extensions/browser/api/web_view/web_view_internal_api.cc:112: auto rvh =
render_view_host();
Why use auto here?  It subtracts, rather than adds, to the readability.

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/scrip...
File extensions/browser/script_executor.h (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/scrip...
extensions/browser/script_executor.h:88: void ExecuteScript(const
HostID::HostType& host_type,
two things:
- passing an enum by const & is silly. :)
- host_type + host_id = HostId, so why not just pass a HostId instead of both
parameters?

https://codereview.chromium.org/942533003/diff/80001/extensions/renderer/web_...
File extensions/renderer/web_ui_injection_host.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/renderer/web_...
extensions/renderer/web_ui_injection_host.cc:18: return scoped_ptr<const
InjectionHost>(new WebUIInjectionHost(host_id));
This create function doesn't really abstract out any work, so I'm not sure it's
worthwhile.

[Xi Han, 2015-02-26 15:13:45]

Hi Devlin: please review other changes first. I will introduce a test for this
patch ASAP. Thanks:)

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
extensions/browser/api/execute_code_function.cc:39: : host_id_(new HostID()) {
On 2015/02/25 17:22:36, Devlin wrote:
> Why is this is a scoped ptr if it's never null?

Because we want to set its value in the init() function, while HostID doesn't
provide any setter.

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/w...
File extensions/browser/api/web_view/web_view_internal_api.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/w...
extensions/browser/api/web_view/web_view_internal_api.cc:112: auto rvh =
render_view_host();
On 2015/02/25 17:22:36, Devlin wrote:
> Why use auto here?  It subtracts, rather than adds, to the readability.

Updated.

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/scrip...
File extensions/browser/script_executor.h (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/scrip...
extensions/browser/script_executor.h:88: void ExecuteScript(const
HostID::HostType& host_type,
On 2015/02/25 17:22:36, Devlin wrote:
> two things:
> - passing an enum by const & is silly. :)
> - host_type + host_id = HostId, so why not just pass a HostId instead of both
> parameters?

Good point, updated.

https://codereview.chromium.org/942533003/diff/80001/extensions/renderer/web_...
File extensions/renderer/web_ui_injection_host.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/renderer/web_...
extensions/renderer/web_ui_injection_host.cc:18: return scoped_ptr<const
InjectionHost>(new WebUIInjectionHost(host_id));
On 2015/02/25 17:22:36, Devlin wrote:
> This create function doesn't really abstract out any work, so I'm not sure
it's
> worthwhile.

That is ok, remove it for now.

[Devlin, 2015-02-26 17:35:24]

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
extensions/browser/api/execute_code_function.cc:39: : host_id_(new HostID()) {
On 2015/02/26 15:13:45, Xi Han wrote:
> On 2015/02/25 17:22:36, Devlin wrote:
> > Why is this is a scoped ptr if it's never null?
> 
> Because we want to set its value in the init() function, while HostID doesn't
> provide any setter.

But we can just do host_id_ = HostId(foo, bar), right?

[Xi Han, 2015-02-26 19:58:00]

PTAL, thanks!

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/942533003/diff/80001/extensions/browser/api/e...
extensions/browser/api/execute_code_function.cc:39: : host_id_(new HostID()) {
On 2015/02/26 17:35:24, Devlin wrote:
> On 2015/02/26 15:13:45, Xi Han wrote:
> > On 2015/02/25 17:22:36, Devlin wrote:
> > > Why is this is a scoped ptr if it's never null?
> > 
> > Because we want to set its value in the init() function, while HostID
doesn't
> > provide any setter.
> 
> But we can just do host_id_ = HostId(foo, bar), right?

You are right:) Sorry I was in the old memory that HostID doesn't have the
assignment operator.

[Devlin, 2015-02-27 17:11:36]

Couple of small things, but really just waiting on a test.

https://codereview.chromium.org/942533003/diff/120001/extensions/browser/api/...
File extensions/browser/api/web_view/web_view_internal_api.cc (right):

https://codereview.chromium.org/942533003/diff/120001/extensions/browser/api/...
extensions/browser/api/web_view/web_view_internal_api.cc:111: const
content::RenderViewHost* rvh = render_view_host();
This can actually be simplified to:
content::WebContents* web_contents = GetSenderWebContents();
if (web_contents && web_contents->GetWebUI()) {
  ...
}

https://codereview.chromium.org/942533003/diff/120001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/942533003/diff/120001/extensions/common/exten...
extensions/common/extension_messages.h:113: IPC_STRUCT_MEMBER(HostID::HostType,
host_type)
Why not just pass the HostID?

[Xi Han, 2015-02-27 21:07:46]

Hi Devlin: I update the IPC message to contain HostID now, PTAL, thanks!

https://codereview.chromium.org/942533003/diff/120001/extensions/browser/api/...
File extensions/browser/api/web_view/web_view_internal_api.cc (right):

https://codereview.chromium.org/942533003/diff/120001/extensions/browser/api/...
extensions/browser/api/web_view/web_view_internal_api.cc:111: const
content::RenderViewHost* rvh = render_view_host();
On 2015/02/27 17:11:36, Devlin wrote:
> This can actually be simplified to:
> content::WebContents* web_contents = GetSenderWebContents();
> if (web_contents && web_contents->GetWebUI()) {
>   ...
> }

Done.

https://codereview.chromium.org/942533003/diff/120001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/942533003/diff/120001/extensions/common/exten...
extensions/common/extension_messages.h:113: IPC_STRUCT_MEMBER(HostID::HostType,
host_type)
On 2015/02/27 17:11:36, Devlin wrote:
> Why not just pass the HostID?

Updated.

[Devlin, 2015-02-27 21:50:40]

On 2015/02/27 21:07:46, Xi Han wrote:
> Hi Devlin: I update the IPC message to contain HostID now, PTAL, thanks!
> 
>
https://codereview.chromium.org/942533003/diff/120001/extensions/browser/api/...
> File extensions/browser/api/web_view/web_view_internal_api.cc (right):
> 
>
https://codereview.chromium.org/942533003/diff/120001/extensions/browser/api/...
> extensions/browser/api/web_view/web_view_internal_api.cc:111: const
> content::RenderViewHost* rvh = render_view_host();
> On 2015/02/27 17:11:36, Devlin wrote:
> > This can actually be simplified to:
> > content::WebContents* web_contents = GetSenderWebContents();
> > if (web_contents && web_contents->GetWebUI()) {
> >   ...
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/942533003/diff/120001/extensions/common/exten...
> File extensions/common/extension_messages.h (right):
> 
>
https://codereview.chromium.org/942533003/diff/120001/extensions/common/exten...
> extensions/common/extension_messages.h:113:
IPC_STRUCT_MEMBER(HostID::HostType,
> host_type)
> On 2015/02/27 17:11:36, Devlin wrote:
> > Why not just pass the HostID?
> 
> Updated.

As I mentioned before, I'm really just waiting on a test for this.  Otherwise,
it pretty much lg.

[Xi Han, 2015-03-06 22:53:28]

Patchset #6 (id:160001) has been deleted

[Xi Han, 2015-03-06 23:07:45]

Thanks for xiyuan@'s help, a test is added to call webview.executeScript on
webUI, PTAL, thanks!

[xiyuan, 2015-03-07 04:40:27]

xiyuan@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiyuan, 2015-03-07 04:40:28]

webui/* LGTM

[Devlin, 2015-03-09 16:04:42]

lgtm with nits

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
chrome/test/data/webui/webview_execute_script_test.js:20: code:
'document.body.style.backgroundColor'
nit: add a semicolon.

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
chrome/test/data/webui/webview_execute_script_test.js:26: code:
'document.body.style.backgroundColor = \'red\''
nit: add a semicolon.

https://codereview.chromium.org/942533003/diff/200001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/942533003/diff/200001/extensions/common/exten...
extensions/common/extension_messages.h:113: IPC_STRUCT_MEMBER(HostID, host_id)
I'm getting more and more nervous about the size of this struct, since we need
to keep track of everywhere it's initialized and update it each time we
add/modify a field.  I think there should ideally be a way of validating it to
make sure it's okay, but I don't really know what a good way to do that is. 
Hmm...

(No action required, just thinking out loud.)

https://codereview.chromium.org/942533003/diff/200001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/942533003/diff/200001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.cc:85:
DCHECK(injection_host->id().type() == HostID::EXTENSIONS);
use DCHECK_EQ

[Xi Han, 2015-03-09 17:59:55]

hanxi@chromium.org changed reviewers:
+ kenrb@chromium.org

[Xi Han, 2015-03-09 17:59:56]

kenrb@chromium.org: Please review changes in 

-extensions/common/extension_messages.h
-extensions/common/extension_messages.cc.

Thanks!

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
chrome/test/data/webui/webview_execute_script_test.js:20: code:
'document.body.style.backgroundColor'
On 2015/03/09 16:04:42, Devlin wrote:
> nit: add a semicolon.

Tried to add a semicolon, but the semicolon caused the tests failed. Check the
format again, and it doesn't have the semicolon in the example.

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
chrome/test/data/webui/webview_execute_script_test.js:26: code:
'document.body.style.backgroundColor = \'red\''
On 2015/03/09 16:04:42, Devlin wrote:
> nit: add a semicolon.

Same above.

https://codereview.chromium.org/942533003/diff/200001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/942533003/diff/200001/extensions/common/exten...
extensions/common/extension_messages.h:113: IPC_STRUCT_MEMBER(HostID, host_id)
On 2015/03/09 16:04:42, Devlin wrote:
> I'm getting more and more nervous about the size of this struct, since we need
> to keep track of everywhere it's initialized and update it each time we
> add/modify a field.  I think there should ideally be a way of validating it to
> make sure it's okay, but I don't really know what a good way to do that is. 
> Hmm...
> 
> (No action required, just thinking out loud.)

Acknowledged.

https://codereview.chromium.org/942533003/diff/200001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/942533003/diff/200001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.cc:85:
DCHECK(injection_host->id().type() == HostID::EXTENSIONS);
On 2015/03/09 16:04:42, Devlin wrote:
> use DCHECK_EQ

Done.

[Devlin, 2015-03-09 18:07:59]

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
chrome/test/data/webui/webview_execute_script_test.js:20: code:
'document.body.style.backgroundColor'
On 2015/03/09 17:59:56, Xi Han wrote:
> On 2015/03/09 16:04:42, Devlin wrote:
> > nit: add a semicolon.
> 
> Tried to add a semicolon, but the semicolon caused the tests failed. Check the
> format again, and it doesn't have the semicolon in the example.

I find that very surprising.  To be clear, I'm suggesting this become:
code: 'document.body.style.backgroundColor;'
and below:
code: 'document.body.style.backgroundColor = \'red\';'

[Xi Han, 2015-03-09 18:28:33]

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
File chrome/test/data/webui/webview_execute_script_test.js (right):

https://codereview.chromium.org/942533003/diff/200001/chrome/test/data/webui/...
chrome/test/data/webui/webview_execute_script_test.js:20: code:
'document.body.style.backgroundColor'
On 2015/03/09 18:07:59, Devlin wrote:
> On 2015/03/09 17:59:56, Xi Han wrote:
> > On 2015/03/09 16:04:42, Devlin wrote:
> > > nit: add a semicolon.
> > 
> > Tried to add a semicolon, but the semicolon caused the tests failed. Check
the
> > format again, and it doesn't have the semicolon in the example.
> 
> I find that very surprising.  To be clear, I'm suggesting this become:
> code: 'document.body.style.backgroundColor;'
> and below:
> code: 'document.body.style.backgroundColor = \'red\';'

You are right. I put the semicolon after the "'", which caused the failures.

[kenrb, 2015-03-09 19:36:25]

ipc lgtm

[Xi Han, 2015-03-09 20:21:01]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2015-03-09 20:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, rdevlin.cronin@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/942533003/#ps240001
(title: "nits")

[commit-bot: I haz the power, 2015-03-09 20:21:36]

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

[commit-bot: I haz the power, 2015-03-09 20:47:14]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2015-03-09 20:49:51]

Patchset 9 (id:??) landed as
https://crrev.com/79f7a57302cae3f7afcd53c6dff542e32bada999
Cr-Commit-Position: refs/heads/master@{#319727}