[wip] Profile: Sketch of a creation system.

Profiles: There should be an equivalent profile creation message to the ProfileDependencyManager for non-lazy initialization of services that need to be started with the profile.

BUG=77155
TEST=none


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

[Elliot Glaysher, 2011-06-30 23:55:03]

This is more a design review to see if you have any better ideas than this for
creating our services.

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile.cc (left):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile.cc:231:
BackgroundContentsServiceFactory::GetForProfile(this);
(Realized I missed this one)

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_dependency_manager.cc:26: // FooServiceFactory
here will scale or is desireable long term.
Does anyone have any ideas on something cleaner here?

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_impl.cc:300:
&ProfileImpl::EnsureSessionServiceCreated);
Why do we do this? Things seem to work if I leave this out.

[Miranda Callahan, 2011-07-01 15:20:24]

Thanks, Elliot -- added comments.

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_dependency_manager.cc:26: // FooServiceFactory
here will scale or is desireable long term.
On 2011/06/30 23:55:03, Elliot Glaysher wrote:
> Does anyone have any ideas on something cleaner here?

Perhaps I'm not seeing the problem correctly, but I don't see this method as
terribly unclean.

I see the problem thusly: we have a set of services that need to start up on
Profile creation. Having them all collected in a single place (instead of spread
out throughout initialization, as they are now) seems like a positive thing.

A couple questions, though:

1. It seems to me as if we should only use this method for factories that *must*
be initialized during Profile creation; as many factories as possible shouldn't
be created here, but instead be lazily created on first access. (I think this
also implies that our scaling problems may not be as bad as one might at first
fear.)

2. Why is the TabRestoreServiceFactory initialized here, instead of lazily at
first usage (probably in the Browser ctor, AFAICT)?

3. Same with TemplateURLServiceFactory.

Do we gain something by pulling creation up front instead of amortizing lazily
on access that I'm not seeing clearly?

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_dependency_manager.cc:83: // Iterate in reverse
destruction order for creation.
You'll probably have to call BuildDestructionOrder here first, right?

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_impl.cc:300:
&ProfileImpl::EnsureSessionServiceCreated);
On 2011/06/30 23:55:03, Elliot Glaysher wrote:
> Why do we do this? Things seem to work if I leave this out.

Good question. I tracked through the code and saw no clear reason to delay; svn
annotation says that this line was part of the original commit. Maybe the thing
to do is take it out in a CL on its own, so it's easier to track any possible
weird consequences (startup regressions, races between the session service
getting the initial BROWSER_OPENED notification, and the construction of the
profile (which is now partially asynchronous), unknown unknowns, etc. ...)

But removing 500ms delays that seem groundless sounds like a Very Good Thing.

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_impl.cc:370:
BackgroundContentsServiceFactory::GetForProfile(this);
Should be removed, along with a few other factory initializations in DoFinalInit
-- but you knew that.

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_keyed_service_factory.h:81: virtual bool
ServiceIsCreatedWithProfile();
I wonder if we really need this function; won't any factory that appears in the
AssertFactoriesBuilt method be one of the ones for which
ServiceIsCreatedWithProfile will return true?

[Elliot Glaysher, 2011-07-01 17:29:45]

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_dependency_manager.cc:26: // FooServiceFactory
here will scale or is desireable long term.
On 2011/07/01 15:20:24, Miranda Callahan wrote:
> 1. It seems to me as if we should only use this method for factories that
*must*
> be initialized during Profile creation; as many factories as possible
shouldn't
> be created here, but instead be lazily created on first access. (I think this
> also implies that our scaling problems may not be as bad as one might at first
> fear.)

And what about their dependencies? And once we add their dependencies, why not
add everything for clarity?

> 2. Why is the TabRestoreServiceFactory initialized here, instead of lazily at
> first usage (probably in the Browser ctor, AFAICT)?
>
> 3. Same with TemplateURLServiceFactory.

Because its testing usage isn't lazy; TabRestoreServiceFactory needs to be
active to say "TestingProfiles should have NULL mes by default."

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_dependency_manager.cc:83: // Iterate in reverse
destruction order for creation.
On 2011/07/01 15:20:24, Miranda Callahan wrote:
> You'll probably have to call BuildDestructionOrder here first, right?

