Introduce AppsClient and use it in apps to get the loaded profiles list.

Introduce AppsClient and use it in apps to get the loaded profiles list.

This breaks one dependency from the apps component to g_browser_process
and some dependencies on profile.

BUG=159366
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222988

[tapted, 2013-09-04 01:08:35]

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc
File apps/apps_client.cc (right):

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc#newcode7
apps/apps_client.cc:7: #include "base/basictypes.h"
nit: I don't think this is used for anything

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc#newco...
apps/apps_client.cc:24: return;
nit: blank line after early return?

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.h
File apps/apps_client.h (right):

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.h#newcode5
apps/apps_client.h:5: #ifndef APPS_COMMON_APPS_CLIENT_H_
include guard has '_COMMON_' - maybe from an earlier iteration

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
File apps/shell_window_registry.cc (right):

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.cc:12: #include
"content/public/browser/devtools_agent_host.h"
nit: #include content/public/browser/browser_context.h?

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.cc:235: return;
nit (while you're here): blank line after early return?

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registry.h
File apps/shell_window_registry.h (right):

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.h:48: explicit
ShellWindowRegistry(content::BrowserContext* context);
nit: should this be forward-declared? It's probably coming in via an include.

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.h:53: //
ShellWindowRegistry::Factory::GetForBrowserContext.
nit: usually I see ShellWindowRegistry::Factory::GetForBrowserContext() for
referring to functions

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
File chrome/browser/apps/chrome_apps_client.h (right):

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
chrome/browser/apps/chrome_apps_client.h:11: #include "base/lazy_instance.h"
I think you can forward declare this, with 

template <typename T> struct DefaultLazyInstanceTraits;

This would match the guide in singleton.h. Someone has probably just been uhhh
lazy updating lazy_instance.h.

But would this be better as a Singleton anyway? LazyInstance is needed if you
need multiple instances of the same type, but this is bound to a single instance
for the type ChromeAppsClient.

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
chrome/browser/apps/chrome_apps_client.h:20: virtual
std::vector<content::BrowserContext*> GetLoadedBrowserContexts()
nit: overrides after newly introduced functions? I think it applies for static
fns too.

[tapted, 2013-09-04 01:17:52]

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
File chrome/browser/apps/chrome_apps_client.cc (right):

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
chrome/browser/apps/chrome_apps_client.cc:10: static
base::LazyInstance<ChromeAppsClient> g_client =
BTW, I think you can use leaky traits (singleton or 

base::LazyInstance<ChromeAppsClient>::Leaky g_client

). This would avoid a static destructor being added when Chrome is exiting.
Although, it's currently a no-op (below), so hopefully the compiler is smart
enough to elide it anyway..

[benwells, 2013-09-10 16:48:50]

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc
File apps/apps_client.cc (right):

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc#newcode7
apps/apps_client.cc:7: #include "base/basictypes.h"
On 2013/09/04 01:08:35, tapted wrote:
> nit: I don't think this is used for anything

Surprisingly I get this error without it:
../../apps/apps_client.cc:11:24: error: use of undeclared identifier 'NULL'
AppsClient* g_client = NULL;

There is probably an even more basic header I could include to get NULL but this
seems fine to me.

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc#newco...
apps/apps_client.cc:24: return;
On 2013/09/04 01:08:35, tapted wrote:
> nit: blank line after early return?

I mainly do that to break up slightly longer blocks, but for consistency: Done.

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.h
File apps/apps_client.h (right):

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.h#newcode5
apps/apps_client.h:5: #ifndef APPS_COMMON_APPS_CLIENT_H_
On 2013/09/04 01:08:35, tapted wrote:
> include guard has '_COMMON_' - maybe from an earlier iteration

Done.

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
File apps/shell_window_registry.cc (right):

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.cc:12: #include
"content/public/browser/devtools_agent_host.h"
On 2013/09/04 01:08:35, tapted wrote:
> nit: #include content/public/browser/browser_context.h?

Done.

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.cc:235: return;
On 2013/09/04 01:08:35, tapted wrote:
> nit (while you're here): blank line after early return?

Done.

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registry.h
File apps/shell_window_registry.h (right):

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.h:48: explicit
ShellWindowRegistry(content::BrowserContext* context);
On 2013/09/04 01:08:35, tapted wrote:
> nit: should this be forward-declared? It's probably coming in via an include.

Done.

https://codereview.chromium.org/23524005/diff/11001/apps/shell_window_registr...
apps/shell_window_registry.h:53: //
ShellWindowRegistry::Factory::GetForBrowserContext.
On 2013/09/04 01:08:35, tapted wrote:
> nit: usually I see ShellWindowRegistry::Factory::GetForBrowserContext() for
> referring to functions

Done.

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
File chrome/browser/apps/chrome_apps_client.cc (right):

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
chrome/browser/apps/chrome_apps_client.cc:10: static
base::LazyInstance<ChromeAppsClient> g_client =
On 2013/09/04 01:17:53, tapted wrote:
> BTW, I think you can use leaky traits (singleton or 
> 
> base::LazyInstance<ChromeAppsClient>::Leaky g_client
> 
> ). This would avoid a static destructor being added when Chrome is exiting.
> Although, it's currently a no-op (below), so hopefully the compiler is smart
> enough to elide it anyway..

Done.

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
File chrome/browser/apps/chrome_apps_client.h (right):

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
chrome/browser/apps/chrome_apps_client.h:11: #include "base/lazy_instance.h"
On 2013/09/04 01:08:35, tapted wrote:
> I think you can forward declare this, with 
> 
> template <typename T> struct DefaultLazyInstanceTraits;
> 
> This would match the guide in singleton.h. Someone has probably just been uhhh
> lazy updating lazy_instance.h.
> 
> But would this be better as a Singleton anyway? LazyInstance is needed if you
> need multiple instances of the same type, but this is bound to a single
instance
> for the type ChromeAppsClient.

Done.

https://codereview.chromium.org/23524005/diff/11001/chrome/browser/apps/chrom...
chrome/browser/apps/chrome_apps_client.h:20: virtual
std::vector<content::BrowserContext*> GetLoadedBrowserContexts()
On 2013/09/04 01:08:35, tapted wrote:
> nit: overrides after newly introduced functions? I think it applies for static
> fns too.

Actually it should be private. Moved to private.

[tapted, 2013-09-10 20:46:08]

lgtm

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc
File apps/apps_client.cc (right):

https://codereview.chromium.org/23524005/diff/11001/apps/apps_client.cc#newcode7
apps/apps_client.cc:7: #include "base/basictypes.h"
On 2013/09/10 16:48:50, benwells wrote:
> On 2013/09/04 01:08:35, tapted wrote:
> > nit: I don't think this is used for anything
> 
> Surprisingly I get this error without it:
> ../../apps/apps_client.cc:11:24: error: use of undeclared identifier 'NULL'
> AppsClient* g_client = NULL;
> 
> There is probably an even more basic header I could include to get NULL but
this
> seems fine to me.

Ah drat. nullptr can not come soon enough..

[benwells, 2013-09-10 20:58:07]

+sky for owners review.

[sky, 2013-09-11 02:13:25]

Ben, what files do you need me to review here?

[benwells, 2013-09-11 04:39:24]

On 2013/09/11 02:13:25, sky wrote:
> Ben, what files do you need me to review here?

Sorry, I should have made that clear.

Everything but apps/ and chrome/browser/apps needs owners.

Probably the only interesting thing is chrome/browser/browser_process_impl.cc

[benwells, 2013-09-11 04:39:26]

On 2013/09/11 02:13:25, sky wrote:
> Ben, what files do you need me to review here?

Sorry, I should have made that clear.

Everything but apps/ and chrome/browser/apps needs owners.

Probably the only interesting thing is chrome/browser/browser_process_impl.cc

[sky, 2013-09-11 16:13:41]

LGTM

[commit-bot: I haz the power, 2013-09-12 00:56:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/23524005/45002

[commit-bot: I haz the power, 2013-09-12 00:56:35]

Failed to apply patch for chrome/chrome_browser_extensions.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser_extensions.gypi
  Hunk #1 FAILED at 64.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/chrome_browser_extensions.gypi.rej

Patch:       chrome/chrome_browser_extensions.gypi
Index: chrome/chrome_browser_extensions.gypi
diff --git a/chrome/chrome_browser_extensions.gypi
b/chrome/chrome_browser_extensions.gypi
index
4f86fca4a3d8e07841fdecc8dadf5173157b7790..5d3042c08f31ee70eb3936ceb4ffc6933add2106
100644
--- a/chrome/chrome_browser_extensions.gypi
+++ b/chrome/chrome_browser_extensions.gypi
@@ -64,6 +64,8 @@
         '../extensions/browser/pref_names.h',
         '../extensions/browser/view_type_utils.cc',
         '../extensions/browser/view_type_utils.h',
+        'browser/apps/chrome_apps_client.cc',
+        'browser/apps/chrome_apps_client.h',
         'browser/apps/shortcut_manager.cc',
         'browser/apps/shortcut_manager.h',
         'browser/apps/shortcut_manager_factory.cc',

[commit-bot: I haz the power, 2013-09-13 01:10:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/23524005/73001

[commit-bot: I haz the power, 2013-09-13 06:55:23]

Change committed as 222988