Implement script API:executeScript

programmatically inject scripts from an extension.
BUG=12465

[tangjie, 2009-09-03 10:28:07]

Hi Aaron, Erik and Matt,

  Please help review this path. Thanks!

Jerry

[Matt Perry, 2009-09-03 18:52:05]

Here's some initial feedback. I'll add more after lunch.

Mark the bug number in your issue description using "BUG=12465
" - that way our scripts will pick it up.

In the future, for CLs with multiple reviewers, please point out which files you
want each reviewer to focus on.

http://codereview.chromium.org/173556/diff/13001/14028
File chrome/browser/extensions/extension_browsertests_misc.cc (right):

http://codereview.chromium.org/173556/diff/13001/14028#newcode130
Line 130:
AppendASCII("Extensions").AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj").
We're starting to overload the extensions under good/ a little too much. Can you
move the testing code out into its own extension under
chrome/test/data/extensions/uitest/ instead?

http://codereview.chromium.org/173556/diff/13001/14001
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/13001/14001#newcode841
Line 841: // The script should be ran after page is loaded.
nit: should be run

http://codereview.chromium.org/173556/diff/13001/14001#newcode848
Line 848: DictionaryValue* script_files;
nit: script_files isn't really the right term, since it can include js and css
code directly. script_info maybe

http://codereview.chromium.org/173556/diff/13001/14001#newcode862
Line 862: UserScript::LoadScriptContent(&script_file);
this runs on the UI thread. we can't do blocking file IO on the UI thread.
you'll need to move this off to the file thread and execute the scripts
asynchronously.

http://codereview.chromium.org/173556/diff/13001/14002
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/173556/diff/13001/14002#newcode99
Line 99: private:
move this to the top, while you're at it

http://codereview.chromium.org/173556/diff/13001/14002#newcode111
Line 111: private:
move this to the top of the class

