Create BaseWindow and ExtensionWindowWrapper for extension API access to Panels

Create BaseWindow and ExtensionWindowWrapper for extension API access to Panels

Tests are being re-enabled in http://codereview.chromium.org/9372126/. There appears to already be pretty reasonable test coverage.

BUG=113412, 98759
TEST=Ensure all tests (especially browser_tests --gtest_filter=ExtensionApiTest*) run on all platforms.


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

[stevenjb, 2012-02-22 02:20:20]

This is still a work-in-progress but is ready for initial feedback. Any comments
are welcome, especially regarding class names and overall direction. I'll be
working on testing and adding tests in the meanwhile.

[sky, 2012-02-22 21:46:25]

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/brows...
File chrome/browser/extensions/browser_extension_window_wrapper.cc (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/brows...
chrome/browser/extensions/browser_extension_window_wrapper.cc:27:
DictionaryValue* result = new DictionaryValue();
There's a lot of duplicate code between this and the Browser implementation.
Since ExtensionWindowWrapper is not pure virtual promote the common code.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_window_wrapper.cc (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:11: 
nit: only one newline.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:16: BaseWindow* window,
Profile* profile) :
: on next line.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:59: if
((*iter)->GetSessionId().id() == id)
nit: combine if statements.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:85: if
((*iter)->MatchesProfile(profile, include_incognito)) {
nit: combine if

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_window_wrapper.h (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:30: typedef
std::list<ExtensionWindowWrapper*> WindowList;
Does this need to be in both classes?

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:39: // Returns true if the
window matches the profile.
Newline between methods.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:41: const Profile* profile,
bool allow_incognito) const;
nit: each param on its own line.
Does this method need to be virtual?

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:47: bool populate_tabs)
const = 0;
use enum

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:60: class
ExtensionWindowWrapperList {
How come this is in the same file as ExtensionWindowWrapper? Seems similar to if
browser and browser_list.h were in the same header.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:70: ExtensionWindowWrapper*
FindWindowById(
nit: when you wrap, each param on its own line. Here, 75 and 79

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:75: const Profile* profile,
bool include_incognito) const;
bool -> enum

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:93: 
nit: just one newline.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h
File chrome/browser/ui/base_window.h (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h...
chrome/browser/ui/base_window.h:9: #include <list>
Remove this.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h...
chrome/browser/ui/base_window.h:25: // TODO(beng): REMOVE?
If you need this, then I suspect the TODO is no longer valid.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h...
chrome/browser/ui/base_window.h:26: // Returns true if the frame is maximized
(aka zoomed).
Be consistent. Use window every where to match the name of the class.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/views/aura/pa...
File chrome/browser/ui/views/aura/panel_view_aura.cc (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/views/aura/pa...
chrome/browser/ui/views/aura/panel_view_aura.cc:189: PanelViewAura* panel_view,
PanelHost* panel_host) :
: on next line.

[sky, 2012-02-22 21:48:03]

One more comment. ExtensionWindowWrapper seems a bit misnamed to me. Maybe
ExtensionWindowController? ExtensionWindowHost?

[stevenjb, 2012-02-23 01:09:19]

On 2012/02/22 21:48:03, sky wrote:
> One more comment. ExtensionWindowWrapper seems a bit misnamed to me. Maybe
> ExtensionWindowController? ExtensionWindowHost?

I agree that ExtensionWindowWrapper is a sucky name. 

I'm wary of Host because that implies to me that the class actually hosts the
window, which isn't really true. 

ExtensionWindowController seems OK.

Other possibilities:
ExtensionWindowHolder? 
ExtensionWindowList::Member? or ::Entry? ::Delegate? That seems cumbersome, but
has the advantage of reducing the number of new source files.
ExtensionWindowInterface?
ExtensionWindowApi?

[sky, 2012-02-23 01:30:21]

ExtensionWindowController SGTM

On Wed, Feb 22, 2012 at 5:09 PM,  <stevenjb@chromium.org> wrote:
> On 2012/02/22 21:48:03, sky wrote:
>>
>> One more comment. ExtensionWindowWrapper seems a bit misnamed to me. Maybe
>> ExtensionWindowController? ExtensionWindowHost?
>
>
> I agree that ExtensionWindowWrapper is a sucky name.
>
> I'm wary of Host because that implies to me that the class actually hosts
> the
> window, which isn't really true.
>
> ExtensionWindowController seems OK.
>
> Other possibilities:
> ExtensionWindowHolder?
> ExtensionWindowList::Member? or ::Entry? ::Delegate? That seems cumbersome,
> but
> has the advantage of reducing the number of new source files.
> ExtensionWindowInterface?
> ExtensionWindowApi?
>
>
>
> http://codereview.chromium.org/9428018/

[stevenjb, 2012-02-23 02:19:15]

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/brows...
File chrome/browser/extensions/browser_extension_window_wrapper.cc (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/brows...
chrome/browser/extensions/browser_extension_window_wrapper.cc:27:
DictionaryValue* result = new DictionaryValue();
On 2012/02/22 21:46:25, sky wrote:
> There's a lot of duplicate code between this and the Browser implementation.
> Since ExtensionWindowWrapper is not pure virtual promote the common code.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_window_wrapper.cc (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:11: 
On 2012/02/22 21:46:25, sky wrote:
> nit: only one newline.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:16: BaseWindow* window,
Profile* profile) :
On 2012/02/22 21:46:25, sky wrote:
> : on next line.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:59: if
((*iter)->GetSessionId().id() == id)
On 2012/02/22 21:46:25, sky wrote:
> nit: combine if statements.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.cc:85: if
((*iter)->MatchesProfile(profile, include_incognito)) {
On 2012/02/22 21:46:25, sky wrote:
> nit: combine if

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_window_wrapper.h (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:30: typedef
std::list<ExtensionWindowWrapper*> WindowList;
On 2012/02/22 21:46:25, sky wrote:
> Does this need to be in both classes?

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:39: // Returns true if the
window matches the profile.
On 2012/02/22 21:46:25, sky wrote:
> Newline between methods.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:41: const Profile* profile,
bool allow_incognito) const;
On 2012/02/22 21:46:25, sky wrote:
> nit: each param on its own line.
> Does this method need to be virtual?

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:47: bool populate_tabs)
const = 0;
On 2012/02/22 21:46:25, sky wrote:
> use enum
Made two functions.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:60: class
ExtensionWindowWrapperList {
On 2012/02/22 21:46:25, sky wrote:
> How come this is in the same file as ExtensionWindowWrapper? Seems similar to
if
> browser and browser_list.h were in the same header.
Planning to move once renamed.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:70: ExtensionWindowWrapper*
FindWindowById(
On 2012/02/22 21:46:25, sky wrote:
> nit: when you wrap, each param on its own line. Here, 75 and 79

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:75: const Profile* profile,
bool include_incognito) const;
On 2012/02/22 21:46:25, sky wrote:
> bool -> enum

We pass include_incognito as a bool pretty much everywhere associated with a
profile. If we change that I'd rather do it globally and consistently.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_window_wrapper.h:93: 
On 2012/02/22 21:46:25, sky wrote:
> nit: just one newline.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h
File chrome/browser/ui/base_window.h (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h...
chrome/browser/ui/base_window.h:9: #include <list>
On 2012/02/22 21:46:25, sky wrote:
> Remove this.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h...
chrome/browser/ui/base_window.h:25: // TODO(beng): REMOVE?
On 2012/02/22 21:46:25, sky wrote:
> If you need this, then I suspect the TODO is no longer valid.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/base_window.h...
chrome/browser/ui/base_window.h:26: // Returns true if the frame is maximized
(aka zoomed).
On 2012/02/22 21:46:25, sky wrote:
> Be consistent. Use window every where to match the name of the class.

Done.

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/views/aura/pa...
File chrome/browser/ui/views/aura/panel_view_aura.cc (right):

http://codereview.chromium.org/9428018/diff/1/chrome/browser/ui/views/aura/pa...
chrome/browser/ui/views/aura/panel_view_aura.cc:189: PanelViewAura* panel_view,
PanelHost* panel_host) :
On 2012/02/22 21:46:25, sky wrote:
> : on next line.

Done.

[stevenjb, 2012-02-23 02:20:28]

Note: patch set #3 includes changes from http://codereview.chromium.org/9372126/
so that tests will run. Will rebase after that is reviewed/committed.

[Mihai Parparita -not on Chrome, 2012-02-23 02:35:36]

Overall design seems fine to me too.

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
chrome/browser/extensions/browser_extension_window_controller.cc:27:
BrowserExtensionWindowController::CreateWindowValue() const {
I don't know if this is what Scott meant when he said to "promote the common
code", but there's still a bunch of repetition between this and
PanelExtensionWindowController::CreateWindowValue(). It seems like a lot of
properties can be set from a BaseWindow instance.

Perhaps the ExtensionWindowController::CreateWindowValue implementation should
be implemented in terms of BaseWindow (for things like session ID, bounds,
incognito, etc.) , and subclasses could override it, call it, and add any
additional properties that they need.

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:133:
ExtensionWindowController** wrapper) {
Nit: calling these variables "controller" instead of "wrapper" seems like it'll
be less confusing.

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:136: // If there is a
windowed browser associated with this extension, use that.
When does this have different behavior from
ExtensionWindowList::GetInstance()->CurrentWindow()?

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/panel_view_aura.cc (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/panel_view_aura.cc:258:
ExtensionWindowList::GetInstance()->RemoveExtensionWindow(
Why not do the registration/deregistration in the ExtensionWindowController
constructor/destructor, instead of having each subclass do it?

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/panel_view_aura.h (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/panel_view_aura.h:23: class DictionaryValue;
This can be removed.

[sky, 2012-02-23 17:13:47]

Ben needed to do a bit of research on this one, so I'm holding off
until he's done.

  -Scott

On Wed, Feb 22, 2012 at 6:35 PM,  <mihaip@chromium.org> wrote:
> Overall design seems fine to me too.
>
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
> File chrome/browser/extensions/browser_extension_window_controller.cc
> (right):
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
> chrome/browser/extensions/browser_extension_window_controller.cc:27:
> BrowserExtensionWindowController::CreateWindowValue() const {
> I don't know if this is what Scott meant when he said to "promote the
> common code", but there's still a bunch of repetition between this and
> PanelExtensionWindowController::CreateWindowValue(). It seems like a lot
> of properties can be set from a BaseWindow instance.
>
> Perhaps the ExtensionWindowController::CreateWindowValue implementation
> should be implemented in terms of BaseWindow (for things like session
> ID, bounds, incognito, etc.) , and subclasses could override it, call
> it, and add any additional properties that they need.
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_tabs_module.cc (right):
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_tabs_module.cc:133:
> ExtensionWindowController** wrapper) {
> Nit: calling these variables "controller" instead of "wrapper" seems
> like it'll be less confusing.
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_tabs_module.cc:136: // If there is a
> windowed browser associated with this extension, use that.
> When does this have different behavior from
> ExtensionWindowList::GetInstance()->CurrentWindow()?
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> File chrome/browser/ui/views/aura/panel_view_aura.cc (right):
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> chrome/browser/ui/views/aura/panel_view_aura.cc:258:
> ExtensionWindowList::GetInstance()->RemoveExtensionWindow(
> Why not do the registration/deregistration in the
> ExtensionWindowController constructor/destructor, instead of having each
> subclass do it?
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> File chrome/browser/ui/views/aura/panel_view_aura.h (right):
>
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> chrome/browser/ui/views/aura/panel_view_aura.h:23: class
> DictionaryValue;
> This can be removed.
>
> http://codereview.chromium.org/9428018/

[stevenjb, 2012-02-23 19:57:51]

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
chrome/browser/extensions/browser_extension_window_controller.cc:27:
BrowserExtensionWindowController::CreateWindowValue() const {
On 2012/02/23 02:35:36, Mihai Parparita wrote:
> I don't know if this is what Scott meant when he said to "promote the common
> code", but there's still a bunch of repetition between this and
> PanelExtensionWindowController::CreateWindowValue(). It seems like a lot of
> properties can be set from a BaseWindow instance.
> 
> Perhaps the ExtensionWindowController::CreateWindowValue implementation should
> be implemented in terms of BaseWindow (for things like session ID, bounds,
> incognito, etc.) , and subclasses could override it, call it, and add any
> additional properties that they need.

I forgot to update my comment. It turns out that there is not that much that is
actually common here (only two properties: Id and Focused). I opted not to put
those in a base class implementation, I personally find that pattern confusing
and error prone outside of constructors, but could do so if that is preferred. I
did add a helper function for setting the bounds properties.

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:133:
ExtensionWindowController** wrapper) {
On 2012/02/23 02:35:36, Mihai Parparita wrote:
> Nit: calling these variables "controller" instead of "wrapper" seems like
it'll
> be less confusing.
Yeah, missed that, thanks. Done.

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:136: // If there is a
windowed browser associated with this extension, use that.
On 2012/02/23 02:35:36, Mihai Parparita wrote:
> When does this have different behavior from
> ExtensionWindowList::GetInstance()->CurrentWindow()?
When function->dispatcher()->delegate()->GetBrowser() != the focused browser.

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/panel_view_aura.cc (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/panel_view_aura.cc:258:
ExtensionWindowList::GetInstance()->RemoveExtensionWindow(
On 2012/02/23 02:35:36, Mihai Parparita wrote:
> Why not do the registration/deregistration in the ExtensionWindowController
> constructor/destructor, instead of having each subclass do it?

Good suggestion, I can do that now. (The registration code was added early,
before that would have worked.) Done.

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/panel_view_aura.h (right):

http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/panel_view_aura.h:23: class DictionaryValue;
On 2012/02/23 02:35:36, Mihai Parparita wrote:
> This can be removed.

Done.

[Ben Goodger (Google), 2012-02-24 00:15:03]

For the record, I talked to Steven and this seems like a worthwhile
improvement.

-Ben

On Thu, Feb 23, 2012 at 9:13 AM, Scott Violet <sky@chromium.org> wrote:

> Ben needed to do a bit of research on this one, so I'm holding off
> until he's done.
>
>  -Scott
>
> On Wed, Feb 22, 2012 at 6:35 PM,  <mihaip@chromium.org> wrote:
> > Overall design seems fine to me too.
> >
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
> > File chrome/browser/extensions/browser_extension_window_controller.cc
> > (right):
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/br...
> > chrome/browser/extensions/browser_extension_window_controller.cc:27:
> > BrowserExtensionWindowController::CreateWindowValue() const {
> > I don't know if this is what Scott meant when he said to "promote the
> > common code", but there's still a bunch of repetition between this and
> > PanelExtensionWindowController::CreateWindowValue(). It seems like a lot
> > of properties can be set from a BaseWindow instance.
> >
> > Perhaps the ExtensionWindowController::CreateWindowValue implementation
> > should be implemented in terms of BaseWindow (for things like session
> > ID, bounds, incognito, etc.) , and subclasses could override it, call
> > it, and add any additional properties that they need.
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
> > File chrome/browser/extensions/extension_tabs_module.cc (right):
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
> > chrome/browser/extensions/extension_tabs_module.cc:133:
> > ExtensionWindowController** wrapper) {
> > Nit: calling these variables "controller" instead of "wrapper" seems
> > like it'll be less confusing.
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/ex...
> > chrome/browser/extensions/extension_tabs_module.cc:136: // If there is a
> > windowed browser associated with this extension, use that.
> > When does this have different behavior from
> > ExtensionWindowList::GetInstance()->CurrentWindow()?
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> > File chrome/browser/ui/views/aura/panel_view_aura.cc (right):
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> > chrome/browser/ui/views/aura/panel_view_aura.cc:258:
> > ExtensionWindowList::GetInstance()->RemoveExtensionWindow(
> > Why not do the registration/deregistration in the
> > ExtensionWindowController constructor/destructor, instead of having each
> > subclass do it?
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> > File chrome/browser/ui/views/aura/panel_view_aura.h (right):
> >
> >
>
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/ui/views/aura...
> > chrome/browser/ui/views/aura/panel_view_aura.h:23: class
> > DictionaryValue;
> > This can be removed.
> >
> > http://codereview.chromium.org/9428018/
>

[sky, 2012-02-24 04:38:23]

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/b...
chrome/browser/extensions/browser_extension_window_controller.cc:70: bool
is_fullscreen, const GURL& extension_url) const {
nit: each param on its own line.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_window_controller.h (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_window_controller.h:37: BaseWindow* window()
const { return window_; }
nit: since you're returning a non-const value, this shouldn't be const either.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_window_controller.h:40: virtual bool
MatchesProfile(Profile* profile, bool allow_incognito) const;
Does this need to be virtual?

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_window_list.h (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_window_list.h:29: bool include_incognito,
nit: use enum here and every where else.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/panel_view_aura.cc (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/panel_view_aura.cc:210:
result->SetInteger(keys::kIdKey, panel_view_->session_id().id());
Seems like you could share more code between the two CreateWindowValue
implementations.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/panel_view_aura.cc:213:
result->SetBoolean(keys::kFocusedKey, false);
Why is this always false?

http://codereview.chromium.org/9428018/diff/12003/ui/views/widget/native_widg...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9428018/diff/12003/ui/views/widget/native_widg...
ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
I don't think this makes sense here. Some layout managers may enforce a
different policy as to what the restore bounds is allowed to be.

[jennb, 2012-02-24 05:18:43]

Nice. Wish we had BaseWindow for non-Aura Panels. Would have made for much
easier unit testing.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_window_list.h (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_window_list.h:32: // Returns the focused or
last added  window matching the profile, or NULL.
nit: extra space after 'added'

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:71: virtual void Close() = 0;
Doesn't this cause problems to have the same name as the BaseWindow method? Or
did you mean to have this as part of the comments? Ditto for Show().

[stevenjb, 2012-02-24 19:25:06]

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/b...
chrome/browser/extensions/browser_extension_window_controller.cc:70: bool
is_fullscreen, const GURL& extension_url) const {
On 2012/02/24 04:38:24, sky wrote:
> nit: each param on its own line.

Done.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_window_controller.h (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_window_controller.h:37: BaseWindow* window()
const { return window_; }
On 2012/02/24 04:38:24, sky wrote:
> nit: since you're returning a non-const value, this shouldn't be const either.

I copied the pattern from browser.h. Since this does not own window_, I think
const is reasonable.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_window_controller.h:40: virtual bool
MatchesProfile(Profile* profile, bool allow_incognito) const;
On 2012/02/24 04:38:24, sky wrote:
> Does this need to be virtual?
It does not any more, thanks. Done.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_window_list.h (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_window_list.h:32: // Returns the focused or
last added  window matching the profile, or NULL.
On 2012/02/24 05:18:43, jennb wrote:
> nit: extra space after 'added'

Done.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:71: virtual void Close() = 0;
On 2012/02/24 05:18:43, jennb wrote:
> Doesn't this cause problems to have the same name as the BaseWindow method? Or
> did you mean to have this as part of the comments? Ditto for Show().

All this does is override a pure virtual with a pure virtual which doesn't cause
any problems with the compiler (at least on Linux).

I did this to make it easier to read (since most editors will highlight the
code). However, I spoke with a couple of other developers, and we did think of
one potential problem: If someone removes Close() or Show() in BaseWindow, they
might not realize they need to remove them here which could cause unexpected
behavior.

So, since maintainability trumps readability, I will comment these.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/panel_view_aura.cc (right):

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/panel_view_aura.cc:210:
result->SetInteger(keys::kIdKey, panel_view_->session_id().id());
On 2012/02/24 04:38:24, sky wrote:
> Seems like you could share more code between the two CreateWindowValue
> implementations.

Done.

http://codereview.chromium.org/9428018/diff/12003/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/panel_view_aura.cc:213:
result->SetBoolean(keys::kFocusedKey, false);
On 2012/02/24 04:38:24, sky wrote:
> Why is this always false?
Oversight. Fixed.

http://codereview.chromium.org/9428018/diff/12003/ui/views/widget/native_widg...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9428018/diff/12003/ui/views/widget/native_widg...
ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
On 2012/02/24 04:38:24, sky wrote:
> I don't think this makes sense here. Some layout managers may enforce a
> different policy as to what the restore bounds is allowed to be.

See http://codereview.chromium.org/9372126/

[sky, 2012-02-24 20:21:00]

LGTM

[commit-bot: I haz the power, 2012-02-28 01:43:12]

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

[commit-bot: I haz the power, 2012-02-28 02:55:38]

Try job failure for 9428018-22011 (retry) (retry) on linux_rel for step
"compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-28 16:34:20]

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

[commit-bot: I haz the power, 2012-02-28 17:50:17]

Try job failure for 9428018-22011 (retry) (retry) on linux_rel for step
"check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-28 18:11:28]

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

[commit-bot: I haz the power, 2012-02-28 23:46:08]

Change committed as 124054