Profiles: Really fix refcounted services.

Profiles: Really fix refcounted services.

Previous attempts to share code between ProfileKeyedServiceFactory and RefcountedProfileKeyedServiceFactory were ill advised. The core logic code must maintain refcounted data in a scoped_refptr<> for the entire lifecycle, across interface and implementation, preventing any real abstraction here.

This removes the common ProfileKeyedBase and splits the two worlds entirely in two. This has quite a bit of exact code with different types now.

Fallout from this is far reaching:

- Since there isn't one common heiarchy, now the testing methods either return ProfileKeyedServices or scoped_refptr<RefcountedProfileKeyedService>s. This is a lot of change.

- Many consumers handled the refcounted ptrs as raw pointers, which probably isn't a good idea.

- There is now a bunch of code duplication.

BUG=118196, 77155
R=mirandac,tim
TBR=jhawkins,sky,scottbyer

[Elliot Glaysher, 2012-03-14 22:01:40]

mirandac: profiles, full review
tim: real review on the sync/ directory

Do not despair at the size of this patch! Most of the real work is just in the
profiles/ directory and the rest is just updating to the new interface.

[tim (not reviewing), 2012-03-15 07:35:42]

sync/ LGTM

[Miranda Callahan, 2012-03-15 15:31:07]

First set of comments; second set coming this afternoon. (Also, you should
probably edit the issue title. :-)

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_base_factory.cc:41:
dependency_manager_->AssertProfileWasntDestroyed(profile);
These assertions that the profile wasn't destroyed twice in one method call
scare me and make me think we need some more clever way of making sure that
profile destruction timing doesn't interfere with factory methods. This isn't to
say that this should be done right now; just a comment.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_base_factory.cc:117: void
ProfileKeyedBaseFactory::MarkPreferencesSetOn(Profile* profile) {
Should we add a DCHECK(!ArePreferencesSetOn(profile)) here?

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.cc:38:
SetTestingFactory(profile, factory);
Do we actually need to call SetTestingFactory(GetProfileToUse(profile), factory)
or does this not matter?

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.cc:67: 
nit: remove blank line, and sentence-ify the "not found but creation allowed"
line above it.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.cc:68: // Check to see if
we have a per-Profile factory
nit: s/factory/factory:/

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.cc:82: // not found,
creation forbidden
nit: sentence-ify comment above.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:11: #include
"base/basictypes.h"
nit: b before c except after e. actually, also after e.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:31: typedef
ProfileKeyedService* (*FactoryFunction)(Profile* profile);
Shouldn't FactoryFunction be described as "A function that takes a Profile* and
returns the PKS associated with that Profile"? I'm not sure what you mean by
"replaces the (possibly internal) object used by this factory" wrt this function
definition.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:35: //
nit: remove blank line.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:37: // second call to
SetTestingFactory() is allowed. If the FactoryFunction is
instead of "a second call", maybe "multiple calls ... are allowed."

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:44: // not be NULL;
nit: s/;/.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:64: //
ServiceRedirectedInIncognito() and ServiceHasOwnInstanceInIncognito().
Change comment to reflect the fact that we're using GetProfileToUse to deal with
incognito selection; otherwise we're referencing internal details of a base
class method from a subclass, which means we might fall prey to inattentive
maintainers.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:69: // Maps |profile| to
|provider| with debug checks to prevent duplication.
s/|provider|/|service|/

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:87: // default
implementation removes it from |mapping_| and deletes the pointer.
Every time I see "ProfileShutdown" and "ProfileDestroyed" I have a heart attack,
and have to look to make sure that the profiles aren't actually being shutdown
and destroyed, and then I remember what's going on here. I wonder if this whole
comment should be moved to the PDM's DestroyProfileServices method, which is
where this stuff is actually happening, again under the rule of Comment the
Details Where They Are Actually Happening -- and then here we can just say what
these methods actually do. This will also be helpful because they're called
outside of the notification system described here -- for example, when we set a
new factory in testing. 

One might also rename them according to what they do, and not according to the
context in which they're generally called, to also avoid confusion from easily
frightened people such as myself. If you think I should just be less easily
scared, that's fine too. :-)

[Miranda Callahan, 2012-03-15 16:55:35]

Second set of comments. A bunch are repeating things in refcounted PKSF that I
said in the un-ref-counted version.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_base_factory.h (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_base_factory.h:15: class ProfileKeyedBase;
remove

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc
(right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc:41:
SetTestingFactory(profile, factory);
see my comment in PKSF -- SetTestingFactory(GetProfileToUse(profile), factory)?

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc:70: // not
found but creation allowed
nit as before: sentencing

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc:72: // Check
to see if we have a per-Profile factory
nit as before: append a period.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc:86: // not
found, creation forbidden
nit: I sentence thee

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc:91:
mapping_.insert(std::make_pair(profile, service));
These two lines are encapsulated in the "Associate" method in PKSF; either do
that here as well, or un-do-that in PKSF.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/refcounted_profile_keyed_service_factory.h (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.h:29: //
factory. For the majority of cases, this is the object returned to users.
As before: "Shouldn't FactoryFunction be described as "A function that takes a
Profile* and returns the PKS associated with that Profile"? I'm not sure what
you mean by "replaces the (possibly internal) object used by this factory" wrt
this function definition."

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.h:35: //
nit: remove blank line.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.h:37: // second
call to SetTestingFactory() is allowed. If the FactoryFunction is
nit as before: instead of "a second call", maybe "multiple calls ... are
allowed."

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.h:44: // not be
NULL;
nit: s/;/./

https://chromiumcodereview.appspot.com/9703038/diff/4008/tools/valgrind/memch...
File tools/valgrind/memcheck/suppressions.txt (left):

https://chromiumcodereview.appspot.com/9703038/diff/4008/tools/valgrind/memch...
tools/valgrind/memcheck/suppressions.txt:5943:
fun:_ZN20PasswordStoreFactory13GetForProfileEP7ProfileNS0_17ServiceAccessTypeE
:-D

[Elliot Glaysher, 2012-03-15 18:02:40]

I believe I've gotten everything.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_base_factory.cc:41:
dependency_manager_->AssertProfileWasntDestroyed(profile);
On 2012/03/15 15:31:07, Miranda Callahan wrote:
> These assertions that the profile wasn't destroyed twice in one method call
> scare me and make me think we need some more clever way of making sure that
> profile destruction timing doesn't interfere with factory methods. This isn't
to
> say that this should be done right now; just a comment.

While suboptimal, we're checking a different Profile object in this case.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_base_factory.cc:117: void
ProfileKeyedBaseFactory::MarkPreferencesSetOn(Profile* profile) {
On 2012/03/15 15:31:07, Miranda Callahan wrote:
> Should we add a DCHECK(!ArePreferencesSetOn(profile)) here?

Done.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.cc:38:
SetTestingFactory(profile, factory);
On 2012/03/15 15:31:07, Miranda Callahan wrote:
> Do we actually need to call SetTestingFactory(GetProfileToUse(profile),
factory)
> or does this not matter?

IIRC, that would be incorrect. There are a few tests that set different mocks on
the incognito/normal profile.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:64: //
ServiceRedirectedInIncognito() and ServiceHasOwnInstanceInIncognito().
On 2012/03/15 15:31:07, Miranda Callahan wrote:
> Change comment to reflect the fact that we're using GetProfileToUse to deal
with
> incognito selection; otherwise we're referencing internal details of a base
> class method from a subclass, which means we might fall prey to inattentive
> maintainers.

Done.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:87: // default
implementation removes it from |mapping_| and deletes the pointer.
On 2012/03/15 15:31:07, Miranda Callahan wrote:
> Every time I see "ProfileShutdown" and "ProfileDestroyed" I have a heart
attack,
> and have to look to make sure that the profiles aren't actually being shutdown
> and destroyed, and then I remember what's going on here. I wonder if this
whole
> comment should be moved to the PDM's DestroyProfileServices method, which is
> where this stuff is actually happening, again under the rule of Comment the
> Details Where They Are Actually Happening -- and then here we can just say
what
> these methods actually do. This will also be helpful because they're called
> outside of the notification system described here -- for example, when we set
a
> new factory in testing. 
> 
> One might also rename them according to what they do, and not according to the
> context in which they're generally called, to also avoid confusion from easily
> frightened people such as myself. If you think I should just be less easily
> scared, that's fine too. :-)

Can I do that as a separate patch? Adding another method rename is a lot of
refactoring in an already busy patch.

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc
(right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc:91:
mapping_.insert(std::make_pair(profile, service));
On 2012/03/15 16:55:35, Miranda Callahan wrote:
> These two lines are encapsulated in the "Associate" method in PKSF; either do
> that here as well, or un-do-that in PKSF.

I only did that in PKSF because only in the non-refcounted version are there
cases where a factory subclass needs those details. Made symmetric though, since
that's The Right Thing.

https://chromiumcodereview.appspot.com/9703038/diff/4008/tools/valgrind/memch...
File tools/valgrind/memcheck/suppressions.txt (left):

https://chromiumcodereview.appspot.com/9703038/diff/4008/tools/valgrind/memch...
tools/valgrind/memcheck/suppressions.txt:5943:
fun:_ZN20PasswordStoreFactory13GetForProfileEP7ProfileNS0_17ServiceAccessTypeE
On 2012/03/15 16:55:35, Miranda Callahan wrote:
> :-D

Yes, well, this is what happens when you have inconsistent scoped_refptr<>
usage.

[Miranda Callahan, 2012-03-15 18:11:21]

LGTM!

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_base_factory.cc:41:
dependency_manager_->AssertProfileWasntDestroyed(profile);
On 2012/03/15 18:02:41, Elliot Glaysher wrote:
> On 2012/03/15 15:31:07, Miranda Callahan wrote:
> > These assertions that the profile wasn't destroyed twice in one method call
> > scare me and make me think we need some more clever way of making sure that
> > profile destruction timing doesn't interfere with factory methods. This
isn't
> to
> > say that this should be done right now; just a comment.
> 
> While suboptimal, we're checking a different Profile object in this case.

Good point!

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
File chrome/browser/profiles/profile_keyed_service_factory.h (right):

https://chromiumcodereview.appspot.com/9703038/diff/4008/chrome/browser/profi...
chrome/browser/profiles/profile_keyed_service_factory.h:87: // default
implementation removes it from |mapping_| and deletes the pointer.
On 2012/03/15 18:02:41, Elliot Glaysher wrote:
> On 2012/03/15 15:31:07, Miranda Callahan wrote:
> > Every time I see "ProfileShutdown" and "ProfileDestroyed" I have a heart
> attack,
> > and have to look to make sure that the profiles aren't actually being
shutdown
> > and destroyed, and then I remember what's going on here. I wonder if this
> whole
> > comment should be moved to the PDM's DestroyProfileServices method, which is
> > where this stuff is actually happening, again under the rule of Comment the
> > Details Where They Are Actually Happening -- and then here we can just say
> what
> > these methods actually do. This will also be helpful because they're called
> > outside of the notification system described here -- for example, when we
set
> a
> > new factory in testing. 
> > 
> > One might also rename them according to what they do, and not according to
the
> > context in which they're generally called, to also avoid confusion from
easily
> > frightened people such as myself. If you think I should just be less easily
> > scared, that's fine too. :-)
> 
> Can I do that as a separate patch? Adding another method rename is a lot of
> refactoring in an already busy patch.

Oh yes, definitely.

[commit-bot: I haz the power, 2012-03-15 23:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/9703038/7008

[commit-bot: I haz the power, 2012-03-16 00:00:26]

Presubmit check for 9703038-7008 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui
    chrome/browser/search_engines
    chrome/browser/printing/cloud_print
    chrome/browser/tabs

Presubmit checks took 4.7s to calculate.

[Elliot Glaysher, 2012-03-16 00:03:20]

TBRing OWNERs. These are just a class rename.

[James Hawkins, 2012-03-16 00:05:49]

lgtm

[commit-bot: I haz the power, 2012-03-16 00:34:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/9703038/7008

[commit-bot: I haz the power, 2012-03-16 04:32:45]

Commit queue had an internal error.
Something went really wrong, probably a crash, a hickup or
simply the monkeys went out for diner.
The relevant dude was pinged already.