Ensure the ExtensionSystem is ready before initializing an ExtensionAppModelBuilder

Ensure the ExtensionSystem is ready before initializing an ExtensionAppModelBuilder

Some code paths currently result in an AppListSyncableService being
created for a profile before the profile is fully initialized. In this
case, the ExtensionSystem can be used, but the Extension*Service* is not
yet created, and has not finished loading all the extensions. Attempting
to populate the app launcher model at this point results in no apps
being found.

This CL adds a check in AppListSyncableService to see if the
ExtensionSystem is ready. If not, wait for it to become ready and
populate apps then.

BUG=321514
TEST=(On mac)Have two signed-in profiles; Have app launcher showing
Profile "A", Close all browser windows except for a browser showing
profile "B", Quit Chrome, Show app launcher [Chrome starts], Switch app
launcher to profile "B". App launcher should show apps, and not be
blank.

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

[tapted, 2013-11-20 11:03:25]

Steven could you take a first look?

But if this looks the wrong direction you should probably take the bug off me,
since you have a better idea where AppListSyncableService is headed.

Got this going just before leaving work, so there are some things (noted) to do
once I'm back at my desk in the morning. But it looks like the bots are happy.

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_unittest.h (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.h:49: static void
InitializeExtensionServiceForProfile(
should return ExtensionService*

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.h:80: base::ScopedTempDir*
temp_dir_);
temp_dir_ shouldn't have a trailing underscore

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:164: DCHECK(service);
maybe CHECK() - opinions?

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:32:
DependsOn(extensions::ExtensionSystemFactory::GetInstance());
note: This wasn't needed as part of the fix, but I think it belongs.

https://codereview.chromium.org/77773002/diff/170001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:2786:
'browser/ui/app_list/extension_app_model_builder_unittest.cc',
This was just so I could test on my mac - I don't think it will expose any new
codepaths. But we should do this for more things in a follow-up.

[stevenjb, 2013-11-20 19:41:14]

This generally lgtm with one suggestion and one note. Thanks for fixing this.

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.cc:491:
scoped_ptr<TestingProfile>* profile_ptr,
This is pretty confusing. Use of scoped_ptr<>& and *& below is especially
confusing. I think we should break this up into a function that creates the
profile and returns a scoped_ptr<TestingProfile>, and a function that creates,
initializes, and returns the ExtensionService, i.e. something like:

profile_ = CreateTestingProfile(params);
service_ = InitializeExtensionServiceForProfile(params, profile_.get());

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:166: return;
We shouldn't have both {D}CHECK(service) and a !service test. {D}CHECK indicates
a code path that should never happen.

I assume this was just added for testing and should be removed; there may be
tests where service is NULL? If not, the early exit should be removed (and the
code will crash immediately below if service is NULL).

[benwells, 2013-11-21 00:47:47]

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.cc:491:
scoped_ptr<TestingProfile>* profile_ptr,
On 2013/11/20 19:41:15, stevenjb wrote:
> This is pretty confusing. Use of scoped_ptr<>& and *& below is especially
> confusing. I think we should break this up into a function that creates the
> profile and returns a scoped_ptr<TestingProfile>, and a function that creates,
> initializes, and returns the ExtensionService, i.e. something like:
> 
> profile_ = CreateTestingProfile(params);
> service_ = InitializeExtensionServiceForProfile(params, profile_.get());

Yep, seems like you could encapsulate the bits you need in a more clean way.

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.cc:526: service =
system->CreateExtensionService(
huh? Why so many *'s and &'s??
more importantly, why assign twice in a row?

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_unittest.h (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.h:49: static void
InitializeExtensionServiceForProfile(
On 2013/11/20 11:03:25, tapted wrote:
> should return ExtensionService*

yes :)

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:23:
extensions::ExtensionSystem::Get(profile)->extension_service();
I prefer to pass this into the constructor, and have the factory obtain it. This
makes it clearer in the factory why the dependson is there, and means you can do
fun stuff in tests like pass in a test service.

[tapted, 2013-11-21 03:29:55]

PTAL

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.cc:491:
scoped_ptr<TestingProfile>* profile_ptr,
On 2013/11/21 00:47:47, benwells wrote:
> On 2013/11/20 19:41:15, stevenjb wrote:
> > This is pretty confusing. Use of scoped_ptr<>& and *& below is especially
> > confusing. I think we should break this up into a function that creates the
> > profile and returns a scoped_ptr<TestingProfile>, and a function that
creates,
> > initializes, and returns the ExtensionService, i.e. something like:
> > 
> > profile_ = CreateTestingProfile(params);
> > service_ = InitializeExtensionServiceForProfile(params, profile_.get());
> 
> Yep, seems like you could encapsulate the bits you need in a more clean way.

Done.

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.cc:526: service =
system->CreateExtensionService(
On 2013/11/21 00:47:47, benwells wrote:
> huh? Why so many *'s and &'s??
> more importantly, why assign twice in a row?

All fixed :) - at this stage I was still aiming on getting bots to report in..

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
File chrome/browser/extensions/extension_service_unittest.h (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.h:49: static void
InitializeExtensionServiceForProfile(
On 2013/11/21 00:47:47, benwells wrote:
> On 2013/11/20 11:03:25, tapted wrote:
> > should return ExtensionService*
> 
> yes :)

Done.

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.h:80: base::ScopedTempDir*
temp_dir_);
On 2013/11/20 11:03:25, tapted wrote:
> temp_dir_ shouldn't have a trailing underscore

Done.

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:166: return;
On 2013/11/20 19:41:15, stevenjb wrote:
> We shouldn't have both {D}CHECK(service) and a !service test. {D}CHECK
indicates
> a code path that should never happen.
> 
> I assume this was just added for testing and should be removed; there may be
> tests where service is NULL? If not, the early exit should be removed (and the
> code will crash immediately below if service is NULL).

Yep - bot runs suggest that the separate !service check is no longer necessary
for tests, with the other changes in this CL. Now just DCHECK (Done.)

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:23:
extensions::ExtensionSystem::Get(profile)->extension_service();
On 2013/11/21 00:47:47, benwells wrote:
> I prefer to pass this into the constructor, and have the factory obtain it.
This
> makes it clearer in the factory why the dependson is there, and means you can
do
> fun stuff in tests like pass in a test service.

Done modulo ExtensionSystem/ExtensionsService. (aside: anyone else thinking
those names are backward? Maybe we should rename ExtensionService ->
`ProfileExtensionSet` or `ExtensionSetManager`)

[stevenjb, 2013-11-21 06:34:11]

Much cleaner, thanks. Still lgtm.

[benwells, 2013-11-21 09:48:11]

lgtm

About naming - yeah it's a bit whacky. Plan is for ExtensionSystem to dissipate
- it is mostly a collection of services - and for ExtensionService to end up as
a one or more simpler things like ExtensionRegistry.

https://codereview.chromium.org/77773002/diff/400002/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/77773002/diff/400002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:21:
extensions::ExtensionSystem* extension_system)
nit: can you make this an ExtensionService? ExtensionSystem should die one day
if we're lucky.

[tapted, 2013-11-21 10:39:03]

https://codereview.chromium.org/77773002/diff/400002/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/77773002/diff/400002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:21:
extensions::ExtensionSystem* extension_system)
On 2013/11/21 09:48:11, benwells wrote:
> nit: can you make this an ExtensionService? ExtensionSystem should die one day
> if we're lucky.

Done. (I felt too evil passing in something that could be NULL, so ++comments).
Being NULL, it also means we can't rely on extension_service->profile(), but
_system didn't have that anyway.

[commit-bot: I haz the power, 2013-11-21 10:40:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/77773002/530001

[commit-bot: I haz the power, 2013-11-21 13:05:27]

Change committed as 236513