[Autofill] Run autofill-profile de-dupe after sync starts if sync is enabled.

Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updates the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

In all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org

Committed: https://crrev.com/d594dc5f82b5836f5f0b4ba8d52b8563ff3c2f27
Cr-Commit-Position: refs/heads/master@{#406026}

[Roger McFarlane (Chromium), 2016-07-12 17:37:15]

PTAL?

[Mathieu, 2016-07-12 17:46:10]

Description was changed from

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updated the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

I all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org
==========

to

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updated the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

I all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org
==========

[Mathieu, 2016-07-12 17:46:39]

Description was changed from

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updated the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

I all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org
==========

to

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updates the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

In all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org
==========

[Mathieu, 2016-07-12 18:10:12]

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
components/autofill/core/browser/personal_data_manager.cc:747: void
PersonalDataManager::Refresh() {
Refresh is called all over the place in this file. Could there be a more
appropriate place to start the dedupe? I don't know that it's guaranteed that
sync is initialized at this point.

[Roger McFarlane (Chromium), 2016-07-12 18:42:20]

Thanks. Another look?

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
components/autofill/core/browser/personal_data_manager.cc:747: void
PersonalDataManager::Refresh() {
On 2016/07/12 18:10:12, Mathieu Perreault wrote:
> Refresh is called all over the place in this file. Could there be a more
> appropriate place to start the dedupe? I don't know that it's guaranteed that
> sync is initialized at this point.

Done.

[sebsg, 2016-07-12 18:59:53]

Awesome! I'd like to have zea@ take a look but this lgtm.

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
components/autofill/core/browser/personal_data_manager.cc:306: // We want to
know if we when, if at all, we need to run autofill profile
nit: We want to know if we when, ... -> We want to know when, ... ?

https://codereview.chromium.org/2142123002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1632: if
(!is_autofill_profile_dedupe_pending_) {
optional: This is checked in the two call sites for this method so it does not
seem necessary. But I don't really mind if you leave it.

https://codereview.chromium.org/2142123002/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1634: }
nit: Remove the {} from the one-line if. To be more consistent with the rest of
the file.

[Roger McFarlane (Chromium), 2016-07-12 19:52:48]

rogerm@chromium.org changed reviewers:
+ zea@chromium.org

[Roger McFarlane (Chromium), 2016-07-12 19:52:49]

thanks.

+zea@chromium.org for sync insight

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/1/components/autofill/core/br...
components/autofill/core/browser/personal_data_manager.cc:306: // We want to
know if we when, if at all, we need to run autofill profile
On 2016/07/12 18:59:53, sebsg wrote:
> nit: We want to know if we when, ... -> We want to know when, ... ?

Done.

https://codereview.chromium.org/2142123002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1632: if
(!is_autofill_profile_dedupe_pending_) {
On 2016/07/12 18:59:53, sebsg wrote:
> optional: This is checked in the two call sites for this method so it does not
> seem necessary. But I don't really mind if you leave it.

Yeah, I went back and forth on this one too.

I think it's clearer in AutofillMultipleChanged() to check there (as opposed to
calling the function all the time with move of a "MaybeApplyDedupingRoutine"
semantic (although maybe the solution is a rename?).  But the tests largely
expect the function to police itself.

https://codereview.chromium.org/2142123002/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1634: }
On 2016/07/12 18:59:53, sebsg wrote:
> nit: Remove the {} from the one-line if. To be more consistent with the rest
of
> the file.

Done.

[Nicolas Zea, 2016-07-12 20:04:48]

Couple questions

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:219:
personal_data_->OnSyncServiceConfigured(client_->GetSyncService());
How do you know that the sync service is configured at this point in time? Kind
of seems like a strange place to make this call, but I'm not too familiar with
the autofill initialization process.

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:403: if
(is_autofill_profile_dedupe_pending_)
Could this short circuit the delay from sync you set above?

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1642: VLOG(1)
nit: did you mean to do DVLOG, rather than VLOG? (VLOG statements should
typically be limited to debugging existing issues, as they introduce unnecessary
strings into the binary).

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:314: // then we can
just let the notification that sync has started trigger the
where is the notification that sync has started coming form?

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:84: void
OnSyncServiceConfigured(sync_driver::SyncService* sync_service);
Perhaps rename this OnSyncServiceInitialized? "Configured" has a special meaning
within sync, as in it implies that sync has gone through the datatype
configuration (which implies the datatypes have started). That may not be the
case when this is called.

[Roger McFarlane (Chromium), 2016-07-12 21:23:27]

Thanks for the great questions and comments.

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:219:
personal_data_->OnSyncServiceConfigured(client_->GetSyncService());
On 2016/07/12 20:04:47, Nicolas Zea wrote:
> How do you know that the sync service is configured at this point in time?
Kind
> of seems like a strange place to make this call, but I'm not too familiar with
> the autofill initialization process.

Because we're in the context of the AutofillManager, which is given a bunch of
existing and otherwise usable things to manage.

OnSyncServiceConfigured might be an overly strong name. Perhaps it would be
better named "OnValidSyncServicePointerAvailable()"

:)

All I really need at this point is a valid SyncService instance that I can query
about whether it will eventually be doing autofill profile syncing.

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:403: if
(is_autofill_profile_dedupe_pending_)
On 2016/07/12 20:04:47, Nicolas Zea wrote:
> Could this short circuit the delay from sync you set above?

Not that I've seen

From what I can tell, this doesn't get called for the first time until the end
of MergeDataAndStartSyncing(). and if sync isn't active, it doesn't get called
at all.

That said, this is based on observational data. I don't have the full picture of
all the corner cases of how this works.

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:1642: VLOG(1)
On 2016/07/12 20:04:47, Nicolas Zea wrote:
> nit: did you mean to do DVLOG, rather than VLOG? (VLOG statements should
> typically be limited to debugging existing issues, as they introduce
unnecessary
> strings into the binary).

Yes, thanks!

Done.

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:314: // then we can
just let the notification that sync has started trigger the
On 2016/07/12 20:04:47, Nicolas Zea wrote:
> where is the notification that sync has started coming form?

AutofillProfileSyncableProfile::MergeDataAndStartSyncing()

https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata...

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/2142123002/diff/60001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.h:84: void
OnSyncServiceConfigured(sync_driver::SyncService* sync_service);
On 2016/07/12 20:04:47, Nicolas Zea wrote:
> Perhaps rename this OnSyncServiceInitialized? "Configured" has a special
meaning
> within sync, as in it implies that sync has gone through the datatype
> configuration (which implies the datatypes have started). That may not be the
> case when this is called.

Done.

[Nicolas Zea, 2016-07-12 23:16:17]

One suggestion

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:403: if
(is_autofill_profile_dedupe_pending_)
On 2016/07/12 21:23:26, Roger McFarlane (Chromium) wrote:
> On 2016/07/12 20:04:47, Nicolas Zea wrote:
> > Could this short circuit the delay from sync you set above?
> 
> Not that I've seen
> 
> From what I can tell, this doesn't get called for the first time until the end
> of MergeDataAndStartSyncing(). and if sync isn't active, it doesn't get called
> at all.
> 
> That said, this is based on observational data. I don't have the full picture
of
> all the corner cases of how this works.

I'm worried this might be a bit brittle. I think it would be better to have a
OnSyncMergeFinished method for any post-sync work you want to perform, and have
the MergeDataAndStartSyncing method call it directly.

[Roger McFarlane (Chromium), 2016-07-13 21:10:30]

On 2016/07/12 23:16:17, Nicolas Zea wrote:
> One suggestion
> 
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> File components/autofill/core/browser/personal_data_manager.cc (right):
> 
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> components/autofill/core/browser/personal_data_manager.cc:403: if
> (is_autofill_profile_dedupe_pending_)
> On 2016/07/12 21:23:26, Roger McFarlane (Chromium) wrote:
> > On 2016/07/12 20:04:47, Nicolas Zea wrote:
> > > Could this short circuit the delay from sync you set above?
> > 
> > Not that I've seen
> > 
> > From what I can tell, this doesn't get called for the first time until the
end
> > of MergeDataAndStartSyncing(). and if sync isn't active, it doesn't get
called
> > at all.
> > 
> > That said, this is based on observational data. I don't have the full
picture
> of
> > all the corner cases of how this works.
> 
> I'm worried this might be a bit brittle. I think it would be better to have a
> OnSyncMergeFinished method for any post-sync work you want to perform, and
have
> the MergeDataAndStartSyncing method call it directly.

will do.

[Roger McFarlane (Chromium), 2016-07-13 21:44:38]

On 2016/07/13 21:10:30, Roger McFarlane (Chromium) wrote:
> On 2016/07/12 23:16:17, Nicolas Zea wrote:
> > One suggestion
> > 
> >
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> > File components/autofill/core/browser/personal_data_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> > components/autofill/core/browser/personal_data_manager.cc:403: if
> > (is_autofill_profile_dedupe_pending_)
> > On 2016/07/12 21:23:26, Roger McFarlane (Chromium) wrote:
> > > On 2016/07/12 20:04:47, Nicolas Zea wrote:
> > > > Could this short circuit the delay from sync you set above?
> > > 
> > > Not that I've seen
> > > 
> > > From what I can tell, this doesn't get called for the first time until the
> end
> > > of MergeDataAndStartSyncing(). and if sync isn't active, it doesn't get
> called
> > > at all.
> > > 
> > > That said, this is based on observational data. I don't have the full
> picture
> > of
> > > all the corner cases of how this works.
> > 
> > I'm worried this might be a bit brittle. I think it would be better to have
a
> > OnSyncMergeFinished method for any post-sync work you want to perform, and
> have
> > the MergeDataAndStartSyncing method call it directly.
> 
> will do.

So, I'm looking at adding a handler to one or both of the
AutofillWebDataServiceObserver* interfaces.

I'm not intimate with the thread affinity of this stuff, but it looks like the
hook I've added currently running on the UI thread after sync start (because
PersonalDataManager implements AutofillWebDataServiceObserverOnUIThread). That
doesn't seem appropriate.

i.e., 

Some quick spelunking seems to indicate that
PersonalDataManager::OnWebDataServiceRequestDone is called on the DB thread and
PersonalDataManager::AutofillMultipleChanged is called on the UI thread.

Thoughts?

[Nicolas Zea, 2016-07-13 22:02:59]

On 2016/07/13 21:44:38, Roger McFarlane (Chromium) wrote:
> On 2016/07/13 21:10:30, Roger McFarlane (Chromium) wrote:
> > On 2016/07/12 23:16:17, Nicolas Zea wrote:
> > > One suggestion
> > > 
> > >
> >
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> > > File components/autofill/core/browser/personal_data_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> > > components/autofill/core/browser/personal_data_manager.cc:403: if
> > > (is_autofill_profile_dedupe_pending_)
> > > On 2016/07/12 21:23:26, Roger McFarlane (Chromium) wrote:
> > > > On 2016/07/12 20:04:47, Nicolas Zea wrote:
> > > > > Could this short circuit the delay from sync you set above?
> > > > 
> > > > Not that I've seen
> > > > 
> > > > From what I can tell, this doesn't get called for the first time until
the
> > end
> > > > of MergeDataAndStartSyncing(). and if sync isn't active, it doesn't get
> > called
> > > > at all.
> > > > 
> > > > That said, this is based on observational data. I don't have the full
> > picture
> > > of
> > > > all the corner cases of how this works.
> > > 
> > > I'm worried this might be a bit brittle. I think it would be better to
have
> a
> > > OnSyncMergeFinished method for any post-sync work you want to perform, and
> > have
> > > the MergeDataAndStartSyncing method call it directly.
> > 
> > will do.
> 
> So, I'm looking at adding a handler to one or both of the
> AutofillWebDataServiceObserver* interfaces.
> 
> I'm not intimate with the thread affinity of this stuff, but it looks like the
> hook I've added currently running on the UI thread after sync start (because
> PersonalDataManager implements AutofillWebDataServiceObserverOnUIThread). That
> doesn't seem appropriate.
> 
> i.e., 
> 
> Some quick spelunking seems to indicate that
> PersonalDataManager::OnWebDataServiceRequestDone is called on the DB thread
and
> PersonalDataManager::AutofillMultipleChanged is called on the UI thread.
> 
> Thoughts?

Not sure about the thread semantics of the PersonalDataManger, but note that the
AutofillProfileSyncableService lives on the DB thread.

[Roger McFarlane (Chromium), 2016-07-13 22:53:40]

Yes, but the notification the CL is currently leveraging invokes callbacks
on both the DB and (for PersonalDataManager) UI threads.

On Jul 13, 2016 6:02 PM, <zea@chromium.org> wrote:

> On 2016/07/13 21:44:38, Roger McFarlane (Chromium) wrote:
> > On 2016/07/13 21:10:30, Roger McFarlane (Chromium) wrote:
> > > On 2016/07/12 23:16:17, Nicolas Zea wrote:
> > > > One suggestion
> > > >
> > > >
> > >
> >
>
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> > > > File components/autofill/core/browser/personal_data_manager.cc
> (right):
> > > >
> > > >
> > >
> >
>
>
https://codereview.chromium.org/2142123002/diff/40001/components/autofill/cor...
> > > > components/autofill/core/browser/personal_data_manager.cc:403: if
> > > > (is_autofill_profile_dedupe_pending_)
> > > > On 2016/07/12 21:23:26, Roger McFarlane (Chromium) wrote:
> > > > > On 2016/07/12 20:04:47, Nicolas Zea wrote:
> > > > > > Could this short circuit the delay from sync you set above?
> > > > >
> > > > > Not that I've seen
> > > > >
> > > > > From what I can tell, this doesn't get called for the first time
> until
> the
> > > end
> > > > > of MergeDataAndStartSyncing(). and if sync isn't active, it
> doesn't get
> > > called
> > > > > at all.
> > > > >
> > > > > That said, this is based on observational data. I don't have the
> full
> > > picture
> > > > of
> > > > > all the corner cases of how this works.
> > > >
> > > > I'm worried this might be a bit brittle. I think it would be better
> to
> have
> > a
> > > > OnSyncMergeFinished method for any post-sync work you want to
> perform, and
> > > have
> > > > the MergeDataAndStartSyncing method call it directly.
> > >
> > > will do.
> >
> > So, I'm looking at adding a handler to one or both of the
> > AutofillWebDataServiceObserver* interfaces.
> >
> > I'm not intimate with the thread affinity of this stuff, but it looks
> like the
> > hook I've added currently running on the UI thread after sync start
> (because
> > PersonalDataManager implements
> AutofillWebDataServiceObserverOnUIThread). That
> > doesn't seem appropriate.
> >
> > i.e.,
> >
> > Some quick spelunking seems to indicate that
> > PersonalDataManager::OnWebDataServiceRequestDone is called on the DB
> thread
> and
> > PersonalDataManager::AutofillMultipleChanged is called on the UI thread.
> >
> > Thoughts?
>
> Not sure about the thread semantics of the PersonalDataManger, but note
> that the
> AutofillProfileSyncableService lives on the DB thread.
>
> https://codereview.chromium.org/2142123002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Roger McFarlane (Chromium), 2016-07-13 22:57:52]

To summarize:

I'm thinking the dedup should run on the DB thread. Was seeking
confirmation.  :)

I'll look at getting making sure it's done there.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Roger McFarlane (Chromium), 2016-07-14 21:47:11]

