Remove AppsClient::GetLoadedBrowserContexts

Remove AppsClient::GetLoadedBrowserContexts

Move GetAppWindowForNativeWindowAnyProfile(), IsAppWindowRegisteredInAnyProfile(), CloseAllAppWindows() from extensions::AppWindowRegistry to a new class AppWindowRegistryUtil.
These functions are only needed by chrome.
Also, putting these functions in chrome/browser/apps
allows them to get the loaded profiles directly from ProfileManager rather than
requiring the AppsClient::GetLoadedBrowserContexts delegate method.

BUG=411352
TBR=sky@chromium.org for include fix & class rename under chrome/browser

Committed: https://crrev.com/c350c3cd2a6ae2fb9f9bca512f596fb8ef7ab79b
Cr-Commit-Position: refs/heads/master@{#296886}

[hashimoto, 2014-09-19 04:55:09]

Patchset #1 (id:1) has been deleted

[hashimoto, 2014-09-19 05:52:28]

hashimoto@chromium.org changed reviewers:
+ tapted@chromium.org

[hashimoto, 2014-09-19 05:53:32]

tapted@, could you review this change as an owner of apps directories?

[tapted, 2014-09-19 07:59:03]

Is app_shim_handler_mac.cc the only reason why app_window_registry_util.cc needs
to be in src/apps rather than chrome/browser/apps?

If so, I think we should just move apps/app_shim to chrome/browser/apps/app_shim
first. I think that's been the plan for a while (see comments in
src/apps/app_shim/DEPS), but chrome/browser/apps didn't exist at the time.

https://codereview.chromium.org/586613002/diff/20001/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/586613002/diff/20001/apps/DEPS#newcode21
apps/DEPS:21: "+chrome/browser/browser_process.h",
We shouldn't be adding things here..

[hashimoto, 2014-09-19 08:37:13]

On 2014/09/19 07:59:03, tapted wrote:
> Is app_shim_handler_mac.cc the only reason why app_window_registry_util.cc
needs
> to be in src/apps rather than chrome/browser/apps?
I simply put it in src/apps because originally AppWindowRegistry was there
before I moved it to extensions/browser/app_window.
> 
> If so, I think we should just move apps/app_shim to
chrome/browser/apps/app_shim
> first. I think that's been the plan for a while (see comments in
> src/apps/app_shim/DEPS), but chrome/browser/apps didn't exist at the time.
Thanks for letting me know about the plan.
Made a CL to do that https://codereview.chromium.org/584823002/.
> 
> https://codereview.chromium.org/586613002/diff/20001/apps/DEPS
> File apps/DEPS (right):
> 
> https://codereview.chromium.org/586613002/diff/20001/apps/DEPS#newcode21
> apps/DEPS:21: "+chrome/browser/browser_process.h",
> We shouldn't be adding things here..

[hashimoto, 2014-09-25 05:07:26]

Thank you for moving app_shim code to chrome/browser/apps!

Uploaded a new patch set which adds AppWindowRegistryUtil to chrome/browser/apps
PTAL

[tapted, 2014-09-25 05:47:34]

lgtm The CL description need an update (it's no longer
apps::AppWindowRegistryUtil) 

But I would also say ~"Putting AppWindowRegistryUtil in chrome/browser/apps
allows it to get the loaded profiles directly from ProfileManager rather than
requiring the AppsClient::GetLoadedBrowserContexts delegate method"

One note below, but I don't think it needs an action unless other OWNERS
complain.

https://codereview.chromium.org/586613002/diff/60001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_registry_util.h (right):

https://codereview.chromium.org/586613002/diff/60001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_registry_util.h:15: class AppWindowRegistryUtil {
I think the style guide lightly discourages classes that are merely collections
of static functions [1]. Although here you could argue that these are tied to
the AppWindowRegistry class (although without needing friend access).

I think I like how it looks now, but could be swayed easily accept a namespace
(or even global functions..).

[1] within
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_St...

[hashimoto, 2014-09-25 06:51:55]

https://codereview.chromium.org/586613002/diff/60001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_registry_util.h (right):

https://codereview.chromium.org/586613002/diff/60001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_registry_util.h:15: class AppWindowRegistryUtil {
On 2014/09/25 05:47:34, tapted wrote:
> I think the style guide lightly discourages classes that are merely
collections
> of static functions [1]. Although here you could argue that these are tied to
> the AppWindowRegistry class (although without needing friend access).
> 
> I think I like how it looks now, but could be swayed easily accept a namespace
> (or even global functions..).
> 
> [1] within
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_St...

I introduced this AppWindowRegistryUtil class because I was too lazy to coin
function names which make sense without the class name in the global namespace
:-/
But, it seems these function names are descriptive enough so it might be OK to
put them in the global namespace.

Though I feel a bit hesitant to put them in the global namespace as it's
generally not a good idea.
Maybe they should be put in a namespace "apps"? (no file in chrome/browser/apps
is using namespace "apps", but files under app_shim are using it.)

[tapted, 2014-09-25 07:07:29]

