createWindow api call.

Implement tabs.createWindow extension api call. Required: (a) new RVHDelegate & TabContentsDelegate method(s) CreateExtensionFunctionDispatcher() so that the dispatcher could be created with (an optional) browser attached to it, while avoiding having render_host depend on browser.h

BUG=11092
:
R=aa,mpComplete,darin,pkasting


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14710

[Aaron Boodman, 2009-04-20 04:23:19]

LGTM

Also, once you merge, consider using Erik's EXTENSION_FUNCTION_VALIDATE macro to
validate the input more tightly.

http://codereview.chromium.org/79070/diff/1/4
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/79070/diff/1/4#newcode43
Line 43: ExtensionFunctionDispatcher* dispatcher() { return dispatcher_; }
Looks like maybe this could be protected? Or just move dispatcher_ into
protected?

http://codereview.chromium.org/79070/diff/1/11
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/79070/diff/1/11#newcode112
Line 112: additionalProperties: false
This is no longer needed.

[darin (slow to review), 2009-04-20 07:51:13]

http://codereview.chromium.org/79070/diff/1/9
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/79070/diff/1/9#newcode23
Line 23: class Browser;
it is wrong for this file to depend on Browser.  nothing renderer_host should
depend on Browser.

http://codereview.chromium.org/79070/diff/1/9#newcode153
Line 153: virtual Browser *GetBrowser() { return NULL; }
please do not add this method.

[darin (slow to review), 2009-04-20 07:52:41]

if you want to talk to the Browser, you need to do so indirectly.  The RVH calls
methods on its delegate, which is usually implemented by the TabContents, and
that calls methods on its TabContentsDelegate, which is usually implemented by
the Browser.

[Aaron Boodman, 2009-04-20 17:45:17]

On 2009/04/20 07:52:41, darin wrote:
> if you want to talk to the Browser, you need to do so indirectly.  The RVH
calls
> methods on its delegate, which is usually implemented by the TabContents, and
> that calls methods on its TabContentsDelegate, which is usually implemented by
> the Browser.

Ok, so, what about either modifying the existing CreateWindow() to work for our
use case, or else renaming it to CreatePopupWindow() and creating a new
CreateWindow() that works for us.

We would need to pass in dimensions and initial URL and get back windowId (which
is a bit weird since it will only be used for extensions, but that's all I can
think of).

[Matt Perry, 2009-04-20 19:00:27]

http://codereview.chromium.org/79070/diff/1/6
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/79070/diff/1/6#newcode34
Line 34: RenderViewHostDelegate *GetRenderViewHostDelegate();
use: Type* name();

http://codereview.chromium.org/79070/diff/1/7
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79070/diff/1/7#newcode110
Line 110: gfx::Rect bounds = browser->window()->GetNormalBounds();
Since GetBrowser is a no go, I can think of 2 fairly easy alternatives:
- adding a GetContainerBounds on the RenderViewHostDelegateView.
- adding a CreateBrowserWindow that does the same thing as Ctrl+N.

[rafaelw, 2009-04-22 21:47:10]

Made Matt & Aaron's changes.

Backed out GetBrowser() call from RVHDelegate per Darin's request and
implemented CreateExtensionFunctionDispatcher() call in both RVH and
TabContentsDelegate, per Darin & Aaron's discussion. Both calls implement
returning NULL in the header and specific implementations where there is some
reasonable possibility of hosting an extension are implemented to contain a
ExFunctionDispatcher that at least contains a valid RVH and possibly a Browser
if relevant.

Also, added a mod suggested by Peter to allow the extension createWindow() call
to use the WindowSizer logic to size & position the new window to something
reasonable in the default case.

Peter, please review this change in window_sizer.cc and usage of the api in
extension_tabs_module.cc.

http://codereview.chromium.org/79070/diff/1/4
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/79070/diff/1/4#newcode43
Line 43: ExtensionFunctionDispatcher* dispatcher() { return dispatcher_; }
On 2009/04/20 04:23:19, Aaron Boodman wrote:
> Looks like maybe this could be protected? Or just move dispatcher_ into
> protected?

Done.

http://codereview.chromium.org/79070/diff/1/6
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/79070/diff/1/6#newcode34
Line 34: RenderViewHostDelegate *GetRenderViewHostDelegate();
On 2009/04/20 19:00:27, Matt Perry wrote:
> use: Type* name();

Done.

http://codereview.chromium.org/79070/diff/1/9
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/79070/diff/1/9#newcode23
Line 23: class Browser;
On 2009/04/20 07:51:13, darin wrote:
> it is wrong for this file to depend on Browser.  nothing renderer_host should
> depend on Browser.

