Take extensions out of Profile into a profile-keyed service, ExtensionSystem.

Take extensions out of Profile into a profile-keyed service, ExtensionSystem.

Move InitExtensions into ExtensionSystem.
Remove a few accessors (ExtensionDevToolsManager, ExtensionMessageService). The others have too many callers to fix in one go.

BUG=104095
TEST=Open and close an incognito window; should not crash.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124817

[Yoyo Zhou, 2012-02-09 01:34:55]

erg/mirandac: whichever of you would rather review this, please do.
aa: please review for sensibility for extensions.

[Elliot Glaysher, 2012-02-09 20:10:24]

A few notes:

- Uncomment the DependsOn lines in web_intents_registry_factory.cc and
profile_sync_service_factory.cc. You should be able to remove some of the
explicit initing in other services.

- If you want your ExtensionService created at Profile creation time and not
lazily, override the ServiceIsCreatedWithProfile() method on the factories.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_system_factory.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:30: return new
ExtensionSystemImpl::Delegate(profile->GetOriginalProfile());
Since this is redirected in incognito, I'm pretty sure you'll be passed the
original profile.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:39:
GetForProfile(profile)->message_service()->DestroyingProfile();
You probably want this in you ExtensionService::Shutdown() method instead. You
won't need to override ProfileShutdown() here. (I never thought of using
GetForProfile() here and wonder if it is safe...)

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:46:
ProfileDependencyManager::GetInstance()) {
I assume that either ExtensionSystemDelegateFactory or ExtensionSystemFactory is
going to depend on a lot of different systems. Please add commented out lines in
one of these constructors that are DependsOn() statments for them. (See
profile_sync_service_factory.cc as an example.)

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:76:
GetForProfile(profile)->DestroyProcessManager();
Same as above.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
child_profile.get(), false);
Shouldn't this be a call to OTRProfileImpl::Init()?

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:667: extensions_disabled,
GetExtensionPrefValueMap());
In both the OTRProfileImpl and here, you have this block of extension
initialization. Can this be moved into the factory? A large motivation for the
factorization (other than shutdown crashing) was moving per-service
initialization out of the profile implementations so it would be easier for, ie,
Chrome for Android to just not compile the ExtensionService in without ifdefing
the Profile code.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
File chrome/browser/sync/glue/app_data_type_controller.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
chrome/browser/sync/glue/app_data_type_controller.cc:32:
ExtensionSystemFactory::GetForProfile(profile_)->Init(true);
This shouldn't be necessary; ExtensionService should be depended on by the whole
ProfileSyncService; you just need to uncomment the DependsOn line in
profile_sync_service_factory.cc.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/extens...
File chrome/browser/sync/glue/extension_data_type_controller.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/extens...
chrome/browser/sync/glue/extension_data_type_controller.cc:32:
ExtensionSystemFactory::GetForProfile(profile_)->Init(true);
Ditto.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/extens...
File chrome/browser/sync/glue/extension_setting_data_type_controller.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/extens...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:67:
ExtensionSystemFactory::GetForProfile(profile_)->Init(true);
Ditto.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/theme_...
File chrome/browser/sync/glue/theme_data_type_controller.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/theme_...
chrome/browser/sync/glue/theme_data_type_controller.cc:32:
ExtensionSystemFactory::GetForProfile(profile_)->Init(true);
Ditto.

[Miranda Callahan, 2012-02-09 22:10:04]

I concentrated on the code in the Profile classes -- just a couple comments.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl.cc:215: return
ExtensionSystemFactory::GetForProfile(this)->extension_service();
see my comments in the regular profile_impl code; same here.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:601: ExtensionService*
ProfileImpl::GetExtensionService() {
Will it eventually be possible to remove this method, and the three below? I
just did a cursory glance through the code, and it looks like this will be a
ridiculous monster patch; but it would be nice to remove the factory entirely
from the Profile, to greatly increase its modularity.

And maybe in the meantime add big warnings that these functions are deprecated,
and users should directly pass the ExtensionSystemFactory a profile pointer to
get an ExtensionService*?

[Yoyo Zhou, 2012-02-16 21:49:20]

What are the guidelines on ServiceIsCreatedWithProfile? For instance, I think
for OTR profiles, the service wants to be created at startup, whereas with
Profile it doesn't matter.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_system_factory.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:30: return new
ExtensionSystemImpl::Delegate(profile->GetOriginalProfile());
On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> Since this is redirected in incognito, I'm pretty sure you'll be passed the
> original profile.

Done.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:39:
GetForProfile(profile)->message_service()->DestroyingProfile();
On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> You probably want this in you ExtensionService::Shutdown() method instead. You
> won't need to override ProfileShutdown() here. (I never thought of using
> GetForProfile() here and wonder if it is safe...)

