[Autofill] Dedupe profiles on each major version.

Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest

TBR=thestig@chromium.org
For the DEPS file where I added components/version_info. Thanks!

Committed: https://crrev.com/75b5126356c52587b9f3a8d8c5554529528c021e
Cr-Commit-Position: refs/heads/master@{#400773}

[sebsg, 2016-06-20 00:24:00]

Patchset #4 (id:60001) has been deleted

[sebsg, 2016-06-20 00:24:14]

Patchset #3 (id:40001) has been deleted

[sebsg, 2016-06-20 00:24:20]

Patchset #2 (id:20001) has been deleted

[sebsg, 2016-06-20 00:24:27]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-06-20 00:25:29]

Description was changed from

==========
[Autofill] Dedupe profiles on major version (WIP).

BUG=620422
==========

to

==========
Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest
==========

[sebsg, 2016-06-20 01:45:15]

sebsg@chromium.org changed reviewers:
+ mathp@chromium.org, rogerm@chromium.org

[sebsg, 2016-06-20 01:46:15]

Hi! could you please take a look? Thanks!

[Mathieu, 2016-06-20 12:10:08]

looks great! Lovely tests. 

Roger can you have a look?

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

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:329: // run them on
the same on the same query response.
fix sentence

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:330: if
(ApplyDedupingRoutine())
do you mean if (!ApplyDedupingRoutine())?

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1586: if
(pref_service_->GetInteger(prefs::kAutofillLastVersionDeduped) <
let's return early for the inverse condition here

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1643: // merged.
nit: "merged into another profile".

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1658: // because it
means that the profile has already been merged. Also don't
this line is clear enough, I think we can remove.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1665: // Try to merge
the two profiles.
Can you be more specific? I thought |profile_to_merge| was the most relevant due
to frecency, and now it's being deleted and the less relevant is kept. Expand a
little bit on what SaveAdditionalInfo does with the most relevant information,
so that the reader is not confused.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4406: //
frencency, the result of the merge should be in this profile at the end of
*frecency

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4450: //
Tests that ApplyDedupingRoutine merges the profile values correctly, ie:
i.e.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4547: //
Create a profile to dedupe wiht a higher frecency score.
fix sentence. Also, mention it's verified.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4615: //
Tests that ApplyDedupingRoutine only keeps the verified profile with it's
its

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4872: //
Tests that it merges the diffent set of similar profiles independantly and
independently

[sebsg, 2016-06-20 14:57:18]

Thanks for the comments, could you take another look? Thanks!

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

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:329: // run them on
the same on the same query response.
On 2016/06/20 12:10:07, Mathieu Perreault wrote:
> fix sentence

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:330: if
(ApplyDedupingRoutine())
On 2016/06/20 12:10:07, Mathieu Perreault wrote:
> do you mean if (!ApplyDedupingRoutine())?

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1586: if
(pref_service_->GetInteger(prefs::kAutofillLastVersionDeduped) <
On 2016/06/20 12:10:07, Mathieu Perreault wrote:
> let's return early for the inverse condition here

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1643: // merged.
On 2016/06/20 12:10:07, Mathieu Perreault wrote:
> nit: "merged into another profile".

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1658: // because it
means that the profile has already been merged. Also don't
On 2016/06/20 12:10:07, Mathieu Perreault wrote:
> this line is clear enough, I think we can remove.

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1665: // Try to merge
the two profiles.
On 2016/06/20 12:10:07, Mathieu Perreault wrote:
> Can you be more specific? I thought |profile_to_merge| was the most relevant
due
> to frecency, and now it's being deleted and the less relevant is kept. Expand
a
> little bit on what SaveAdditionalInfo does with the most relevant information,
> so that the reader is not confused.

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4406: //
frencency, the result of the merge should be in this profile at the end of
On 2016/06/20 12:10:08, Mathieu Perreault wrote:
> *frecency

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4450: //
Tests that ApplyDedupingRoutine merges the profile values correctly, ie:
On 2016/06/20 12:10:08, Mathieu Perreault wrote:
> i.e.

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4547: //
Create a profile to dedupe wiht a higher frecency score.
On 2016/06/20 12:10:07, Mathieu Perreault wrote:
> fix sentence. Also, mention it's verified.

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4615: //
Tests that ApplyDedupingRoutine only keeps the verified profile with it's
On 2016/06/20 12:10:08, Mathieu Perreault wrote:
> its

Done.

https://codereview.chromium.org/2074253002/diff/120001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:4872: //
Tests that it merges the diffent set of similar profiles independantly and
On 2016/06/20 12:10:08, Mathieu Perreault wrote:
> independently

Done.

[Mathieu, 2016-06-20 15:48:54]

Thanks lgtm

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

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1586: if
(!(pref_service_->GetInteger(prefs::kAutofillLastVersionDeduped) <
I think I would prefer >=, it's cleaner.

[Roger McFarlane (Chromium), 2016-06-20 18:40:04]

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

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1585: // Check if the
deduping routine was run on this major version.
nit:

...was already run...

or, better yet,

...has already been run...

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1591:
std::set<std::string> profile_guids_to_delete;
micro-optimizations:

* Why reference the profiles by guid? Why not a set of pointers?
* std::unordered_set

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1596:
pref_service_->SetInteger(prefs::kAutofillLastVersionDeduped,
unless there is no possibility of failure beyond this point, do this at the end,
right before the refresh.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1618:
std::set<std::string>* profile_guids_to_delete) {
unordered_set, reserved existing_profiles.size()

?

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1635:
AutofillMetrics::LogNumberOfProfilesConsideredForDedupe(
move the log to the beginning... since it's not a part of the actual dedup algo.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1652: if (i + 1 <
existing_profiles->size()) {
this is redundant with the bounds in the enclosed for

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1680: if
(profile_to_merge->IsVerified())
this check seems too late.

I think you need to check if existing_profile->IsVerified before trying to save
additional info into it, no?

[sebsg, 2016-06-20 19:12:20]

Thanks for the comments. Another look?

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

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1585: // Check if the
deduping routine was run on this major version.
On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> nit:
> 
> ...was already run...
> 
> or, better yet,
> 
> ...has already been run...

Done.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1586: if
(!(pref_service_->GetInteger(prefs::kAutofillLastVersionDeduped) <
On 2016/06/20 15:48:54, Mathieu Perreault wrote:
> I think I would prefer >=, it's cleaner.

Done.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1591:
std::set<std::string> profile_guids_to_delete;
On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> micro-optimizations:
> 
> * Why reference the profiles by guid? Why not a set of pointers?
> * std::unordered_set

Done.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1596:
pref_service_->SetInteger(prefs::kAutofillLastVersionDeduped,
On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> unless there is no possibility of failure beyond this point, do this at the
end,
> right before the refresh.

Done.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1618:
std::set<std::string>* profile_guids_to_delete) {
On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> unordered_set, reserved existing_profiles.size()
> 
> ?

Done.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1635:
AutofillMetrics::LogNumberOfProfilesConsideredForDedupe(
On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> move the log to the beginning... since it's not a part of the actual dedup
algo.

Done.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1652: if (i + 1 <
existing_profiles->size()) {
On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> this is redundant with the bounds in the enclosed for

Done.

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1680: if
(profile_to_merge->IsVerified())
On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> this check seems too late.
> 
> I think you need to check if existing_profile->IsVerified before trying to
save
> additional info into it, no?
> 
The |existing_profile| can (and should sometimes) be verified. If that's the
case, SaveAdditionnalInfo will not call OverwriteWith. So the verified profile
will not be changed but the similar |profile_to_merge| will be deleted. However,
|profile_to_merge| should never be verified, because verified profiles do not
merge into other profiles.

When entering the inner loop at line 1655 we are sure that |profile_to_merge| is
not verified. The only moment where it can become verified is right after the
assignation at line 1676. That's why that condition is there.

[Roger McFarlane (Chromium), 2016-06-20 19:18:37]

lgtm

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

https://codereview.chromium.org/2074253002/diff/140001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.cc:1680: if
(profile_to_merge->IsVerified())
On 2016/06/20 19:12:19, sebsg wrote:
> On 2016/06/20 18:40:04, Roger McFarlane (Chromium) wrote:
> > this check seems too late.
> > 
> > I think you need to check if existing_profile->IsVerified before trying to
> save
> > additional info into it, no?
> > 
> The |existing_profile| can (and should sometimes) be verified. If that's the
> case, SaveAdditionnalInfo will not call OverwriteWith. So the verified profile
> will not be changed but the similar |profile_to_merge| will be deleted.
However,
> |profile_to_merge| should never be verified, because verified profiles do not
> merge into other profiles.
> 
> When entering the inner loop at line 1655 we are sure that |profile_to_merge|
is
> not verified. The only moment where it can become verified is right after the
> assignation at line 1676. That's why that condition is there.

Acknowledged.

[sebsg, 2016-06-20 19:19:28]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-06-20 19:19:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2074253002/#ps160001
(title: "Addressed rogerm's comments")

[commit-bot: I haz the power, 2016-06-20 19:19:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2074253002/160001

[commit-bot: I haz the power, 2016-06-20 19:29:36]

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

[commit-bot: I haz the power, 2016-06-20 19:29:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sebsg, 2016-06-20 19:35:41]

Description was changed from

==========
Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest
==========

to

==========
Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest

TBR=thestig@chromium.org
For the DEPS file where I added components/version_info. Thanks!
==========

[sebsg, 2016-06-20 19:35:52]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2016-06-20 19:36:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2074253002/160001

[commit-bot: I haz the power, 2016-06-20 20:46:45]

Description was changed from

==========
Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest

TBR=thestig@chromium.org
For the DEPS file where I added components/version_info. Thanks!
==========

to

==========
Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest

TBR=thestig@chromium.org
For the DEPS file where I added components/version_info. Thanks!
==========

[commit-bot: I haz the power, 2016-06-20 20:46:49]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-06-20 20:48:55]

Description was changed from

==========
Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest

TBR=thestig@chromium.org
For the DEPS file where I added components/version_info. Thanks!
==========

to

==========
Add a profile deduping routine to run on each major version.

BUG=620422
TEST=PersonalDataManagerTest

TBR=thestig@chromium.org
For the DEPS file where I added components/version_info. Thanks!

Committed: https://crrev.com/75b5126356c52587b9f3a8d8c5554529528c021e
Cr-Commit-Position: refs/heads/master@{#400773}
==========

[commit-bot: I haz the power, 2016-06-20 20:48:56]

Patchset 5 (id:??) landed as
https://crrev.com/75b5126356c52587b9f3a8d8c5554529528c021e
Cr-Commit-Position: refs/heads/master@{#400773}