Autofill extend profiles to include multi-valued fields, part 2.

Autofill extend profiles to include multi-valued fields, part 2.

No functional change.  This partitions the |ContactInfo| class into separate name, email, and company parts.  AutoFillProfile is changed to store its component members directly instead of in a map (in preparation for multiple values).  And the |Clone| operation is deprecated from the |FormGroup| class on down through the inheritance tree.

BUG=65625
TEST=All existing unit tests.

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

[Ilya Sherman, 2011-03-09 00:50:50]

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

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager_unittest.cc:678:
"00000000-0000-0000-0000-000000000001")));
Won't this leak memory?  Or did we have a double-free before?

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

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.cc:112: : FormGroup() {
Should we include this in the initialization lists above as well?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.cc:155: FormGroupMap info = info_map();
How about creating a wrapper around find() rather than constructing a map?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.cc:434: }
How about making this a constant array data member for the class?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.h:44: const string16& input,
nit: Perhaps use "value" to parallel SetInfo() below?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:179: string16 NameInfo::MiddleInitial()
const {
Is this method ever used?  It seems like we should just deprecate middle
initials except when talking to the server.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:387: if (email_ == text)
This check should probably be case-insensitive.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:404: if
(FindInfoMatchesHelper(EMAIL_ADDRESS, info, &match))
I think it would be clearer just to inline the logic of FindInfoMachesHelper()
here -- it would actually make the code shorter.  (Same applies below.)

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:425: NOTREACHED();
nit: This seems redundant with the DCHECK above.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
File chrome/browser/autofill/contact_info.h (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.h:35: typedef std::vector<string16>
NameTokens;
I don't think this typedef helps readability at all.  Would you be opposed to
getting rid of it?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.h:40: // initial, and last name.
What does "middle, middle initial" mean?  Presumably we would only ever return
the full middle name if it's available?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.h:143: // FormGroup implementation:
nit: I thought we were dropping the " implementation" text in this sort of
annotation -- am I misremembering?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/fax_num...
File chrome/browser/autofill/fax_number.cc (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/fax_num...
chrome/browser/autofill/fax_number.cc:9: FaxNumber::FaxNumber(const FaxNumber&
phone) : PhoneNumber(phone) {}
nit: perhaps "fax" rather than "phone"? (and below as well)

[dhollowa, 2011-03-09 01:55:46]

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

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager_unittest.cc:678:
"00000000-0000-0000-0000-000000000001")));
No, this AddProfile call takes ownership.  It adds it to a scoped vector in the
PDM.
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> Won't this leak memory?  Or did we have a double-free before?

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

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.cc:112: : FormGroup() {
It is only required for copy constructors, it is implicit with default
constructors.

On 2011/03/09 00:50:50, Ilya Sherman wrote:
> Should we include this in the initialization lists above as well?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.cc:155: FormGroupMap info = info_map();
No, the association between key/value is set up by the |AutoFillProfile| class,
there is not inherent mapping in the vector.

On 2011/03/09 00:50:50, Ilya Sherman wrote:
> How about creating a wrapper around find() rather than constructing a map?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.cc:434: }
No, I want it like this because I'm planning to extend it dynamically with the
multi-valued work.  Introducing a cache complicates things without much benefit.

On 2011/03/09 00:50:50, Ilya Sherman wrote:
> How about making this a constant array data member for the class?

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.h:44: const string16& input,
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> nit: Perhaps use "value" to parallel SetInfo() below?

Done.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:179: string16 NameInfo::MiddleInitial()
const {
It is not.  But NameField is set up to parse, so we should be.  We should either
deprecate MI everywhere or implement it.  I'd vote implement it.

On 2011/03/09 00:50:50, Ilya Sherman wrote:
> Is this method ever used?  It seems like we should just deprecate middle
> initials except when talking to the server.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:387: if (email_ == text)
I've added a TODO here to investigate.

On 2011/03/09 00:50:50, Ilya Sherman wrote:
> This check should probably be case-insensitive.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:404: if
(FindInfoMatchesHelper(EMAIL_ADDRESS, info, &match))
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> I think it would be clearer just to inline the logic of FindInfoMachesHelper()
> here -- it would actually make the code shorter.  (Same applies below.)

Done.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:425: NOTREACHED();
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> nit: This seems redundant with the DCHECK above.

Done.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
File chrome/browser/autofill/contact_info.h (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.h:35: typedef std::vector<string16>
NameTokens;
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> I don't think this typedef helps readability at all.  Would you be opposed to
> getting rid of it?

Done.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.h:40: // initial, and last name.
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> What does "middle, middle initial" mean?  Presumably we would only ever return
> the full middle name if it's available?

Done.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.h:143: // FormGroup implementation:
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> nit: I thought we were dropping the " implementation" text in this sort of
> annotation -- am I misremembering?

Done.  Here and in AutoFillProfile.

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/fax_num...
File chrome/browser/autofill/fax_number.cc (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/fax_num...
chrome/browser/autofill/fax_number.cc:9: FaxNumber::FaxNumber(const FaxNumber&
phone) : PhoneNumber(phone) {}
On 2011/03/09 00:50:50, Ilya Sherman wrote:
> nit: perhaps "fax" rather than "phone"? (and below as well)

Done.

[Ilya Sherman, 2011-03-09 02:01:22]

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

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_profile.cc:155: FormGroupMap info = info_map();
On 2011/03/09 01:55:47, dhollowa wrote:
> No, the association between key/value is set up by the |AutoFillProfile|
class,
> there is not inherent mapping in the vector.

What I meant was: We pretty much always want info_map().find(group)->second --
why not create a method that does that?

> 
> On 2011/03/09 00:50:50, Ilya Sherman wrote:
> > How about creating a wrapper around find() rather than constructing a map?
>

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/6650014/diff/1/chrome/browser/autofill/contact...
chrome/browser/autofill/contact_info.cc:179: string16 NameInfo::MiddleInitial()
const {
On 2011/03/09 01:55:47, dhollowa wrote:
> It is not.  But NameField is set up to parse, so we should be.  We should
either
> deprecate MI everywhere or implement it.  I'd vote implement it.

Why not just store a parsed initial as a middle name?  If we ever care about the
distinction, we can treat a middle name of length 1 as a middle name.

> 
> On 2011/03/09 00:50:50, Ilya Sherman wrote:
> > Is this method ever used?  It seems like we should just deprecate middle
> > initials except when talking to the server.
>

[Ilya Sherman, 2011-03-09 02:19:13]

LGTM

(As discussed offline, the two points from my previous comment will be looked at
in separate CLs)

http://codereview.chromium.org/6650014/diff/10001/chrome/browser/autofill/con...
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/6650014/diff/10001/chrome/browser/autofill/con...
chrome/browser/autofill/contact_info.cc:411: }
nit: We can simplify a bit by combining the outer if/else clauses here, and
below as well.

[dhollowa, 2011-03-09 03:11:50]

http://codereview.chromium.org/6650014/diff/10001/chrome/browser/autofill/con...
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/6650014/diff/10001/chrome/browser/autofill/con...
chrome/browser/autofill/contact_info.cc:411: }
On 2011/03/09 02:19:13, Ilya Sherman wrote:
> nit: We can simplify a bit by combining the outer if/else clauses here, and
> below as well.

Done.