Move ExtensionWindowController and related into extensions namespace

Move ExtensionWindowController and related into extensions namespace

Moved ExtensionWindowController, BrowserExtensionWindowController,
ExtensionWindowList to extensions namespace; renamed appropriately.

BUG=136874, 117261
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147083

[Devlin, 2012-07-12 22:09:49]

Simple refactor, please review when you get a chance.
mihaip: c/b/extensions
pkasting: c/b/ui

Thanks in advance.

[Mihai Parparita -not on Chrome, 2012-07-12 23:34:54]

LGTM

[Peter Kasting, 2012-07-13 01:33:43]

Rubber-stamp LGTM

[stevenjb, 2012-07-16 17:58:44]

I'm actually kind of wary of naming this 'WindowController', even inside the
extensions namespace. 
e.g. OSX already has BrowserWindowController which is unrelated, so on OSX we
would have have two unrelated BrowserWindowController classes in different
namespaces (but both under src/chrome). This make searches and debugging
confusing.
Also, even though I don't see any conflicts, 'WindowList' does not seem very
descriptive, even inside the extensions namespace.

I would much rather keep the class names as they are and just move them into the
extensions namespace. I'd rather have a bit of redundancy in
extensions::ExtensionWindowController/List than the potential confusion (it will
also make this CL a lot smaller).

[Devlin, 2012-07-16 20:51:37]

On 2012/07/16 17:58:44, stevenjb (chromium) wrote:
> I'm actually kind of wary of naming this 'WindowController', even inside the
> extensions namespace. 
> e.g. OSX already has BrowserWindowController which is unrelated, so on OSX we
> would have have two unrelated BrowserWindowController classes in different
> namespaces (but both under src/chrome). This make searches and debugging
> confusing.
> Also, even though I don't see any conflicts, 'WindowList' does not seem very
> descriptive, even inside the extensions namespace.
> 
> I would much rather keep the class names as they are and just move them into
the
> extensions namespace. I'd rather have a bit of redundancy in
> extensions::ExtensionWindowController/List than the potential confusion (it
will
> also make this CL a lot smaller).

Dang...

I was hoping to be able to have some form of uniformity between extensions
classes (most of the newer ones don't have the Extension prefix and substitute
the extensions:: namespace). Vagueness in the class name isn't a huge deal,
because anything using the "vague" name is itself within the extensions system.
Anything outside it uses the whole extensions::MyClass, and it isn't really
ambiguous (precedent from other examples of seemingly vague extensions class
names that have been in the codebase, such as "ProcessMap", "StateStore",
"ManagementPolicy", etc).

But having two classes with the same name *is* a problem, especially when they
are both in src/chrome. If we can't find a better solution, I can put these back
to their original names, but it'd be nice to avoid that. Since
WindowController/List don't conflict with anything, could we keep those as they
are (again, these will always be referenced with extensions::Window____ outside
of the extensions system), and change the name of
BrowserExtensionWindowController to extensions::WindowControllerFromBrowser or
extensions::WindowControllerWithBrowser or something similar to avoid the
conflict?

As I said, if this doesn't sound good, I can change the names back.

Thanks for the review :)

[stevenjb, 2012-07-16 21:31:09]

On 2012/07/16 20:51:37, D Cronin wrote:
> On 2012/07/16 17:58:44, stevenjb (chromium) wrote:
> > I'm actually kind of wary of naming this 'WindowController', even inside the
> > extensions namespace. 
> > e.g. OSX already has BrowserWindowController which is unrelated, so on OSX
we
> > would have have two unrelated BrowserWindowController classes in different
> > namespaces (but both under src/chrome). This make searches and debugging
> > confusing.
> > Also, even though I don't see any conflicts, 'WindowList' does not seem very
> > descriptive, even inside the extensions namespace.
> > 
> > I would much rather keep the class names as they are and just move them into
> the
> > extensions namespace. I'd rather have a bit of redundancy in
> > extensions::ExtensionWindowController/List than the potential confusion (it
> will
> > also make this CL a lot smaller).
> 
> Dang...
> 
> I was hoping to be able to have some form of uniformity between extensions
> classes (most of the newer ones don't have the Extension prefix and substitute
> the extensions:: namespace). Vagueness in the class name isn't a huge deal,
> because anything using the "vague" name is itself within the extensions
system.
> Anything outside it uses the whole extensions::MyClass, and it isn't really
> ambiguous (precedent from other examples of seemingly vague extensions class
> names that have been in the codebase, such as "ProcessMap", "StateStore",
> "ManagementPolicy", etc).
> 
> But having two classes with the same name *is* a problem, especially when they
> are both in src/chrome. If we can't find a better solution, I can put these
back
> to their original names, but it'd be nice to avoid that. Since
> WindowController/List don't conflict with anything, could we keep those as
they
> are (again, these will always be referenced with extensions::Window____
outside
> of the extensions system), and change the name of
> BrowserExtensionWindowController to extensions::WindowControllerFromBrowser or
> extensions::WindowControllerWithBrowser or something similar to avoid the
> conflict?
> 
> As I said, if this doesn't sound good, I can change the names back.
> 
> Thanks for the review :)

