Sync removal of Default apps.

Sync removal of Default apps.

This CL implements TYPE_REMOVE_DEFAULT_APP. It also includes a fair bit
of cleanup of AppListSyncableService, which on the one hand I apologize
for, but on the other hand I think the result is more straightforward.

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

[xiyuan, 2013-12-19 00:28:33]

LGTM

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.cc:462:
model_->item_list()->DeleteItem(sync_item->item_id);
Do we need to explicitly delete here? The ExtensionAppModelBuilder should do
this when the app/extension is uninstalled. Right?

[stevenjb, 2013-12-19 01:12:01]

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.cc:462:
model_->item_list()->DeleteItem(sync_item->item_id);
On 2013/12/19 00:28:34, xiyuan wrote:
> Do we need to explicitly delete here? The ExtensionAppModelBuilder should do
> this when the app/extension is uninstalled. Right?
Good point. Probably cleaner not to remove this from the model here.
Done.

[tapted, 2013-12-19 01:20:57]

https://codereview.chromium.org/118463002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc
(right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:45: 
nit: remove extra blank line

https://codereview.chromium.org/118463002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:427: int
default_app_index = i;
`i` seems a bit out of place, outside of a loop. Since it's not used later, I'd
maybe just use literals; 0 above, and `1` here, and mention in the comment above
(e.g. append ", after the non-default app").

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.cc:193: if (item_type ==
ExtensionAppItem::kAppType) {
I think this is comparing pointers - maybe use std::string for item_type?

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.h:23: class
ExtensionService;
nit: this foward-dec probably isn't needed any longer

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:8: #include
"chrome/browser/extensions/extension_system.h"
This header might not be needed, if we go with the change below.

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:43: return new
AppListSyncableService(profile);
Is there a secondary reason for this change, apart from style? I had this
originally at
https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...,
and benwells@ suggested it was clearer to feed in the ExtensionService
dependency here explicitly.

[stevenjb, 2013-12-19 01:48:44]

https://codereview.chromium.org/118463002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc
(right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:45: 
On 2013/12/19 01:20:57, tapted wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/118463002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:427: int
default_app_index = i;
On 2013/12/19 01:20:57, tapted wrote:
> `i` seems a bit out of place, outside of a loop. Since it's not used later,
I'd
> maybe just use literals; 0 above, and `1` here, and mention in the comment
above
> (e.g. append ", after the non-default app").

Done.

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.cc:193: if (item_type ==
ExtensionAppItem::kAppType) {
On 2013/12/19 01:20:57, tapted wrote:
> I think this is comparing pointers - maybe use std::string for item_type?

It's actually intended to compare pointers, see declaration of GetAppType() in
app_list_item{_model}.h

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service.h:23: class
ExtensionService;
On 2013/12/19 01:20:57, tapted wrote:
> nit: this foward-dec probably isn't needed any longer

Done.

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:43: return new
AppListSyncableService(profile);
On 2013/12/19 01:20:57, tapted wrote:
> Is there a secondary reason for this change, apart from style? I had this
> originally at
>
https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...,
> and benwells@ suggested it was clearer to feed in the ExtensionService
> dependency here explicitly.

Hmm, I guess that's a fair point. I did this so that we weren't doing the lookup
in multiple places. Instead I'll store extension_service_ as a member of
AppListSyncableService, independent of profile_.

[tapted, 2013-12-19 02:18:01]

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/118463002/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:43: return new
AppListSyncableService(profile);
On 2013/12/19 01:48:44, stevenjb wrote:
> On 2013/12/19 01:20:57, tapted wrote:
> > Is there a secondary reason for this change, apart from style? I had this
> > originally at
> >
>
https://codereview.chromium.org/77773002/diff/170001/chrome/browser/ui/app_li...,
> > and benwells@ suggested it was clearer to feed in the ExtensionService
> > dependency here explicitly.
> 
> Hmm, I guess that's a fair point. I did this so that we weren't doing the
lookup
> in multiple places. Instead I'll store extension_service_ as a member of
> AppListSyncableService, independent of profile_.

Hm. Caching the extension service data member/cache scares me a bit, but it does
seem silly to fetch it in both places. TBH, I think I like the earlier patchset
better (sorry!). I think having to now use the ExtensionService in multiple
places in AppListSyncableService is perhaps reason enough not to pass it in
here.

We could also still pass it in here, to be explicit. But either way, I think I
prefer the anonymous GetExtensionService(Profile*) function over the
ExtensionService* data member.

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

https://codereview.chromium.org/118463002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:167: BuildModel();
If you go with the cached |extension_service|, I think you'll need a to re-fetch
it here, now we know it's non-NULL with
extensions::ExtensionSystem::Get(profile)->extension_service();

[stevenjb, 2013-12-19 18:13:03]

The reason I decided to cache the ExtensionService* is that it makes it clear
that this is a service that should never change after construction, same as
Profile. Caching it makes that more clear. Since we have the DependsOn()
statement in AppListSyncableServiceFactory, this relationship makes the most
sense to me.

Alternately we could probably remove that DependsOn statement and rely on the
NOTIFICATION_EXTENSIONS_READY notification to inform AppListSyncableService both
that ExtensionService exists and extensions are loaded (instead of just the
later), but I think that is more likely to have subtle edge cases than the
explicit dependency.

Another reason to go this route is that I was talking with jamescook@ and we may
be doing some re-factoring of ExtensionService that would allow
AppListSyncableService to depend on ExtensionServiceInterface (or a better named
interface) instead, reducing the src/chrome/ dependencies (ideally this class
could be moved to src/ui/app_list once enough of the extensions code is moved to
src/extensions).

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

https://codereview.chromium.org/118463002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:167: BuildModel();
On 2013/12/19 02:18:02, tapted wrote:
> If you go with the cached |extension_service|, I think you'll need a to
re-fetch
> it here, now we know it's non-NULL with
> extensions::ExtensionSystem::Get(profile)->extension_service();

I changed the constructor to call BuildModel() directly if extension_service is
NULL. See other note.

[tapted, 2013-12-20 02:18:16]

On 2013/12/19 18:13:03, stevenjb wrote:
> The reason I decided to cache the ExtensionService* is that it makes it clear
> that this is a service that should never change after construction, same as
> Profile. Caching it makes that more clear. Since we have the DependsOn()
> statement in AppListSyncableServiceFactory, this relationship makes the most
> sense to me.

Yep - I agree. Unfortunately ExtensionService isn't profile keyed -- only
ExtensionSystem, so we can't depend on it in the same way. Yet, to show the app
launcher for a particular profile, we kinda need to depend on it. I think in the
current structure, it's not possible to have an AppListSyncableService that is
profile-keyed as well as "unchanging".

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

https://codereview.chromium.org/118463002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:167: BuildModel();
On 2013/12/19 18:13:04, stevenjb wrote:
> On 2013/12/19 02:18:02, tapted wrote:
> > If you go with the cached |extension_service|, I think you'll need a to
> re-fetch
> > it here, now we know it's non-NULL with
> > extensions::ExtensionSystem::Get(profile)->extension_service();
> 
> I changed the constructor to call BuildModel() directly if extension_service
is
> NULL. See other note.

I'm pretty sure that if extension_service is NULL, it will still be NULL by the
time it gets to:

BuildModel()
 +-> [non-sync] ExtensionAppModelBuilder::InitializeWithProfile(..) 
    +-> BuildModel()
      +-> PopulateApps()
        +-> AppListControllerDelegate::GetApps() {

  ExtensionService* service =
      extensions::ExtensionSystem::Get(profile)->extension_service();
  DCHECK(service);
  out_apps->InsertAll(*service->extensions());
  out_apps->InsertAll(*service->disabled_extensions());
  out_apps->InsertAll(*service->terminated_extensions());

}

So we will might need to move the NOTIFICATION_EXTENSIONS_READY observer to
AppListControllerDelegate to get this to work.

[tapted, 2013-12-20 02:29:06]

+benwells / -tapted

I'm officially on leave for two weeks, so I'm subbing in Ben ;). I'll probably
still be scanning email, but I don't want to block anything. Ben is pretty close
both to the app_list code, and (along with jamescook) the app_shell refactoring
efforts that are planned for ExtensionService.

And sorry for the timezone lag - my email triage was a bit special this morning.
Don't hesitate to ping me on IM if I'm green in some way and you're waiting for
something.

[Nicolas Zea, 2013-12-20 20:44:30]

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:216: // Otherwise,
delete the REMOVE_DEFAULT entry.
I'm not sure I understand the purpose of this. If you have a remove_default
entry, don't you need to keep it around for other clients? Why would you delete
it?

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:225:
sync_items_.erase(item_id);
should you return after this?

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:276: FROM_HERE,
syncer::SyncChangeList(1, sync_change));
Are you creating two incompatible changes for the same item here (a DELETE, then
an ADD)?

[stevenjb, 2013-12-20 20:56:06]

On 2013/12/20 02:18:16, tapted wrote:
> On 2013/12/19 18:13:03, stevenjb wrote:
> > The reason I decided to cache the ExtensionService* is that it makes it
clear
> > that this is a service that should never change after construction, same as
> > Profile. Caching it makes that more clear. Since we have the DependsOn()
> > statement in AppListSyncableServiceFactory, this relationship makes the most
> > sense to me.
> 
> Yep - I agree. Unfortunately ExtensionService isn't profile keyed -- only
> ExtensionSystem, so we can't depend on it in the same way. Yet, to show the
app
> launcher for a particular profile, we kinda need to depend on it. I think in
the
> current structure, it's not possible to have an AppListSyncableService that is
> profile-keyed as well as "unchanging".
> 

So, ExtensionService is owned by ExtensionSystem which is profile keyed.

I think the best way to make this all more clear is to pass ExtensionSystem to
AppListSyncableService() and store that. When and if we separate
ExtensionService
as a separate keyed service, we can pass that instead.

[stevenjb, 2013-12-20 21:05:55]

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

https://codereview.chromium.org/118463002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:167: BuildModel();
On 2013/12/20 02:18:17, tapted wrote:
> On 2013/12/19 18:13:04, stevenjb wrote:
> > On 2013/12/19 02:18:02, tapted wrote:
> > > If you go with the cached |extension_service|, I think you'll need a to
> > re-fetch
> > > it here, now we know it's non-NULL with
> > > extensions::ExtensionSystem::Get(profile)->extension_service();
> > 
> > I changed the constructor to call BuildModel() directly if extension_service
> is
> > NULL. See other note.
> 
> I'm pretty sure that if extension_service is NULL, it will still be NULL by
the
> time it gets to:
> 
> BuildModel()
>  +-> [non-sync] ExtensionAppModelBuilder::InitializeWithProfile(..) 
>     +-> BuildModel()
>       +-> PopulateApps()
>         +-> AppListControllerDelegate::GetApps() {
> 
>   ExtensionService* service =
>       extensions::ExtensionSystem::Get(profile)->extension_service();
>   DCHECK(service);
>   out_apps->InsertAll(*service->extensions());
>   out_apps->InsertAll(*service->disabled_extensions());
>   out_apps->InsertAll(*service->terminated_extensions());
> 
> }
> 
> So we will might need to move the NOTIFICATION_EXTENSIONS_READY observer to
> AppListControllerDelegate to get this to work.


Ha, good catch. Not much point in handling a NULL extension_service in one place
if it can't be NULL elsewhere. So we just won't call BuildModel if there is no
service - that could only happen in a test that isn't expecting the service to
do anything anyway. The result will be the same as before, just that instead of
waiting for a notification that is never going to happen we add a comment and
early exit in the constructor to be clear.

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:216: // Otherwise,
delete the REMOVE_DEFAULT entry.
On 2013/12/20 20:44:30, Nicolas Zea wrote:
> I'm not sure I understand the purpose of this. If you have a remove_default
> entry, don't you need to keep it around for other clients? Why would you
delete
> it?

I updated this comment and the comment above to clarify; this case is for when a
removed Default app is getting added as a non-Default app, i.e. installed from
the store.

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:225:
sync_items_.erase(item_id);
On 2013/12/20 20:44:30, Nicolas Zea wrote:
> should you return after this?
No, now we add the app as usual. Comment added.

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:276: FROM_HERE,
syncer::SyncChangeList(1, sync_change));
On 2013/12/20 20:44:30, Nicolas Zea wrote:
> Are you creating two incompatible changes for the same item here (a DELETE,
then
> an ADD)? 

If the app is a Default app, we will DELETE any APP entry, then *possibly* add a
REMOVE_DEFAULT entry. I believe the sync system may combine these to a single
UPDATE change, which we will handle correctly. Is that right, will that work?

[Nicolas Zea, 2013-12-30 19:48:59]

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:276: FROM_HERE,
syncer::SyncChangeList(1, sync_change));
On 2013/12/20 21:05:56, stevenjb wrote:
> On 2013/12/20 20:44:30, Nicolas Zea wrote:
> > Are you creating two incompatible changes for the same item here (a DELETE,
> then
> > an ADD)? 
> 
> If the app is a Default app, we will DELETE any APP entry, then *possibly* add
a
> REMOVE_DEFAULT entry. I believe the sync system may combine these to a single
> UPDATE change, which we will handle correctly. Is that right, will that work?

