Cleanup: give ChromeAppSorting a BrowserContext

Cleanup: give ChromeAppSorting a BrowserContext
This allows us to remove the yucky AppSorting::SetExtensionSyncService method, as well as ExtensionSyncService::SyncOrderingChange.

BUG=None

Committed: https://crrev.com/11ce5d50e71686e2335ede01ca54a902175ca07c
Cr-Commit-Position: refs/heads/master@{#340440}

[Marc Treib, 2015-07-23 14:49:22]

treib@chromium.org changed reviewers:
+ kalman@chromium.org

[Marc Treib, 2015-07-23 14:49:23]

Small cleanup as discussed. PTAL!

[not at google - send to devlin, 2015-07-23 15:35:11]

This is a nice cleanup - and lgtm if you just want to submit it in its current
form - but it would be quite a bit nicer if you went that extra stage and moved
ownership to ExtensionSystem. I'm more than happy for that to be part of this
patch.

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_app_sorting.cc (right):

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:544: if (!browser_context_)
Comment // null for testing.

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:549: const Extension* extension
= service->GetInstalledExtension(extension_id);
Better:

ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context);
const Extension* extension = registry->GetInstalledExtension(extension_id);

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:61:
ExtensionsBrowserClient::Get()->CreateAppSorting(nullptr).Pass(),
use profile_ here not nullptr?

https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
extensions/browser/extensions_browser_client.h:164: virtual
scoped_ptr<AppSorting> CreateAppSorting(
Even more cleanup:

This stuff would all be much simpler if there were an "app_sorting()" method on
ExtensionSystem, which ExtensionSystemImpl overrides to return a
ChromeAppSorting, and everything else returns a NullAppSorting/nullptr (and why
are both acceptable? eh whatever).

I.e. shift ownership of AppSorting from ExtensionPrefs to ExtensionSystem.

That would let you delete AppSorting::SetExtensionScopedPrefs, and instead
ChromeAppSorting could just retrieve the ExtensionPrefs instance from the
BrowserContext it's given.

It would also let you simply delete ExtensionsBrowserClient::CreateAppSorting
rather than updating it.

[Marc Treib, 2015-07-24 11:38:34]

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_app_sorting.cc (right):

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:544: if (!browser_context_)
On 2015/07/23 15:35:11, kalman wrote:
> Comment // null for testing.

Done.

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:549: const Extension* extension
= service->GetInstalledExtension(extension_id);
On 2015/07/23 15:35:11, kalman wrote:
> Better:
> 
> ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context);
> const Extension* extension = registry->GetInstalledExtension(extension_id);

Done.

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/1251323003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/test_extension_system.cc:61:
ExtensionsBrowserClient::Get()->CreateAppSorting(nullptr).Pass(),
On 2015/07/23 15:35:11, kalman wrote:
> use profile_ here not nullptr?

D'oh. Done.