I propose to

  1. remove the AutofillWebDataServiceObserverOnUIThread interface
     (this is a subset of the AutofillWebDataServiceObserverOnDBThread
interface)
  2. have PersonalDataManager implement the DB thread version instead
  3. Have the PersonalDataManager::AutofillMultipleChanged event POST the
     real handler to the UI thread

This is exactly the same behavior that happens currently, except the knowledge
about posting to the UI thread is CURRENTLY in the AutofillWebDataService
implementation and will be MOVED to the PersonalDataManager implementation.

Lastly:

  4. Add a SyncStarted(syncer::ModelType) method
AutofillWebDataServiceObserverOnDBThread.
  5. Trigger dedupe (if enabled) in PersonalDAtaManager on this event.
  6. (optional?) Generate the event in the other
SyncableService::MergeDataAndStartSyncing methods.  

Rationale/Discussion:

* AutofillWebDataServiceObserverOnUIThread has exactly one (1) subclass:
    -> PersonalDataManager

* AutofillWebDataServiceObserverOnUIThread is otherwise referenced only in
  AutofillWebDataService to manage the (single element) ui thread subscriber
  list and post a call to the handler to the UI thread.

* Otherwise, AutofillWebDataService (and its sibling subclasses of
SyncableService)
  only deal with AutofillWebDataServiceObserverOnDBThread observers.

