Move ownership of AppSorting from ExtensionPrefs to ExtensionSystem

Move ownership of AppSorting from ExtensionPrefs to ExtensionSystem

BUG=None

Committed: https://crrev.com/926ee2dccf73a971e1f78e80d09231f8ba19aff3
Cr-Commit-Position: refs/heads/master@{#342093}

[Marc Treib, 2015-07-29 09:53:32]

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

[Marc Treib, 2015-07-29 09:53:33]

So, I tried moving the ownership of AppSorting to the ExtensionSystem, but now I
think that AppSorting should become a KeyedService rather than being owned by
the ExtensionSystem.
Some specific comments/questions below. PTAL

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

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:210:
ExtensionPrefs::Get(profile_)->set_app_sorting(app_sorting_.get());
This is super ugly :(  But when the ExtensionPrefs is constructed, the
AppSorting doesn't exist yet.
Also a whole bunch of tests fail, basically everything that uses an
ExtensionService without setting up the full ExtensionSystem, because the
AppSorting is null. I think if we wanna go forward with this, then AppSorting
should become a KeyedService, so it'll just be constructed on demand. WDYT?

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

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/exte...
extensions/browser/extension_prefs.h:503: AppSorting* app_sorting() const {
return app_sorting_; }
This accessor should be removed, and all callers changed to call the
ExtensionSystem instead (or a new AppSortingFactory, if we decide to go that
way).

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/exte...
File extensions/browser/extension_prefs_factory.cc (right):

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/exte...
extensions/browser/extension_prefs_factory.cc:53:
ExtensionPrefValueMapFactory::GetForBrowserContext(context),
It'd be nice if we could just pass in the AppSorting here, but it doesn't exist
yet at this point. The AppSortingFactory (which this would then DependsOn())
would solve that.

[not at google - send to devlin, 2015-07-29 18:22:03]

Thanks for doing this.

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

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:97: if
(ExtensionPrefs::Get(browser_context_)->ReadPrefAsInteger(
I would hold onto a reference to this at the start of MigrateAppIndex or
something, it's used several times.

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:404:
ExtensionPrefs::Get(browser_context_)->UpdateExtensionPref(
Here as well.

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

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.h:30: void Initialize(const
extensions::ExtensionIdList& extension_ids) override;
I'd like to take this off the public interface, and have ChromeAppSorting handle
this itself.

For example, you could the strategy that ExtensionSystem::ready() uses and add
an ExtensionPrefs::ready().

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

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:210:
ExtensionPrefs::Get(profile_)->set_app_sorting(app_sorting_.get());
On 2015/07/29 09:53:32, Marc Treib wrote:
> This is super ugly :(  But when the ExtensionPrefs is constructed, the
> AppSorting doesn't exist yet.
> Also a whole bunch of tests fail, basically everything that uses an
> ExtensionService without setting up the full ExtensionSystem, because the
> AppSorting is null. I think if we wanna go forward with this, then AppSorting
> should become a KeyedService, so it'll just be constructed on demand. WDYT?

I don't think making something a KeyedService is the best way to solve that.
Could you make TestExtensionSystem return a NullAppSorting?

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window_geometry_cache_unittest.cc
(right):

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window_geometry_cache_unittest.cc:101:
extension_prefs_->set_app_sorting(app_sorting.get());
... would making TestExtensionSystem use a NullAppSorting solve all of these,
and let you remove set_app_sorting?

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

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/exte...
extensions/browser/extension_prefs.h:503: AppSorting* app_sorting() const {
return app_sorting_; }
On 2015/07/29 09:53:32, Marc Treib wrote:
> This accessor should be removed, and all callers changed to call the
> ExtensionSystem instead (or a new AppSortingFactory, if we decide to go that
> way).

SGTM.

[Marc Treib, 2015-07-30 13:58:54]

Patchset #3 (id:40001) has been deleted

[Marc Treib, 2015-07-30 14:04:12]

Next iteration, PTAL.
Some cleanups remain to be done (removing AppSorting::Initialize and
ExtensionPrefs::app_sorting). I'll get to those when everything else is looking
good.

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

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:97: if
(ExtensionPrefs::Get(browser_context_)->ReadPrefAsInteger(
On 2015/07/29 18:22:03, kalman wrote:
> I would hold onto a reference to this at the start of MigrateAppIndex or
> something, it's used several times.

Done.

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.cc:404:
ExtensionPrefs::Get(browser_context_)->UpdateExtensionPref(
On 2015/07/29 18:22:03, kalman wrote:
> Here as well.

Done.

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

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_app_sorting.h:30: void Initialize(const
extensions::ExtensionIdList& extension_ids) override;
On 2015/07/29 18:22:03, kalman wrote:
> I'd like to take this off the public interface, and have ChromeAppSorting
handle
> this itself.

Agreed. I'll do this when the remaining stuff is sorted out.

> For example, you could the strategy that ExtensionSystem::ready() uses and add
> an ExtensionPrefs::ready().

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

https://codereview.chromium.org/1254363004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:210:
ExtensionPrefs::Get(profile_)->set_app_sorting(app_sorting_.get());
On 2015/07/29 18:22:03, kalman wrote:
> On 2015/07/29 09:53:32, Marc Treib wrote:
> > This is super ugly :(  But when the ExtensionPrefs is constructed, the
> > AppSorting doesn't exist yet.
> > Also a whole bunch of tests fail, basically everything that uses an
> > ExtensionService without setting up the full ExtensionSystem, because the
> > AppSorting is null. I think if we wanna go forward with this, then
AppSorting
> > should become a KeyedService, so it'll just be constructed on demand. WDYT?
> 
> I don't think making something a KeyedService is the best way to solve that.
> Could you make TestExtensionSystem return a NullAppSorting?

Done. Together with an set_app_sorting call, this seems to fix all the
"unrelated" test failures.

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window_geometry_cache_unittest.cc
(right):

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window_geometry_cache_unittest.cc:101:
extension_prefs_->set_app_sorting(app_sorting.get());
On 2015/07/29 18:22:03, kalman wrote:
> ... would making TestExtensionSystem use a NullAppSorting solve all of these,
> and let you remove set_app_sorting?

..and instead have ExtensionPrefs use
|ExtensionSystem::Get(context)->app_sorting()| instead to get the AppSorting?
Probably possible, yes, but not trivial - right now ExtensionPrefs doesn't have
a BrowserContext, and giving it one is non-trivial, since not all unit tests
even set up a profile.

https://codereview.chromium.org/1254363004/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1254363004/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:378: return g_instance_for_testing;
This is super-hacky. We should really go through the ExtensionPrefsFactory,
which already has a SetInstanceForTesting method, but that doesn't work without
a BrowserContext :(

[not at google - send to devlin, 2015-07-30 18:21:00]

This lgtm, the part about ExtensionPrefs being explicitly bound to a
BrowserContext would clean up the final rough edges here. Did you want to do
that as part of this patch?

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window_geometry_cache_unittest.cc
(right):

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window_geometry_cache_unittest.cc:101:
extension_prefs_->set_app_sorting(app_sorting.get());
On 2015/07/30 14:04:12, Marc Treib wrote:
> On 2015/07/29 18:22:03, kalman wrote:
> > ... would making TestExtensionSystem use a NullAppSorting solve all of
these,
> > and let you remove set_app_sorting?
> 
> ..and instead have ExtensionPrefs use
> |ExtensionSystem::Get(context)->app_sorting()| instead to get the AppSorting?
> Probably possible, yes, but not trivial - right now ExtensionPrefs doesn't
have
> a BrowserContext, and giving it one is non-trivial, since not all unit tests
> even set up a profile.

Ah yeah. I saw ExtensionPrefs::Get(BrowserContext*) and figured there was
already a dependency. That is the function that everybody does call, so plugging
it through to actually a member variable of ExtensionPrefs is conceptually
straightforward?

Side note, why does ExtensionPrefs extend KeyedService?
BrowserContextKeyedService I could understand, but what is the point in
extending just KeyedService.

[Marc Treib, 2015-07-31 08:17:49]

Argh, turns out I didn't fix all the tests yet, after all :-/
I'll take a look again later when I find the time.

I'd still like to pull AppSorting::Initialize into the ctor before submitting.
Everything else I'd probably defer to another CL.

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window_geometry_cache_unittest.cc
(right):

https://codereview.chromium.org/1254363004/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window_geometry_cache_unittest.cc:101:
extension_prefs_->set_app_sorting(app_sorting.get());
On 2015/07/30 18:21:00, kalman wrote:
> On 2015/07/30 14:04:12, Marc Treib wrote:
> > On 2015/07/29 18:22:03, kalman wrote:
> > > ... would making TestExtensionSystem use a NullAppSorting solve all of
> these,
> > > and let you remove set_app_sorting?
> > 
> > ..and instead have ExtensionPrefs use
> > |ExtensionSystem::Get(context)->app_sorting()| instead to get the
AppSorting?
> > Probably possible, yes, but not trivial - right now ExtensionPrefs doesn't
> have
> > a BrowserContext, and giving it one is non-trivial, since not all unit tests
> > even set up a profile.
> 
> Ah yeah. I saw ExtensionPrefs::Get(BrowserContext*) and figured there was
> already a dependency. That is the function that everybody does call, so
plugging
> it through to actually a member variable of ExtensionPrefs is conceptually
> straightforward?

Yes, the problem is just with tests that use an ExtensionPrefs, but not a
profile. Those will need special massaging to make this work.

> Side note, why does ExtensionPrefs extend KeyedService?
> BrowserContextKeyedService I could understand, but what is the point in
> extending just KeyedService.

BrowserContextKeyedService doesn't exist (anymore), I guess it's just been
renamed to KeyedService since it doesn't really relate to a BrowserContext?
BrowserContextKeyedService*Factory* exists though, and returns a KeyedService,
see
https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...

[Marc Treib, 2015-07-31 13:38:28]

Patchset #4 (id:80001) has been deleted

[Marc Treib, 2015-08-03 09:23:44]

Patchset #6 (id:90033) has been deleted

[Marc Treib, 2015-08-03 09:23:55]

Patchset #5 (id:90032) has been deleted

[Marc Treib, 2015-08-03 15:28:04]

Okay, looks like I've *finally* managed to fix all the tests again. I've also
done most of the remaining cleanup: AppSorting::Initialize is gone, and so is
ExtensionPrefs::set_app_sorting (ExtensionPrefs gets an explicit BrowserContext
instead, and gets its AppSorting through the ExtensionSystem).

I've had to change quite a lot of test setup code, so this probably deserves
another close look.

https://codereview.chromium.org/1254363004/diff/130001/chrome/browser/extensi...
File chrome/browser/extensions/test_extension_system.cc (right):

https://codereview.chromium.org/1254363004/diff/130001/chrome/browser/extensi...
chrome/browser/extensions/test_extension_system.cc:37: app_sorting_(new
ChromeAppSorting(profile_)) {
Turns out a bunch of tests depend on this returning an actual ChromeAppSorting
rather than a NullAppSorting.

https://codereview.chromium.org/1254363004/diff/130001/chrome/test/base/testi...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/1254363004/diff/130001/chrome/test/base/testi...
chrome/test/base/testing_profile.cc:437: // Note that the GetPrefs() creates a
TestingPrefService, therefore
This has moved from TestExtensionSystem to here. That's necessary to get the
order of construction right: The ExtensionPrefs must be constructed before the
ExtensionSystem (in particular, before the AppSorting).

[not at google - send to devlin, 2015-08-03 21:12:08]

lgtm. The main comment to look for there is the one about "This seems to be the
only case where ExtensionPrefs is created with a null BrowserContext".

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/chrome_extensions_browser_client.cc:18: #include
"chrome/browser/extensions/chrome_app_sorting.h"
You can remove this #include.

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/extension_system_impl.h:21: class
StateStoreNotificationObserver;
Unrelated cleanup: of these, ContentVerifier and SharedUserScriptMaster don't
need to be forward-declared, because they're part of an override.

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/test_extension_prefs.cc:85: const ExtensionPrefs&
TestExtensionPrefs::const_prefs() const {
Is there even any point in this const version? When I grep for "const
TestExtensionPrefs" nothing shows up.

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/test_extension_system.h:31: class
StandardManagementPolicyProvider;
Cleanup: InstallVerifier and StandardManagementPolicyProvider aren't used, and
RuntimeData and SharedUserScriptMaster are unnecessary because they're
overrides.

https://codereview.chromium.org/1254363004/diff/170001/chrome/test/base/testi...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/test/base/testi...
chrome/test/base/testing_profile.cc:443: scoped_ptr<extensions::ExtensionPrefs>
extension_prefs =
btw "extension_prefs(ExtensionPrefs::Create))" is more terse than
"extension_prefs = make_scoped_ptr(ExtensionPrefs::Create)" though not a big
deal.

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/app...
File extensions/browser/app_window/app_window_geometry_cache_unittest.cc
(right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/app...
extensions/browser/app_window/app_window_geometry_cache_unittest.cc:94: nullptr,
This seems to be the only case where ExtensionPrefs is created with a null
BrowserContext; and why does it need to be? You already have access to a
browser_context(), is there a reason you can't pass that through? If you do that
you can get rid of the null checks and the comments explaining the null
checks...

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/ext...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/ext...
extensions/browser/extension_prefs.cc:1199: if (app_sorting())
Add "// null in tests" after this?

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/ext...
extensions/browser/extension_prefs.cc:2109: if (needs_sort_ordinal &&
app_sorting())
another case of null-in-tests?

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/tes...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/tes...
extensions/browser/test_extensions_browser_client.cc:8: #include
"extensions/browser/app_sorting.h"
You can remove this #include.

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
File extensions/shell/browser/shell_extension_system.h (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
extensions/shell/browser/shell_extension_system.h:30: class
SharedUserScriptMaster;
Cleanup: RendererStartupHelper and ProcessManager are never used, and InfoMap
and SharedUserScriptMaster aren't needed because they're overrides.

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
File extensions/shell/browser/shell_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
extensions/shell/browser/shell_extensions_browser_client.cc:12: #include
"extensions/browser/app_sorting.h"
You can remove this #include.

[Marc Treib, 2015-08-04 08:36:29]

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/chrome_extensions_browser_client.cc:18: #include
"chrome/browser/extensions/chrome_app_sorting.h"
On 2015/08/03 21:12:08, kalman wrote:
> You can remove this #include.

Done.

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/extension_system_impl.h:21: class
StateStoreNotificationObserver;
On 2015/08/03 21:12:08, kalman wrote:
> Unrelated cleanup: of these, ContentVerifier and SharedUserScriptMaster don't
> need to be forward-declared, because they're part of an override.

Done.

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/test_extension_prefs.cc:85: const ExtensionPrefs&
TestExtensionPrefs::const_prefs() const {
On 2015/08/03 21:12:08, kalman wrote:
> Is there even any point in this const version? When I grep for "const
> TestExtensionPrefs" nothing shows up.

I hadn't checked. You're right, it's not used anywhere. Removed it.

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/browser/extensi...
chrome/browser/extensions/test_extension_system.h:31: class
StandardManagementPolicyProvider;
On 2015/08/03 21:12:08, kalman wrote:
> Cleanup: InstallVerifier and StandardManagementPolicyProvider aren't used, and
> RuntimeData and SharedUserScriptMaster are unnecessary because they're
> overrides.

And ExtensionPrefs also isn't used :)
Done.

https://codereview.chromium.org/1254363004/diff/170001/chrome/test/base/testi...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/chrome/test/base/testi...
chrome/test/base/testing_profile.cc:443: scoped_ptr<extensions::ExtensionPrefs>
extension_prefs =
On 2015/08/03 21:12:08, kalman wrote:
> btw "extension_prefs(ExtensionPrefs::Create))" is more terse than
> "extension_prefs = make_scoped_ptr(ExtensionPrefs::Create)" though not a big
> deal.

Done.

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/app...
File extensions/browser/app_window/app_window_geometry_cache_unittest.cc
(right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/app...
extensions/browser/app_window/app_window_geometry_cache_unittest.cc:94: nullptr,
On 2015/08/03 21:12:08, kalman wrote:
> This seems to be the only case where ExtensionPrefs is created with a null
> BrowserContext; and why does it need to be? You already have access to a
> browser_context(), is there a reason you can't pass that through? If you do
that
> you can get rid of the null checks and the comments explaining the null
> checks...

D'oh, somehow I didn't see the |browser_context()| call right below here.
Passing it through now, and removed the null checks.

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/ext...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/ext...
extensions/browser/extension_prefs.cc:1199: if (app_sorting())
On 2015/08/03 21:12:08, kalman wrote:
> Add "// null in tests" after this?

Not necessary anymore :)

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/ext...
extensions/browser/extension_prefs.cc:2109: if (needs_sort_ordinal &&
app_sorting())
On 2015/08/03 21:12:08, kalman wrote:
> another case of null-in-tests?

Also not necessary anymore.

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/tes...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/browser/tes...
extensions/browser/test_extensions_browser_client.cc:8: #include
"extensions/browser/app_sorting.h"
On 2015/08/03 21:12:08, kalman wrote:
> You can remove this #include.

Done.

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
File extensions/shell/browser/shell_extension_system.h (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
extensions/shell/browser/shell_extension_system.h:30: class
SharedUserScriptMaster;
On 2015/08/03 21:12:08, kalman wrote:
> Cleanup: RendererStartupHelper and ProcessManager are never used, and InfoMap
> and SharedUserScriptMaster aren't needed because they're overrides.

Done.

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
File extensions/shell/browser/shell_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254363004/diff/170001/extensions/shell/brows...
extensions/shell/browser/shell_extensions_browser_client.cc:12: #include
"extensions/browser/app_sorting.h"
On 2015/08/03 21:12:08, kalman wrote:
> You can remove this #include.

Done.

[Marc Treib, 2015-08-04 09:13:02]

+mgiuca FYI: This CL removes some CHECKs you had added for crbug.com/476648
(they've become obsolete with this change). From looking at the bug though, it
looks like it has been resolved anyway.

[Marc Treib, 2015-08-04 09:14:46]

treib@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Marc Treib, 2015-08-04 09:14:46]

+phajdan.jr: Can I get an OWNERS review for testing_profile.cc please? Had to
move some code out of test_extension_system.cc in there.

[Matt Giuca, 2015-08-05 00:24:34]

> +mgiuca FYI: This CL removes some CHECKs you had added for crbug.com/476648
> (they've become obsolete with this change). From looking at the bug though, it
> looks like it has been resolved anyway.

sgtm

[Paweł Hajdan Jr., 2015-08-05 19:08:00]

chrome/test LGTM

[Marc Treib, 2015-08-06 07:37:21]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-08-06 07:37:22]

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/1254363004/#ps190001
(title: "review")

[commit-bot: I haz the power, 2015-08-06 07:37:36]

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

[commit-bot: I haz the power, 2015-08-06 08:07:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 08:07:17]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Marc Treib, 2015-08-06 09:20:45]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-08-06 09:20:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1254363004/#ps210001
(title: "add missing include")

[commit-bot: I haz the power, 2015-08-06 09:20:54]

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

[commit-bot: I haz the power, 2015-08-06 10:55:51]

Committed patchset #9 (id:210001)

[commit-bot: I haz the power, 2015-08-06 10:56:34]

Patchset 9 (id:??) landed as
https://crrev.com/926ee2dccf73a971e1f78e80d09231f8ba19aff3
Cr-Commit-Position: refs/heads/master@{#342093}