AutoFill: Aggregate profile data. Remove the AutoFill InfoBar.

AutoFill: Aggregate profile data. Remove the AutoFill InfoBar. Remove more remnants of shipping address and CVV.

BUG=47426, 47423
TEST=PersonalDataManager.*

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

[dhollowa, 2010-06-28 22:57:28]

http://codereview.chromium.org/2818033/diff/1/4
File chrome/browser/autofill/autofill_common_unittest.cc (right):

http://codereview.chromium.org/2818033/diff/1/4#newcode5
chrome/browser/autofill/autofill_common_unittest.cc:5: #include
"chrome/browser/autofill/autofill_common_unittest.h"
nit: Blank line after this one please.

http://codereview.chromium.org/2818033/diff/1/4#newcode21
chrome/browser/autofill/autofill_common_unittest.cc:21: #define
CHECK_AND_SET(type, value) \
Please use inline function here instead.

http://codereview.chromium.org/2818033/diff/1/4#newcode47
chrome/browser/autofill/autofill_common_unittest.cc:47: void
SetCreditCardInfo(CreditCard* credit_card,
To keep parity with |SetProfileInfo| should have CHECK_AND_SET here too (but
with inline function of course).

http://codereview.chromium.org/2818033/diff/1/4#newcode68
chrome/browser/autofill/autofill_common_unittest.cc:68: }  // namespace
nit: namespace autofill_unittest

http://codereview.chromium.org/2818033/diff/1/5
File chrome/browser/autofill/autofill_common_unittest.h (right):

http://codereview.chromium.org/2818033/diff/1/5#newcode13
chrome/browser/autofill/autofill_common_unittest.h:13: }
nit: // namespace webkit_glue

http://codereview.chromium.org/2818033/diff/1/5#newcode44
chrome/browser/autofill/autofill_common_unittest.h:44: }  // namespace
nit: // namespace autofill_unittest

http://codereview.chromium.org/2818033/diff/1/15
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2818033/diff/1/15#newcode115
chrome/browser/autofill/autofill_profile_unittest.cc:115: "Adams",
"declaration_guy@gmail.com", NULL, NULL, NULL, NULL, NULL,
I think we're ok here, but what happens if the only difference between a and b
is a NULL string verus and empty string ""?  My read of the code leads me to
believe that AutoFillProfile::GetFieldText will normalize NULL to "", so we'll
be ok.  You agree?

http://codereview.chromium.org/2818033/diff/1/15#newcode148
chrome/browser/autofill/autofill_profile_unittest.cc:148:
EXPECT_FALSE(a->IntersectionOfTypesHasEqualValues(*b));
Would be worth a test for when a and b do not intersect.  Maybe worth a comment
in the header as well.

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

http://codereview.chromium.org/2818033/diff/1/21#newcode81
chrome/browser/autofill/personal_data_manager.cc:81: // |profiles| may be NULL
nit: end with a period.  (And below).

http://codereview.chromium.org/2818033/diff/1/21#newcode89
chrome/browser/autofill/personal_data_manager.cc:89:
CancelPendingQuery(&pending_creditcards_query_);
Should we have locking parity here too?

[James Hawkins, 2010-06-28 23:29:58]

http://codereview.chromium.org/2818033/diff/1/4
File chrome/browser/autofill/autofill_common_unittest.cc (right):

http://codereview.chromium.org/2818033/diff/1/4#newcode5
chrome/browser/autofill/autofill_common_unittest.cc:5: #include
"chrome/browser/autofill/autofill_common_unittest.h"
On 2010/06/28 22:57:28, dhollowa wrote:
> nit: Blank line after this one please.

Done.

http://codereview.chromium.org/2818033/diff/1/4#newcode21
chrome/browser/autofill/autofill_common_unittest.cc:21: #define
CHECK_AND_SET(type, value) \
On 2010/06/28 22:57:28, dhollowa wrote:
> Please use inline function here instead.

Done.

http://codereview.chromium.org/2818033/diff/1/4#newcode47
chrome/browser/autofill/autofill_common_unittest.cc:47: void
SetCreditCardInfo(CreditCard* credit_card,
On 2010/06/28 22:57:28, dhollowa wrote:
> To keep parity with |SetProfileInfo| should have CHECK_AND_SET here too (but
> with inline function of course).

Done.

http://codereview.chromium.org/2818033/diff/1/4#newcode68
chrome/browser/autofill/autofill_common_unittest.cc:68: }  // namespace
On 2010/06/28 22:57:28, dhollowa wrote:
> nit: namespace autofill_unittest

Done.

http://codereview.chromium.org/2818033/diff/1/5
File chrome/browser/autofill/autofill_common_unittest.h (right):

http://codereview.chromium.org/2818033/diff/1/5#newcode13
chrome/browser/autofill/autofill_common_unittest.h:13: }
On 2010/06/28 22:57:28, dhollowa wrote:
> nit: // namespace webkit_glue

Done.

http://codereview.chromium.org/2818033/diff/1/5#newcode44
chrome/browser/autofill/autofill_common_unittest.h:44: }  // namespace
On 2010/06/28 22:57:28, dhollowa wrote:
> nit: // namespace autofill_unittest

Done.

http://codereview.chromium.org/2818033/diff/1/15
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2818033/diff/1/15#newcode115
chrome/browser/autofill/autofill_profile_unittest.cc:115: "Adams",
"declaration_guy@gmail.com", NULL, NULL, NULL, NULL, NULL,
On 2010/06/28 22:57:28, dhollowa wrote:
> I think we're ok here, but what happens if the only difference between a and b
> is a NULL string verus and empty string ""?  My read of the code leads me to
> believe that AutoFillProfile::GetFieldText will normalize NULL to "", so we'll
> be ok.  You agree?