* It would be nice to encapsulate the generation of this event at the location
where
  MergeDataAndStartSyncing is called, but that occurs at one level of
abstraction too high.

I don't think it's as large a change is it may sound.

Thoughts?

[Mathieu, 2016-07-15 13:54:19]

On 2016/07/14 21:47:11, Roger McFarlane (Chromium) wrote:
> I propose to
> 
>   1. remove the AutofillWebDataServiceObserverOnUIThread interface
>      (this is a subset of the AutofillWebDataServiceObserverOnDBThread
> interface)
>   2. have PersonalDataManager implement the DB thread version instead
>   3. Have the PersonalDataManager::AutofillMultipleChanged event POST the
>      real handler to the UI thread
> 
> This is exactly the same behavior that happens currently, except the knowledge
> about posting to the UI thread is CURRENTLY in the AutofillWebDataService
> implementation and will be MOVED to the PersonalDataManager implementation.
> 
> Lastly:
> 
>   4. Add a SyncStarted(syncer::ModelType) method
> AutofillWebDataServiceObserverOnDBThread.
>   5. Trigger dedupe (if enabled) in PersonalDAtaManager on this event.
>   6. (optional?) Generate the event in the other
> SyncableService::MergeDataAndStartSyncing methods.  
> 
> Rationale/Discussion:
> 
> * AutofillWebDataServiceObserverOnUIThread has exactly one (1) subclass:
>     -> PersonalDataManager
> 
> * AutofillWebDataServiceObserverOnUIThread is otherwise referenced only in
>   AutofillWebDataService to manage the (single element) ui thread subscriber
>   list and post a call to the handler to the UI thread.
> 
> * Otherwise, AutofillWebDataService (and its sibling subclasses of
> SyncableService)
>   only deal with AutofillWebDataServiceObserverOnDBThread observers.
> 
> * It would be nice to encapsulate the generation of this event at the location
> where
>   MergeDataAndStartSyncing is called, but that occurs at one level of
> abstraction too high.
> 
> I don't think it's as large a change is it may sound.
> 
> Thoughts?

