Add API: getBackgroundPageView and getToolstripViews

Add API: executeScript

[Erik does not do reviews, 2009-07-22 19:37:01]

http://codereview.chromium.org/159067/diff/1167/126
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/159067/diff/1167/126#newcode331
Line 331: ViewType::Type view_type = ViewType::TAB_CONTENTS;
why have this default?  this should never be used.

http://codereview.chromium.org/159067/diff/1167/126#newcode339
Line 339: }
else NOTREACHED().

http://codereview.chromium.org/159067/diff/1167/131
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/159067/diff/1167/131#newcode1004
Line 1004: struct ParamTraits<ViewType::Type> {
This is unfortunate complexity.  Could you please add a comment referencing bug
16866 so that others understand why this is here?

http://codereview.chromium.org/159067/diff/1167/137
File chrome/renderer/extensions/event_bindings.h (right):

http://codereview.chromium.org/159067/diff/1167/137#newcode34
Line 34: // Handle the browser window which the context is attached to is
changed.
newline above comment.  
The wording and method names don't seem to match.  Should it just be
HandleWindowIdChanged?

http://codereview.chromium.org/159067/diff/1167/138
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/159067/diff/1167/138#newcode133
Line 133: bool return_all_views = (args.Length() == 1 &&
args[0]->IsUndefined());
looks like args.Length() is required to be 1.  maybe just return early at the
top if not rather than testing multiple places.

http://codereview.chromium.org/159067/diff/1167/138#newcode134
Line 134: // If there is one parameter specifies browser window id, return
toolstrip
newline above comment

http://codereview.chromium.org/159067/diff/1167/138#newcode150
Line 150: v8::Local<v8::Array> views = v8::Array::New(contexts.size());
I would skip pre-initializing the size.  With multiple windows, this will always
be too big, and the common case will be 1 anyway, so you don't need to really
worry about the perf of the array growing.

http://codereview.chromium.org/159067/diff/1167/138#newcode156
Line 156: (*it)->browser_window_id != browser_window_id)
use {} when you have a multiline conditional

[tangjie, 2009-07-23 12:13:23]

http://codereview.chromium.org/159067/diff/1167/126
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/159067/diff/1167/126#newcode331
Line 331: ViewType::Type view_type = ViewType::TAB_CONTENTS;
On 2009/07/22 19:37:01, Erik Kay wrote:
> why have this default?  this should never be used.

Done.

http://codereview.chromium.org/159067/diff/1167/126#newcode339
Line 339: }
On 2009/07/22 19:37:01, Erik Kay wrote:
> else NOTREACHED().

Done.

http://codereview.chromium.org/159067/diff/1167/131
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/159067/diff/1167/131#newcode1004
Line 1004: struct ParamTraits<ViewType::Type> {
I did something to try to resolve this bug, please have a look at.
On 2009/07/22 19:37:01, Erik Kay wrote:
> This is unfortunate complexity.  Could you please add a comment referencing
bug
> 16866 so that others understand why this is here?

http://codereview.chromium.org/159067/diff/1167/137
File chrome/renderer/extensions/event_bindings.h (right):

http://codereview.chromium.org/159067/diff/1167/137#newcode34
Line 34: // Handle the browser window which the context is attached to is
changed.
On 2009/07/22 19:37:01, Erik Kay wrote:
> newline above comment.  
> The wording and method names don't seem to match.  Should it just be
> HandleWindowIdChanged?

Done.

http://codereview.chromium.org/159067/diff/1167/138
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/159067/diff/1167/138#newcode133
Line 133: bool return_all_views = (args.Length() == 1 &&
args[0]->IsUndefined());
On 2009/07/22 19:37:01, Erik Kay wrote:
> looks like args.Length() is required to be 1.  maybe just return early at the
> top if not rather than testing multiple places.

Done.

http://codereview.chromium.org/159067/diff/1167/138#newcode134
Line 134: // If there is one parameter specifies browser window id, return
toolstrip
On 2009/07/22 19:37:01, Erik Kay wrote:
> newline above comment

Done.

http://codereview.chromium.org/159067/diff/1167/138#newcode150
Line 150: v8::Local<v8::Array> views = v8::Array::New(contexts.size());
On 2009/07/22 19:37:01, Erik Kay wrote:
> I would skip pre-initializing the size.  With multiple windows, this will
always
> be too big, and the common case will be 1 anyway, so you don't need to really
> worry about the perf of the array growing.

Done.

http://codereview.chromium.org/159067/diff/1167/138#newcode156
Line 156: (*it)->browser_window_id != browser_window_id)
On 2009/07/22 19:37:01, Erik Kay wrote:
> use {} when you have a multiline conditional