It should theoretically work, but it's a bit confusing to understand. I think
you can rewrite it differently though, by checking whether the app is default
first, and creating/updating as a remove default item, otherwise performing the
deletion.

https://codereview.chromium.org/118463002/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:215:
UninstallExtension(extension_system_->extension_service(), item_id);
Has the App already been installed at this point? It seems odd to go about the
install process unnecessarily if we can detect ahead of time that we're going to
have to uninstall it.

[stevenjb, 2014-01-02 21:15:48]

PTAL

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:276: FROM_HERE,
syncer::SyncChangeList(1, sync_change));
On 2013/12/30 19:49:00, Nicolas Zea wrote:
> On 2013/12/20 21:05:56, stevenjb wrote:
> > On 2013/12/20 20:44:30, Nicolas Zea wrote:
> > > Are you creating two incompatible changes for the same item here (a
DELETE,
> > then
> > > an ADD)? 
> > 
> > If the app is a Default app, we will DELETE any APP entry, then *possibly*
add
> a
> > REMOVE_DEFAULT entry. I believe the sync system may combine these to a
single
> > UPDATE change, which we will handle correctly. Is that right, will that
work?
> 
> It should theoretically work, but it's a bit confusing to understand. I think
> you can rewrite it differently though, by checking whether the app is default
> first, and creating/updating as a remove default item, otherwise performing
the
> deletion.