As discussed offline, it adds non-trivial complexity if the PersonalDataManager
to POST to the DB thread (I think it was part of the design that it wouldn't do
so but rather rely on the AutofillWebDataService to post DB tasks and come back
with the result). 

Also, given we have two entrypaths to this logic (non-sync, in PDM, and
with-sync, from AutofillProfileSyncableService), I think it makes sense to have
a notification on the UI thread if it's triggered from sync. Having a specific
call and not reusing AutofillMultipleChanged sounds good to me, too.

[Roger McFarlane (Chromium), 2016-07-15 19:15:44]

The CQ bit was checked by rogerm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 19:16:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roger McFarlane (Chromium), 2016-07-15 19:20:57]

On 2016/07/15 13:54:19, Mathieu Perreault wrote:
> On 2016/07/14 21:47:11, Roger McFarlane (Chromium) wrote:
> > I propose to
> > 
> >   1. remove the AutofillWebDataServiceObserverOnUIThread interface
> >      (this is a subset of the AutofillWebDataServiceObserverOnDBThread
> > interface)
> >   2. have PersonalDataManager implement the DB thread version instead
> >   3. Have the PersonalDataManager::AutofillMultipleChanged event POST the
> >      real handler to the UI thread
> > 
> > This is exactly the same behavior that happens currently, except the
knowledge
> > about posting to the UI thread is CURRENTLY in the AutofillWebDataService
> > implementation and will be MOVED to the PersonalDataManager implementation.
> > 
> > Lastly:
> > 
> >   4. Add a SyncStarted(syncer::ModelType) method
> > AutofillWebDataServiceObserverOnDBThread.
> >   5. Trigger dedupe (if enabled) in PersonalDAtaManager on this event.
> >   6. (optional?) Generate the event in the other
> > SyncableService::MergeDataAndStartSyncing methods.  
> > 
> > Rationale/Discussion:
> > 
> > * AutofillWebDataServiceObserverOnUIThread has exactly one (1) subclass:
> >     -> PersonalDataManager
> > 
> > * AutofillWebDataServiceObserverOnUIThread is otherwise referenced only in
> >   AutofillWebDataService to manage the (single element) ui thread subscriber
> >   list and post a call to the handler to the UI thread.
> > 
> > * Otherwise, AutofillWebDataService (and its sibling subclasses of
> > SyncableService)
> >   only deal with AutofillWebDataServiceObserverOnDBThread observers.
> > 
> > * It would be nice to encapsulate the generation of this event at the
location
> > where
> >   MergeDataAndStartSyncing is called, but that occurs at one level of
> > abstraction too high.
> > 
> > I don't think it's as large a change is it may sound.
> > 
> > Thoughts?
> 
> As discussed offline, it adds non-trivial complexity if the
PersonalDataManager
> to POST to the DB thread (I think it was part of the design that it wouldn't
do
> so but rather rely on the AutofillWebDataService to post DB tasks and come
back
> with the result). 
> 
> Also, given we have two entrypaths to this logic (non-sync, in PDM, and
> with-sync, from AutofillProfileSyncableService), I think it makes sense to
have
> a notification on the UI thread if it's triggered from sync. Having a specific
> call and not reusing AutofillMultipleChanged sounds good to me, too.