Hmm, it sounds like you're saying that it's ok to have generic class names like
WindowList inside a namespace, but we shouldn't have two classes with the same
name (BrowserWindowController) in different namespaces :)

In principal, I don't have a problem with using simple names inside a namespace.
However, I do think that WindowController and WindowList have the potential to
be especially confusing, since "window" is such a broadly used term.

How about this:
* Leave extension::WindowController as you have it, it's actually reasonably
localized in its use.
* Rename extension::WindowList to extension::WindowControllerList which more
accurately describes the class anyway.
* Leave BrowserExtensionWindowController outside the extension:: namespace and
keep its old name; if anything it should probably be part of browser:: not
extension:: as it by definition has Browser dependencies. (The original goal of
ExtensionWindowController was to reduce and eventually eliminate Browser
dependencies in the extensions code).

[Devlin, 2012-07-17 17:09:44]

On 2012/07/16 21:31:09, stevenjb (chromium) wrote:
> On 2012/07/16 20:51:37, D Cronin wrote:
> > On 2012/07/16 17:58:44, stevenjb (chromium) wrote:
> > > I'm actually kind of wary of naming this 'WindowController', even inside
the
> > > extensions namespace. 
> > > e.g. OSX already has BrowserWindowController which is unrelated, so on OSX
> we
> > > would have have two unrelated BrowserWindowController classes in different
> > > namespaces (but both under src/chrome). This make searches and debugging
> > > confusing.
> > > Also, even though I don't see any conflicts, 'WindowList' does not seem
very
> > > descriptive, even inside the extensions namespace.
> > > 
> > > I would much rather keep the class names as they are and just move them
into
> > the
> > > extensions namespace. I'd rather have a bit of redundancy in
> > > extensions::ExtensionWindowController/List than the potential confusion
(it
> > will
> > > also make this CL a lot smaller).
> > 
> > Dang...
> > 
> > I was hoping to be able to have some form of uniformity between extensions
> > classes (most of the newer ones don't have the Extension prefix and
substitute
> > the extensions:: namespace). Vagueness in the class name isn't a huge deal,
> > because anything using the "vague" name is itself within the extensions
> system.
> > Anything outside it uses the whole extensions::MyClass, and it isn't really
> > ambiguous (precedent from other examples of seemingly vague extensions class
> > names that have been in the codebase, such as "ProcessMap", "StateStore",
> > "ManagementPolicy", etc).
> > 
> > But having two classes with the same name *is* a problem, especially when
they
> > are both in src/chrome. If we can't find a better solution, I can put these
> back
> > to their original names, but it'd be nice to avoid that. Since
> > WindowController/List don't conflict with anything, could we keep those as
> they
> > are (again, these will always be referenced with extensions::Window____
> outside
> > of the extensions system), and change the name of
> > BrowserExtensionWindowController to extensions::WindowControllerFromBrowser
or
> > extensions::WindowControllerWithBrowser or something similar to avoid the
> > conflict?
> > 
> > As I said, if this doesn't sound good, I can change the names back.
> > 
> > Thanks for the review :)
> 
> Hmm, it sounds like you're saying that it's ok to have generic class names
like
> WindowList inside a namespace, but we shouldn't have two classes with the same
> name (BrowserWindowController) in different namespaces :)
> 
> In principal, I don't have a problem with using simple names inside a
namespace.
> However, I do think that WindowController and WindowList have the potential to
> be especially confusing, since "window" is such a broadly used term.
> 
> How about this:
> * Leave extension::WindowController as you have it, it's actually reasonably
> localized in its use.
> * Rename extension::WindowList to extension::WindowControllerList which more
> accurately describes the class anyway.
> * Leave BrowserExtensionWindowController outside the extension:: namespace and
> keep its old name; if anything it should probably be part of browser:: not
> extension:: as it by definition has Browser dependencies. (The original goal
of
> ExtensionWindowController was to reduce and eventually eliminate Browser
> dependencies in the extensions code).

Sounds good to me :)
Done.

[stevenjb, 2012-07-17 17:58:30]

Thanks!
LGTM

[commit-bot: I haz the power, 2012-07-17 18:47:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10735061/1...

[commit-bot: I haz the power, 2012-07-17 20:36:59]

Change committed as 147083