Ah... I didn't read profile_keyed_service.h. This would make a *lot* more sense.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:46:
ProfileDependencyManager::GetInstance()) {
On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> I assume that either ExtensionSystemDelegateFactory or ExtensionSystemFactory
is
> going to depend on a lot of different systems. Please add commented out lines
in
> one of these constructors that are DependsOn() statments for them. (See
> profile_sync_service_factory.cc as an example.)

I only found reference to 3 factories, all from ExtensionService. I added them
here.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_system_factory.cc:76:
GetForProfile(profile)->DestroyProcessManager();
On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> Same as above.

Done.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
child_profile.get(), false);
On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> Shouldn't this be a call to OTRProfileImpl::Init()?

It certainly seems like it should be, but I recall that the test crashes when I
do that. I can try that again.

(FYI, this test fails a CHECK in Debug without this change because this OTR
profile is getting created abnormally and doesn't have Init called on it, so
services are destroyed in its destructor without having being created.)

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:601: ExtensionService*
ProfileImpl::GetExtensionService() {
On 2012/02/09 22:10:04, Miranda Callahan wrote:
> Will it eventually be possible to remove this method, and the three below? I
> just did a cursory glance through the code, and it looks like this will be a
> ridiculous monster patch; but it would be nice to remove the factory entirely
> from the Profile, to greatly increase its modularity.
> 
> And maybe in the meantime add big warnings that these functions are
deprecated,
> and users should directly pass the ExtensionSystemFactory a profile pointer to
> get an ExtensionService*?

Right, this would an even more enormous change if I tried to remove all of these
getters at once. I've got TODOs in profile.h, but I don't know where the
warnings are most appropriate; I could stick them there too.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:667: extensions_disabled,
GetExtensionPrefValueMap());
On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> In both the OTRProfileImpl and here, you have this block of extension
> initialization. Can this be moved into the factory? A large motivation for the
> factorization (other than shutdown crashing) was moving per-service
> initialization out of the profile implementations so it would be easier for,
ie,
> Chrome for Android to just not compile the ExtensionService in without
ifdefing
> the Profile code.

Good point. I think this should be possible. But I think
GetExtensionPrefValueMap would have to be public, right? Is there a workaround
for that?

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
File chrome/browser/sync/glue/app_data_type_controller.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
chrome/browser/sync/glue/app_data_type_controller.cc:32:
ExtensionSystemFactory::GetForProfile(profile_)->Init(true);
On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> This shouldn't be necessary; ExtensionService should be depended on by the
whole
> ProfileSyncService; you just need to uncomment the DependsOn line in
> profile_sync_service_factory.cc.

This is actually calling InitExtensions, which can't be done easily in
BuildServiceInstanceFor because it takes an argument here. (I briefly looked
into refactoring it to not take the argument, but it seemed not too
straightforward.)

[Elliot Glaysher, 2012-02-21 18:54:50]

On 2012/02/16 21:49:20, Yoyo Zhou wrote:
> What are the guidelines on ServiceIsCreatedWithProfile? For instance, I think
> for OTR profiles, the service wants to be created at startup, whereas with
> Profile it doesn't matter.

I could see the ServiceIsCreatedWithProfile() method being modified to take a
Profile* object to detect incognito or testing state.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
> chrome/browser/profiles/profile_impl.cc:667: extensions_disabled,
> GetExtensionPrefValueMap());
> On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > In both the OTRProfileImpl and here, you have this block of extension
> > initialization. Can this be moved into the factory? A large motivation for
the
> > factorization (other than shutdown crashing) was moving per-service
> > initialization out of the profile implementations so it would be easier for,
> ie,
> > Chrome for Android to just not compile the ExtensionService in without
> ifdefing
> > the Profile code.
> 
> Good point. I think this should be possible. But I think
> GetExtensionPrefValueMap would have to be public, right? Is there a workaround
> for that?