Done.

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_impl.cc:370:
BackgroundContentsServiceFactory::GetForProfile(this);
On 2011/07/01 15:20:24, Miranda Callahan wrote:
> Should be removed, along with a few other factory initializations in
DoFinalInit
> -- but you knew that.

I'm going to ignore the PinnedTabServiceFactory call for now.  There are a few
other things wrong with it that would be better to do all at once.

[Miranda Callahan, 2011-07-01 19:00:21]

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_dependency_manager.cc:26: // FooServiceFactory
here will scale or is desireable long term.
On 2011/07/01 17:29:45, Elliot Glaysher wrote:
> On 2011/07/01 15:20:24, Miranda Callahan wrote:
> > 1. It seems to me as if we should only use this method for factories that
> *must*
> > be initialized during Profile creation; as many factories as possible
> shouldn't
> > be created here, but instead be lazily created on first access. (I think
this
> > also implies that our scaling problems may not be as bad as one might at
first
> > fear.)
> 
> And what about their dependencies? And once we add their dependencies, why not
> add everything for clarity?

Ok, "what about their dependencies" is a good argument. 

I still think this is a clean solution: it limits knowledge of the
ServiceFactory classes being initialized to the DependencyManager, and it places
their initialization in exactly the right spot.

> 
> > 2. Why is the TabRestoreServiceFactory initialized here, instead of lazily
at
> > first usage (probably in the Browser ctor, AFAICT)?
> >
> > 3. Same with TemplateURLServiceFactory.
> 
> Because its testing usage isn't lazy; TabRestoreServiceFactory needs to be
> active to say "TestingProfiles should have NULL mes by default." 

If there are many of these cases, would it make sense to have a special
TestAssertFactoriesBuilt method that's called only in the testing case?

[Elliot Glaysher, 2011-07-06 17:26:18]

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/7285014/diff/17/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_dependency_manager.cc:26: // FooServiceFactory
here will scale or is desireable long term.
> > Because its testing usage isn't lazy; TabRestoreServiceFactory needs to be
> > active to say "TestingProfiles should have NULL mes by default." 
> 
> If there are many of these cases, would it make sense to have a special
> TestAssertFactoriesBuilt method that's called only in the testing case? 

I am not sure what you're proposing.

[Elliot Glaysher, 2011-07-20 18:24:30]

Miranda: for real this time.

[Miranda Callahan, 2011-07-20 20:53:09]

LGTM with nits and requests in a couple places for more expansive comments.

http://codereview.chromium.org/7285014/diff/9001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/7285014/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_dependency_manager.cc:26: // FooServiceFactory
here will scale or is desireable long term.
Could you also add a comment here describing the function, in addition to your
TODO?

http://codereview.chromium.org/7285014/diff/9001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_dependency_manager.h (right):

http://codereview.chromium.org/7285014/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_dependency_manager.h:35: // require
configurations
nit: this comment kind of trails off; do you want to mention something about the
configurations, and then close the sentence with a "."? I would also mention
that it actually creates services, and say which kinds of services are (and
should be) created here. The more you say that will help future people use this
paradigm, the better...

http://codereview.chromium.org/7285014/diff/9001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

http://codereview.chromium.org/7285014/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_keyed_service_factory.h:79: // GetForProfile()
is called on our subclass. Some service need to be created
s/service/services

http://codereview.chromium.org/7285014/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_keyed_service_factory.h:86: //
SetTestingFactory() to make sure our profile's don't directly refer to the
s/e's/es

[Torne, 2011-07-21 10:51:45]

LGTM also.

The list of services in AssertFactoriesBuilt is kinda yucky but I can't think of
an alternative that doesn't involve global constructor magic or other things
that are generally frowned upon. (see also the huge lists of RegisterPrefs calls
all over the prefs system which also appear to be in need of some global
registration mechanism that doesn't require enumerating everything...)

[commit-bot: I haz the power, 2011-07-22 08:17:11]

Change committed as 93574