Done.

[Erik does not do reviews, 2009-07-23 16:34:26]

It's a little unfortunate that the IPC change is combined with this CL.  It
might be better for it to be split into its own review.  There are better people
to review this change like darin, brettw, mpcomplete or jam.  Could you please
loop one of them in for the IPC changes?

http://codereview.chromium.org/159067/diff/2067/415
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/159067/diff/2067/415#newcode340
Line 340: NOTREACHED();
hmm.  I guess in non-debug code, this can fall through to below and view_type is
uninitialized.  The three obvious fixes would be to just make this a CHECK and
crash always, to set a view type in this else branch or to just return.  Looking
at the code, this really shouldn't be able to happen, so I guess I don't have a
really strong opinion.

http://codereview.chromium.org/159067/diff/2067/430
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/159067/diff/2067/430#newcode740
Line 740: // This tab may be attached to another browser window, we should
notify
None of your new code is depending on the window id from a TabContents that
you're passing down, so this won't break anything at the moment, but technically
when you get a TabDetachedAt message below, the tab is no longer associated with
any window id, so maybe we need to pass down -1?  Maybe look at what the tab API
for extensions does in this case (you could ask rafaelw).

http://codereview.chromium.org/159067/diff/2067/421
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/159067/diff/2067/421#newcode810
Line 810: ViewType::Type view_type_;
Add a comment explaining the purpose of this.

