Autofill Multi-Value: Additional profiles with same name and address not added to DOM UI

Autofill Multi-Value: Additional profiles with same name and address not added to DOM UI

Separates out the merging from |PersonalDataManager::AddProfile| so that merging only happens on form submission, and not when the user edits profiles manually via preferences.

BUG=80341
TEST=PersonalDataManagerTest.AddProfile

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

[Ilya Sherman, 2011-04-27 23:32:43]

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

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:852: SetProfiles(&profiles);
nit: Will we still be calling SetProfiles() here, or do we want a more
individuated call into the database here as well?  I notice that you didn't add
a TODO for crbug.com/73068 here.

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

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:103:
MessageLoop::current()->Run();
nit: Please move lines 97-103 to the top of the function, since they do not
depend on |profile0|.

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:125:
personal_data_->AddProfile(profile0a);
nit: Why not just """personal_data_->AddProfile(profile0);"""?

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:143: "94102", "USA",
"4158889999", "4152223333");
nit: This would be tidier if written as:

AutofillProfile profile1 = profile0;
profile1.SetInfo(EMAIL_ADDRESS, "john@smith.com";

[Ilya Sherman, 2011-04-27 23:39:49]

LGTM w/ nits addressed

[dhollowa, 2011-04-28 00:08:46]

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

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:852: SetProfiles(&profiles);
TBD, however since the merge can either add or update I'd predict SetProfiles is
the right tool for the job.

On 2011/04/27 23:32:43, Ilya Sherman wrote:
> nit: Will we still be calling SetProfiles() here, or do we want a more
> individuated call into the database here as well?  I notice that you didn't
add
> a TODO for crbug.com/73068 here.

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

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:103:
MessageLoop::current()->Run();
On 2011/04/27 23:32:43, Ilya Sherman wrote:
> nit: Please move lines 97-103 to the top of the function, since they do not
> depend on |profile0|.

Done.

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:125:
personal_data_->AddProfile(profile0a);
To ensure that the same guid values don't cause rejection.  I'm testing for the
values.  I agree with the comment below though, so I'll do a copy and generate a
new guid.

On 2011/04/27 23:32:43, Ilya Sherman wrote:
> nit: Why not just """personal_data_->AddProfile(profile0);"""?

http://codereview.chromium.org/6903091/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:143: "94102", "USA",
"4158889999", "4152223333");
On 2011/04/27 23:32:43, Ilya Sherman wrote:
> nit: This would be tidier if written as:
> 
> AutofillProfile profile1 = profile0;
> profile1.SetInfo(EMAIL_ADDRESS, "john@smith.com";

Done.  But the guid has to be accounted for.

[Ilya Sherman, 2011-04-28 00:26:11]

LGTM, thanks.