Done.

http://codereview.chromium.org/79070/diff/1/9#newcode153
Line 153: virtual Browser *GetBrowser() { return NULL; }
On 2009/04/20 07:51:13, darin wrote:
> please do not add this method.

Done.

http://codereview.chromium.org/79070/diff/1/11
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/79070/diff/1/11#newcode112
Line 112: additionalProperties: false
On 2009/04/20 04:23:19, Aaron Boodman wrote:
> This is no longer needed.

Done.

[Peter Kasting, 2009-04-22 21:57:42]

Parts you asked me to review generally look OK, lots of small things.

http://codereview.chromium.org/79070/diff/6001/6007
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79070/diff/6001/6007#newcode94
Line 94: // it's position can default relative to it's browser window.
Nit: it's -> its.  Also this grammar is a bit weird; perhaps "default" -> "be
set"?

http://codereview.chromium.org/79070/diff/6001/6007#newcode96
Line 96: if (browser == NULL) {
Nit: No { }

http://codereview.chromium.org/79070/diff/6001/6007#newcode100
Line 100: // Try to position the new browser relative it's originating browser
window.
Nit: it's -> to its

http://codereview.chromium.org/79070/diff/6001/6007#newcode104
Line 104: WindowSizer::GetBrowserWindowBounds(L"", empty_bounds, &bounds,
&maximized,
L"" -> std::wstring()

http://codereview.chromium.org/79070/diff/6001/6007#newcode127
Line 127: new_window->AddTabWithURL(*(url.get()), GURL(), PageTransition::TYPED,
You do not want TYPED.  Probably LINK.  Not sure what the high-level usage of
this looks like or I could tell you for sure.  TYPED will hose up the omnibox
stats, though.

http://codereview.chromium.org/79070/diff/6001/6026
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/79070/diff/6001/6026#newcode129
Line 129: // If a reference browser is set, use it's window. Otherwise find last
Nit: it's -> its

http://codereview.chromium.org/79070/diff/6001/6026#newcode133
Line 133: window = browser_->window();
Nit: Add DCHECK(window) here to parallel other block

http://codereview.chromium.org/79070/diff/6001/6026#newcode137
Line 137: for (; (window == NULL) && (it != end); ++it) {
Nit: No need to add (window == NULL), instead just add "break;" at the bottom of
the conditional body below.

http://codereview.chromium.org/79070/diff/6001/6027
File chrome/browser/window_sizer.h (right):

http://codereview.chromium.org/79070/diff/6001/6027#newcode97
Line 97: // |browser| can be optionally provided and it's bounds will be used,
Nit: it's -> its.  Also I would write more here.  For example,

"Normally, the returns size is based on the bounds calculated by calling
GetLastActiveWindowState().  To explicitly specify a particular window to base
the new bounds on, pass in a non-NULL value for |browser|."

http://codereview.chromium.org/79070/diff/6001/6027#newcode103
Line 103: Browser* browser);
Nit: in params before outparams, so make this one of the first couple args.

[Matt Perry, 2009-04-22 22:00:25]

looks mostly good

http://codereview.chromium.org/79070/diff/6001/6006
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/79070/diff/6001/6006#newcode37
Line 37: Browser* GetBrowser() { return browser_; }
nit: you can call it browser() since it's a simple getter.

http://codereview.chromium.org/79070/diff/6001/6007
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79070/diff/6001/6007#newcode94
Line 94: // it's position can default relative to it's browser window.
its

http://codereview.chromium.org/79070/diff/6001/6007#newcode104
Line 104: WindowSizer::GetBrowserWindowBounds(L"", empty_bounds, &bounds,
&maximized,
Don't you want to offset 'bounds' by 10,10?

http://codereview.chromium.org/79070/diff/6001/6016
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/79070/diff/6001/6016#newcode166
Line 166: RenderViewHost* render_view_host) { return NULL; }
indent 4 spaces from 'virtual'

http://codereview.chromium.org/79070/diff/6001/6027
File chrome/browser/window_sizer.h (right):

http://codereview.chromium.org/79070/diff/6001/6027#newcode97
Line 97: // |browser| can be optionally provided and it's bounds will be used,
its

[rafaelw, 2009-04-22 23:13:44]

Made changes for Matt & Peter's comments.

http://codereview.chromium.org/79070/diff/6001/6006
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/79070/diff/6001/6006#newcode37
Line 37: Browser* GetBrowser() { return browser_; }
On 2009/04/22 22:00:25, Matt Perry wrote:
> nit: you can call it browser() since it's a simple getter.

Done.

http://codereview.chromium.org/79070/diff/6001/6007
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79070/diff/6001/6007#newcode94
Line 94: // it's position can default relative to it's browser window.
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: it's -> its.  Also this grammar is a bit weird; perhaps "default" -> "be
> set"?

Done.

http://codereview.chromium.org/79070/diff/6001/6007#newcode94
Line 94: // it's position can default relative to it's browser window.
On 2009/04/22 22:00:25, Matt Perry wrote:
> its

Done.

http://codereview.chromium.org/79070/diff/6001/6007#newcode96
Line 96: if (browser == NULL) {
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: No { }

Done. Sorry -- this one is taking some getting used to.

http://codereview.chromium.org/79070/diff/6001/6007#newcode100
Line 100: // Try to position the new browser relative it's originating browser
window.
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: it's -> to its

Done.

http://codereview.chromium.org/79070/diff/6001/6007#newcode104
Line 104: WindowSizer::GetBrowserWindowBounds(L"", empty_bounds, &bounds,
&maximized,
On 2009/04/22 21:57:42, pkasting wrote:
> L"" -> std::wstring()

Done.

http://codereview.chromium.org/79070/diff/6001/6007#newcode104
Line 104: WindowSizer::GetBrowserWindowBounds(L"", empty_bounds, &bounds,
&maximized,
On 2009/04/22 22:00:25, Matt Perry wrote:
> Don't you want to offset 'bounds' by 10,10?

The call actually does this (surprised me too). I've added a comment explaining
that it does.

http://codereview.chromium.org/79070/diff/6001/6007#newcode127
Line 127: new_window->AddTabWithURL(*(url.get()), GURL(), PageTransition::TYPED,
On 2009/04/22 21:57:42, pkasting wrote:
> You do not want TYPED.  Probably LINK.  Not sure what the high-level usage of
> this looks like or I could tell you for sure.  TYPED will hose up the omnibox
> stats, though.

Done.

http://codereview.chromium.org/79070/diff/6001/6016
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/79070/diff/6001/6016#newcode166
Line 166: RenderViewHost* render_view_host) { return NULL; }
On 2009/04/22 22:00:25, Matt Perry wrote:
> indent 4 spaces from 'virtual'

Done.

http://codereview.chromium.org/79070/diff/6001/6026
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/79070/diff/6001/6026#newcode129
Line 129: // If a reference browser is set, use it's window. Otherwise find last
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: it's -> its

Done.

http://codereview.chromium.org/79070/diff/6001/6026#newcode133
Line 133: window = browser_->window();
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: Add DCHECK(window) here to parallel other block

Done.

http://codereview.chromium.org/79070/diff/6001/6026#newcode137
Line 137: for (; (window == NULL) && (it != end); ++it) {
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: No need to add (window == NULL), instead just add "break;" at the bottom
of
> the conditional body below.

Done.

http://codereview.chromium.org/79070/diff/6001/6027
File chrome/browser/window_sizer.h (right):

http://codereview.chromium.org/79070/diff/6001/6027#newcode97
Line 97: // |browser| can be optionally provided and it's bounds will be used,
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: it's -> its.  Also I would write more here.  For example,
> 
> "Normally, the returns size is based on the bounds calculated by calling
> GetLastActiveWindowState().  To explicitly specify a particular window to base
> the new bounds on, pass in a non-NULL value for |browser|."

Done.

http://codereview.chromium.org/79070/diff/6001/6027#newcode97
Line 97: // |browser| can be optionally provided and it's bounds will be used,
On 2009/04/22 22:00:25, Matt Perry wrote:
> its

Done.

http://codereview.chromium.org/79070/diff/6001/6027#newcode103
Line 103: Browser* browser);
On 2009/04/22 21:57:42, pkasting wrote:
> Nit: in params before outparams, so make this one of the first couple args.

Done.

[Matt Perry, 2009-04-22 23:14:27]

LGTM

[Peter Kasting, 2009-04-22 23:18:02]

LGTM with nits

http://codereview.chromium.org/79070/diff/5078/5084
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79070/diff/5078/5084#newcode94
Line 94: // its position can be set relative to it's browser window.
Nit: it's -> its

http://codereview.chromium.org/79070/diff/5078/5084#newcode128
Line 128: true, -1, NULL);
Nit: I would line these up with the * above

[Matt Perry, 2009-04-22 23:37:06]

http://codereview.chromium.org/79070/diff/5078/5084
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79070/diff/5078/5084#newcode97
Line 97: browser = BrowserList::GetLastActive();
Erik brought up a good point related to this in my CL.  This will return a
Browser regardless of profile.  We probably want a GetLastActiveFromProfile()
function.

[rafaelw, 2009-04-23 21:04:11]

http://codereview.chromium.org/79070/diff/5078/5084
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79070/diff/5078/5084#newcode94
Line 94: // its position can be set relative to it's browser window.
On 2009/04/22 23:18:02, pkasting wrote:
> Nit: it's -> its

Done.

http://codereview.chromium.org/79070/diff/5078/5084#newcode97
Line 97: browser = BrowserList::GetLastActive();
On 2009/04/22 23:37:06, Matt Perry wrote:
> Erik brought up a good point related to this in my CL.  This will return a
> Browser regardless of profile.  We probably want a GetLastActiveFromProfile()
> function.

It's a good point and i think there's more to WRT that elsewhere in this file,
but I'm going to do that in a seperate CL (this one is getting big). 

I've created a Profile::GetLastActiveWithProfile(Profile*). (I used 'With'
rather than 'From' to be consistent with FindBrowserWithType() and
FindBrowserWithID()). It is used here instead of ::GetLastActive().

Done.

http://codereview.chromium.org/79070/diff/5078/5084#newcode128
Line 128: true, -1, NULL);
On 2009/04/22 23:18:02, pkasting wrote:
> Nit: I would line these up with the * above

Done.

[darin (slow to review), 2009-04-27 16:11:52]

Please add a more detailed CL description.  Be sure to reference a bug number
(BUG=XXX) and the reviewers (R=foo,bar,baz).  Thanks!

Otherwise LGTM

http://codereview.chromium.org/79070/diff/5130/4119
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/79070/diff/5130/4119#newcode22
Line 22: #include "chrome/browser/extensions/extension_function_dispatcher.h"
nit: please insert into the above list of includes in alphabetical order

http://codereview.chromium.org/79070/diff/5130/4136
File chrome/browser/window_sizer.h (right):

http://codereview.chromium.org/79070/diff/5130/4136#newcode96
Line 96: // See documentation for DetermineWindowBounds below. Normally,
nit: maybe say "If browser is NULL, then..."  That is a bit more clear than
"Normally,"

[Aaron Boodman, 2009-04-27 21:10:43]

lgtm

http://codereview.chromium.org/79070/diff/8099/9047
File chrome/browser/browser.h (right):

http://codereview.chromium.org/79070/diff/8099/9047#newcode415
Line 415: 
Extraneous newline

http://codereview.chromium.org/79070/diff/8099/9035
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/79070/diff/8099/9035#newcode74
Line 74: ExtensionFunctionDispatcher* dispatcher_;
Needs comment. Can you add one to callback_id_ while you're here?

http://codereview.chromium.org/79070/diff/8099/9040
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/79070/diff/8099/9040#newcode158
Line 158: { return NULL; }
Formatting here looks weird. Maybe put return and the final brace on their own
lines?

http://codereview.chromium.org/79070/diff/8099/9045
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/79070/diff/8099/9045#newcode159
Line 159: { return NULL; }
Same here.

[rafaelw, 2009-04-28 00:41:47]

http://codereview.chromium.org/79070/diff/8099/9047
File chrome/browser/browser.h (right):

http://codereview.chromium.org/79070/diff/8099/9047#newcode415
Line 415: 
On 2009/04/27 21:10:43, Aaron Boodman wrote:
> Extraneous newline

Done.

http://codereview.chromium.org/79070/diff/8099/9035
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/79070/diff/8099/9035#newcode74
Line 74: ExtensionFunctionDispatcher* dispatcher_;
On 2009/04/27 21:10:43, Aaron Boodman wrote:
> Needs comment. Can you add one to callback_id_ while you're here?

Done.

http://codereview.chromium.org/79070/diff/8099/9040
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/79070/diff/8099/9040#newcode158
Line 158: { return NULL; }
On 2009/04/27 21:10:43, Aaron Boodman wrote:
> Formatting here looks weird. Maybe put return and the final brace on their own
> lines?

Done.

http://codereview.chromium.org/79070/diff/8099/9045
File chrome/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/79070/diff/8099/9045#newcode159
Line 159: { return NULL; }
On 2009/04/27 21:10:43, Aaron Boodman wrote:
> Same here.

Done.

[Aaron Boodman, 2009-04-28 00:54:18]

LGTM