I don't see making that public as that big a deal.

> 
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
> File chrome/browser/sync/glue/app_data_type_controller.cc (right):
> 
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
> chrome/browser/sync/glue/app_data_type_controller.cc:32:
> ExtensionSystemFactory::GetForProfile(profile_)->Init(true);
> On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > This shouldn't be necessary; ExtensionService should be depended on by the
> whole
> > ProfileSyncService; you just need to uncomment the DependsOn line in
> > profile_sync_service_factory.cc.
> 
> This is actually calling InitExtensions, which can't be done easily in
> BuildServiceInstanceFor because it takes an argument here. (I briefly looked
> into refactoring it to not take the argument, but it seemed not too
> straightforward.)

That's regrettable, but OK. Ignore this one.

[Yoyo Zhou, 2012-02-21 20:33:51]

On Tue, Feb 21, 2012 at 10:54 AM,  <erg@chromium.org> wrote:
> On 2012/02/16 21:49:20, Yoyo Zhou wrote:
>>
>> What are the guidelines on ServiceIsCreatedWithProfile? For instance, I
>> think
>> for OTR profiles, the service wants to be created at startup, whereas with
>> Profile it doesn't matter.
>
>
> I could see the ServiceIsCreatedWithProfile() method being modified to take
> a
> Profile* object to detect incognito or testing state.
>
>
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/profile...
>>
>> chrome/browser/profiles/profile_impl.cc:667: extensions_disabled,
>> GetExtensionPrefValueMap());
>> On 2012/02/09 20:10:24, Elliot Glaysher wrote:
>> > In both the OTRProfileImpl and here, you have this block of extension
>> > initialization. Can this be moved into the factory? A large motivation
>> > for
>
> the
>>
>> > factorization (other than shutdown crashing) was moving per-service
>> > initialization out of the profile implementations so it would be easier
>> > for,
>> ie,
>> > Chrome for Android to just not compile the ExtensionService in without
>> ifdefing
>> > the Profile code.
>
>
>> Good point. I think this should be possible. But I think
>> GetExtensionPrefValueMap would have to be public, right? Is there a
>> workaround
>> for that?
>
>
> I don't see making that public as that big a deal.

It is only a member of ProfileImpl, so it would increase the surface
of the Profile interface (unless I use another static_cast here).

Maybe a better solution is to make ExtensionPrefValueMap its own PKS?
It would be probably in a league with SessionService in that it's
needed *before* prefs are loaded (in OnPrefsLoaded is where
CreateProfileServices is called).

>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
>>
>> File chrome/browser/sync/glue/app_data_type_controller.cc (right):
>
>
>
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/sync/glue/app_da...
>>
>> chrome/browser/sync/glue/app_data_type_controller.cc:32:
>> ExtensionSystemFactory::GetForProfile(profile_)->Init(true);
>> On 2012/02/09 20:10:24, Elliot Glaysher wrote:
>> > This shouldn't be necessary; ExtensionService should be depended on by
>> > the
>> whole
>> > ProfileSyncService; you just need to uncomment the DependsOn line in
>> > profile_sync_service_factory.cc.
>
>
>> This is actually calling InitExtensions, which can't be done easily in
>> BuildServiceInstanceFor because it takes an argument here. (I briefly
>> looked
>> into refactoring it to not take the argument, but it seemed not too
>> straightforward.)
>
>
> That's regrettable, but OK. Ignore this one.
>
> http://codereview.chromium.org/9369013/

[Elliot Glaysher, 2012-02-21 20:54:26]

> It is only a member of ProfileImpl, so it would increase the surface
> of the Profile interface (unless I use another static_cast here).

I don't mind temporarily expanding the Profile interface.
 
> Maybe a better solution is to make ExtensionPrefValueMap its own PKS?

I had assumed that was your next action; Chrome/Android wouldn't want it so it
shouldn't go in the Profile.

> It would be probably in a league with SessionService in that it's
> needed *before* prefs are loaded (in OnPrefsLoaded is where
> CreateProfileServices is called).