http://codereview.chromium.org/173556/diff/13001/14009
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/173556/diff/13001/14009#newcode848
Line 848: if (base::SharedMemory::IsHandleValid(handle_for_process)) {
return false if it's invalid?

http://codereview.chromium.org/173556/diff/13001/14006
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/173556/diff/13001/14006#newcode555
Line 555: "css": {"type": "array", "items": {"type": "string"}, "minItems": 1,
"optional": true, "description": "array of CSS files to execute,in order."},
nit: space after comma

http://codereview.chromium.org/173556/diff/13001/14006#newcode556
Line 556: "css_code": {"type": "array", "items": {"type": "string"}, "minItems":
1, "optional": true, "description": "array of CSS text to execute,in order."},
nit: space after comma

http://codereview.chromium.org/173556/diff/13001/14006#newcode557
Line 557: "js": {"type": "array", "items": {"type": "string"}, "minItems": 1,
"optional": true, "description": "array of JavaScript files to execute,in
order."},
nit: space after comma

http://codereview.chromium.org/173556/diff/13001/14006#newcode558
Line 558: "js_code": {"type": "array", "items": {"type": "string"}, "minItems":
1, "optional": true, "description": "array of JavaScript source code to
execute,in order."}
nit: space after comma

http://codereview.chromium.org/173556/diff/13001/14006#newcode574
Line 574: "description": "Whether this call is successfull"
"successful."

http://codereview.chromium.org/173556/diff/13001/14014
File chrome/common/extensions/user_script.h (right):

http://codereview.chromium.org/173556/diff/13001/14014#newcode34
Line 34: DOCUMENT_RUN_SCRIPT,
please comment this

[Erik does not do reviews, 2009-09-03 19:01:03]

I'll defer to Aaron and Matt here unless there's something specific
you'd like me to look at.

Erik


On Thu, Sep 3, 2009 at 3:27 AM, <tangjie@google.com> wrote:
> Reviewers: Aaron Boodman, Erik Kay, Matt Perry,
>
> Message:
> Hi Aaron, Erik and Matt,
>
> =A0Please help review this patch. Thanks!
>
> Jerry
>
> Description:
> programmatically inject scripts from an extension
>
> Please refer to
> issue:http://code.google.com/p/chromium/issues/detail?id=3D12465
>
>
> Please review this at http://codereview.chromium.org/173556
>
> SVN Base: http://src.chromium.org/svn/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/extensions/extension_browsertests_misc.cc
> =A0M =A0 =A0 chrome/browser/extensions/extension_function_dispatcher.cc
> =A0M =A0 =A0 chrome/browser/extensions/extension_tabs_module.h
> =A0M =A0 =A0 chrome/browser/extensions/extension_tabs_module.cc
> =A0M =A0 =A0 chrome/browser/extensions/extension_tabs_module_constants.h
> =A0M =A0 =A0 chrome/browser/extensions/extension_tabs_module_constants.cc
> =A0M =A0 =A0 chrome/browser/extensions/user_script_master.h
> =A0M =A0 =A0 chrome/browser/extensions/user_script_master.cc
> =A0M =A0 =A0 chrome/browser/renderer_host/render_view_host.h
> =A0M =A0 =A0 chrome/browser/renderer_host/render_view_host.cc
> =A0M =A0 =A0 chrome/browser/tab_contents/tab_contents.h
> =A0M =A0 =A0 chrome/browser/tab_contents/tab_contents.cc
> =A0M =A0 =A0 chrome/common/extensions/api/extension_api.json
> =A0M =A0 =A0 chrome/common/extensions/extension.h
> =A0M =A0 =A0 chrome/common/extensions/extension.cc
> =A0M =A0 =A0 chrome/common/extensions/extension_constants.h
> =A0M =A0 =A0 chrome/common/extensions/extension_constants.cc
> =A0M =A0 =A0 chrome/common/extensions/user_script.h
> =A0M =A0 =A0 chrome/common/extensions/user_script.cc
> =A0M =A0 =A0 chrome/common/notification_type.h
> =A0M =A0 =A0 chrome/common/render_messages_internal.h
> =A0M =A0 =A0 chrome/renderer/render_view.h
> =A0M =A0 =A0 chrome/renderer/render_view.cc
> =A0M =A0 =A0 chrome/renderer/user_script_slave.h
> =A0M =A0 =A0 chrome/renderer/user_script_slave.cc
> =A0A
> chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpgl=
gmj/1.0.0.0/script3.js
> =A0A
> chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpgl=
gmj/1.0.0.0/script4.js
> =A0A
> chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpgl=
gmj/1.0.0.0/script5.js
> =A0A
> chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpgl=
gmj/1.0.0.0/test_executescript.html
> =A0M
> chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpgl=
gmj/1.0.0.0/toolstrip1.html
>
>
>

[Matt Perry, 2009-09-03 21:54:07]

some more comments

http://codereview.chromium.org/173556/diff/13001/14017
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/173556/diff/13001/14017#newcode230
Line 230: bool /* whether the script is ran successfully */)
remove "is"

http://codereview.chromium.org/173556/diff/13001/14022
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/173556/diff/13001/14022#newcode2830
Line 2830: shared_memory.reset(shared_memory_arg);
you can do this in one line:
  scoped_ptr shared_mem(shared_memory_arg)

http://codereview.chromium.org/173556/diff/13001/14021
File chrome/renderer/user_script_slave.h (right):

http://codereview.chromium.org/173556/diff/13001/14021#newcode37
Line 37: static bool UnSerialize(
nit: Deserialize

http://codereview.chromium.org/173556/diff/13001/14021#newcode39
Line 39: std::vector<UserScript*>* scripts, base::SharedMemory** shared_memory);
maybe this should take a scoped_ptr<base::SharedMemory*>* to ensure that it gets
deleted.

[tangjie, 2009-09-04 17:57:00]

http://codereview.chromium.org/173556/diff/13001/14028
File chrome/browser/extensions/extension_browsertests_misc.cc (right):

http://codereview.chromium.org/173556/diff/13001/14028#newcode130
Line 130:
AppendASCII("Extensions").AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj").
On 2009/09/03 18:52:05, Matt Perry wrote:
> We're starting to overload the extensions under good/ a little too much. Can
you
> move the testing code out into its own extension under
> chrome/test/data/extensions/uitest/ instead?

Done.

http://codereview.chromium.org/173556/diff/13001/14001
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/13001/14001#newcode841
Line 841: // The script should be ran after page is loaded.
On 2009/09/03 18:52:05, Matt Perry wrote:
> nit: should be run

Done.

http://codereview.chromium.org/173556/diff/13001/14001#newcode848
Line 848: DictionaryValue* script_files;
On 2009/09/03 18:52:05, Matt Perry wrote:
> nit: script_files isn't really the right term, since it can include js and css
> code directly. script_info maybe

Done.

http://codereview.chromium.org/173556/diff/13001/14001#newcode862
Line 862: UserScript::LoadScriptContent(&script_file);
On 2009/09/03 18:52:05, Matt Perry wrote:
> this runs on the UI thread. we can't do blocking file IO on the UI thread.
> you'll need to move this off to the file thread and execute the scripts
> asynchronously.

Done.

http://codereview.chromium.org/173556/diff/13001/14002
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/173556/diff/13001/14002#newcode99
Line 99: private:
On 2009/09/03 18:52:05, Matt Perry wrote:
> move this to the top, while you're at it

Done.

http://codereview.chromium.org/173556/diff/13001/14002#newcode111
Line 111: private:
On 2009/09/03 18:52:05, Matt Perry wrote:
> move this to the top of the class

Done.

http://codereview.chromium.org/173556/diff/13001/14009
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/173556/diff/13001/14009#newcode848
Line 848: if (base::SharedMemory::IsHandleValid(handle_for_process)) {
On 2009/09/03 18:52:05, Matt Perry wrote:
> return false if it's invalid?

Done.

http://codereview.chromium.org/173556/diff/13001/14006
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/173556/diff/13001/14006#newcode555
Line 555: "css": {"type": "array", "items": {"type": "string"}, "minItems": 1,
"optional": true, "description": "array of CSS files to execute,in order."},
On 2009/09/03 18:52:05, Matt Perry wrote:
> nit: space after comma

Done.

http://codereview.chromium.org/173556/diff/13001/14006#newcode556
Line 556: "css_code": {"type": "array", "items": {"type": "string"}, "minItems":
1, "optional": true, "description": "array of CSS text to execute,in order."},
On 2009/09/03 18:52:05, Matt Perry wrote:
> nit: space after comma

Done.

http://codereview.chromium.org/173556/diff/13001/14006#newcode557
Line 557: "js": {"type": "array", "items": {"type": "string"}, "minItems": 1,
"optional": true, "description": "array of JavaScript files to execute,in
order."},
On 2009/09/03 18:52:05, Matt Perry wrote:
> nit: space after comma

Done.

http://codereview.chromium.org/173556/diff/13001/14006#newcode558
Line 558: "js_code": {"type": "array", "items": {"type": "string"}, "minItems":
1, "optional": true, "description": "array of JavaScript source code to
execute,in order."}
On 2009/09/03 18:52:05, Matt Perry wrote:
> nit: space after comma

Done.

http://codereview.chromium.org/173556/diff/13001/14006#newcode574
Line 574: "description": "Whether this call is successfull"
On 2009/09/03 18:52:05, Matt Perry wrote:
> "successful."

Done.

http://codereview.chromium.org/173556/diff/13001/14014
File chrome/common/extensions/user_script.h (right):

http://codereview.chromium.org/173556/diff/13001/14014#newcode34
Line 34: DOCUMENT_RUN_SCRIPT,
On 2009/09/03 18:52:05, Matt Perry wrote:
> please comment this

Done.

http://codereview.chromium.org/173556/diff/13001/14017
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/173556/diff/13001/14017#newcode230
Line 230: bool /* whether the script is ran successfully */)
On 2009/09/03 21:54:08, Matt Perry wrote:
> remove "is"

Done.

http://codereview.chromium.org/173556/diff/13001/14022
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/173556/diff/13001/14022#newcode2830
Line 2830: shared_memory.reset(shared_memory_arg);
On 2009/09/03 21:54:08, Matt Perry wrote:
> you can do this in one line:
>   scoped_ptr shared_mem(shared_memory_arg)

Done.

http://codereview.chromium.org/173556/diff/13001/14021
File chrome/renderer/user_script_slave.h (right):

http://codereview.chromium.org/173556/diff/13001/14021#newcode37
Line 37: static bool UnSerialize(
On 2009/09/03 21:54:08, Matt Perry wrote:
> nit: Deserialize

Done.

http://codereview.chromium.org/173556/diff/13001/14021#newcode39
Line 39: std::vector<UserScript*>* scripts, base::SharedMemory** shared_memory);
On 2009/09/03 21:54:08, Matt Perry wrote:
> maybe this should take a scoped_ptr<base::SharedMemory*>* to ensure that it
gets
> deleted.

Done.

[tangjie, 2009-09-08 02:11:38]

Hi Aaron and Matt,
  Could you please help review this patch in next round? Thanks!

Jerry

On Sat, Sep 5, 2009 at 1:57 AM, <tangjie@google.com> wrote:

>
> http://codereview.chromium.org/173556/diff/13001/14028
> File chrome/browser/extensions/extension_browsertests_misc.cc (right):
>
> http://codereview.chromium.org/173556/diff/13001/14028#newcode130
> Line 130:
> AppendASCII("Extensions").AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj").
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> We're starting to overload the extensions under good/ a little too
>>
> much. Can you
>
>> move the testing code out into its own extension under
>> chrome/test/data/extensions/uitest/ instead?
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14001
> File chrome/browser/extensions/extension_tabs_module.cc (right):
>
> http://codereview.chromium.org/173556/diff/13001/14001#newcode841
> Line 841: // The script should be ran after page is loaded.
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> nit: should be run
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14001#newcode848
> Line 848: DictionaryValue* script_files;
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> nit: script_files isn't really the right term, since it can include js
>>
> and css
>
>> code directly. script_info maybe
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14001#newcode862
> Line 862: UserScript::LoadScriptContent(&script_file);
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> this runs on the UI thread. we can't do blocking file IO on the UI
>>
> thread.
>
>> you'll need to move this off to the file thread and execute the
>>
> scripts
>
>> asynchronously.
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14002
> File chrome/browser/extensions/extension_tabs_module.h (right):
>
> http://codereview.chromium.org/173556/diff/13001/14002#newcode99
> Line 99: private:
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> move this to the top, while you're at it
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14002#newcode111
> Line 111: private:
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> move this to the top of the class
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14009
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
> http://codereview.chromium.org/173556/diff/13001/14009#newcode848
> Line 848: if (base::SharedMemory::IsHandleValid(handle_for_process)) {
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> return false if it's invalid?
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14006
> File chrome/common/extensions/api/extension_api.json (right):
>
> http://codereview.chromium.org/173556/diff/13001/14006#newcode555
> Line 555: "css": {"type": "array", "items": {"type": "string"},
> "minItems": 1, "optional": true, "description": "array of CSS files to
> execute,in order."},
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> nit: space after comma
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14006#newcode556
> Line 556: "css_code": {"type": "array", "items": {"type": "string"},
> "minItems": 1, "optional": true, "description": "array of CSS text to
> execute,in order."},
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> nit: space after comma
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14006#newcode557
> Line 557: "js": {"type": "array", "items": {"type": "string"},
> "minItems": 1, "optional": true, "description": "array of JavaScript
> files to execute,in order."},
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> nit: space after comma
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14006#newcode558
> Line 558: "js_code": {"type": "array", "items": {"type": "string"},
> "minItems": 1, "optional": true, "description": "array of JavaScript
> source code to execute,in order."}
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> nit: space after comma
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14006#newcode574
> Line 574: "description": "Whether this call is successfull"
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> "successful."
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14014
> File chrome/common/extensions/user_script.h (right):
>
> http://codereview.chromium.org/173556/diff/13001/14014#newcode34
> Line 34: DOCUMENT_RUN_SCRIPT,
> On 2009/09/03 18:52:05, Matt Perry wrote:
>
>> please comment this
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14017
> File chrome/common/render_messages_internal.h (right):
>
> http://codereview.chromium.org/173556/diff/13001/14017#newcode230
> Line 230: bool /* whether the script is ran successfully */)
> On 2009/09/03 21:54:08, Matt Perry wrote:
>
>> remove "is"
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14022
> File chrome/renderer/render_view.cc (right):
>
> http://codereview.chromium.org/173556/diff/13001/14022#newcode2830
> Line 2830: shared_memory.reset(shared_memory_arg);
> On 2009/09/03 21:54:08, Matt Perry wrote:
>
>> you can do this in one line:
>>   scoped_ptr shared_mem(shared_memory_arg)
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14021
> File chrome/renderer/user_script_slave.h (right):
>
> http://codereview.chromium.org/173556/diff/13001/14021#newcode37
> Line 37: static bool UnSerialize(
> On 2009/09/03 21:54:08, Matt Perry wrote:
>
>> nit: Deserialize
>>
>
> Done.
>
> http://codereview.chromium.org/173556/diff/13001/14021#newcode39
> Line 39: std::vector<UserScript*>* scripts, base::SharedMemory**
> shared_memory);
> On 2009/09/03 21:54:08, Matt Perry wrote:
>
>> maybe this should take a scoped_ptr<base::SharedMemory*>* to ensure
>>
> that it gets
>
>> deleted.
>>
>
> Done.
>
>
> http://codereview.chromium.org/173556
>

[Aaron Boodman, 2009-09-08 08:21:47]

http://codereview.chromium.org/173556/diff/19001/10003
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/19001/10003#newcode835
Line 835: &error_)) {
Nit: indent "&error" under "execute_tab_id_".

http://codereview.chromium.org/173556/diff/19001/10003#newcode843
Line 843: if (contents->is_loading()) {
It would be better to just wait until the page has completed loading. You can
see examples of this with chrome.tabs.getLanguage().

http://codereview.chromium.org/173556/diff/19001/10003#newcode855
Line 855: if (!extension->LoadUserScriptHelper(script_info, 0, false, &error,
I do not think it is a good idea to try and reuse the UserScript infrastructure
for this feature. Even though there are clearly some similarities to what you
are trying to do, there are also a lot of differences (no need for match
patterns, the desire to handle raw script source in addition to script files).

I think it would be cleaner to handle this case separately. Just parse the
arguments manually and add some new IPC messages to send the script data to the
renderer. On the renderer side, you can just call the same methods that
UserScriptSlave calls on WebFrame to do the injection.

http://codereview.chromium.org/173556/diff/19001/10008
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/173556/diff/19001/10008#newcode561
Line 561: "name": "executeScript",
I prefer the signatures specified here:
http://dev.chromium.org/developers/design-documents/extensions/executecontent...

Specifically, I would like executeScript() and insertCSS() to be separate
methods because the noun 'script' doesn't make sense if it can also be CSS.

http://codereview.chromium.org/173556/diff/19001/10008#newcode584
Line 584: {"name": "success", "type": "boolean", "description": "If the scripts
are executed successfully, it's true; otherwise it's false."}
We indicate errors via chrome.extension.lastError, so there is no need for the
success param.

In the future, the param should be the value returned from eval() (the value of
the last statement), but I do not think that executeInIsolatedWorld() returns
this now. So for now, just have onSuccess take no parameters.

[tangjie, 2009-09-08 09:51:38]

http://codereview.chromium.org/173556/diff/19001/10003
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/19001/10003#newcode855
Line 855: if (!extension->LoadUserScriptHelper(script_info, 0, false, &error,
Why I try to reuse existing UserScript is when I write code for this feature, I
find there are lots of duplications between them, like how to parse input JSON
data, load script contents from disk, data transferring between renderhost and
renderer, and we don't need invent another data structure to hold script data.
Yes, there are two things UserScript can't cover or conflict with this
feature:match patterns and handle raw script source. But for the match patterns,
I introduce one flag to distinguish load-extension and run-script, if we are
running script, don't need match patterns but load raw source code.
In renderer side, also using this flag to skip matching patterns in
UserScriptSlave, since there are also lots of duplications between them like
deserialize script to run and setup chrome.self, I decide to reuse it.
I think the change is not big, just split some functions, write one new helper
function and introduce one new flag.
Any thoughts? Thanks!
On 2009/09/08 08:21:47, Aaron Boodman wrote:
> I do not think it is a good idea to try and reuse the UserScript
infrastructure
> for this feature. Even though there are clearly some similarities to what you
> are trying to do, there are also a lot of differences (no need for match
> patterns, the desire to handle raw script source in addition to script files).
> 
> I think it would be cleaner to handle this case separately. Just parse the
> arguments manually and add some new IPC messages to send the script data to
the
> renderer. On the renderer side, you can just call the same methods that
> UserScriptSlave calls on WebFrame to do the injection.

[Aaron Boodman, 2009-09-08 10:35:29]

On Tue, Sep 8, 2009 at 2:51 AM, <tangjie@google.com> wrote:
>
> http://codereview.chromium.org/173556/diff/19001/10003
> File chrome/browser/extensions/extension_tabs_module.cc (right):
>
> http://codereview.chromium.org/173556/diff/19001/10003#newcode855
> Line 855: if (!extension->LoadUserScriptHelper(script_info, 0, false,
> &error,
> Why I try to reuse existing UserScript is when I write code for this
> feature, I find there are lots of duplications between them, like how to
> parse input JSON data, load script contents from disk, data transferring
> between renderhost and renderer, and we don't need invent another data
> structure to hold script data.

Reusing the code to parse the JSON data is not correct: the parameters
have different names and structures.

> Yes, there are two things UserScript can't cover or conflict with this
> feature:match patterns and handle raw script source. But for the match
> patterns, I introduce one flag to distinguish load-extension and
> run-script, if we are running script, don't need match patterns but load
> raw source code.
> In renderer side, also using this flag to skip matching patterns in
> UserScriptSlave, since there are also lots of duplications between them
> like deserialize script to run and setup chrome.self, I decide to reuse
> it.

I am OK reusing the code in UserScriptSlave that sets up chrome.self.
But I don't think that the UserScript class, or any of the supporting
infrastructure above it in RenderView or in the browser process should
be reused.

- a

[tangjie, 2009-09-10 17:32:23]

http://codereview.chromium.org/173556/diff/19001/10003
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/19001/10003#newcode835
Line 835: &error_)) {
On 2009/09/08 08:21:47, Aaron Boodman wrote:
> Nit: indent "&error" under "execute_tab_id_".

Done.

http://codereview.chromium.org/173556/diff/19001/10003#newcode843
Line 843: if (contents->is_loading()) {
On 2009/09/08 08:21:47, Aaron Boodman wrote:
> It would be better to just wait until the page has completed loading. You can
> see examples of this with chrome.tabs.getLanguage().

Done.

http://codereview.chromium.org/173556/diff/19001/10003#newcode855
Line 855: if (!extension->LoadUserScriptHelper(script_info, 0, false, &error,
On 2009/09/08 08:21:47, Aaron Boodman wrote:
> I do not think it is a good idea to try and reuse the UserScript
infrastructure
> for this feature. Even though there are clearly some similarities to what you
> are trying to do, there are also a lot of differences (no need for match
> patterns, the desire to handle raw script source in addition to script files).
> 
> I think it would be cleaner to handle this case separately. Just parse the
> arguments manually and add some new IPC messages to send the script data to
the
> renderer. On the renderer side, you can just call the same methods that
> UserScriptSlave calls on WebFrame to do the injection.

Done.

http://codereview.chromium.org/173556/diff/19001/10008
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/173556/diff/19001/10008#newcode561
Line 561: "name": "executeScript",
On 2009/09/08 08:21:47, Aaron Boodman wrote:
> I prefer the signatures specified here:
>
http://dev.chromium.org/developers/design-documents/extensions/executecontent...
> 
> Specifically, I would like executeScript() and insertCSS() to be separate
> methods because the noun 'script' doesn't make sense if it can also be CSS.

Done.

http://codereview.chromium.org/173556/diff/19001/10008#newcode584
Line 584: {"name": "success", "type": "boolean", "description": "If the scripts
are executed successfully, it's true; otherwise it's false."}
On 2009/09/08 08:21:47, Aaron Boodman wrote:
> We indicate errors via chrome.extension.lastError, so there is no need for the
> success param.
> 
> In the future, the param should be the value returned from eval() (the value
of
> the last statement), but I do not think that executeInIsolatedWorld() returns
> this now. So for now, just have onSuccess take no parameters.

Done.

[Aaron Boodman, 2009-09-11 00:29:28]

Some high level comments:

=================================================

After seeing the implementation, I'd like to change this API a bit, to:

void executeScript(int tabId, {optional string source, optional string path},
                   callback(bool success))
void insertCSS(int tabId, {optional string source, optional string path},
               callback(bool success))

Basically, you can only specify a single chunk of source code or file at a time.
This should simplify both the implementation and usage of the API.

===

We should not attempt to stop this function from being called multiple times.

Any of these functions should be able to be called multiple times on the same
tab, either from the same or different extensions.

Therefore to send any response to the caller, you need to send
ExtensionFunction's request_id() member all the way through into the renderer,
then send it back. Then, each ExtensionFunction should compare responses to its
own request_id() to know whether it is a success or failure.

=================================================

http://codereview.chromium.org/173556/diff/11143/11144
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/11143/11144#newcode840
Line 840: bool ExecuteFunction::RunImpl() {
General note: we will need to do a permissions check on this function. I have a
code review pending that should make this easy. See:
http://codereview.chromium.org/199074 for how this will work.

http://codereview.chromium.org/173556/diff/11143/11144#newcode854
Line 854: if (!tab_value->IsType(Value::TYPE_NULL)) {
It would simplify the logic in here to have something like this:

if (tab_value->IsType(Value::TYPE_NULL)) {
  browser = dispatcher()->GetBrowser();
  // Implement a new helper, GetDefaultTab().
  GetDefaultTab(browser, &execute_tab_id_, &tab_contents_, ...);
} else {
  EXTENSION_FUNCTION_VALIDATE(tab_value->GetAsInteger(&execute_tab_id_));
}

The GetDefaultTab() that is specifically for getting the default tab information
would be better than overloading the existing GetTabById() function's behavior.
It also allows you to get rid of the call later to
ExtensionTabUtil::GetTabId(contents).

Finally, it doesn't seem to be like any of this information is actually used
here except for tab id, and that is only used to insert into the pending set,
which I think we should remove (see other comment).

If that is true, I think this logic can be removed and consolidated with the
existing call to GetTabByID() in ::Execute().

http://codereview.chromium.org/173556/diff/11143/11144#newcode889
Line 889: Extension* extension = service->GetExtensionById(extension_id());
Note: my code review adds a GetExtension() function to ExtnesionFunction base
class.

http://codereview.chromium.org/173556/diff/11143/11144#newcode908
Line 908: ui_loop_ = NULL;
You should initialize the ui_loop_ to NULL in the constructor. That will avoid
the need to initialize it explicitly here.

http://codereview.chromium.org/173556/diff/11143/11144#newcode913
Line 913: AddRef();  // balanced in Observe() and ExecuteScripts()
I don't think there's any need to AddRef()/Release() explicitly here because the
PostTask() infrastructure does it as well. Try it out. I think the other
ExtensionFunctions that doe this could be changed as well (you don't need to
though).

http://codereview.chromium.org/173556/diff/11143/11144#newcode919
Line 919: bool ret = true;
A better variable name here would be "success".

http://codereview.chromium.org/173556/diff/11143/11144#newcode920
Line 920: for (size_t k = 0; k < path_vector_size; ++k) {
Name loop counters starting with "i".

http://codereview.chromium.org/173556/diff/11143/11144#newcode939
Line 939: if (load_successfully) {
If the explicit AddRef()/Release() is not needed, you can reverse this and
return early here.

http://codereview.chromium.org/173556/diff/11143/11144#newcode946
Line 946: StartExecute(contents, code_vector_);
You will still need an explicit AddRef() here though, while you wait for the
success/fail notification.

http://codereview.chromium.org/173556/diff/11143/11145
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/173556/diff/11143/11145#newcode113
Line 113: class ExecuteFunction : public AsyncExtensionFunction,
Since these functions are more complex than the others in here, I think it is
worth pulling them out into their own file.

http://codereview.chromium.org/173556/diff/11143/11145#newcode125
Line 125: // Derived classes should overwrite this function to specify the
Separate each of these method declarations with a newline please.

http://codereview.chromium.org/173556/diff/11143/11145#newcode144
Line 144: // Id of tab which executes code.
Separate each of these member declarations with a newline.

http://codereview.chromium.org/173556/diff/11143/11146
File chrome/browser/extensions/extension_tabs_module_constants.cc (right):

http://codereview.chromium.org/173556/diff/11143/11146#newcode66
Line 66: const char kNoCodeOrFileToExecute[] = "There is no code or file to
execute!";
Change this to:

No source code or file specified.

(no exclamation point)

[tangjie, 2009-09-14 13:27:54]

http://codereview.chromium.org/173556/diff/11143/11144
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/11143/11144#newcode840
Line 840: bool ExecuteFunction::RunImpl() {
I changed my code to call CanAccessHost to check permissions, but there is
problem when I write unittest,the testing page's url begins with "file://", is
disallowed, so unittest would fail because of this.
My solution could be:
1. Start test server to host the testing page.
2. allow extension to access "file://" to add it in manifest file, currently we
only support http:// and https://
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> General note: we will need to do a permissions check on this function. I have
a
> code review pending that should make this easy. See:
> http://codereview.chromium.org/199074 for how this will work.

http://codereview.chromium.org/173556/diff/11143/11144#newcode854
Line 854: if (!tab_value->IsType(Value::TYPE_NULL)) {
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> It would simplify the logic in here to have something like this:
> 
> if (tab_value->IsType(Value::TYPE_NULL)) {
>   browser = dispatcher()->GetBrowser();
>   // Implement a new helper, GetDefaultTab().
>   GetDefaultTab(browser, &execute_tab_id_, &tab_contents_, ...);
> } else {
>   EXTENSION_FUNCTION_VALIDATE(tab_value->GetAsInteger(&execute_tab_id_));
> }
> 
> The GetDefaultTab() that is specifically for getting the default tab
information
> would be better than overloading the existing GetTabById() function's
behavior.
> It also allows you to get rid of the call later to
> ExtensionTabUtil::GetTabId(contents).
> 
> Finally, it doesn't seem to be like any of this information is actually used
> here except for tab id, and that is only used to insert into the pending set,
> which I think we should remove (see other comment).
> 
> If that is true, I think this logic can be removed and consolidated with the
> existing call to GetTabByID() in ::Execute().

Done.

http://codereview.chromium.org/173556/diff/11143/11144#newcode889
Line 889: Extension* extension = service->GetExtensionById(extension_id());
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> Note: my code review adds a GetExtension() function to ExtnesionFunction base
> class.

Done.

http://codereview.chromium.org/173556/diff/11143/11144#newcode908
Line 908: ui_loop_ = NULL;
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> You should initialize the ui_loop_ to NULL in the constructor. That will avoid
> the need to initialize it explicitly here.

Done.

http://codereview.chromium.org/173556/diff/11143/11144#newcode913
Line 913: AddRef();  // balanced in Observe() and ExecuteScripts()
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> I don't think there's any need to AddRef()/Release() explicitly here because
the
> PostTask() infrastructure does it as well. Try it out. I think the other
> ExtensionFunctions that doe this could be changed as well (you don't need to
> though).

Done.

http://codereview.chromium.org/173556/diff/11143/11144#newcode919
Line 919: bool ret = true;
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> A better variable name here would be "success".

Done.

http://codereview.chromium.org/173556/diff/11143/11144#newcode920
Line 920: for (size_t k = 0; k < path_vector_size; ++k) {
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> Name loop counters starting with "i".

Done.

http://codereview.chromium.org/173556/diff/11143/11144#newcode939
Line 939: if (load_successfully) {
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> If the explicit AddRef()/Release() is not needed, you can reverse this and
> return early here.

Done.

http://codereview.chromium.org/173556/diff/11143/11144#newcode946
Line 946: StartExecute(contents, code_vector_);
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> You will still need an explicit AddRef() here though, while you wait for the
> success/fail notification.

Done.

http://codereview.chromium.org/173556/diff/11143/11145
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/173556/diff/11143/11145#newcode113
Line 113: class ExecuteFunction : public AsyncExtensionFunction,
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> Since these functions are more complex than the others in here, I think it is
> worth pulling them out into their own file.

Done.

http://codereview.chromium.org/173556/diff/11143/11145#newcode125
Line 125: // Derived classes should overwrite this function to specify the
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> Separate each of these method declarations with a newline please.

Done.

http://codereview.chromium.org/173556/diff/11143/11145#newcode144
Line 144: // Id of tab which executes code.
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> Separate each of these member declarations with a newline.

Done.

http://codereview.chromium.org/173556/diff/11143/11146
File chrome/browser/extensions/extension_tabs_module_constants.cc (right):

http://codereview.chromium.org/173556/diff/11143/11146#newcode66
Line 66: const char kNoCodeOrFileToExecute[] = "There is no code or file to
execute!";
On 2009/09/11 00:29:28, Aaron Boodman wrote:
> Change this to:
> 
> No source code or file specified.
> 
> (no exclamation point)

Done.

[Matt Perry, 2009-09-14 19:11:38]

http://codereview.chromium.org/173556/diff/11143/11160
File chrome/browser/extensions/extension_browsertests_misc.cc (right):

http://codereview.chromium.org/173556/diff/11143/11160#newcode135
Line 135: // hosting toolstrip1.html.
nit: toolstrip.html

http://codereview.chromium.org/173556/diff/13048/13074
File chrome/browser/extensions/extension_execute_function.cc (right):

http://codereview.chromium.org/173556/diff/13048/13074#newcode91
Line 91: void ExecuteFunction::LoadFiles() {
nit: should be LoadFile now

http://codereview.chromium.org/173556/diff/13048/13074#newcode108
Line 108: void ExecuteFunction::Execute(bool load_successfully) {
I think this would be better as two functions: ReportLoadError and Execute.

http://codereview.chromium.org/173556/diff/13048/13075
File chrome/browser/extensions/extension_execute_function.h (right):

http://codereview.chromium.org/173556/diff/13048/13075#newcode35
Line 35: // Derived classes should overwrite this function to specify the
nit: "override"

http://codereview.chromium.org/173556/diff/13048/13075#newcode71
Line 71: std::vector<std::string> code_string_vector_;
style nit: putting the type in the name seems redundant to me. code_strings_
would be a better name.

http://codereview.chromium.org/173556/diff/13048/13075#newcode71
Line 71: std::vector<std::string> code_string_vector_;
On second thought, is it possible to have multiple code strings now? I thought
the API was supposed to take a single argument, which was either a string of JS,
or a file to execute. If that's the case, this can just be a single string.

http://codereview.chromium.org/173556/diff/13048/13051
File chrome/browser/extensions/extension_tabs_module_constants.cc (right):

http://codereview.chromium.org/173556/diff/13048/13051#newcode68
Line 68: const char kNoCodeOrFileToExecute[] = "No source code or file
specified.";
add suffix "Error" for consistency

http://codereview.chromium.org/173556/diff/13048/13060
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/173556/diff/13048/13060#newcode216
Line 216: TAB_SCRIPT_EXECUTED,
Looks like these are no longer used.

http://codereview.chromium.org/173556/diff/13048/13059
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/173556/diff/13048/13059#newcode251
Line 251: IPC_MESSAGE_ROUTED2(ViewMsg_ExecuteScriptsFinished,
Looks like these are no longer used.

http://codereview.chromium.org/173556/diff/13048/13064
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/173556/diff/13048/13064#newcode542
Line 542: while (!pending_css_execution_queue_.empty()) {
This seems like the wrong place to be doing this. You want to wait until we're
done loading to execute, right? A better place would be in
didCreateDocumentElement perhaps (where user scripts with "at_start" are
injected).

http://codereview.chromium.org/173556/diff/13048/13062
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/173556/diff/13048/13062#newcode916
Line 916: struct ScriptExecutionInfo : public
base::RefCounted<ScriptExecutionInfo> {
Please add comments to these classes to explain why they are needed.

http://codereview.chromium.org/173556/diff/13048/13071
File chrome/test/data/extensions/uitest/executescript_api_call/manifest.json
(right):

http://codereview.chromium.org/173556/diff/13048/13071#newcode2
Line 2: "key":
"MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDuUZGKCDbff6IRaxa4Pue7PPkxwPaNhGT3JEqppEsNWFjM80imEdqMbf3lrWqEfaHgaNku7nlpwPO1mu3/4Hr+XdNa5MhfnOnuPee4hyTLwOs3Vzz81wpbdzUxZSi2OmqMyI5oTaBYICfNHLwcuc65N5dbt6WKGeKgTpp4v7j7zwIDAQAB",
FYI, you don't actually need the key for extensions that are loaded via
LoadExtension.

[Aaron Boodman, 2009-09-15 05:07:27]

Getting closer! Thanks for your patience!

http://codereview.chromium.org/173556/diff/13048/13074
File chrome/browser/extensions/extension_execute_function.cc (right):

http://codereview.chromium.org/173556/diff/13048/13074#newcode35
Line 35: if (!browser ||
nit: don't fall back to the default if (!browser). Instead return the error
kNoCurrentWindowError (see other instances of this in extension_tabs_module.cc).

http://codereview.chromium.org/173556/diff/13048/13074#newcode45
Line 45: if (!browser || !contents)
Nit: this cannot be true here, right? If so, assert.

http://codereview.chromium.org/173556/diff/13048/13074#newcode48
Line 48: /*if (!GetExtension()->CanAccessHost(contents->GetURL())) {
You can test this by starting a server and using the host 127.0.0.1:1337. See
extension_javascript_url_unittest.cc for an example.

http://codereview.chromium.org/173556/diff/13048/13075
File chrome/browser/extensions/extension_execute_function.h (right):

http://codereview.chromium.org/173556/diff/13048/13075#newcode23
Line 23: class ExecuteFunction : public AsyncExtensionFunction,
Class needs comments.

http://codereview.chromium.org/173556/diff/13048/13075#newcode26
Line 26: ExecuteFunction() : execute_tab_id_(-1), ui_loop_(NULL) {}
Idea: you can get rid of this base class and the two separate functions:

- combine the two functions into one, and just have it have a "type" property
(css or js). You can figure this out by looking at the value of name() from the
base class.

Then send the type in the IPC message and have the renderer decide whether to
call insertCSS or executeJS as the last moment.

Then this class could change to ExecuteCodeInTabFunction.

http://codereview.chromium.org/173556/diff/13048/13075#newcode71
Line 71: std::vector<std::string> code_string_vector_;
Let's just have the API require that only one or the other of the file and
source is valid. That simplifies things and this can just be a string.

http://codereview.chromium.org/173556/diff/13048/13075#newcode81
Line 81: return L"js_file";
Name the properties "code" and "file" in both cases. That way you can also get
rid of this virtual method.

http://codereview.chromium.org/173556/diff/13048/13049
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/13048/13049#newcode161
Line 161: if (contents && browser) {
DCHECK(contents);
DCHECK(browser);
DCHECK(tab_id);

[tangjie, 2009-09-15 18:16:34]

http://codereview.chromium.org/173556/diff/13048/13074
File chrome/browser/extensions/extension_execute_function.cc (right):

http://codereview.chromium.org/173556/diff/13048/13074#newcode35
Line 35: if (!browser ||
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> nit: don't fall back to the default if (!browser). Instead return the error
> kNoCurrentWindowError (see other instances of this in
extension_tabs_module.cc).

Done.

http://codereview.chromium.org/173556/diff/13048/13074#newcode45
Line 45: if (!browser || !contents)
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> Nit: this cannot be true here, right? If so, assert.

Done.

http://codereview.chromium.org/173556/diff/13048/13074#newcode48
Line 48: /*if (!GetExtension()->CanAccessHost(contents->GetURL())) {
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> You can test this by starting a server and using the host 127.0.0.1:1337. See
> extension_javascript_url_unittest.cc for an example.

Done.

http://codereview.chromium.org/173556/diff/13048/13074#newcode91
Line 91: void ExecuteFunction::LoadFiles() {
On 2009/09/14 19:11:39, Matt Perry wrote:
> nit: should be LoadFile now

Done.

http://codereview.chromium.org/173556/diff/13048/13074#newcode108
Line 108: void ExecuteFunction::Execute(bool load_successfully) {
On 2009/09/14 19:11:39, Matt Perry wrote:
> I think this would be better as two functions: ReportLoadError and Execute.

Done.

http://codereview.chromium.org/173556/diff/13048/13075
File chrome/browser/extensions/extension_execute_function.h (right):

http://codereview.chromium.org/173556/diff/13048/13075#newcode23
Line 23: class ExecuteFunction : public AsyncExtensionFunction,
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> Class needs comments.

Done.

http://codereview.chromium.org/173556/diff/13048/13075#newcode26
Line 26: ExecuteFunction() : execute_tab_id_(-1), ui_loop_(NULL) {}
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> Idea: you can get rid of this base class and the two separate functions:
> 
> - combine the two functions into one, and just have it have a "type" property
> (css or js). You can figure this out by looking at the value of name() from
the
> base class.
> 
> Then send the type in the IPC message and have the renderer decide whether to
> call insertCSS or executeJS as the last moment.
> 
> Then this class could change to ExecuteCodeInTabFunction.

Good suggestion, Done.

http://codereview.chromium.org/173556/diff/13048/13075#newcode35
Line 35: // Derived classes should overwrite this function to specify the
On 2009/09/14 19:11:39, Matt Perry wrote:
> nit: "override"

Done.

http://codereview.chromium.org/173556/diff/13048/13075#newcode71
Line 71: std::vector<std::string> code_string_vector_;
On 2009/09/14 19:11:39, Matt Perry wrote:
> style nit: putting the type in the name seems redundant to me. code_strings_
> would be a better name.

Done.

http://codereview.chromium.org/173556/diff/13048/13075#newcode71
Line 71: std::vector<std::string> code_string_vector_;
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> Let's just have the API require that only one or the other of the file and
> source is valid. That simplifies things and this can just be a string.

Done.

http://codereview.chromium.org/173556/diff/13048/13075#newcode81
Line 81: return L"js_file";
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> Name the properties "code" and "file" in both cases. That way you can also get
> rid of this virtual method.

Done.

http://codereview.chromium.org/173556/diff/13048/13049
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/13048/13049#newcode161
Line 161: if (contents && browser) {
On 2009/09/15 05:07:27, Aaron Boodman wrote:
> DCHECK(contents);
> DCHECK(browser);
> DCHECK(tab_id);

Done.

http://codereview.chromium.org/173556/diff/13048/13051
File chrome/browser/extensions/extension_tabs_module_constants.cc (right):

http://codereview.chromium.org/173556/diff/13048/13051#newcode68
Line 68: const char kNoCodeOrFileToExecute[] = "No source code or file
specified.";
On 2009/09/14 19:11:39, Matt Perry wrote:
> add suffix "Error" for consistency

Done.

http://codereview.chromium.org/173556/diff/13048/13060
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/173556/diff/13048/13060#newcode216
Line 216: TAB_SCRIPT_EXECUTED,
Now I send them to Renderer to make renderer distinguish css and js. 
On 2009/09/14 19:11:39, Matt Perry wrote:
> Looks like these are no longer used.

http://codereview.chromium.org/173556/diff/13048/13059
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/173556/diff/13048/13059#newcode251
Line 251: IPC_MESSAGE_ROUTED2(ViewMsg_ExecuteScriptsFinished,
I changed it to ViewMsg_ExecuteCodeFinished to notify RenderViewHost execution
is finished
On 2009/09/14 19:11:39, Matt Perry wrote:
> Looks like these are no longer used.

http://codereview.chromium.org/173556/diff/13048/13064
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/173556/diff/13048/13064#newcode542
Line 542: while (!pending_css_execution_queue_.empty()) {
On 2009/09/14 19:11:39, Matt Perry wrote:
> This seems like the wrong place to be doing this. You want to wait until we're
> done loading to execute, right? A better place would be in
> didCreateDocumentElement perhaps (where user scripts with "at_start" are
> injected).

Done.

http://codereview.chromium.org/173556/diff/13048/13062
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/173556/diff/13048/13062#newcode916
Line 916: struct ScriptExecutionInfo : public
base::RefCounted<ScriptExecutionInfo> {
On 2009/09/14 19:11:39, Matt Perry wrote:
> Please add comments to these classes to explain why they are needed.

Done.

http://codereview.chromium.org/173556/diff/13048/13071
File chrome/test/data/extensions/uitest/executescript_api_call/manifest.json
(right):

http://codereview.chromium.org/173556/diff/13048/13071#newcode2
Line 2: "key":
"MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDuUZGKCDbff6IRaxa4Pue7PPkxwPaNhGT3JEqppEsNWFjM80imEdqMbf3lrWqEfaHgaNku7nlpwPO1mu3/4Hr+XdNa5MhfnOnuPee4hyTLwOs3Vzz81wpbdzUxZSi2OmqMyI5oTaBYICfNHLwcuc65N5dbt6WKGeKgTpp4v7j7zwIDAQAB",
On 2009/09/14 19:11:39, Matt Perry wrote:
> FYI, you don't actually need the key for extensions that are loaded via
> LoadExtension.

Done.

[Aaron Boodman, 2009-09-15 19:17:26]

http://codereview.chromium.org/173556/diff/18073/24097
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/173556/diff/18073/24097#newcode45
Line 45: if (!tab_value)
Nit: you don't need to check this here: EXTENSION_FUNCTION_VALIDATE includes a
return false.

http://codereview.chromium.org/173556/diff/18073/24097#newcode63
Line 63: DCHECK(browser && contents);
Nit: we usually DCHECK each things separately, so that we can easily see which
assumption failed.

http://codereview.chromium.org/173556/diff/18073/24097#newcode65
Line 65: if (!GetExtension()->CanAccessHost(contents->GetURL())) {
Also make sure to add a test test that this security check is working.

http://codereview.chromium.org/173556/diff/18073/24097#newcode71
Line 71: std::vector<std::wstring> script_content_vector;
This variable is unused.

http://codereview.chromium.org/173556/diff/18073/24097#newcode72
Line 72: std::wstring code_content;
There is also DictionaryValue::GetString(const wchar_t*, std::string*), so that
you don't need to declare this as wstring and convert to string.

http://codereview.chromium.org/173556/diff/18073/24097#newcode84
Line 84: std::wstring relative_path;
Same comment here with wstring. In general, we try to prefer utf8 strings, so
double check when you see yourself using wstring that there isn't an overload
somewhere that would let you avoid it.

http://codereview.chromium.org/173556/diff/18073/24097#newcode104
Line 104: bool success = true;
Nit: I think it would read slightly cleaner to get rid of the success variable
and just move the ui_loop_->PostTask() calls into the if/else branch for loading
the fie.

http://codereview.chromium.org/173556/diff/18073/24097#newcode144
Line 144: Source<void>(reinterpret_cast<void*>(request_id())));
Interesting. This is a usage of notification registration I've not seen that
much in our code. Will let mpcomplete chime in on it.

It feels a bit odd to me to have a request_id() be a source, but I guess it
works.

http://codereview.chromium.org/173556/diff/18073/24096
File chrome/browser/extensions/execute_code_in_tab_function.h (right):

http://codereview.chromium.org/173556/diff/18073/24096#newcode18
Line 18: // Implement API call tabs.executeScript and tabs.injectCSS.
injectCSS -> insertCSS

http://codereview.chromium.org/173556/diff/18073/24088
File chrome/browser/extensions/extension_browsertests_misc.cc (right):

http://codereview.chromium.org/173556/diff/18073/24088#newcode130
Line 130: #if defined(OS_WIN)  // TODO(port) -- enable
You should implement this test using our api test framework instead.

See, as an example, browser/extensions/extension_javascript_url_apitest.cc

Make sure to test the case where an extension calls executeScript and/or
insertCSS several times in a row.

http://codereview.chromium.org/173556/diff/18073/24072
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/18073/24072#newcode164
Line 164: if (contents && browser) {
What I meant here is that once you add the DCHECK, you can remove the if
(contents && browser) check here, and the if (tab_id) check below.

In general, we don't usually both DHCECK something |and| test it then return
false. If it is DHECK-worthy, it means that we really think this doesn't happen
in real life.

http://codereview.chromium.org/173556/diff/18073/24083
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/173556/diff/18073/24083#newcode216
Line 216: TAB_SCRIPT_EXECUTED,
You only need one notification. You don't need to send the 'type' of execution
because each request is uniquely identified by its request_id.

[Matt Perry, 2009-09-15 19:38:09]

http://codereview.chromium.org/173556/diff/18073/24097
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/173556/diff/18073/24097#newcode144
Line 144: Source<void>(reinterpret_cast<void*>(request_id())));
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> Interesting. This is a usage of notification registration I've not seen that
> much in our code. Will let mpcomplete chime in on it.
> 
> It feels a bit odd to me to have a request_id() be a source, but I guess it
> works.

The problem is that it's not 64-bit safe. I think what you have to do instead is
listen to AllSources, and put the request_id into the Details object so that you
can check for the right notification in your Observe function. Something like
Details< std::pair<int, bool> >.

[Matt Perry, 2009-09-15 21:07:32]

Almost there.

http://codereview.chromium.org/173556/diff/18073/24097
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/173556/diff/18073/24097#newcode146
Line 146: contents->ExecuteCode(request_id(), extension_id(),
notification_type_,
it feels wrong to use notification_type to indicate whether you want to execute
script or css. render_view shouldn't have to know that a notification is
involved. how about just a boolean?

http://codereview.chromium.org/173556/diff/18073/24098
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/173556/diff/18073/24098#newcode2130
Line 2130: struct SimilarTypeTraits<NotificationType::Type> {
please remove this after changing the IPC message to not pass a
NotificationType. It doesn't really make sense to pass a NotificationType
between processes.

[tangjie, 2009-09-16 15:17:34]

http://codereview.chromium.org/173556/diff/18073/24097
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/173556/diff/18073/24097#newcode45
Line 45: if (!tab_value)
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> Nit: you don't need to check this here: EXTENSION_FUNCTION_VALIDATE includes a
> return false.

Done.

http://codereview.chromium.org/173556/diff/18073/24097#newcode63
Line 63: DCHECK(browser && contents);
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> Nit: we usually DCHECK each things separately, so that we can easily see which
> assumption failed.

Done.

http://codereview.chromium.org/173556/diff/18073/24097#newcode65
Line 65: if (!GetExtension()->CanAccessHost(contents->GetURL())) {
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> Also make sure to add a test test that this security check is working.

Done.

http://codereview.chromium.org/173556/diff/18073/24097#newcode71
Line 71: std::vector<std::wstring> script_content_vector;
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> This variable is unused.

Done.

http://codereview.chromium.org/173556/diff/18073/24097#newcode72
Line 72: std::wstring code_content;
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> There is also DictionaryValue::GetString(const wchar_t*, std::string*), so
that
> you don't need to declare this as wstring and convert to string.

Done.

http://codereview.chromium.org/173556/diff/18073/24097#newcode84
Line 84: std::wstring relative_path;
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> Same comment here with wstring. In general, we try to prefer utf8 strings, so
> double check when you see yourself using wstring that there isn't an overload
> somewhere that would let you avoid it.

Done.

http://codereview.chromium.org/173556/diff/18073/24097#newcode104
Line 104: bool success = true;
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> Nit: I think it would read slightly cleaner to get rid of the success variable
> and just move the ui_loop_->PostTask() calls into the if/else branch for
loading
> the fie.

Done.

http://codereview.chromium.org/173556/diff/18073/24097#newcode144
Line 144: Source<void>(reinterpret_cast<void*>(request_id())));
Yes, I realize that there is risk that int value may conflict with another
registered object's pointer value.
On 2009/09/15 19:38:09, Matt Perry wrote:
> On 2009/09/15 19:17:27, Aaron Boodman wrote:
> > Interesting. This is a usage of notification registration I've not seen that
> > much in our code. Will let mpcomplete chime in on it.
> > 
> > It feels a bit odd to me to have a request_id() be a source, but I guess it
> > works.
> 
> The problem is that it's not 64-bit safe. I think what you have to do instead
is
> listen to AllSources, and put the request_id into the Details object so that
you
> can check for the right notification in your Observe function. Something like
> Details< std::pair<int, bool> >.

http://codereview.chromium.org/173556/diff/18073/24097#newcode146
Line 146: contents->ExecuteCode(request_id(), extension_id(),
notification_type_,
On 2009/09/15 21:07:32, Matt Perry wrote:
> it feels wrong to use notification_type to indicate whether you want to
execute
> script or css. render_view shouldn't have to know that a notification is
> involved. how about just a boolean?

Done.

http://codereview.chromium.org/173556/diff/18073/24096
File chrome/browser/extensions/execute_code_in_tab_function.h (right):

http://codereview.chromium.org/173556/diff/18073/24096#newcode18
Line 18: // Implement API call tabs.executeScript and tabs.injectCSS.
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> injectCSS -> insertCSS

Done.

http://codereview.chromium.org/173556/diff/18073/24088
File chrome/browser/extensions/extension_browsertests_misc.cc (right):

http://codereview.chromium.org/173556/diff/18073/24088#newcode130
Line 130: #if defined(OS_WIN)  // TODO(port) -- enable
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> You should implement this test using our api test framework instead.
> 
> See, as an example, browser/extensions/extension_javascript_url_apitest.cc
> 
> Make sure to test the case where an extension calls executeScript and/or
> insertCSS several times in a row.

Done.

http://codereview.chromium.org/173556/diff/18073/24072
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/173556/diff/18073/24072#newcode164
Line 164: if (contents && browser) {
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> What I meant here is that once you add the DCHECK, you can remove the if
> (contents && browser) check here, and the if (tab_id) check below.
> 
> In general, we don't usually both DHCECK something |and| test it then return
> false. If it is DHECK-worthy, it means that we really think this doesn't
happen
> in real life.

Done.

http://codereview.chromium.org/173556/diff/18073/24083
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/173556/diff/18073/24083#newcode216
Line 216: TAB_SCRIPT_EXECUTED,
On 2009/09/15 19:17:27, Aaron Boodman wrote:
> You only need one notification. You don't need to send the 'type' of execution
> because each request is uniquely identified by its request_id.

Done.

http://codereview.chromium.org/173556/diff/18073/24098
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/173556/diff/18073/24098#newcode2130
Line 2130: struct SimilarTypeTraits<NotificationType::Type> {
On 2009/09/15 21:07:32, Matt Perry wrote:
> please remove this after changing the IPC message to not pass a
> NotificationType. It doesn't really make sense to pass a NotificationType
> between processes.

Done.

[Aaron Boodman, 2009-09-16 18:43:34]

LGTM with some last nits.

*phew*!

http://codereview.chromium.org/173556/diff/20087/20106
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/173556/diff/20087/20106#newcode103
Line 103: LOG(WARNING) << "Failed to load user script file: "
This message should be put in error_ instead of logged out, so that developers
receive it. (see comment about SendResponse(false) below).

http://codereview.chromium.org/173556/diff/20087/20106#newcode117
Line 117: result_.reset(Value::CreateBooleanValue(false));
The success callback shouldn't take a boolean param, it should take no params.
Failure should be indicated by using our built in failure mechanism that all
extension functions share.

So I think that ReportError() can go, and you can change the code in LoadFile()
to do ui_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
&ExecuteCodeInTabFunction::SendResponse, false));

Then remove all the code that sets the result_ member.

http://codereview.chromium.org/173556/diff/20087/20106#newcode124
Line 124: if (ExtensionTabUtil::GetTabById(execute_tab_id_, profile(), &browser,
NULL,
Reverse this check and return early.

http://codereview.chromium.org/173556/diff/20087/20110
File chrome/test/data/extensions/api_test/executescript/test.html (right):

http://codereview.chromium.org/173556/diff/20087/20110#newcode7
Line 7: var relativePath =
'/files/extensions/api_test/executescript/test_executescript.html';
80 cols

[Matt Perry, 2009-09-16 20:26:06]

LGTM!

[tangjie, 2009-09-17 06:45:19]

http://codereview.chromium.org/173556/diff/20087/20106
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/173556/diff/20087/20106#newcode103
Line 103: LOG(WARNING) << "Failed to load user script file: "
On 2009/09/16 18:43:34, Aaron Boodman wrote:
> This message should be put in error_ instead of logged out, so that developers
> receive it. (see comment about SendResponse(false) below).

Done.

http://codereview.chromium.org/173556/diff/20087/20106#newcode117
Line 117: result_.reset(Value::CreateBooleanValue(false));
On 2009/09/16 18:43:34, Aaron Boodman wrote:
> The success callback shouldn't take a boolean param, it should take no params.
> Failure should be indicated by using our built in failure mechanism that all
> extension functions share.
> 
> So I think that ReportError() can go, and you can change the code in
LoadFile()
> to do ui_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
> &ExecuteCodeInTabFunction::SendResponse, false));
> 
> Then remove all the code that sets the result_ member.

Done.

http://codereview.chromium.org/173556/diff/20087/20106#newcode124
Line 124: if (ExtensionTabUtil::GetTabById(execute_tab_id_, profile(), &browser,
NULL,
On 2009/09/16 18:43:34, Aaron Boodman wrote:
> Reverse this check and return early.

Done.

http://codereview.chromium.org/173556/diff/20087/20110
File chrome/test/data/extensions/api_test/executescript/test.html (right):

http://codereview.chromium.org/173556/diff/20087/20110#newcode7
Line 7: var relativePath =
'/files/extensions/api_test/executescript/test_executescript.html';
On 2009/09/16 18:43:34, Aaron Boodman wrote:
> 80 cols

Done.