On 2014/09/25 06:51:55, hashimoto wrote:
>
https://codereview.chromium.org/586613002/diff/60001/chrome/browser/apps/app_...
> File chrome/browser/apps/app_window_registry_util.h (right):
> 
>
https://codereview.chromium.org/586613002/diff/60001/chrome/browser/apps/app_...
> chrome/browser/apps/app_window_registry_util.h:15: class AppWindowRegistryUtil
{
> On 2014/09/25 05:47:34, tapted wrote:
> > I think the style guide lightly discourages classes that are merely
> collections
> > of static functions [1]. Although here you could argue that these are tied
to
> > the AppWindowRegistry class (although without needing friend access).
> > 
> > I think I like how it looks now, but could be swayed easily accept a
namespace
> > (or even global functions..).
> > 
> > [1] within
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_St...
> 
> I introduced this AppWindowRegistryUtil class because I was too lazy to coin
> function names which make sense without the class name in the global namespace
> :-/
> But, it seems these function names are descriptive enough so it might be OK to
> put them in the global namespace.
> 
> Though I feel a bit hesitant to put them in the global namespace as it's
> generally not a good idea.
> Maybe they should be put in a namespace "apps"? (no file in
chrome/browser/apps
> is using namespace "apps", but files under app_shim are using it.)

I'd leave it as-is :)

`namespace apps` is used by src/apps, so probably wouldn't fit here (app_shim
should probably stop using it - I'll update http://crbug.com/266705 ). Stuff in
src/chrome typically has no namespace, but there are exceptions. And you're
right these names are probably descriptive enough, but the link back to the
AppWindowRegistry at the call sites is a nice thing I think. It makes it obvious
that these calls are operating on information held in AppWindowRegistry which
would be lost otherwise.

[hashimoto, 2014-09-25 07:27:29]

OK, then let's go ahead with the AppWindowRegistryUtil class.
Thanks!

On Thu, Sep 25, 2014 at 4:07 PM, <tapted@chromium.org> wrote:

> On 2014/09/25 06:51:55, hashimoto wrote:
>
> https://codereview.chromium.org/586613002/diff/60001/
> chrome/browser/apps/app_window_registry_util.h
>
>> File chrome/browser/apps/app_window_registry_util.h (right):
>>
>
>
> https://codereview.chromium.org/586613002/diff/60001/
> chrome/browser/apps/app_window_registry_util.h#newcode15
>
>> chrome/browser/apps/app_window_registry_util.h:15: class
>> AppWindowRegistryUtil
>>
> {
>
>> On 2014/09/25 05:47:34, tapted wrote:
>> > I think the style guide lightly discourages classes that are merely
>> collections
>> > of static functions [1]. Although here you could argue that these are
>> tied
>>
> to
>
>> > the AppWindowRegistry class (although without needing friend access).
>> >
>> > I think I like how it looks now, but could be swayed easily accept a
>>
> namespace
>
>> > (or even global functions..).
>> >
>> > [1] within
>> >
>>
>
> http://google-styleguide.googlecode.com/svn/trunk/
> cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
>
>  I introduced this AppWindowRegistryUtil class because I was too lazy to
>> coin
>> function names which make sense without the class name in the global
>> namespace
>> :-/
>> But, it seems these function names are descriptive enough so it might be
>> OK to
>> put them in the global namespace.
>>
>
>  Though I feel a bit hesitant to put them in the global namespace as it's
>> generally not a good idea.
>> Maybe they should be put in a namespace "apps"? (no file in
>>
> chrome/browser/apps
>
>> is using namespace "apps", but files under app_shim are using it.)
>>
>
> I'd leave it as-is :)
>
> `namespace apps` is used by src/apps, so probably wouldn't fit here
> (app_shim
> should probably stop using it - I'll update http://crbug.com/266705 ).
> Stuff in
> src/chrome typically has no namespace, but there are exceptions. And you're
> right these names are probably descriptive enough, but the link back to the
> AppWindowRegistry at the call sites is a nice thing I think. It makes it
> obvious
> that these calls are operating on information held in AppWindowRegistry
> which
> would be lost otherwise.
>
>
>
> https://codereview.chromium.org/586613002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2014-09-25 07:28:11]

hashimoto@chromium.org changed reviewers:
+ oshima@chromium.org, rockot@chromium.org

[hashimoto, 2014-09-25 07:28:11]

oshima@: Please review this change as an owner of athena.
rockot@: Please review this change as an owner of extensions.

[Ken Rockot(use gerrit already), 2014-09-25 17:14:32]

lgtm

[oshima, 2014-09-25 17:27:50]

athena/ lgtm

[hashimoto, 2014-09-26 04:46:12]

hashimoto@chromium.org changed reviewers:
+ sky@chromium.org

[hashimoto, 2014-09-26 04:46:29]

TBRing sky@chromium.org for include fix & class rename under chrome/browser

[hashimoto, 2014-09-26 04:46:35]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-09-26 04:47:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/586613002/60001

[commit-bot: I haz the power, 2014-09-26 04:51:44]

Committed patchset #3 (id:60001) as d31c468af9fd7b29132fbeb4e4bb9a072d66b1c8

[commit-bot: I haz the power, 2014-09-26 04:52:17]

Patchset 3 (id:??) landed as
https://crrev.com/c350c3cd2a6ae2fb9f9bca512f596fb8ef7ab79b
Cr-Commit-Position: refs/heads/master@{#296886}