This would just mean that it's lazily initialized, and it's first access is
before the first pass init. I *think* that that's OK (and if there are any
assumptions that it isn't, then they're wrong since I can see this happening
quite a bit with some of the earlier services.)

[Yoyo Zhou, 2012-02-22 03:02:04]

I changed this to use ServiceIsCreatedWithProfile and promoted
GetExtensionPrefValueMap to Profile temporarily.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
child_profile.get(), false);
On 2012/02/16 21:49:20, Yoyo Zhou wrote:
> On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > Shouldn't this be a call to OTRProfileImpl::Init()?
> 
> It certainly seems like it should be, but I recall that the test crashes when
I
> do that. I can try that again.
> 
> (FYI, this test fails a CHECK in Debug without this change because this OTR
> profile is getting created abnormally and doesn't have Init called on it, so
> services are destroyed in its destructor without having being created.)

Here is the crash with explicitly calling Init (I think it's the same if we use
parent_profile->CreateOffTheRecordProfile):


[32530:32530:0217/181204:7629766454379:FATAL:browser_thread_impl.cc(344)] Check
failed: thread. 
Backtrace:
	base::debug::StackTrace::StackTrace() [0x3dbd1f6]
	logging::LogMessage::~LogMessage() [0x3de4c30]
	content::BrowserThread::UnsafeGetMessageLoopForThread() [0x305ba56]
	ProxyServiceFactory::CreateProxyConfigService() [0x19ec59c]
	ProfileIOData::InitializeOnUIThread() [0x1b1ac5a]
	OffTheRecordProfileIOData::Handle::LazyInitialize() [0x1af4f6b]
	OffTheRecordProfileIOData::Handle::GetChromeURLDataManagerBackendGetter()
[0x1af485f]
	OffTheRecordProfileImpl::GetChromeURLDataManager() [0x1af34ab]
	OffTheRecordProfileImpl::Init() [0x1af1b54]
	OffTheRecordProfileImplTest_GetHostZoomMap_Test::TestBody() [0xc4c5ac]

[Elliot Glaysher, 2012-02-27 20:49:08]

miranda, could you comment? This is start to LG.

[Miranda Callahan, 2012-02-27 21:59:05]

Profile stuff LGTM -- totally agree that the ExtensionPrefValueMap should be a
PKS, but not having it do that in this change is utterly reasonable. Just one
question about the test stuff.

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
child_profile.get(), false);
On 2012/02/22 03:02:05, Yoyo Zhou wrote:
> On 2012/02/16 21:49:20, Yoyo Zhou wrote:
> > On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > > Shouldn't this be a call to OTRProfileImpl::Init()?
> > 
> > It certainly seems like it should be, but I recall that the test crashes
when
> I
> > do that. I can try that again.
> > 
> > (FYI, this test fails a CHECK in Debug without this change because this OTR
> > profile is getting created abnormally and doesn't have Init called on it, so
> > services are destroyed in its destructor without having being created.)
> 
> Here is the crash with explicitly calling Init (I think it's the same if we
use
> parent_profile->CreateOffTheRecordProfile):
> 
> 
> [32530:32530:0217/181204:7629766454379:FATAL:browser_thread_impl.cc(344)]
Check
> failed: thread. 
> Backtrace:
> 	base::debug::StackTrace::StackTrace() [0x3dbd1f6]
> 	logging::LogMessage::~LogMessage() [0x3de4c30]
> 	content::BrowserThread::UnsafeGetMessageLoopForThread() [0x305ba56]
> 	ProxyServiceFactory::CreateProxyConfigService() [0x19ec59c]
> 	ProfileIOData::InitializeOnUIThread() [0x1b1ac5a]
> 	OffTheRecordProfileIOData::Handle::LazyInitialize() [0x1af4f6b]
> 	OffTheRecordProfileIOData::Handle::GetChromeURLDataManagerBackendGetter()
> [0x1af485f]
> 	OffTheRecordProfileImpl::GetChromeURLDataManager() [0x1af34ab]
> 	OffTheRecordProfileImpl::Init() [0x1af1b54]
> 	OffTheRecordProfileImplTest_GetHostZoomMap_Test::TestBody() [0xc4c5ac]

