Make executeScript and insertCSS inject code into all frames.

Make executeScript and insertCSS inject code into all frames.

[tangjie1, 2009-11-17 02:18:12]

Hi aa,

  Please help review this patch.

Thanks!

Jerry

[darin (slow to review), 2009-11-17 07:31:27]

some minor drive-by style nits:

http://codereview.chromium.org/401007/diff/3001/47
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/401007/diff/3001/47#newcode2994
Line 2994: bool RenderView::GetChildrenFrames(WebFrame* parent_frame,
formatting nit:

void Foo(A a,
         B b)

or

void Foo(
  A a,
  B b)

http://codereview.chromium.org/401007/diff/3001/47#newcode2995
Line 2995: std::vector<WebFrame* >* frames_vector) const {
nit: no space before ">"

http://codereview.chromium.org/401007/diff/3001/47#newcode2998
Line 2998: for (WebKit::WebFrame* child_frame = parent_frame->firstChild();
child_frame;
nit: no "WebKit::" prefix here

http://codereview.chromium.org/401007/diff/3001/47#newcode3815
Line 3815: std::vector<WebFrame* > frame_vector;
nit: no space before ">"

http://codereview.chromium.org/401007/diff/3001/47#newcode3821
Line 3821: for (std::vector<WebFrame* >::iterator frame_it =
frame_vector.begin();
ditto

[tangjie1, 2009-11-17 19:49:54]

http://codereview.chromium.org/401007/diff/3001/47
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/401007/diff/3001/47#newcode2994
Line 2994: bool RenderView::GetChildrenFrames(WebFrame* parent_frame,
On 2009/11/17 07:31:28, darin wrote:
> formatting nit:
> 
> void Foo(A a,
>          B b)
> 
> or
> 
> void Foo(
>   A a,
>   B b)

Done.

http://codereview.chromium.org/401007/diff/3001/47#newcode2995
Line 2995: std::vector<WebFrame* >* frames_vector) const {
On 2009/11/17 07:31:28, darin wrote:
> nit: no space before ">"

Done.

http://codereview.chromium.org/401007/diff/3001/47#newcode2998
Line 2998: for (WebKit::WebFrame* child_frame = parent_frame->firstChild();
child_frame;
On 2009/11/17 07:31:28, darin wrote:
> nit: no "WebKit::" prefix here

Done.

http://codereview.chromium.org/401007/diff/3001/47#newcode3815
Line 3815: std::vector<WebFrame* > frame_vector;
On 2009/11/17 07:31:28, darin wrote:
> nit: no space before ">"

Done.

http://codereview.chromium.org/401007/diff/3001/47#newcode3821
Line 3821: for (std::vector<WebFrame* >::iterator frame_it =
frame_vector.begin();
On 2009/11/17 07:31:28, darin wrote:
> ditto

Done.

[Aaron Boodman, 2009-11-18 00:31:56]

http://codereview.chromium.org/401007/diff/1010/2009
File chrome/browser/extensions/execute_code_in_tab_function.h (right):

http://codereview.chromium.org/401007/diff/1010/2009#newcode51
Line 51: bool inject_to_all_frames_;
Suggest naming this member just "all_frames_".

http://codereview.chromium.org/401007/diff/1010/2013
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/401007/diff/1010/2013#newcode368
Line 368: bool inject_into_all_frames);
Again, this param can just be "all_frames".

http://codereview.chromium.org/401007/diff/1010/2014
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/401007/diff/1010/2014#newcode727
Line 727: {"type": "boolean", "name": "allFrames", "optional": true,
"description": "If allFrames is true, this function injects scripts into all
frames of current page. By default, it's false and only inject scripts into top
main frame."},
This property should be a member of the scriptDef object, and the scriptDef
object's name should be changed to "details".

http://codereview.chromium.org/401007/diff/1010/2017
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/401007/diff/1010/2017#newcode3818
Line 3818: if (inject_into_all_frames) {
Don't need braces for one line if statements.

http://codereview.chromium.org/401007/diff/1010/2016
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/401007/diff/1010/2016#newcode614
Line 614: bool inject_into_all_frames);
all_frames again.

http://codereview.chromium.org/401007/diff/1010/2016#newcode683
Line 683: // Get all children frames of parent_frame, returned by frames_vector.
Missing newline between these two methods.

http://codereview.chromium.org/401007/diff/1010/2016#newcode684
Line 684: bool GetChildrenFrames(WebKit::WebFrame* parent_frame,
Rename this method GetAllChildFrames().

http://codereview.chromium.org/401007/diff/1010/2019
File chrome/test/data/extensions/api_test/executescript_in_frame/script.js
(right):

http://codereview.chromium.org/401007/diff/1010/2019#newcode3
Line 3: name = name.replace(/([A-Z])/g, '-$1');
You don't need this part that munges the name since you can just feed it the
name in the right format to start.

http://codereview.chromium.org/401007/diff/1010/2021
File chrome/test/data/extensions/api_test/executescript_in_frame/test.html
(right):

http://codereview.chromium.org/401007/diff/1010/2021#newcode18
Line 18: script_file.code = "var extensionPort = chrome.extension.connect();
extensionPort.postMessage({message: document.title});";
line length.

[tangjie1, 2009-11-19 18:39:14]

http://codereview.chromium.org/401007/diff/1010/2009
File chrome/browser/extensions/execute_code_in_tab_function.h (right):

http://codereview.chromium.org/401007/diff/1010/2009#newcode51
Line 51: bool inject_to_all_frames_;
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> Suggest naming this member just "all_frames_".

Done.

http://codereview.chromium.org/401007/diff/1010/2013
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/401007/diff/1010/2013#newcode368
Line 368: bool inject_into_all_frames);
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> Again, this param can just be "all_frames".

Done.

http://codereview.chromium.org/401007/diff/1010/2014
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/401007/diff/1010/2014#newcode727
Line 727: {"type": "boolean", "name": "allFrames", "optional": true,
"description": "If allFrames is true, this function injects scripts into all
frames of current page. By default, it's false and only inject scripts into top
main frame."},
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> This property should be a member of the scriptDef object, and the scriptDef
> object's name should be changed to "details".

Done.

http://codereview.chromium.org/401007/diff/1010/2017
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/401007/diff/1010/2017#newcode3818
Line 3818: if (inject_into_all_frames) {
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> Don't need braces for one line if statements.

Done.

http://codereview.chromium.org/401007/diff/1010/2016
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/401007/diff/1010/2016#newcode614
Line 614: bool inject_into_all_frames);
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> all_frames again.

Done.

http://codereview.chromium.org/401007/diff/1010/2016#newcode683
Line 683: // Get all children frames of parent_frame, returned by frames_vector.
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> Missing newline between these two methods.

Done.

http://codereview.chromium.org/401007/diff/1010/2016#newcode684
Line 684: bool GetChildrenFrames(WebKit::WebFrame* parent_frame,
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> Rename this method GetAllChildFrames().

Done.

http://codereview.chromium.org/401007/diff/1010/2019
File chrome/test/data/extensions/api_test/executescript_in_frame/script.js
(right):

http://codereview.chromium.org/401007/diff/1010/2019#newcode3
Line 3: name = name.replace(/([A-Z])/g, '-$1');
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> You don't need this part that munges the name since you can just feed it the
> name in the right format to start.

Done.

http://codereview.chromium.org/401007/diff/1010/2021
File chrome/test/data/extensions/api_test/executescript_in_frame/test.html
(right):

http://codereview.chromium.org/401007/diff/1010/2021#newcode18
Line 18: script_file.code = "var extensionPort = chrome.extension.connect();
extensionPort.postMessage({message: document.title});";
On 2009/11/18 00:31:56, Aaron Boodman wrote:
> line length.

Done.

[Aaron Boodman, 2009-11-19 21:16:19]

lgtm

http://codereview.chromium.org/401007/diff/11001/11008
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/401007/diff/11001/11008#newcode721
Line 721: "description": "Note: code or file can't be specified at the same
time.",
Make the description:

Details of the script to run. Either the "code" or "file" property must be set,
but both may not be set at the same time.