Profile shouldn't own PersonalDataManager

Profile shouldn't own PersonalDataManager

Converts PersonalDataManager over to be a ProfileKeyedService.  Decouples Sync code from PersonalDataManager and moves to a notification-based update.  Changes various test mocks over to the new ProfileKeyedService mechanisms.

BUG=94026
TEST=PersonalDataManagerTest.*:AutofillTest.*:AutofillMetricsTests.*:AutofillDataTypeControllerTest.*: and other Sync tests.

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

[dhollowa, 2011-09-23 02:20:38]

Sorry for the size of this one!  It was all pretty interwoven, no obvious way to
split it up.

isherman:   Autofill changes
erg:        ProfileKeyedService and Profile changes
lipalani:   Sync changes and WebDataService change
pkasting:   Owner of chrome/browser/ui
jhawkins:   Owner of chrome/browser/ui/webui
phajdan.jr: Owner of chrome/test

[tfarina, 2011-09-23 02:30:44]

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_factory.h (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_factory.h:9: #include
"base/memory/singleton.h"
You can forward declare this instead.

template <typename T> struct DefaultSingletonTraits;

[Ilya Sherman, 2011-09-23 03:56:34]

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager_unittest.cc:65: return NULL;
I don't really understand this pattern...  Why do we have the
ProfileKeyedService construct a NULL object, but then test with a different
object?  I would expect all of the code invoked by any given test to agree on
what the PersonalDataManager instance is.  (This comes up also in
autofill_metrics_unittest.)

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager_unittest.cc:466: // Weak reference.
nit: I'd prefer this to be on the same line as the variable, so that it's that
much cleaner what this is referring to.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:218: Refresh();
Why does sync need to inform the PDM to refresh, beyond notifications already
sent by the WDS?

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.h:159: // Except for tests which
subclass.
nit: I find this comment a bit confusing as written; and it seems redundant with
the comment immediately above.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_factory.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_factory.cc:25: // TODO(erg): For
Shutdown() order, we need to:
Is this TODO intentionally unimplemented for now?

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_factory.cc:35:
personal_data_manager->Init(profile);
Do we still need to provide a constructor and an Init method separately?  If so,
is that purely for testing?  Should the method be private, i.e. only accessible
to friends?

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:64:
profile_.reset(NULL);
nit: Are these calls to reset() really necessary?  They seem kind of redundant
with the use of a scoped_ptr.  In fact... does profile_ even need to be a
pointer?

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

http://codereview.chromium.org/7967024/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl.cc:15: #include
"chrome/browser/autofill/personal_data_manager_factory.h"
nit: What relies on this #include?

http://codereview.chromium.org/7967024/diff/1/chrome/browser/sync/glue/autofi...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/sync/glue/autofi...
chrome/browser/sync/glue/autofill_model_associator.cc:32: #include
"content/common/notification_source.h"
nit: I'm not seeing why these header additions are needed.

[dhollowa, 2011-09-23 16:30:03]

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager_unittest.cc:65: return NULL;
This is how the original code worked, just via a different mechanism.  For
testing, the global PDM (that is accessed via the profile) is NULL.  The local
tests have a mocked form where they return the (local testing) PDM via
mechanisms specific to the tests.

On 2011/09/23 03:56:34, Ilya Sherman wrote:
> I don't really understand this pattern...  Why do we have the
> ProfileKeyedService construct a NULL object, but then test with a different
> object?  I would expect all of the code invoked by any given test to agree on
> what the PersonalDataManager instance is.  (This comes up also in
> autofill_metrics_unittest.)

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager_unittest.cc:466: // Weak reference.
I prefer it as-is.  Having the comment follow doesn't scale if more
documentation is added.  There is no ambiguity about what this refers to.

On 2011/09/23 03:56:34, Ilya Sherman wrote:
> nit: I'd prefer this to be on the same line as the variable, so that it's that
> much cleaner what this is referring to.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:218: Refresh();
On 2011/09/23 03:56:34, Ilya Sherman wrote:
> Why does sync need to inform the PDM to refresh, beyond notifications already
> sent by the WDS?

The WDS notifications are not seen by the UI thread (except for
NOTIFICATION_WEB_DATABASE_LOADED).  The Sync code manipulates the WebDatabase
directly (not via WDS) in some cases, so needs to inform the PDM that changes
have been made.

This notification replaces an existing (tightly coupled) mechanism where Sync
was calling Refresh() directly.  I've changed it to this so that Sync does not
need to affect the lifetime of the PDM.  It just sends a notification and either
the PDM is alive and responsive, or it is not.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.h:159: // Except for tests which
subclass.
On 2011/09/23 03:56:34, Ilya Sherman wrote:
> nit: I find this comment a bit confusing as written; and it seems redundant
with
> the comment immediately above.

Done.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_factory.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_factory.cc:25: // TODO(erg): For
Shutdown() order, we need to:
On 2011/09/23 03:56:34, Ilya Sherman wrote:
> Is this TODO intentionally unimplemented for now?

Yes, this is a note to erg@ to set up dependencies explicitly once the other
"services" have moved to the new API.  There are similar TODOs in other factory
constructors.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_factory.cc:35:
personal_data_manager->Init(profile);
On 2011/09/23 03:56:34, Ilya Sherman wrote:
> Do we still need to provide a constructor and an Init method separately?  If
so,
> is that purely for testing?  Should the method be private, i.e. only
accessible
> to friends?

Yes, the two-phase init is required for testing.  The common pattern is to
subclass the PDM and then mock specific calls to sever ties to the WDS backend. 
I agree about making it private.  I've done that.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_factory.h (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_factory.h:9: #include
"base/memory/singleton.h"
On 2011/09/23 02:30:44, tfarina wrote:
> You can forward declare this instead.
> 
> template <typename T> struct DefaultSingletonTraits;

Done.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:64:
profile_.reset(NULL);
On 2011/09/23 03:56:34, Ilya Sherman wrote:
> nit: Are these calls to reset() really necessary?  They seem kind of redundant
> with the use of a scoped_ptr.  In fact... does profile_ even need to be a
> pointer?

Yes, these are necessary.  Destruction order needs to be imposed specifically.

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

http://codereview.chromium.org/7967024/diff/1/chrome/browser/profiles/off_the...
chrome/browser/profiles/off_the_record_profile_impl.cc:15: #include
"chrome/browser/autofill/personal_data_manager_factory.h"
On 2011/09/23 03:56:34, Ilya Sherman wrote:
> nit: What relies on this #include?

Nothing.  Removed.

http://codereview.chromium.org/7967024/diff/1/chrome/browser/sync/glue/autofi...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/sync/glue/autofi...
chrome/browser/sync/glue/autofill_model_associator.cc:32: #include
"content/common/notification_source.h"
On 2011/09/23 03:56:34, Ilya Sherman wrote:
> nit: I'm not seeing why these header additions are needed.

Vestigial.  Removed.

[lipalani1, 2011-09-23 17:59:23]

The changes look good from sync side. But before we L G T M it can you throw
some light on what happens during chrome exit. Can you make sure the order is
ProfileSyncService dies before the autofill service. The reverse could cause a
crash.

[Elliot Glaysher, 2011-09-23 18:01:44]

On Fri, Sep 23, 2011 at 10:59 AM,  <lipalani@chromium.org> wrote:
> The changes look good from sync side. But before we L G T M it can you throw
> some light on what happens during chrome exit. Can you make sure the order
> is
> ProfileSyncService dies before the autofill service. The reverse could cause
> a
> crash.

Right now, the sync service moved itself before all PKSF shutdown in
profile_impl.cc. When stevet/tim(?) port ProfileSyncService to PKSF,
it will have to depend on PersonalDataManagerFactory.

> http://codereview.chromium.org/7967024/
>

[Elliot Glaysher, 2011-09-23 18:03:36]

Also, the profile integration parts lgtm.

On 2011/09/23 18:01:44, Elliot Glaysher wrote:
> On Fri, Sep 23, 2011 at 10:59 AM,  <mailto:lipalani@chromium.org> wrote:
> > The changes look good from sync side. But before we L G T M it can you throw
> > some light on what happens during chrome exit. Can you make sure the order
> > is
> > ProfileSyncService dies before the autofill service. The reverse could cause
> > a
> > crash.
> 
> Right now, the sync service moved itself before all PKSF shutdown in
> profile_impl.cc. When stevet/tim(?) port ProfileSyncService to PKSF,
> it will have to depend on PersonalDataManagerFactory.
> 
> > http://codereview.chromium.org/7967024/
> >

[lipalani1, 2011-09-23 18:08:30]

LGTM

http://codereview.chromium.org/7967024/diff/8001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/7967024/diff/8001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_model_associator.cc:200: 
Can you make this a function, just like postoptimisticrefreshtask, so it can be
called from both places?

[lipalani1, 2011-09-23 18:08:42]

LGTM from sync side with one comment.

[Peter Kasting, 2011-09-23 19:02:21]

browser.cc LGTM

[dhollowa, 2011-09-23 19:11:06]

On 2011/09/23 18:01:44, Elliot Glaysher wrote:
> On Fri, Sep 23, 2011 at 10:59 AM,  <mailto:lipalani@chromium.org> wrote:
> > The changes look good from sync side. But before we L G T M it can you throw
> > some light on what happens during chrome exit. Can you make sure the order
> > is
> > ProfileSyncService dies before the autofill service. The reverse could cause
> > a
> > crash.
> 
> Right now, the sync service moved itself before all PKSF shutdown in
> profile_impl.cc. When stevet/tim(?) port ProfileSyncService to PKSF,
> it will have to depend on PersonalDataManagerFactory.
> 
> > http://codereview.chromium.org/7967024/
> >

Yes, the Sync service shuts down prior to the PersonaDataManager.  But with the
notification mechanism that I've set up here it will not matter if the
PersonalDataManager dies before Sync.  The PDM will just de-register its
listener.

[dhollowa, 2011-09-23 20:09:01]

http://codereview.chromium.org/7967024/diff/8001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/7967024/diff/8001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_model_associator.cc:200: 
On 2011/09/23 18:08:30, lipalani1 wrote:
> Can you make this a function, just like postoptimisticrefreshtask, so it can
be
> called from both places?

Yes, done.  I've moved it to WebDataService since that really "owns" this
notification.

[Evan Stade, 2011-09-23 20:22:05]

ui/webui lgtm

[Paweł Hajdan Jr., 2011-09-23 20:24:08]

chrome/test LGTM

[Ilya Sherman, 2011-09-23 21:41:38]

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:64:
profile_.reset(NULL);
On 2011/09/23 16:30:03, dhollowa wrote:
> On 2011/09/23 03:56:34, Ilya Sherman wrote:
> > nit: Are these calls to reset() really necessary?  They seem kind of
redundant
> > with the use of a scoped_ptr.  In fact... does profile_ even need to be a
> > pointer?
> 
> Yes, these are necessary.  Destruction order needs to be imposed specifically.
> 

Ok, in that case, please add a comment noting that this code is needed to impose
the destruction order.

http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_merge_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_merge_unittest.cc:131:
scoped_ptr<PersonalDataManagerMock> personal_data_;
nit: Can this just be |PersonalDataManagerMock personal_data_|?

[dhollowa, 2011-09-23 22:00:57]

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:64:
profile_.reset(NULL);
On 2011/09/23 21:41:38, Ilya Sherman wrote:
> On 2011/09/23 16:30:03, dhollowa wrote:
> > On 2011/09/23 03:56:34, Ilya Sherman wrote:
> > > nit: Are these calls to reset() really necessary?  They seem kind of
> redundant
> > > with the use of a scoped_ptr.  In fact... does profile_ even need to be a
> > > pointer?
> > 
> > Yes, these are necessary.  Destruction order needs to be imposed
specifically.
> > 
> 
> Ok, in that case, please add a comment noting that this code is needed to
impose
> the destruction order.

Done.

http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_merge_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_merge_unittest.cc:131:
scoped_ptr<PersonalDataManagerMock> personal_data_;
On 2011/09/23 21:41:38, Ilya Sherman wrote:
> nit: Can this just be |PersonalDataManagerMock personal_data_|?

No.  The tests below reset the data with each pass of the data driven test.

[Ilya Sherman, 2011-09-23 22:01:31]

LGTM

[Ilya Sherman, 2011-09-23 22:17:43]

http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_merge_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_merge_unittest.cc:131:
scoped_ptr<PersonalDataManagerMock> personal_data_;
On 2011/09/23 22:00:58, dhollowa wrote:
> On 2011/09/23 21:41:38, Ilya Sherman wrote:
> > nit: Can this just be |PersonalDataManagerMock personal_data_|?
> 
> No.  The tests below reset the data with each pass of the data driven test.

Wait, where does this happen?  If you mean the SetUp() method, I believe that is
exactly equivalent to directly having a PersonalDataManagerMock instance
variable (without the scoped_ptr).

[GeorgeY, 2011-09-23 22:53:26]

LGTM

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/7967024/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:64:
profile_.reset(NULL);
FYI, the order of the destruction is reverse to order of initialization, which
is *enforced* by gcc (not by MSVS though).
On 2011/09/23 22:00:58, dhollowa wrote:
> On 2011/09/23 21:41:38, Ilya Sherman wrote:
> > On 2011/09/23 16:30:03, dhollowa wrote:
> > > On 2011/09/23 03:56:34, Ilya Sherman wrote:
> > > > nit: Are these calls to reset() really necessary?  They seem kind of
> > redundant
> > > > with the use of a scoped_ptr.  In fact... does profile_ even need to be
a
> > > > pointer?
> > > 
> > > Yes, these are necessary.  Destruction order needs to be imposed
> specifically.
> > > 
> > 
> > Ok, in that case, please add a comment noting that this code is needed to
> impose
> > the destruction order.
> 
> Done.

[dhollowa, 2011-09-26 14:59:10]

On 2011/09/23 22:17:43, Ilya Sherman wrote:
>
http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
> File chrome/browser/autofill/autofill_merge_unittest.cc (right):
> 
>
http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
> chrome/browser/autofill/autofill_merge_unittest.cc:131:
> scoped_ptr<PersonalDataManagerMock> personal_data_;
> On 2011/09/23 22:00:58, dhollowa wrote:
> > On 2011/09/23 21:41:38, Ilya Sherman wrote:
> > > nit: Can this just be |PersonalDataManagerMock personal_data_|?
> > 
> > No.  The tests below reset the data with each pass of the data driven test.
> 
> Wait, where does this happen?  If you mean the SetUp() method, I believe that
is
> exactly equivalent to directly having a PersonalDataManagerMock instance
> variable (without the scoped_ptr).

AutofillMergeTest::MergeProfiles calls personal_data_->Reset().  But upon closer
inspection... this is |Reset()| and not |reset()|.  So you are correct, we could
eliminate the scoped_ptr.

[dhollowa, 2011-09-26 16:50:16]

On 2011/09/26 14:59:10, dhollowa wrote:
> On 2011/09/23 22:17:43, Ilya Sherman wrote:
> >
>
http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
> > File chrome/browser/autofill/autofill_merge_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/7967024/diff/14001/chrome/browser/autofill/aut...
> > chrome/browser/autofill/autofill_merge_unittest.cc:131:
> > scoped_ptr<PersonalDataManagerMock> personal_data_;
> > On 2011/09/23 22:00:58, dhollowa wrote:
> > > On 2011/09/23 21:41:38, Ilya Sherman wrote:
> > > > nit: Can this just be |PersonalDataManagerMock personal_data_|?
> > > 
> > > No.  The tests below reset the data with each pass of the data driven
test.
> > 
> > Wait, where does this happen?  If you mean the SetUp() method, I believe
that
> is
> > exactly equivalent to directly having a PersonalDataManagerMock instance
> > variable (without the scoped_ptr).
> 
> AutofillMergeTest::MergeProfiles calls personal_data_->Reset().  But upon
closer
> inspection... this is |Reset()| and not |reset()|.  So you are correct, we
could
> eliminate the scoped_ptr.

Follow-up in http://codereview.chromium.org/8043027