http://codereview.chromium.org/159067/diff/2067/428
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/159067/diff/2067/428#newcode229
Line 229: chrome.self.getToolstripViews = function(window_id) {
newline above this

http://codereview.chromium.org/159067/diff/2067/428#newcode232
Line 232: chrome.self.getBackgroundPageView = function() {
newline above this

[Aaron Boodman, 2009-07-23 19:35:33]

I don't like the structure of this change in several ways. This is how I think
it should work:

- In chrome/common/extensions, define ExtensionViewType enum that contains
INVALID, TOOLSTRIP, TAB, and BACKGROUND.
  - It is important to do this in common/ so that renderer and browser can share
these enums

- Modify ExtensionFunctionDispatcher to take ExtensionViewType and a window_id
as arguments
  - It is important to do this in EFD so that the code for sending the IPC can
be shared between tabs and ExtensionHost
  - In EFD's constructor, send an IPC message telling the renderview its
ExtensionViewType and windowId

- Modify RenderView to store extension_view_type_ and extension_window_id_ and
to have getters for them (return INVALID and -1 for extension_view() and
extension_window_id() for non-extension render views).

- Change the current implementation of chrome.extension.getViews() to iterate
all running RenderViews in the process, and then do
render_view->GetMainFrame()->GetContext(). This returns the v8 context, which
you can use to get the global.

- Add implementations for getToolstripViews, getTabViews that do the same thing
as getViews(), but filter the returned views using
render_view->extension_window_id() and render_view->extension_view_type().

Thoughts?

[tangjie, 2009-07-26 16:42:33]

Some inline thoughts...

On Fri, Jul 24, 2009 at 3:35 AM, <aa@chromium.org> wrote:

> I don't like the structure of this change in several ways. This is how I
> think it should work:
>
> - In chrome/common/extensions, define ExtensionViewType enum that
> contains INVALID, TOOLSTRIP, TAB, and BACKGROUND.
>  - It is important to do this in common/ so that renderer and browser
> can share these enums

Yes, I already put an enum in common I just feel that it's used in
RenderView, in renderer side, all views share the same class RenderView, so
should we make it specific for extension only?

>
>
> - Modify ExtensionFunctionDispatcher to take ExtensionViewType and a
> window_id as arguments
>  - It is important to do this in EFD so that the code for sending the
> IPC can be shared between tabs and ExtensionHost
>  - In EFD's constructor, send an IPC message telling the renderview its
> ExtensionViewType and windowId

 The same reason, I feel since other parts also use this logic, so I think
we should put it in RenderViewHost to share with others.

>
>
> - Modify RenderView to store extension_view_type_ and
> extension_window_id_ and to have getters for them (return INVALID and -1
> for extension_view() and extension_window_id() for non-extension render
> views).

 The names (extension_view_type_ and extension_window_id_) seems too
specific, I think maybe others may also use these fields.

>
>
> - Change the current implementation of chrome.extension.getViews() to
> iterate all running RenderViews in the process, and then do
> render_view->GetMainFrame()->GetContext(). This returns the v8 context,
> which you can use to get the global.
>
> - Add implementations for getToolstripViews, getTabViews that do the
> same thing as getViews(), but filter the returned views using
> render_view->extension_window_id() and
> render_view->extension_view_type().

So should we also remove the ContextInfo and ContextList logic, since if we
do so, they are not needed.

>
>
> Thoughts?
>
>
> http://codereview.chromium.org/159067
>

[Aaron Boodman, 2009-08-03 07:35:20]

I know you didn't send this out for comment yet, but I happened to see that you
updated it, so I took a look.

It is looking a lot better to me.

* I would like Matt to take a look at the changes to the IPC infrastructure. I
don't know enough about it to comment.

* Is it possible to make RenderViewHostDelegate::window_id() and view_type()
abstract so that implementors are forced to think about what they should return?

http://codereview.chromium.org/159067/diff/744/2308
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/159067/diff/744/2308#newcode113
Line 113: (*it)->webview()->GetMainFrame()->GetMainWorldScriptContext();
You need to check the URL of the top frame to make sure its host has the same ID
as ExtensionIdFromCurrentContext(). There can be multiple extensions loaded into
a single render process.

http://codereview.chromium.org/159067/diff/744/2308#newcode117
Line 117: v8::String::New("window"));
Getting the window is not necessary. context->Global() is the same object as
context->Global()->Get("window").

http://codereview.chromium.org/159067/diff/744/2303
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/159067/diff/744/2303#newcode850
Line 850: static base::hash_set<RenderView* > render_view_set_;
We aren't allowed to declare static complex objects. You need to make this
hashset use the singleton pattern to create it lazily. See event_bindings.cc for
an example of this.

[tangjie, 2009-08-03 07:44:08]

Yes, I am just waiting for try server's reply. Thanks for your comments.
Jerry

On Mon, Aug 3, 2009 at 3:35 PM, <aa@chromium.org> wrote:

> I know you didn't send this out for comment yet, but I happened to see
> that you updated it, so I took a look.
>
> It is looking a lot better to me.
>
> * I would like Matt to take a look at the changes to the IPC
> infrastructure. I don't know enough about it to comment.
>
> * Is it possible to make RenderViewHostDelegate::window_id() and
> view_type() abstract so that implementors are forced to think about what
> they should return?
>
>
> http://codereview.chromium.org/159067/diff/744/2308
> File chrome/renderer/extensions/extension_process_bindings.cc (right):
>
> http://codereview.chromium.org/159067/diff/744/2308#newcode113
> Line 113: (*it)->webview()->GetMainFrame()->GetMainWorldScriptContext();
> You need to check the URL of the top frame to make sure its host has the
> same ID as ExtensionIdFromCurrentContext(). There can be multiple
> extensions loaded into a single render process.
>
> http://codereview.chromium.org/159067/diff/744/2308#newcode117
> Line 117: v8::String::New("window"));
> Getting the window is not necessary. context->Global() is the same
> object as context->Global()->Get("window").
>
> http://codereview.chromium.org/159067/diff/744/2303
> File chrome/renderer/render_view.h (right):
>
> http://codereview.chromium.org/159067/diff/744/2303#newcode850
> Line 850: static base::hash_set<RenderView* > render_view_set_;
> We aren't allowed to declare static complex objects. You need to make
> this hashset use the singleton pattern to create it lazily. See
> event_bindings.cc for an example of this.
>
>
> http://codereview.chromium.org/159067
>

[tangjie, 2009-08-03 16:29:21]

http://codereview.chromium.org/159067/diff/2067/415
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/159067/diff/2067/415#newcode340
Line 340: NOTREACHED();
On 2009/07/23 16:34:27, Erik Kay wrote:
> hmm.  I guess in non-debug code, this can fall through to below and view_type
is
> uninitialized.  The three obvious fixes would be to just make this a CHECK and
> crash always, to set a view type in this else branch or to just return. 
Looking
> at the code, this really shouldn't be able to happen, so I guess I don't have
a
> really strong opinion.

Done.