Fair enough. Done.

https://codereview.chromium.org/118463002/diff/330001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/118463002/diff/330001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:215:
UninstallExtension(extension_system_->extension_service(), item_id);
On 2013/12/30 19:49:00, Nicolas Zea wrote:
> Has the App already been installed at this point? It seems odd to go about the
> install process unnecessarily if we can detect ahead of time that we're going
to
> have to uninstall it.
Default apps are installed once on creation of a new Profile. This code is
currently independent of the AppList and of App sync in general. While we could
provide a mechanism to query whether or not an existing REMOVE_DEFAULT entry
exists, it is unlikely that sync will have completed at the time default apps
are installed. We would need to delay the installation of default apps until app
+ app list sync has completed. While I agree that would be nice, it seems like
unnecessary complexity for a relatively minor optimization.

[Nicolas Zea, 2014-01-09 00:42:37]

Sorry for the delay, LGTM!

[commit-bot: I haz the power, 2014-01-14 18:54:04]

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

[commit-bot: I haz the power, 2014-01-14 22:55:36]

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

[commit-bot: I haz the power, 2014-01-15 01:25:30]

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

[commit-bot: I haz the power, 2014-01-15 08:24:23]

Change committed as 244846

[tapted, 2014-01-16 07:03:27]

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

https://codereview.chromium.org/118463002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:167: BuildModel();
On 2013/12/19 02:18:02, tapted wrote:
> If you go with the cached |extension_service|, I think you'll need a to
re-fetch
> it here, now we know it's non-NULL with
> extensions::ExtensionSystem::Get(profile)->extension_service();

I haven't looked in-depth, but addressing this might fix http://crbug.com/334963

[stevenjb, 2014-01-18 00:09:54]

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

https://codereview.chromium.org/118463002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:167: BuildModel();
On 2014/01/16 07:03:28, tapted wrote:
> On 2013/12/19 02:18:02, tapted wrote:
> > If you go with the cached |extension_service|, I think you'll need a to
> re-fetch
> > it here, now we know it's non-NULL with
> > extensions::ExtensionSystem::Get(profile)->extension_service();
> 
> I haven't looked in-depth, but addressing this might fix
http://crbug.com/334963

Hmm. I had intended to address this but I guess that change got lost in  the
Holidaze. I will investigate further and test with multi-profile. Thanks for the
reminder!