Ok, per our walkthrough, it looks like the UI thread is indeed the appropriate
place for this to be handled. So I've just added the callback to the
....UIThread interface.

I've also added a break to
AutofillProfileSyncableService::CreateOrUpdateProfile() so that it only merges a
new incoming profile with the first local profile that matches, deferring larger
dedupe merges to the once-per-version profile dedup routing.

[commit-bot: I haz the power, 2016-07-15 20:05:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 20:05:06]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2016-07-15 21:35:35]

Much cleaner! sync lgtm

[Mathieu, 2016-07-18 14:53:20]

Thanks, this lgtm

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1636: if
(!is_autofill_profile_dedupe_pending_)
are these two lines necessary? Seems like callers have already checked before we
got here.

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_webdata_backend.h
(right):

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_webdata_backend.h:42: //
Notifies listeners on both DB and UI threads that sync has started for
Not sure I see how DB thread observers are notified?

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_webdata_service.cc
(right):

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_webdata_service.cc:59: NULL,
nit: nullptr

[Roger McFarlane (Chromium), 2016-07-18 15:27:26]

The CQ bit was checked by rogerm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 15:28:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roger McFarlane (Chromium), 2016-07-18 15:29:19]

Thanks!

mathp@... I addressed your comments and added some more logging.

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1636: if
(!is_autofill_profile_dedupe_pending_)
On 2016/07/18 14:53:19, Mathieu Perreault wrote:
> are these two lines necessary? Seems like callers have already checked before
we
> got here.