http://codereview.chromium.org/159067/diff/2067/430
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/159067/diff/2067/430#newcode740
Line 740: // This tab may be attached to another browser window, we should
notify
Yes, without this, we will not break anything.I just want to make the logic
complete. Event we pass down -1 for window id when we get TabDetachedAt, but we
still need pass down another window id since the tab is immediately inserted
into another window(get TabInsertedAt).
On 2009/07/23 16:34:27, Erik Kay wrote:
> None of your new code is depending on the window id from a TabContents that
> you're passing down, so this won't break anything at the moment, but
technically
> when you get a TabDetachedAt message below, the tab is no longer associated
with
> any window id, so maybe we need to pass down -1?  Maybe look at what the tab
API
> for extensions does in this case (you could ask rafaelw).

http://codereview.chromium.org/159067/diff/2067/421
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/159067/diff/2067/421#newcode810
Line 810: ViewType::Type view_type_;
On 2009/07/23 16:34:27, Erik Kay wrote:
> Add a comment explaining the purpose of this.

Done.

http://codereview.chromium.org/159067/diff/2067/428
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/159067/diff/2067/428#newcode229
Line 229: chrome.self.getToolstripViews = function(window_id) {
On 2009/07/23 16:34:27, Erik Kay wrote:
> newline above this

Done.

http://codereview.chromium.org/159067/diff/2067/428#newcode232
Line 232: chrome.self.getBackgroundPageView = function() {
On 2009/07/23 16:34:27, Erik Kay wrote:
> newline above this

Done.

http://codereview.chromium.org/159067/diff/744/2308
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/159067/diff/744/2308#newcode113
Line 113: (*it)->webview()->GetMainFrame()->GetMainWorldScriptContext();
On 2009/08/03 07:35:20, Aaron Boodman wrote:
> You need to check the URL of the top frame to make sure its host has the same
ID
> as ExtensionIdFromCurrentContext(). There can be multiple extensions loaded
into
> a single render process.

Done.

http://codereview.chromium.org/159067/diff/744/2308#newcode117
Line 117: v8::String::New("window"));
On 2009/08/03 07:35:20, Aaron Boodman wrote:
> Getting the window is not necessary. context->Global() is the same object as
> context->Global()->Get("window").

Done.

http://codereview.chromium.org/159067/diff/744/2303
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/159067/diff/744/2303#newcode850
Line 850: static base::hash_set<RenderView* > render_view_set_;
On 2009/08/03 07:35:20, Aaron Boodman wrote:
> We aren't allowed to declare static complex objects. You need to make this
> hashset use the singleton pattern to create it lazily. See event_bindings.cc
for
> an example of this.

Done.

[Aaron Boodman, 2009-08-03 17:49:51]

A couple more structural issues:

- NotifyRenderViewInfo() is a very general name. I suggest splitting the
ViewType and WindowId concepts apart. Since ViewType cannot currently change
after it is first established, there is only a need for a
RenderViewHost::UpdateViewType() method.

- RenderViewHost should directly send the initial view type and window id into
RenderView just before RenderViewHostDelegate::RenderViewCreated() is called.
That way, implementors of RVHD don't have to remember to call the base version
of RenderViewCreated().

- It would be nice to also store the view type and window id on RenderViewHost
and expose getters for them. I'm sure this will come in handy.

http://codereview.chromium.org/159067/diff/983/2482
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/159067/diff/983/2482#newcode37
Line 37: enum ExtensionHostType {
Nit: I'd just reuse RenderViewType here. Only some values would be valid, but
that seems ok.

http://codereview.chromium.org/159067/diff/983/2482#newcode160
Line 160: ExtensionHostType extension_host_type_;
Nit: try to always think of a useful comment for the state members. I know it
can be difficult when they seem self-explanatory.

For example, if you change this to re-use RenderViewType, you can change the
comment to say "(not all view types are hosted by ExtensionHost).

http://codereview.chromium.org/159067/diff/983/2508
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/159067/diff/983/2508#newcode377
Line 377: virtual int GetWindowID() const = 0;
Add comments explaining what these are and what the implementor is expected to
return.

http://codereview.chromium.org/159067/diff/983/2510
File chrome/browser/tab_contents/interstitial_page.h (right):

http://codereview.chromium.org/159067/diff/983/2510#newcode91
Line 91: return -1;
Interstitials are attached to a window. I am not sure if it is easy to get to,
though. It is OK to leave it returning -1 if it is hard to get to the window id.

http://codereview.chromium.org/159067/diff/983/2492
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/159067/diff/983/2492#newcode107
Line 107: Singleton<RenderView::RenderViewSet>()->render_view_set_.size());
Since this singleton is used in multiple places in this method, pull it out into
a pointer.

http://codereview.chromium.org/159067/diff/983/2492#newcode133
Line 133: DCHECK(Singleton<RenderView::RenderViewSet>()->render_view_set_.size()
> 0);
Same thing here with storing a pointer to the singleton.

http://codereview.chromium.org/159067/diff/983/2492#newcode141
Line 141: if ((*it)->view_type() == ViewType::EXTENSION_BACKGROUND_PAGE) {
Nit: Prefer early exits to reduce indentation. So in this case, reverse the
comparison with EXTENSION_BACKGROUND_PAGE and continue.

http://codereview.chromium.org/159067/diff/983/2492#newcode161
Line 161: if (args.Length() != 1)
What about combining all the different methods that get views into a single
method like:

GetExtensionViews(window_id, view_type)

Then the JavaScript for each of the getViews(), getBackgroundPage(),
getToolstrip(), in extension_process_bindings.js could call the same helper
function. In the case of getBackgroundPage(), it would just return the first
element of the array, if any.

http://codereview.chromium.org/159067/diff/983/2494
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/159067/diff/983/2494#newcode223
Line 223: chrome.self.getToolstripViews = function(window_id) {
Looks like these are supposed to be like this now:

apiFunctions["self.getToolstripViews"]

http://codereview.chromium.org/159067/diff/983/2494#newcode227
Line 227: chrome.self.getBackgroundPageView = function() {
Naming nit:
Let's make these:
getToolstrips()
getBackgroundPage()

The "view" noun is unnecessary typing.

[Matt Perry, 2009-08-03 18:11:56]

http://codereview.chromium.org/159067/diff/744/2317
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/159067/diff/744/2317#newcode2034
Line 2034: struct ParamTraits<ViewType::Type> {
Ah, good to see you're trying to solve this problem.  But you can do it without
changing every ParamTraits in existence :).  Just add a new traits class for the
enums you're interested in.  Take a look at my comment on the relevant bug:
http://code.google.com/p/chromium/issues/detail?id=16866

[tangjie, 2009-08-04 17:10:24]

Yes, I have read your comments on that bug. If I have not misunderstood you,
I think using your solution, we have to add a couple of lines of source code
to do template specification for each enums we're interested in. But I think
the advantage of my proposal is introducing less duplicated code:1. typedef
introduces nothing in binary code.
2. For any new enum, we only need define
template <>
struct ParamTraits<NewEnumType> {
  typedef ViewType::Type param_type;
  typedef int target_type;
};
also introduce nothing in binary code.

Yes, the disadvantage is we have to add typedef in each trait class, but
it's trivial, and also introduce nothing into binary code.

Thanks!

Jerry

On Tue, Aug 4, 2009 at 2:11 AM, <mpcomplete@chromium.org> wrote:

>
> http://codereview.chromium.org/159067/diff/744/2317
> File chrome/common/render_messages.h (right):
>
> http://codereview.chromium.org/159067/diff/744/2317#newcode2034
> Line 2034: struct ParamTraits<ViewType::Type> {
> Ah, good to see you're trying to solve this problem.  But you can do it
> without changing every ParamTraits in existence :).  Just add a new
> traits class for the enums you're interested in.  Take a look at my
> comment on the relevant bug:
> http://code.google.com/p/chromium/issues/detail?id=16866
>
>
> http://codereview.chromium.org/159067
>

[tangjie, 2009-08-06 08:31:18]

http://codereview.chromium.org/159067/diff/983/2482
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/159067/diff/983/2482#newcode37
Line 37: enum ExtensionHostType {
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Nit: I'd just reuse RenderViewType here. Only some values would be valid, but
> that seems ok.

Done.

http://codereview.chromium.org/159067/diff/983/2482#newcode160
Line 160: ExtensionHostType extension_host_type_;
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Nit: try to always think of a useful comment for the state members. I know it
> can be difficult when they seem self-explanatory.
> 
> For example, if you change this to re-use RenderViewType, you can change the
> comment to say "(not all view types are hosted by ExtensionHost).

Done.

http://codereview.chromium.org/159067/diff/983/2508
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/159067/diff/983/2508#newcode377
Line 377: virtual int GetWindowID() const = 0;
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Add comments explaining what these are and what the implementor is expected to
> return.

Done.

http://codereview.chromium.org/159067/diff/983/2510
File chrome/browser/tab_contents/interstitial_page.h (right):

http://codereview.chromium.org/159067/diff/983/2510#newcode91
Line 91: return -1;
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Interstitials are attached to a window. I am not sure if it is easy to get to,
> though. It is OK to leave it returning -1 if it is hard to get to the window
id.

Done.

http://codereview.chromium.org/159067/diff/983/2492
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/159067/diff/983/2492#newcode107
Line 107: Singleton<RenderView::RenderViewSet>()->render_view_set_.size());
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Since this singleton is used in multiple places in this method, pull it out
into
> a pointer.

Done.

http://codereview.chromium.org/159067/diff/983/2492#newcode133
Line 133: DCHECK(Singleton<RenderView::RenderViewSet>()->render_view_set_.size()
> 0);
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Same thing here with storing a pointer to the singleton.

Done.

http://codereview.chromium.org/159067/diff/983/2492#newcode141
Line 141: if ((*it)->view_type() == ViewType::EXTENSION_BACKGROUND_PAGE) {
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Nit: Prefer early exits to reduce indentation. So in this case, reverse the
> comparison with EXTENSION_BACKGROUND_PAGE and continue.

Done.

http://codereview.chromium.org/159067/diff/983/2492#newcode161
Line 161: if (args.Length() != 1)
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> What about combining all the different methods that get views into a single
> method like:
> 
> GetExtensionViews(window_id, view_type)
> 
> Then the JavaScript for each of the getViews(), getBackgroundPage(),
> getToolstrip(), in extension_process_bindings.js could call the same helper
> function. In the case of getBackgroundPage(), it would just return the first
> element of the array, if any.

Done.

http://codereview.chromium.org/159067/diff/983/2494
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/159067/diff/983/2494#newcode223
Line 223: chrome.self.getToolstripViews = function(window_id) {
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Looks like these are supposed to be like this now:
> 
> apiFunctions["self.getToolstripViews"]

Done.

http://codereview.chromium.org/159067/diff/983/2494#newcode227
Line 227: chrome.self.getBackgroundPageView = function() {
On 2009/08/03 17:49:52, Aaron Boodman wrote:
> Naming nit:
> Let's make these:
> getToolstrips()
> getBackgroundPage()
> 
> The "view" noun is unnecessary typing.

Done.

[tangjie, 2009-08-06 08:40:29]

On Tue, Aug 4, 2009 at 1:49 AM, <aa@chromium.org> wrote:

> A couple more structural issues:
>
> - NotifyRenderViewInfo() is a very general name. I suggest splitting the
> ViewType and WindowId concepts apart. Since ViewType cannot currently
> change after it is first established, there is only a need for a
> RenderViewHost::UpdateViewType() method.
>
> - RenderViewHost should directly send the initial view type and window
> id into RenderView just before
> RenderViewHostDelegate::RenderViewCreated() is called. That way,
> implementors of RVHD don't have to remember to call the base version of
> RenderViewCreated().
>
> - It would be nice to also store the view type and window id on
> RenderViewHost and expose getters for them. I'm sure this will come in
> handy.

Actually, we could get it like:
RenderViewHost::delegate()->GetBrowserWindowId()

>
>
>
> http://codereview.chromium.org/159067/diff/983/2482
> File chrome/browser/extensions/extension_host.h (right):
>
> http://codereview.chromium.org/159067/diff/983/2482#newcode37
> Line 37: enum ExtensionHostType {
> Nit: I'd just reuse RenderViewType here. Only some values would be
> valid, but that seems ok.
>
> http://codereview.chromium.org/159067/diff/983/2482#newcode160
> Line 160: ExtensionHostType extension_host_type_;
> Nit: try to always think of a useful comment for the state members. I
> know it can be difficult when they seem self-explanatory.
>
> For example, if you change this to re-use RenderViewType, you can change
> the comment to say "(not all view types are hosted by ExtensionHost).
>
> http://codereview.chromium.org/159067/diff/983/2508
> File chrome/browser/renderer_host/render_view_host_delegate.h (right):
>
> http://codereview.chromium.org/159067/diff/983/2508#newcode377
> Line 377: virtual int GetWindowID() const = 0;
> Add comments explaining what these are and what the implementor is
> expected to return.
>
> http://codereview.chromium.org/159067/diff/983/2510
> File chrome/browser/tab_contents/interstitial_page.h (right):
>
> http://codereview.chromium.org/159067/diff/983/2510#newcode91
> Line 91: return -1;
> Interstitials are attached to a window. I am not sure if it is easy to
> get to, though. It is OK to leave it returning -1 if it is hard to get
> to the window id.
>
> http://codereview.chromium.org/159067/diff/983/2492
> File chrome/renderer/extensions/extension_process_bindings.cc (right):
>
> http://codereview.chromium.org/159067/diff/983/2492#newcode107
> Line 107:
> Singleton<RenderView::RenderViewSet>()->render_view_set_.size());
> Since this singleton is used in multiple places in this method, pull it
> out into a pointer.
>
> http://codereview.chromium.org/159067/diff/983/2492#newcode133
> Line 133:
> DCHECK(Singleton<RenderView::RenderViewSet>()->render_view_set_.size() >
> 0);
> Same thing here with storing a pointer to the singleton.
>
> http://codereview.chromium.org/159067/diff/983/2492#newcode141
> Line 141: if ((*it)->view_type() == ViewType::EXTENSION_BACKGROUND_PAGE)
> {
> Nit: Prefer early exits to reduce indentation. So in this case, reverse
> the comparison with EXTENSION_BACKGROUND_PAGE and continue.
>
> http://codereview.chromium.org/159067/diff/983/2492#newcode161
> Line 161: if (args.Length() != 1)
> What about combining all the different methods that get views into a
> single method like:
>
> GetExtensionViews(window_id, view_type)
>
> Then the JavaScript for each of the getViews(), getBackgroundPage(),
> getToolstrip(), in extension_process_bindings.js could call the same
> helper function. In the case of getBackgroundPage(), it would just
> return the first element of the array, if any.
>
> http://codereview.chromium.org/159067/diff/983/2494
> File chrome/renderer/resources/extension_process_bindings.js (right):
>
> http://codereview.chromium.org/159067/diff/983/2494#newcode223
> Line 223: chrome.self.getToolstripViews = function(window_id) {
> Looks like these are supposed to be like this now:
>
> apiFunctions["self.getToolstripViews"]
>
> http://codereview.chromium.org/159067/diff/983/2494#newcode227
> Line 227: chrome.self.getBackgroundPageView = function() {
> Naming nit:
> Let's make these:
> getToolstrips()
> getBackgroundPage()
>
> The "view" noun is unnecessary typing.
>
>
> http://codereview.chromium.org/159067
>

[Aaron Boodman, 2009-08-06 09:11:24]

OK, the code looks good to me except two API nits below.

Can you please add some tests for the new APIs? You can look in
extension_browsertests_misc.cc for some examples of how to do it.

http://codereview.chromium.org/159067/diff/4126/5101
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/159067/diff/4126/5101#newcode232
Line 232: })();
These should be extension.getViews, extension.getBackgroundPage, etc.

Also, can you add an extension.getTabs()? It would be exactly the same as the
others except looking for type == TAB.

http://codereview.chromium.org/159067/diff/4126/5106
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/159067/diff/4126/5106#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
This file is unchanged.

[tangjie, 2009-08-06 16:30:29]

I will add tests soon. Just want to get your comments on the following issue.

http://codereview.chromium.org/159067/diff/4126/5101
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/159067/diff/4126/5101#newcode232
Line 232: })();
I have added getTabs here.
But for another comment, do you mean by that I should move these APIs into
renderer_extension_bindings.js? And we decide make them as unprivileged
javascript APIs? 
On 2009/08/06 09:11:24, Aaron Boodman wrote:
> These should be extension.getViews, extension.getBackgroundPage, etc.
> 
> Also, can you add an extension.getTabs()? It would be exactly the same as the
> others except looking for type == TAB.

[Aaron Boodman, 2009-08-06 17:28:47]

On Thu, Aug 6, 2009 at 9:30 AM, <tangjie@google.com> wrote:
> http://codereview.chromium.org/159067/diff/4126/5101
> File chrome/renderer/resources/extension_process_bindings.js (right):
>
> http://codereview.chromium.org/159067/diff/4126/5101#newcode232
> Line 232: })();
> I have added getTabs here.
> But for another comment, do you mean by that I should move these APIs
> into renderer_extension_bindings.js? And we decide make them as
> unprivileged javascript APIs?

No, the way you have it is perfect. chrome-extension://... pages can
run in three views:

a) background pages
b) toolstrips
c) tab contents

You can try c) by just typing into the URL bar
chrome-extension://<some extension id>/<path to some resource in
extension>.

