Implement <webview>.addContentScript/removeContentScript API [3]

Implement <webview>.addContentScript/removeContentScript API [3]

This patch enables the code injection for
<webview>.addContentScript/removeContentScript API.

This is the third patch in a series of patches:
1) Implement <webview>.addContentScript/removeContentScript API [1] (https://codereview.chromium.org/959413003)
2) Implement <webview>.addContentScript/removeContentScript API [2] (https://codereview.chromium.org/1056533002)
3) Implement <webview>.addContentScript/removeContentScript API [3] (https://codereview.chromium.org/1058113002)

BUG=461052
Committed: https://crrev.com/0f31f97fc9577a559a527a3421981b75427791b8
Cr-Commit-Position: refs/heads/master@{#327037}

[Xi Han, 2015-04-02 20:30:10]

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

[Fady Samuel, 2015-04-07 20:20:37]

lgtm assuming devlin@ or kalman@ are happy with this approach.

https://codereview.chromium.org/1058113002/diff/1/extensions/browser/api/gues...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/1058113002/diff/1/extensions/browser/api/gues...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:173:
file.set_content(code);
You should probably ask devlin@ or kalman@ whether this is the right approach
for executing code snippets.

[Xi Han, 2015-04-07 20:25:04]

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

[Xi Han, 2015-04-07 20:25:04]

+ Devlin: please review all changes.

+ Xiyuan: please review:
-chrome/browser/ui/webui/webui_webview_browsertest.cc
-chrome/test/data/webui/webview_execute_script_test.js

Thanks.

[Xi Han, 2015-04-07 20:25:52]

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

[Xi Han, 2015-04-07 20:25:52]

+ Xiyuan: please review:
-chrome/browser/ui/webui/webui_webview_browsertest.cc
-chrome/test/data/webui/webview_execute_script_test.js

Thanks.

[xiyuan, 2015-04-07 20:35:02]

webui/* LGTM

But I have a question on the API: why do we need an array for holding the
injected code?

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:124: "type": "array",
Shouldn't a string be sufficient?

[Xi Han, 2015-04-07 20:43:10]

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:124: "type": "array",
On 2015/04/07 20:35:02, xiyuan wrote:
> Shouldn't a string be sufficient?

I just make the "code" be consistent with "js" or "css". So user can inject
multiple code at the same time, just as injecting multiple .js files:)

[Xi Han, 2015-04-07 20:51:13]

Patchset #2 (id:20001) has been deleted

[xiyuan, 2015-04-07 21:13:14]

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:124: "type": "array",
On 2015/04/07 20:43:10, Xi Han wrote:
> On 2015/04/07 20:35:02, xiyuan wrote:
> > Shouldn't a string be sufficient?
> 
> I just make the "code" be consistent with "js" or "css". So user can inject
> multiple code at the same time, just as injecting multiple .js files:)

"js"/"css" refers to files and it makes sense to have an array to combine
multiple files. But "code" essentially are strings and developer could just
concat them if needed. In this regard, array seems a bit unnecessary to me. But
I don't feel strongly about it. So okay to leave as-is.

[xiyuan, 2015-04-08 02:15:35]

One more nit to rename webview_execute_script_test.js ->
webview_content_script_test.js since it is more than executeScript now.

[Xi Han, 2015-04-08 14:47:40]

Renames to webview_content_script_test.js and updates the code property.

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/1058113002/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:124: "type": "array",
On 2015/04/07 21:13:14, xiyuan wrote:
> On 2015/04/07 20:43:10, Xi Han wrote:
> > On 2015/04/07 20:35:02, xiyuan wrote:
> > > Shouldn't a string be sufficient?
> > 
> > I just make the "code" be consistent with "js" or "css". So user can inject
> > multiple code at the same time, just as injecting multiple .js files:)
> 
> "js"/"css" refers to files and it makes sense to have an array to combine
> multiple files. But "code" essentially are strings and developer could just
> concat them if needed. In this regard, array seems a bit unnecessary to me.
But
> I don't feel strongly about it. So okay to leave as-is.

Your point makes sense to me. Updated to a string:)

[xiyuan, 2015-04-08 15:54:02]

LGTM++

https://codereview.chromium.org/1058113002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/webview_content_script_test.js (right):

https://codereview.chromium.org/1058113002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/webview_content_script_test.js:389: "matches":
["http://*/empty*"],
nit: double quote -> single quote in all places.

[Xi Han, 2015-04-16 20:18:32]

Patchset #5 (id:100001) has been deleted

[Fady Samuel, 2015-04-18 15:40:25]

https://codereview.chromium.org/1058113002/diff/120001/extensions/common/api/...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/1058113002/diff/120001/extensions/common/api/...
extensions/common/api/web_view_internal.json:142: "description": "The list of
JavaScript code to be injected into matching pages. These are injected in the
order they appear in this array."
This description is no longer accurate. This is not an array.

[Xi Han, 2015-04-20 13:37:15]

https://codereview.chromium.org/1058113002/diff/120001/extensions/common/api/...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/1058113002/diff/120001/extensions/common/api/...
extensions/common/api/web_view_internal.json:142: "description": "The list of
JavaScript code to be injected into matching pages. These are injected in the
order they appear in this array."
On 2015/04/18 15:40:24, Fady Samuel wrote:
> This description is no longer accurate. This is not an array.

Done.

[Xi Han, 2015-04-27 12:49:30]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2015-04-27 12:49:31]

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

[commit-bot: I haz the power, 2015-04-27 12:49:46]

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

[commit-bot: I haz the power, 2015-04-27 14:10:10]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-04-27 14:11:05]

Patchset 7 (id:??) landed as
https://crrev.com/0f31f97fc9577a559a527a3421981b75427791b8
Cr-Commit-Position: refs/heads/master@{#327037}