This looks like one of those common threading errors in tests. :-(  What if you
explicitly create and start up a UI thread, as is done in
template_url_service_test:

http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...

http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...

[Aaron Boodman, 2012-02-28 00:21:21]

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_message_handler.cc (right):

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_message_handler.cc:45: if (message_service)
{
Can this really not exist?

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_system.h (right):

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.h:31: // and incognito Profiles,
except for the process manager.
Nit: just say "except for where called out in comments", to make this comment
slightly less brittle.

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.h:32: // This interface is analogous
to ExtensionServiceInterface; it supports
Again, no need to mention specific class names which might change. The second
part of the comment stands alone.

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.h:120: class Delegate : public
ProfileKeyedService {
Delegate is a kinda funky name for this class, since the term "delegate" already
has a well-known meaning in the Chrome codebase different than how it is used
here.

Maybe call this Shared?

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_system_factory.cc (right):

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system_factory.cc:13: 
Unneeded linebreak.

[Yoyo Zhou, 2012-02-28 02:03:33]

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
child_profile.get(), false);
On 2012/02/27 21:59:05, Miranda Callahan wrote:
> On 2012/02/22 03:02:05, Yoyo Zhou wrote:
> > On 2012/02/16 21:49:20, Yoyo Zhou wrote:
> > > On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > > > Shouldn't this be a call to OTRProfileImpl::Init()?
> > > 
> > > It certainly seems like it should be, but I recall that the test crashes
> when
> > I
> > > do that. I can try that again.
> > > 
> > > (FYI, this test fails a CHECK in Debug without this change because this
OTR
> > > profile is getting created abnormally and doesn't have Init called on it,
so
> > > services are destroyed in its destructor without having being created.)
> > 
> > Here is the crash with explicitly calling Init (I think it's the same if we
> use
> > parent_profile->CreateOffTheRecordProfile):
> > 
> > 
> > [32530:32530:0217/181204:7629766454379:FATAL:browser_thread_impl.cc(344)]
> Check
> > failed: thread. 
> > Backtrace:
> > 	base::debug::StackTrace::StackTrace() [0x3dbd1f6]
> > 	logging::LogMessage::~LogMessage() [0x3de4c30]
> > 	content::BrowserThread::UnsafeGetMessageLoopForThread() [0x305ba56]
> > 	ProxyServiceFactory::CreateProxyConfigService() [0x19ec59c]
> > 	ProfileIOData::InitializeOnUIThread() [0x1b1ac5a]
> > 	OffTheRecordProfileIOData::Handle::LazyInitialize() [0x1af4f6b]
> > 	OffTheRecordProfileIOData::Handle::GetChromeURLDataManagerBackendGetter()
> > [0x1af485f]
> > 	OffTheRecordProfileImpl::GetChromeURLDataManager() [0x1af34ab]
> > 	OffTheRecordProfileImpl::Init() [0x1af1b54]
> > 	OffTheRecordProfileImplTest_GetHostZoomMap_Test::TestBody() [0xc4c5ac]
> 
> This looks like one of those common threading errors in tests. :-(  What if
you
> explicitly create and start up a UI thread, as is done in
> template_url_service_test:
> 
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...
> 
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...

Alas, BrowserWithTestWindowTest (which this test inherits from) already does
this. Do you know if there's a way to make sure this test thinks it's running on
the UI thread?

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_message_handler.cc (right):

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_message_handler.cc:45: if (message_service)
{
On 2012/02/28 00:21:21, Aaron Boodman wrote:
> Can this really not exist?

Test profiles don't have one, at least.

[Miranda Callahan, 2012-02-28 18:55:52]

On 2012/02/28 02:03:33, Yoyo Zhou wrote:
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
> File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):
> 
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
> chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
> child_profile.get(), false);
> On 2012/02/27 21:59:05, Miranda Callahan wrote:
> > On 2012/02/22 03:02:05, Yoyo Zhou wrote:
> > > On 2012/02/16 21:49:20, Yoyo Zhou wrote:
> > > > On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > > > > Shouldn't this be a call to OTRProfileImpl::Init()?
> > > > 
> > > > It certainly seems like it should be, but I recall that the test crashes
> > when
> > > I
> > > > do that. I can try that again.
> > > > 
> > > > (FYI, this test fails a CHECK in Debug without this change because this
> OTR
> > > > profile is getting created abnormally and doesn't have Init called on
it,
> so
> > > > services are destroyed in its destructor without having being created.)
> > > 
> > > Here is the crash with explicitly calling Init (I think it's the same if
we
> > use
> > > parent_profile->CreateOffTheRecordProfile):
> > > 
> > > 
> > > [32530:32530:0217/181204:7629766454379:FATAL:browser_thread_impl.cc(344)]
> > Check
> > > failed: thread. 
> > > Backtrace:
> > > 	base::debug::StackTrace::StackTrace() [0x3dbd1f6]
> > > 	logging::LogMessage::~LogMessage() [0x3de4c30]
> > > 	content::BrowserThread::UnsafeGetMessageLoopForThread() [0x305ba56]
> > > 	ProxyServiceFactory::CreateProxyConfigService() [0x19ec59c]
> > > 	ProfileIOData::InitializeOnUIThread() [0x1b1ac5a]
> > > 	OffTheRecordProfileIOData::Handle::LazyInitialize() [0x1af4f6b]
> > > 	OffTheRecordProfileIOData::Handle::GetChromeURLDataManagerBackendGetter()
> > > [0x1af485f]
> > > 	OffTheRecordProfileImpl::GetChromeURLDataManager() [0x1af34ab]
> > > 	OffTheRecordProfileImpl::Init() [0x1af1b54]
> > > 	OffTheRecordProfileImplTest_GetHostZoomMap_Test::TestBody() [0xc4c5ac]
> > 
> > This looks like one of those common threading errors in tests. :-(  What if
> you
> > explicitly create and start up a UI thread, as is done in
> > template_url_service_test:
> > 
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...
> > 
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...
> 
> Alas, BrowserWithTestWindowTest (which this test inherits from) already does
> this. Do you know if there's a way to make sure this test thinks it's running
on
> the UI thread?
> 

Alas, I do not, and can't find any obvious way to do it. I'm surprised this
hasn't come up before. :-/ Maybe your solution is correct in this case.

[Yoyo Zhou, 2012-02-28 19:48:04]

On 2012/02/28 18:55:52, Miranda Callahan wrote:
> On 2012/02/28 02:03:33, Yoyo Zhou wrote:
> >
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
> > File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc
(right):
> > 
> >
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
> > chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
> > child_profile.get(), false);
> > On 2012/02/27 21:59:05, Miranda Callahan wrote:
> > > On 2012/02/22 03:02:05, Yoyo Zhou wrote:
> > > > On 2012/02/16 21:49:20, Yoyo Zhou wrote:
> > > > > On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > > > > > Shouldn't this be a call to OTRProfileImpl::Init()?
> > > > > 
> > > > > It certainly seems like it should be, but I recall that the test
crashes
> > > when
> > > > I
> > > > > do that. I can try that again.
> > > > > 
> > > > > (FYI, this test fails a CHECK in Debug without this change because
this
> > OTR
> > > > > profile is getting created abnormally and doesn't have Init called on
> it,
> > so
> > > > > services are destroyed in its destructor without having being
created.)
> > > > 
> > > > Here is the crash with explicitly calling Init (I think it's the same if
> we
> > > use
> > > > parent_profile->CreateOffTheRecordProfile):
> > > > 
> > > > 
> > > >
[32530:32530:0217/181204:7629766454379:FATAL:browser_thread_impl.cc(344)]
> > > Check
> > > > failed: thread. 
> > > > Backtrace:
> > > > 	base::debug::StackTrace::StackTrace() [0x3dbd1f6]
> > > > 	logging::LogMessage::~LogMessage() [0x3de4c30]
> > > > 	content::BrowserThread::UnsafeGetMessageLoopForThread() [0x305ba56]
> > > > 	ProxyServiceFactory::CreateProxyConfigService() [0x19ec59c]
> > > > 	ProfileIOData::InitializeOnUIThread() [0x1b1ac5a]
> > > > 	OffTheRecordProfileIOData::Handle::LazyInitialize() [0x1af4f6b]
> > > >
	OffTheRecordProfileIOData::Handle::GetChromeURLDataManagerBackendGetter()
> > > > [0x1af485f]
> > > > 	OffTheRecordProfileImpl::GetChromeURLDataManager() [0x1af34ab]
> > > > 	OffTheRecordProfileImpl::Init() [0x1af1b54]
> > > > 	OffTheRecordProfileImplTest_GetHostZoomMap_Test::TestBody() [0xc4c5ac]
> > > 
> > > This looks like one of those common threading errors in tests. :-(  What
if
> > you
> > > explicitly create and start up a UI thread, as is done in
> > > template_url_service_test:
> > > 
> > >
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...
> > > 
> > >
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...
> > 
> > Alas, BrowserWithTestWindowTest (which this test inherits from) already does
> > this. Do you know if there's a way to make sure this test thinks it's
running
> on
> > the UI thread?
> > 
> 
> Alas, I do not, and can't find any obvious way to do it. I'm surprised this
> hasn't come up before. :-/ Maybe your solution is correct in this case.

On further inspection from the backtrace, it looks like the missing thread
causing the failing CHECK is actually the IO thread, which
CreateProxyConfigService is expecting. However, it doesn't look possible to have
both a MessageLoopForUI and a MessageLoopForIO in this test (since both message
loops would get initialized on the same thread; I don't know how to get around
this).

Moreover, I realized that the test explicitly calls InitHostZoomMap which would
otherwise be called in Init. I'm not sure if this is by design, or if the author
had originally wanted Init but ran into threading issues.

I'm inclined to leave it the way I have it.

[Miranda Callahan, 2012-02-28 19:49:51]

On 2012/02/28 19:48:04, Yoyo Zhou wrote:
> On 2012/02/28 18:55:52, Miranda Callahan wrote:
> > On 2012/02/28 02:03:33, Yoyo Zhou wrote:
> > >
> >
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
> > > File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc
> (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9369013/diff/1/chrome/browser/profiles/off_the...
> > > chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:151:
> > > child_profile.get(), false);
> > > On 2012/02/27 21:59:05, Miranda Callahan wrote:
> > > > On 2012/02/22 03:02:05, Yoyo Zhou wrote:
> > > > > On 2012/02/16 21:49:20, Yoyo Zhou wrote:
> > > > > > On 2012/02/09 20:10:24, Elliot Glaysher wrote:
> > > > > > > Shouldn't this be a call to OTRProfileImpl::Init()?
> > > > > > 
> > > > > > It certainly seems like it should be, but I recall that the test
> crashes
> > > > when
> > > > > I
> > > > > > do that. I can try that again.
> > > > > > 
> > > > > > (FYI, this test fails a CHECK in Debug without this change because
> this
> > > OTR
> > > > > > profile is getting created abnormally and doesn't have Init called
on
> > it,
> > > so
> > > > > > services are destroyed in its destructor without having being
> created.)
> > > > > 
> > > > > Here is the crash with explicitly calling Init (I think it's the same
if
> > we
> > > > use
> > > > > parent_profile->CreateOffTheRecordProfile):
> > > > > 
> > > > > 
> > > > >
> [32530:32530:0217/181204:7629766454379:FATAL:browser_thread_impl.cc(344)]
> > > > Check
> > > > > failed: thread. 
> > > > > Backtrace:
> > > > > 	base::debug::StackTrace::StackTrace() [0x3dbd1f6]
> > > > > 	logging::LogMessage::~LogMessage() [0x3de4c30]
> > > > > 	content::BrowserThread::UnsafeGetMessageLoopForThread() [0x305ba56]
> > > > > 	ProxyServiceFactory::CreateProxyConfigService() [0x19ec59c]
> > > > > 	ProfileIOData::InitializeOnUIThread() [0x1b1ac5a]
> > > > > 	OffTheRecordProfileIOData::Handle::LazyInitialize() [0x1af4f6b]
> > > > >
> 	OffTheRecordProfileIOData::Handle::GetChromeURLDataManagerBackendGetter()
> > > > > [0x1af485f]
> > > > > 	OffTheRecordProfileImpl::GetChromeURLDataManager() [0x1af34ab]
> > > > > 	OffTheRecordProfileImpl::Init() [0x1af1b54]
> > > > > 	OffTheRecordProfileImplTest_GetHostZoomMap_Test::TestBody()
[0xc4c5ac]
> > > > 
> > > > This looks like one of those common threading errors in tests. :-(  What
> if
> > > you
> > > > explicitly create and start up a UI thread, as is done in
> > > > template_url_service_test:
> > > > 
> > > >
> > >
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...
> > > > 
> > > >
> > >
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/search_engin...
> > > 
> > > Alas, BrowserWithTestWindowTest (which this test inherits from) already
does
> > > this. Do you know if there's a way to make sure this test thinks it's
> running
> > on
> > > the UI thread?
> > > 
> > 
> > Alas, I do not, and can't find any obvious way to do it. I'm surprised this
> > hasn't come up before. :-/ Maybe your solution is correct in this case.
> 
> On further inspection from the backtrace, it looks like the missing thread
> causing the failing CHECK is actually the IO thread, which
> CreateProxyConfigService is expecting. However, it doesn't look possible to
have
> both a MessageLoopForUI and a MessageLoopForIO in this test (since both
message
> loops would get initialized on the same thread; I don't know how to get around
> this).
> 
> Moreover, I realized that the test explicitly calls InitHostZoomMap which
would
> otherwise be called in Init. I'm not sure if this is by design, or if the
author
> had originally wanted Init but ran into threading issues.
> 
> I'm inclined to leave it the way I have it.

Ok, makes sense. Here's another LGTM.

[Yoyo Zhou, 2012-02-28 22:58:25]

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_system.h (right):

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.h:31: // and incognito Profiles,
except for the process manager.
On 2012/02/28 00:21:21, Aaron Boodman wrote:
> Nit: just say "except for where called out in comments", to make this comment
> slightly less brittle.

Done.

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.h:32: // This interface is analogous
to ExtensionServiceInterface; it supports
On 2012/02/28 00:21:21, Aaron Boodman wrote:
> Again, no need to mention specific class names which might change. The second
> part of the comment stands alone.

Done.

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.h:120: class Delegate : public
ProfileKeyedService {
On 2012/02/28 00:21:21, Aaron Boodman wrote:
> Delegate is a kinda funky name for this class, since the term "delegate"
already
> has a well-known meaning in the Chrome codebase different than how it is used
> here.
> 
> Maybe call this Shared?

Done.

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_system_factory.cc (right):

http://codereview.chromium.org/9369013/diff/28044/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system_factory.cc:13: 
On 2012/02/28 00:21:21, Aaron Boodman wrote:
> Unneeded linebreak.

Axed.

[Aaron Boodman, 2012-03-02 03:19:36]

lgtm

[commit-bot: I haz the power, 2012-03-02 18:02:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/9369013/43001

[commit-bot: I haz the power, 2012-03-02 18:02:26]

Can't apply patch for file chrome/browser/profiles/profile_io_data.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/profiles/profile_io_data.cc
Hunk #2 FAILED at 224.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_io_data.cc.rej

[commit-bot: I haz the power, 2012-03-02 18:05:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/9369013/51002

[commit-bot: I haz the power, 2012-03-02 18:05:59]

Presubmit check for 9369013-51002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/sync/glue/theme_data_type_controller.cc,chrome/browser/sync/glue/extension_setting_data_type_controller.cc,chrome/browser/sync/profile_sync_service_factory.cc,chrome/browser/sync/glue/extension_data_type_controller.cc,chrome/browser/sync/glue/preference_data_type_controller.cc,chrome/browser/sync/test/integration/sync_extension_helper.cc,chrome/browser/sync/test/integration/sync_app_helper.cc

Presubmit checks took 3.7s to calculate.

[Yoyo Zhou, 2012-03-02 18:25:06]

akalin: please review for sync.

[akalin, 2012-03-02 23:26:56]

sync lgtm

[commit-bot: I haz the power, 2012-03-02 23:47:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/9369013/53002

[commit-bot: I haz the power, 2012-03-02 23:48:00]

Can't apply patch for file chrome/browser/profiles/profile_impl.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/profiles/profile_impl.cc
Hunk #4 FAILED at 461.
1 out of 10 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_impl.cc.rej

[commit-bot: I haz the power, 2012-03-02 23:51:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/9369013/57001

[commit-bot: I haz the power, 2012-03-03 01:52:08]

Change committed as 124817