- a

[Matt Perry, 2009-08-06 18:32:41]

IPC stuff LGTM

[tangjie, 2009-08-11 16:46:41]

Hi Aaron,
  I have added unittest for these APIs.
  One question is we already have tab API to get tabs, and since we only
store view type in separate process, so in extension process we can't access
tabcontents process, then we can't make getTabs in extension process. Is it
right? So I think getView can only return toolstrip views and background
page.

Jerry

On Fri, Aug 7, 2009 at 1:28 AM, Aaron Boodman <aa@chromium.org> wrote:

> On Thu, Aug 6, 2009 at 9:30 AM, <tangjie@google.com> wrote:
> > http://codereview.chromium.org/159067/diff/4126/5101
> > File chrome/renderer/resources/extension_process_bindings.js (right):
> >
> > http://codereview.chromium.org/159067/diff/4126/5101#newcode232
> > Line 232: })();
> > I have added getTabs here.
> > But for another comment, do you mean by that I should move these APIs
> > into renderer_extension_bindings.js? And we decide make them as
> > unprivileged javascript APIs?
>
> No, the way you have it is perfect. chrome-extension://... pages can
> run in three views:
>
> a) background pages
> b) toolstrips
> c) tab contents
>
> You can try c) by just typing into the URL bar
> chrome-extension://<some extension id>/<path to some resource in
> extension>.
>
> - a
>