https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
extensions/browser/extensions_browser_client.h:164: virtual
scoped_ptr<AppSorting> CreateAppSorting(
On 2015/07/23 15:35:11, kalman wrote:
> Even more cleanup:
> 
> This stuff would all be much simpler if there were an "app_sorting()" method
on
> ExtensionSystem, which ExtensionSystemImpl overrides to return a
> ChromeAppSorting, and everything else returns a NullAppSorting/nullptr (and
why
> are both acceptable? eh whatever).
> 
> I.e. shift ownership of AppSorting from ExtensionPrefs to ExtensionSystem.
> 
> That would let you delete AppSorting::SetExtensionScopedPrefs, and instead
> ChromeAppSorting could just retrieve the ExtensionPrefs instance from the
> BrowserContext it's given.
> 
> It would also let you simply delete ExtensionsBrowserClient::CreateAppSorting
> rather than updating it.

I started on this, but then realized it creates a dependency cycle:
ExtensionSystem depends on ExtensionPrefs, but ExtensionPrefs needs an
AppSorting and so now will depend on ExtensionSystem. That could probably be
worked around with an ExtensionPrefs::set_app_sorting or something like that,
but then we won't really win much by the whole thing. WDYT?

[not at google - send to devlin, 2015-07-24 14:43:54]

https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
extensions/browser/extensions_browser_client.h:164: virtual
scoped_ptr<AppSorting> CreateAppSorting(
On 2015/07/24 11:38:34, Marc Treib wrote:
> On 2015/07/23 15:35:11, kalman wrote:
> > Even more cleanup:
> > 
> > This stuff would all be much simpler if there were an "app_sorting()" method
> on
> > ExtensionSystem, which ExtensionSystemImpl overrides to return a
> > ChromeAppSorting, and everything else returns a NullAppSorting/nullptr (and
> why
> > are both acceptable? eh whatever).
> > 
> > I.e. shift ownership of AppSorting from ExtensionPrefs to ExtensionSystem.
> > 
> > That would let you delete AppSorting::SetExtensionScopedPrefs, and instead
> > ChromeAppSorting could just retrieve the ExtensionPrefs instance from the
> > BrowserContext it's given.
> > 
> > It would also let you simply delete
ExtensionsBrowserClient::CreateAppSorting
> > rather than updating it.
> 
> I started on this, but then realized it creates a dependency cycle:
> ExtensionSystem depends on ExtensionPrefs, but ExtensionPrefs needs an
> AppSorting and so now will depend on ExtensionSystem. That could probably be
> worked around with an ExtensionPrefs::set_app_sorting or something like that,
> but then we won't really win much by the whole thing. WDYT?

That's pretty typical, usually all these things depend on in their constructor
is a Profile/BrowserContext. They fetch the ExtensionPrefs/AppSorting from that,
sometimes via an ExtensionSystem, when needed.

[Marc Treib, 2015-07-24 14:51:02]

On 2015/07/24 14:43:54, kalman wrote:
>
https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
> File extensions/browser/extensions_browser_client.h (right):
> 
>
https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
> extensions/browser/extensions_browser_client.h:164: virtual
> scoped_ptr<AppSorting> CreateAppSorting(
> On 2015/07/24 11:38:34, Marc Treib wrote:
> > On 2015/07/23 15:35:11, kalman wrote:
> > > Even more cleanup:
> > > 
> > > This stuff would all be much simpler if there were an "app_sorting()"
method
> > on
> > > ExtensionSystem, which ExtensionSystemImpl overrides to return a
> > > ChromeAppSorting, and everything else returns a NullAppSorting/nullptr
(and
> > why
> > > are both acceptable? eh whatever).
> > > 
> > > I.e. shift ownership of AppSorting from ExtensionPrefs to ExtensionSystem.
> > > 
> > > That would let you delete AppSorting::SetExtensionScopedPrefs, and instead
> > > ChromeAppSorting could just retrieve the ExtensionPrefs instance from the
> > > BrowserContext it's given.
> > > 
> > > It would also let you simply delete
> ExtensionsBrowserClient::CreateAppSorting
> > > rather than updating it.
> > 
> > I started on this, but then realized it creates a dependency cycle:
> > ExtensionSystem depends on ExtensionPrefs, but ExtensionPrefs needs an
> > AppSorting and so now will depend on ExtensionSystem. That could probably be
> > worked around with an ExtensionPrefs::set_app_sorting or something like
that,
> > but then we won't really win much by the whole thing. WDYT?
> 
> That's pretty typical, usually all these things depend on in their constructor
> is a Profile/BrowserContext. They fetch the ExtensionPrefs/AppSorting from
that,
> sometimes via an ExtensionSystem, when needed.

I was talking about "DependsOn" in the KeyedService sense, i.e.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
In the ExtensionPrefs constructor, the ExtensionSystem doesn't actually exist
yet.

[not at google - send to devlin, 2015-07-24 16:20:05]

On 2015/07/24 14:51:02, Marc Treib wrote:
> On 2015/07/24 14:43:54, kalman wrote:
> >
>
https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
> > File extensions/browser/extensions_browser_client.h (right):
> > 
> >
>
https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
> > extensions/browser/extensions_browser_client.h:164: virtual
> > scoped_ptr<AppSorting> CreateAppSorting(
> > On 2015/07/24 11:38:34, Marc Treib wrote:
> > > On 2015/07/23 15:35:11, kalman wrote:
> > > > Even more cleanup:
> > > > 
> > > > This stuff would all be much simpler if there were an "app_sorting()"
> method
> > > on
> > > > ExtensionSystem, which ExtensionSystemImpl overrides to return a
> > > > ChromeAppSorting, and everything else returns a NullAppSorting/nullptr
> (and
> > > why
> > > > are both acceptable? eh whatever).
> > > > 
> > > > I.e. shift ownership of AppSorting from ExtensionPrefs to
ExtensionSystem.
> > > > 
> > > > That would let you delete AppSorting::SetExtensionScopedPrefs, and
instead
> > > > ChromeAppSorting could just retrieve the ExtensionPrefs instance from
the
> > > > BrowserContext it's given.
> > > > 
> > > > It would also let you simply delete
> > ExtensionsBrowserClient::CreateAppSorting
> > > > rather than updating it.
> > > 
> > > I started on this, but then realized it creates a dependency cycle:
> > > ExtensionSystem depends on ExtensionPrefs, but ExtensionPrefs needs an
> > > AppSorting and so now will depend on ExtensionSystem. That could probably
be
> > > worked around with an ExtensionPrefs::set_app_sorting or something like
> that,
> > > but then we won't really win much by the whole thing. WDYT?
> > 
> > That's pretty typical, usually all these things depend on in their
constructor
> > is a Profile/BrowserContext. They fetch the ExtensionPrefs/AppSorting from
> that,
> > sometimes via an ExtensionSystem, when needed.
> 
> I was talking about "DependsOn" in the KeyedService sense, i.e.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> In the ExtensionPrefs constructor, the ExtensionSystem doesn't actually exist
> yet.

If it's a runtime rather than construction/destruction impl I don't think you
need the DependsOn call, so in this case, I don't think ExtensionPrefs would
need to depend on AppSorting.

I think of it as though every KeyedService is owned by a BrowserContext, and is
constructed with a BrowserContext. At runtime it can access whatever properties
it wants off BrowserContext, however on construction BrowserContext needs to
know which objects to construct first.

[Marc Treib, 2015-07-27 08:02:06]

On 2015/07/24 16:20:05, kalman wrote:
> On 2015/07/24 14:51:02, Marc Treib wrote:
> > On 2015/07/24 14:43:54, kalman wrote:
> > >
> >
>
https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
> > > File extensions/browser/extensions_browser_client.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1251323003/diff/20001/extensions/browser/exte...
> > > extensions/browser/extensions_browser_client.h:164: virtual
> > > scoped_ptr<AppSorting> CreateAppSorting(
> > > On 2015/07/24 11:38:34, Marc Treib wrote:
> > > > On 2015/07/23 15:35:11, kalman wrote:
> > > > > Even more cleanup:
> > > > > 
> > > > > This stuff would all be much simpler if there were an "app_sorting()"
> > method
> > > > on
> > > > > ExtensionSystem, which ExtensionSystemImpl overrides to return a
> > > > > ChromeAppSorting, and everything else returns a NullAppSorting/nullptr
> > (and
> > > > why
> > > > > are both acceptable? eh whatever).
> > > > > 
> > > > > I.e. shift ownership of AppSorting from ExtensionPrefs to
> ExtensionSystem.
> > > > > 
> > > > > That would let you delete AppSorting::SetExtensionScopedPrefs, and
> instead
> > > > > ChromeAppSorting could just retrieve the ExtensionPrefs instance from
> the
> > > > > BrowserContext it's given.
> > > > > 
> > > > > It would also let you simply delete
> > > ExtensionsBrowserClient::CreateAppSorting
> > > > > rather than updating it.
> > > > 
> > > > I started on this, but then realized it creates a dependency cycle:
> > > > ExtensionSystem depends on ExtensionPrefs, but ExtensionPrefs needs an
> > > > AppSorting and so now will depend on ExtensionSystem. That could
probably
> be
> > > > worked around with an ExtensionPrefs::set_app_sorting or something like
> > that,
> > > > but then we won't really win much by the whole thing. WDYT?
> > > 
> > > That's pretty typical, usually all these things depend on in their
> constructor
> > > is a Profile/BrowserContext. They fetch the ExtensionPrefs/AppSorting from
> > that,
> > > sometimes via an ExtensionSystem, when needed.
> > 
> > I was talking about "DependsOn" in the KeyedService sense, i.e.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > In the ExtensionPrefs constructor, the ExtensionSystem doesn't actually
exist
> > yet.
> 
> If it's a runtime rather than construction/destruction impl I don't think you
> need the DependsOn call, so in this case, I don't think ExtensionPrefs would
> need to depend on AppSorting.
> 
> I think of it as though every KeyedService is owned by a BrowserContext, and
is
> constructed with a BrowserContext. At runtime it can access whatever
properties
> it wants off BrowserContext, however on construction BrowserContext needs to
> know which objects to construct first.

Alright. I'm gonna submit this CL as-is after all, and make a followup for the
ownership transfer. I think that'll make it easier to discuss.
To be continued...

[Marc Treib, 2015-07-27 08:11:45]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-07-27 08:11:45]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1251323003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-27 08:11:52]

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

[commit-bot: I haz the power, 2015-07-27 08:53:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-27 08:54:13]

Patchset 4 (id:??) landed as
https://crrev.com/11ce5d50e71686e2335ede01ca54a902175ca07c
Cr-Commit-Position: refs/heads/master@{#340440}