there's a slight mismatch between how the tests expect this to be called and how
it is actually called.

The tests treat the function as if it was called "MaybeApplyDedupingRoutine" but
the real call sites subsume the "maybe" and call it only when it will actually
be applied.

In a follow-on CL, I'll fix the tests and turn this into a DCHECK.

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_webdata_backend.h
(right):

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_webdata_backend.h:42: //
Notifies listeners on both DB and UI threads that sync has started for
On 2016/07/18 14:53:19, Mathieu Perreault wrote:
> Not sure I see how DB thread observers are notified?

Copy-paste error, fixed.

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_webdata_service.cc
(right):

https://codereview.chromium.org/2142123002/diff/140001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_webdata_service.cc:59: NULL,
On 2016/07/18 14:53:20, Mathieu Perreault wrote:
> nit: nullptr

Done.

[commit-bot: I haz the power, 2016-07-18 16:36:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 16:36:02]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2016-07-18 17:27:04]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-07-18 17:27:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org,
mathp@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2142123002/#ps160001
(title: "Better logging and fix mathp's comments")

[commit-bot: I haz the power, 2016-07-18 17:27:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 17:34:23]

Description was changed from

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updates the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

In all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org
==========

to

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updates the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

In all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org
==========

[commit-bot: I haz the power, 2016-07-18 17:34:25]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-18 17:35:55]

Description was changed from

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updates the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

In all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org
==========

to

==========
Run autofill-profile de-dupe after sync starts if sync is enabled.

This CL updates the PersonalDataManager to run autofill profile
de-duplication (if enabled) at the appropriate time, depending on
whether autofill profile sync is enabled.

If sync is not enabled, profile de-duplication is run after the
data is initially loaded.

If sync is enabled, profild de-duplication is run after the sync
first becomes active.

In all cases, de-duplication is only run once for a given version
of chromium.

BUG=627519
R=sebsg@chromium.org, mathp@chromium.org

Committed: https://crrev.com/d594dc5f82b5836f5f0b4ba8d52b8563ff3c2f27
Cr-Commit-Position: refs/heads/master@{#406026}
==========

[commit-bot: I haz the power, 2016-07-18 17:35:56]

Patchset 9 (id:??) landed as
https://crrev.com/d594dc5f82b5836f5f0b4ba8d52b8563ff3c2f27
Cr-Commit-Position: refs/heads/master@{#406026}