[Aaron Boodman, 2009-08-11 22:07:39]

On 2009/08/11 16:46:41, tangjie wrote:
> Hi Aaron,
>   I have added unittest for these APIs.
>   One question is we already have tab API to get tabs, and since we only
> store view type in separate process, so in extension process we can't access
> tabcontents process, then we can't make getTabs in extension process. Is it
> right? So I think getView can only return toolstrip views and background
> page.

The chrome.tabs.* APIs are different. They return Tab objects that represent all
the tabs in the current browser.

The API I'm asking for chrome.extension.getTabs() would only return tabs that
are running in the current extension's process. And they wouldn't return
information about the tab, they'd return the global scope of the js running
inside the window.

Make sense?

Basically, if you copy the getToolstrips() method exactly, and rename everything
that says "toolstrip" to "tab", it will work.

LGTM after the addition of getTabs().

[Erik does not do reviews, 2009-08-12 01:25:10]

On Tue, Aug 11, 2009 at 3:07 PM, <aa@chromium.org> wrote:
> On 2009/08/11 16:46:41, tangjie wrote:
>>
>> Hi Aaron,
>> =A0 I have added unittest for these APIs.
>> =A0 One question is we already have tab API to get tabs, and since we
>
> only
>>
>> store view type in separate process, so in extension process we can't
>
> access
>>
>> tabcontents process, then we can't make getTabs in extension process.
>
> Is it
>>
>> right? So I think getView can only return toolstrip views and
>
> background
>>
>> page.
>
> The chrome.tabs.* APIs are different. They return Tab objects that
> represent all the tabs in the current browser.
>
> The API I'm asking for chrome.extension.getTabs() would only return tabs
> that are running in the current extension's process. And they wouldn't
> return information about the tab, they'd return the global scope of the
> js running inside the window.
>
> Make sense?
>
> Basically, if you copy the getToolstrips() method exactly, and rename
> everything that says "toolstrip" to "tab", it will work.
>
> LGTM after the addition of getTabs().
>
> http://codereview.chromium.org/159067
>