NULL in this context means the field type is not set at all.  Empty string means
the type is set and the value is an empty string.

http://codereview.chromium.org/2818033/diff/1/15#newcode148
chrome/browser/autofill/autofill_profile_unittest.cc:148:
EXPECT_FALSE(a->IntersectionOfTypesHasEqualValues(*b));
On 2010/06/28 22:57:28, dhollowa wrote:
> Would be worth a test for when a and b do not intersect.  Maybe worth a
comment
> in the header as well.

Ok.

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

http://codereview.chromium.org/2818033/diff/1/21#newcode81
chrome/browser/autofill/personal_data_manager.cc:81: // |profiles| may be NULL
On 2010/06/28 22:57:28, dhollowa wrote:
> nit: end with a period.  (And below).

Done.

http://codereview.chromium.org/2818033/diff/1/21#newcode89
chrome/browser/autofill/personal_data_manager.cc:89:
CancelPendingQuery(&pending_creditcards_query_);
On 2010/06/28 22:57:28, dhollowa wrote:
> Should we have locking parity here too?

I've removed the added lock, as it was a hack.

[dhollowa, 2010-06-28 23:51:22]

http://codereview.chromium.org/2818033/diff/1/15
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2818033/diff/1/15#newcode115
chrome/browser/autofill/autofill_profile_unittest.cc:115: "Adams",
"declaration_guy@gmail.com", NULL, NULL, NULL, NULL, NULL,
On 2010/06/28 23:29:58, James Hawkins wrote:
> On 2010/06/28 22:57:28, dhollowa wrote:
> > I think we're ok here, but what happens if the only difference between a and
b
> > is a NULL string verus and empty string ""?  My read of the code leads me to
> > believe that AutoFillProfile::GetFieldText will normalize NULL to "", so
we'll
> > be ok.  You agree?
> 
> NULL in this context means the field type is not set at all.  Empty string
means
> the type is set and the value is an empty string.

That's not my read of AutoFillProfile::GetFieldText().  It returns "" when it
encounters missing type.

[James Hawkins, 2010-06-28 23:57:55]

http://codereview.chromium.org/2818033/diff/1/15
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2818033/diff/1/15#newcode115
chrome/browser/autofill/autofill_profile_unittest.cc:115: "Adams",
"declaration_guy@gmail.com", NULL, NULL, NULL, NULL, NULL,
On 2010/06/28 23:51:22, dhollowa wrote:
> On 2010/06/28 23:29:58, James Hawkins wrote:
> > On 2010/06/28 22:57:28, dhollowa wrote:
> > > I think we're ok here, but what happens if the only difference between a
and
> b
> > > is a NULL string verus and empty string ""?  My read of the code leads me
to
> > > believe that AutoFillProfile::GetFieldText will normalize NULL to "", so
> we'll
> > > be ok.  You agree?
> > 
> > NULL in this context means the field type is not set at all.  Empty string
> means
> > the type is set and the value is an empty string.
> 
> That's not my read of AutoFillProfile::GetFieldText().  It returns "" when it
> encounters missing type.
> 

OK; these methods are based on GetAvailableFieldTypes(), which returns types
only for non-empty values.

[dhollowa, 2010-06-30 02:15:36]

That's looking better.  I'm getting clean runs now with unit_tests and manual
testing on Mac.  One thing I did notice was that the credit card records are not
relationally tied with the specified address.  I.e. when I click a credit card
field I see options to fill using all addresses.

[dhollowa, 2010-06-30 02:19:16]

Found a problem on Mac with the "me" card (auxiliary) profiles.  If I fill using
an auxiliary profile and then click the send button, the auxiliary profile data
is added to my local (WebDB) profiles.  That's not right.

[dhollowa, 2010-06-30 20:58:18]

LGTM, pending nit and bots.

http://codereview.chromium.org/2818033/diff/9003/40028
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/2818033/diff/9003/40028#newcode609
chrome/browser/autofill/personal_data_manager.cc:609: // Don't save a web
profile if the data exactly matches an auxiliary profile.
nit: Comment should reflect |IsSubsetOf()| logic.  No longer exact match.

[dhollowa, 2010-06-30 21:04:44]

On 2010/06/30 20:58:18, dhollowa wrote:
> LGTM, pending nit and bots.
> 
> http://codereview.chromium.org/2818033/diff/9003/40028
> File chrome/browser/autofill/personal_data_manager.cc (right):
> 
> http://codereview.chromium.org/2818033/diff/9003/40028#newcode609
> chrome/browser/autofill/personal_data_manager.cc:609: // Don't save a web
> profile if the data exactly matches an auxiliary profile.
> nit: Comment should reflect |IsSubsetOf()| logic.  No longer exact match.

Also, please add to review description that this removes CVC and shipping
address support.

[James Hawkins, 2010-07-01 00:01:26]

http://codereview.chromium.org/2818033/diff/9003/40028
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/2818033/diff/9003/40028#newcode609
chrome/browser/autofill/personal_data_manager.cc:609: // Don't save a web
profile if the data exactly matches an auxiliary profile.
On 2010/06/30 20:58:19, dhollowa wrote:
> nit: Comment should reflect |IsSubsetOf()| logic.  No longer exact match.

Done.