Remove Browser dependency in ExtensionFunctionDispatcher

Remove Browser dependency in ExtensionFunctionDispatcher

Part 2/4 for chrome.tabs support for non browser windows.

BUG=115532
TEST=All browser tests pass.

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

[stevenjb, 2012-04-18 21:34:06]

http://codereview.chromium.org/10021071/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_function.cc (right):

http://codereview.chromium.org/10021071/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_function.cc:185: // If the delegate has an
associated browser, return it.
Note: This code was moved here from ExtensionFunctionDispatcher since it was the
only consumer of it.

http://codereview.chromium.org/10021071/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_window_controller.h (left):

http://codereview.chromium.org/10021071/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_window_controller.h:50: virtual
base::DictionaryValue* CreateWindowValue() const;
Just moved this below where it belongs.

http://codereview.chromium.org/10021071/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_window_controller.h (right):

http://codereview.chromium.org/10021071/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_window_controller.h:34: REASON_NOT_EDITABLE,
Random cleanup; "tab strip" isn't a concept this class should be aware of.

[jeremya, 2012-04-20 03:37:32]

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function.cc (right):

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function.cc:282: SendResponse(response);
What's this doing here?

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

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.h:56: // Returns the
ExtensionWindowController associated with this delegate.
The comment here noted that this method can sometimes return NULL. I think
that's still true, because of background pages -- could you note it here?

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:233:
view()->browser()->extension_window_controller() : NULL;
It would be nice to remove Browser from ExtensionView as well, though I'm not
sure it's worth it considering the imminent destruction of ExtensionHost and
ExtensionView.

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:495: // an associated browser.
Update comment

[Aaron Boodman, 2012-04-20 05:06:05]

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function.cc (right):

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function.cc:185: // If the delegate has an
associated browser, return it.
Can you just call GetExtensionWindowController()->GetBrowser().

[stevenjb, 2012-04-20 18:00:21]

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function.cc (right):

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function.cc:185: // If the delegate has an
associated browser, return it.
On 2012/04/20 05:06:05, Aaron Boodman wrote:
> Can you just call GetExtensionWindowController()->GetBrowser().

We do that first, but the existing code also calls BrowserList::FindAnyBrowser()
if there isn't an associated browser. I'll find the place(s) that need that and
factor the behavior out of here in a follow-up CL.

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function.cc:282: SendResponse(response);
On 2012/04/20 03:37:32, jeremya wrote:
> What's this doing here?
This is just some added debugging code that I found helpful. I can put it into a
separate CL.

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

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.h:56: // Returns the
ExtensionWindowController associated with this delegate.
On 2012/04/20 03:37:32, jeremya wrote:
> The comment here noted that this method can sometimes return NULL. I think
> that's still true, because of background pages -- could you note it here?

Done.

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:233:
view()->browser()->extension_window_controller() : NULL;
On 2012/04/20 03:37:32, jeremya wrote:
> It would be nice to remove Browser from ExtensionView as well, though I'm not
> sure it's worth it considering the imminent destruction of ExtensionHost and
> ExtensionView.

Yeah, I think ExtensionView will go away soon, or get moved to something more
specific and not part of extensions/.

http://codereview.chromium.org/10021071/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:495: // an associated browser.
On 2012/04/20 03:37:32, jeremya wrote:
> Update comment

Done.

[stevenjb, 2012-04-24 18:01:33]

ping?

[Aaron Boodman, 2012-04-24 18:30:50]

LGTM

http://codereview.chromium.org/10021071/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_window_controller.h (right):

http://codereview.chromium.org/10021071/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_window_controller.h:30: class
ExtensionWindowController {
Nit: The noun "Extension" at the beginning of this name makes me think this is
about controlling "extension windows". Really it's a window controller for the
extension system.

This could be clarified by putting it in the extensions namespace and just
calling it "window controller".

Something to consider for a future change. Anyway, we're trying to slowly move
everything into the extensions namespace.

[stevenjb, 2012-04-26 20:36:42]

On 2012/04/24 18:30:50, Aaron Boodman wrote:
> LGTM
> 
>
http://codereview.chromium.org/10021071/diff/18002/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_window_controller.h (right):
> 
>
http://codereview.chromium.org/10021071/diff/18002/chrome/browser/extensions/...
> chrome/browser/extensions/extension_window_controller.h:30: class
> ExtensionWindowController {
> Nit: The noun "Extension" at the beginning of this name makes me think this is
> about controlling "extension windows". Really it's a window controller for the
> extension system.
> 
> This could be clarified by putting it in the extensions namespace and just
> calling it "window controller".
> 
> Something to consider for a future change. Anyway, we're trying to slowly move
> everything into the extensions namespace.

I agree, and will plan to rename this when we move it to src/extensions.

[stevenjb, 2012-05-03 20:02:14]

+sky for OWNER review ( chrome/browser/ui/views/ash/panel_view_aura.cc).

mihai, jeremy, let me know if you have any concerns before I commit this.

[sky, 2012-05-03 21:04:46]

LGTM

http://codereview.chromium.org/10021071/diff/29002/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_host.h (right):

http://codereview.chromium.org/10021071/diff/29002/chrome/browser/notificatio...
chrome/browser/notifications/balloon_host.h:40: virtual
ExtensionWindowController* GetExtensionWindowController()
nit: In this case wrap the function name to the next line

http://codereview.chromium.org/10021071/diff/29002/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/panel_view_aura.cc (right):

http://codereview.chromium.org/10021071/diff/29002/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/panel_view_aura.cc:57: virtual
ExtensionWindowController* GetExtensionWindowController()
nit: wrap function name to next line.

[jeremya, 2012-05-04 00:46:05]

lgtm

[Mihai Parparita -not on Chrome, 2012-05-05 01:03:31]

LGTM

[commit-bot: I haz the power, 2012-05-05 20:27:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/10021071/29002

[commit-bot: I haz the power, 2012-05-05 22:01:48]

Change committed as 135562