Drive-by comment:  since these contents could be in random windows
(window.open) and not just tabs, maybe getTabs() isn't the best name.
I don't have any great suggestions (getTabContents() or
getWebContents() ?), but maybe one of you do...

Erik

[tangjie, 2009-08-12 04:21:28]

OK, I see, we'd like return tabcontents running in extension process. I have
updated it. Thanks!
Jerry

On Wed, Aug 12, 2009 at 6:07 AM, <aa@chromium.org> wrote:

> On 2009/08/11 16:46:41, tangjie wrote:
>
>> Hi Aaron,
>>   I have added unittest for these APIs.
>>   One question is we already have tab API to get tabs, and since we
>>
> only
>
>> store view type in separate process, so in extension process we can't
>>
> access
>
>> tabcontents process, then we can't make getTabs in extension process.
>>
> Is it
>
>> right? So I think getView can only return toolstrip views and
>>
> background
>
>> page.
>>
>
> The chrome.tabs.* APIs are different. They return Tab objects that
> represent all the tabs in the current browser.
>
> The API I'm asking for chrome.extension.getTabs() would only return tabs
> that are running in the current extension's process. And they wouldn't
> return information about the tab, they'd return the global scope of the
> js running inside the window.
>
> Make sense?
>
> Basically, if you copy the getToolstrips() method exactly, and rename
> everything that says "toolstrip" to "tab", it will work.
>
> LGTM after the addition of getTabs().
>
>
> http://codereview.chromium.org/159067
>