Fill in more name fields with requestAutocomplete

Fill in more name fields with requestAutocomplete

Parse full names more intelligently with a rough port of the Android contact name splitter. Only does Western names well; CJK are TODO.

Save and sync the full name separately from the name pieces.

BUG=241886
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278641

[Evan Stade, 2014-06-03 01:36:35]

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:210: SetFullName(value);
I think this needs to happen in SetInfo, so app_locale is available (for
eventual locale-aware name-splitting), but trying to make that change at this
point breaks a lot of tests.

[Ilya Sherman, 2014-06-03 22:53:35]

Thanks, Evan.  This is definitely a nice improvement.  However, it's still based
on heuristics, and hence is going to fail for some users.  What is our plan for
those users?  Just ignore the issue?

https://codereview.chromium.org/310463005/diff/30001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/310463005/diff/30001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_common.cc:47: return
autofill_type.GetStorableType() == NAME_FULL;
Why is this needed?  Shouldn't the NameInfo class handle this internally,
parsing when necessary, but not overridding user-set preferences?  (I guess for
rAc there's currently no way for a user to set anything other than a full name;
but for Autofill there is, so NameInfo should already handle this.  Once it
handles things automatically, I'd rather not add the extra code here.)

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:21: static const char* const
name_prefixes[] = {
nit: No need for "static"; you're already in an anonymous namespace.  Ditto
below.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:33: "d\'", "de", "del", "der",
"di", "la", "le", "mc", "san", "st", "ter",
Out of curiousity, why does the apostrophe need to be escaped?

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:44: base::TrimString(element,
base::ASCIIToUTF16("."), &trimmed_element);
Why not just remove all dots, rather than removing merely some of them?  It
would make the suffixes list look less strange.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:75: }
I think this can be written more simply using std::remove_if:
http://stackoverflow.com/questions/3424962/where-is-erase-if

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:80: base::string16 family;
nit: I think we should be consistent about first/middle/last vs.
given/middle/family within this file.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:83: // TODO(estade): this does
Western name splitting. It should do different
nit: "this" -> "This"

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:85: NameParts SplitName(const
base::string16& name) {
Optional nit: Perhaps "ParseName()" rather than "SplitName()"?

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:91: if (name_tokens.size() > 2)
Why > 2?  Can suffixes ever be valid names?

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:96: // Bad things have
happened; just assume the whole thing is a given name.
nit: Please move this comment into the if-stmt.  Usually, comments outside of
if-stmts say something like "Check for X"; comments within if-stmts say
something like "X is the case, therefore Y".

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:116: name_tokens.back())) {
nit: Please move this into the loop condition.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:125:
reverse_family_tokens.rend());
nit: alignment

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:130: if (name_tokens.size() >
1) {
Optional nit: I think ">= 2" conveys the intent slightly more clearly.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:136: parts.given =
JoinString(name_tokens, base::char16(' '));
Are multi-word given names really more common than multi-word middle or last
names?

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:210: SetFullName(value);
On 2014/06/03 01:36:36, Evan Stade wrote:
> I think this needs to happen in SetInfo, so app_locale is available (for
> eventual locale-aware name-splitting), but trying to make that change at this
> point breaks a lot of tests.

How does it break tests?  Just because they call SetRawInfo() directly?

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:250: last_ = parts.family;
Optional nit: It might be cleaner to just pass in three out-params to
SplitName().

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.h (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.h:50: base::string16 full_name_;
Are you planning to persist this to the database and to Sync it?

[Evan Stade, 2014-06-03 23:55:35]

> However, it's still based
> on heuristics, and hence is going to fail for some users.  > What is our plan
for
> those users?  Just ignore the issue?

As far as requestAutocomplete goes, the official recommendation would be "stick
to full name", i.e. don't use given-name in some mission critical application.
But if you're going to use it to send an email that starts "Hi, John!" and
you're OK with it being slightly inaccurate for a minority of users, then have
at it.

So yes, just ignore the issue and see how far that gets us.

https://codereview.chromium.org/310463005/diff/30001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/310463005/diff/30001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_common.cc:47: return
autofill_type.GetStorableType() == NAME_FULL;
On 2014/06/03 22:53:34, Ilya Sherman wrote:
> Why is this needed?  Shouldn't the NameInfo class handle this internally,
> parsing when necessary, but not overridding user-set preferences?  (I guess
for
> rAc there's currently no way for a user to set anything other than a full
name;
> but for Autofill there is, so NameInfo should already handle this.  Once it
> handles things automatically, I'd rather not add the extra code here.)

rAc intentionally only fills in a subset of autocomplete types. That subset is
defined as the fields that are in the dialog. This function slightly expands
that set. |type| is the type that's in the dialog, and |field_type| is something
in the page. So this makes sure that when filling in the page, name fields are
filled in because they line up with the NAME_FULL <input> in the dialog.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:21: static const char* const
name_prefixes[] = {
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> nit: No need for "static"; you're already in an anonymous namespace.  Ditto
> below.

Done.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:33: "d\'", "de", "del", "der",
"di", "la", "le", "mc", "san", "st", "ter",
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> Out of curiousity, why does the apostrophe need to be escaped?

I just copy-pasta'd that. Turns out it doesn't need to be escaped.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:44: base::TrimString(element,
base::ASCIIToUTF16("."), &trimmed_element);
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> Why not just remove all dots, rather than removing merely some of them?  It
> would make the suffixes list look less strange.

I copied this behavior from the original code (although the original code does
more complicated things as well). I guess that "Ph.D." and "M.D." are common, as
is "Jr.", but "J.R." should not be considered a suffix.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:75: }
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> I think this can be written more simply using std::remove_if:
> http://stackoverflow.com/questions/3424962/where-is-erase-if

but this breaks out of the loop the first time it sees a non-suffix

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:80: base::string16 family;
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> nit: I think we should be consistent about first/middle/last vs.
> given/middle/family within this file.

I'd rather tackle that when we get around to implementing better support for
international names, but in the meantime only add new code that uses generically
correct nomenclature.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:83: // TODO(estade): this does
Western name splitting. It should do different
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> nit: "this" -> "This"

done, although the precedent that exists through Chrome's codebase with a few
rare exceptions is for TODO(estade) to always be followed by a lower case
letter.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:85: NameParts SplitName(const
base::string16& name) {
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> Optional nit: Perhaps "ParseName()" rather than "SplitName()"?

name inspired by com.android.providers.contacts.NameSplitter

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:91: if (name_tokens.size() > 2)
On 2014/06/03 22:53:34, Ilya Sherman wrote:
> Why > 2?  Can suffixes ever be valid names?

Yes. John Ma.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:96: // Bad things have
happened; just assume the whole thing is a given name.
On 2014/06/03 22:53:34, Ilya Sherman wrote:
> nit: Please move this comment into the if-stmt.  Usually, comments outside of
> if-stmts say something like "Check for X"; comments within if-stmts say
> something like "X is the case, therefore Y".

Done.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:116: name_tokens.back())) {
On 2014/06/03 22:53:34, Ilya Sherman wrote:
> nit: Please move this into the loop condition.

Done.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:130: if (name_tokens.size() >
1) {
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> Optional nit: I think ">= 2" conveys the intent slightly more clearly.

sure, why not

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:136: parts.given =
JoinString(name_tokens, base::char16(' '));
On 2014/06/03 22:53:34, Ilya Sherman wrote:
> Are multi-word given names really more common than multi-word middle or last
> names?

I don't know. Someone thought about it and decided so, apparently.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:210: SetFullName(value);
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> On 2014/06/03 01:36:36, Evan Stade wrote:
> > I think this needs to happen in SetInfo, so app_locale is available (for
> > eventual locale-aware name-splitting), but trying to make that change at
this
> > point breaks a lot of tests.
> 
> How does it break tests?  Just because they call SetRawInfo() directly?

Well, a lot of tests do that, yes, and switching them to SetInfo might help. But
a lot also do SetRawMultiInfo() and there's no SetMultiInfo() equivalent. And
there's at least one place that real code calls SetRawMultiInfo(NAME_FULL, ...),
which is in AutofillProfile::OverwriteWithOrAddTo

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:250: last_ = parts.family;
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> Optional nit: It might be cleaner to just pass in three out-params to
> SplitName().

That's true, but I guess I have an intrinsic aversion to outparams.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.h (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.h:50: base::string16 full_name_;
On 2014/06/03 22:53:35, Ilya Sherman wrote:
> Are you planning to persist this to the database and to Sync it?

hrkk.. that would be ideal, yes. I gather that will require more changes. Since
you've probably been the one to handle all database updates for the last N
years, do you have a link to a patch you've done in the past that can serve as
an example for how to do this?

[Ilya Sherman, 2014-06-04 00:13:46]

https://codereview.chromium.org/310463005/diff/30001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/310463005/diff/30001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_common.cc:47: return
autofill_type.GetStorableType() == NAME_FULL;
On 2014/06/03 23:55:34, Evan Stade wrote:
> On 2014/06/03 22:53:34, Ilya Sherman wrote:
> > Why is this needed?  Shouldn't the NameInfo class handle this internally,
> > parsing when necessary, but not overridding user-set preferences?  (I guess
> for
> > rAc there's currently no way for a user to set anything other than a full
> name;
> > but for Autofill there is, so NameInfo should already handle this.  Once it
> > handles things automatically, I'd rather not add the extra code here.)
> 
> rAc intentionally only fills in a subset of autocomplete types. That subset is
> defined as the fields that are in the dialog. This function slightly expands
> that set. |type| is the type that's in the dialog, and |field_type| is
something
> in the page. So this makes sure that when filling in the page, name fields are
> filled in because they line up with the NAME_FULL <input> in the dialog.

Ah, right.  Makes sense.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:44: base::TrimString(element,
base::ASCIIToUTF16("."), &trimmed_element);
On 2014/06/03 23:55:35, Evan Stade wrote:
> On 2014/06/03 22:53:35, Ilya Sherman wrote:
> > Why not just remove all dots, rather than removing merely some of them?  It
> > would make the suffixes list look less strange.
> 
> I copied this behavior from the original code (although the original code does
> more complicated things as well). I guess that "Ph.D." and "M.D." are common,
as
> is "Jr.", but "J.R." should not be considered a suffix.

In that case, how about not stripping trailing dots at all?  "m.d" just looks
really weird.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:75: }
On 2014/06/03 23:55:35, Evan Stade wrote:
> On 2014/06/03 22:53:35, Ilya Sherman wrote:
> > I think this can be written more simply using std::remove_if:
> > http://stackoverflow.com/questions/3424962/where-is-erase-if
> 
> but this breaks out of the loop the first time it sees a non-suffix

Hrm, you're right.  Nevermind.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:80: base::string16 family;
On 2014/06/03 23:55:35, Evan Stade wrote:
> On 2014/06/03 22:53:35, Ilya Sherman wrote:
> > nit: I think we should be consistent about first/middle/last vs.
> > given/middle/family within this file.
> 
> I'd rather tackle that when we get around to implementing better support for
> international names, but in the meantime only add new code that uses
generically
> correct nomenclature.

I'd really prefer to keep the file internally consistent -- not necessarily with
the server types, but at least with names that are implementation details within
the file.  I don't mind changing everything to use the generically correct
nomenclature; but I'd really prefer not to have a mix.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:91: if (name_tokens.size() > 2)
On 2014/06/03 23:55:35, Evan Stade wrote:
> On 2014/06/03 22:53:34, Ilya Sherman wrote:
> > Why > 2?  Can suffixes ever be valid names?
> 
> Yes. John Ma.

Ok... in that case, is "Ma" more likely to be a suffix or a last name?  It seems
wrong to decide based just on the number of tokens in the name.  What if John
has a middle name?

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.h (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.h:50: base::string16 full_name_;
On 2014/06/03 23:55:35, Evan Stade wrote:
> On 2014/06/03 22:53:35, Ilya Sherman wrote:
> > Are you planning to persist this to the database and to Sync it?
> 
> hrkk.. that would be ideal, yes. I gather that will require more changes.
Since
> you've probably been the one to handle all database updates for the last N
> years, do you have a link to a patch you've done in the past that can serve as
> an example for how to do this?

Rouslan was actually the last one to make such a change:
https://codereview.chromium.org/212873003

https://codereview.chromium.org/310463005/diff/50001/components/autofill/core...
File components/autofill/core/browser/contact_info_unittest.cc (right):

https://codereview.chromium.org/310463005/diff/50001/components/autofill/core...
components/autofill/core/browser/contact_info_unittest.cc:34: { "John Ma",
"John", "", "Ma" },
What about "John James Ma"?  "John James Ma, MA"?

[Evan Stade, 2014-06-04 00:24:23]

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:44: base::TrimString(element,
base::ASCIIToUTF16("."), &trimmed_element);
On 2014/06/04 00:13:45, Ilya Sherman wrote:
> On 2014/06/03 23:55:35, Evan Stade wrote:
> > On 2014/06/03 22:53:35, Ilya Sherman wrote:
> > > Why not just remove all dots, rather than removing merely some of them? 
It
> > > would make the suffixes list look less strange.
> > 
> > I copied this behavior from the original code (although the original code
does
> > more complicated things as well). I guess that "Ph.D." and "M.D." are
common,
> as
> > is "Jr.", but "J.R." should not be considered a suffix.
> 
> In that case, how about not stripping trailing dots at all?  "m.d" just looks
> really weird.

But then "John Smith Jr." wouldn't match against "jr".

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:80: base::string16 family;
On 2014/06/04 00:13:45, Ilya Sherman wrote:
> On 2014/06/03 23:55:35, Evan Stade wrote:
> > On 2014/06/03 22:53:35, Ilya Sherman wrote:
> > > nit: I think we should be consistent about first/middle/last vs.
> > > given/middle/family within this file.
> > 
> > I'd rather tackle that when we get around to implementing better support for
> > international names, but in the meantime only add new code that uses
> generically
> > correct nomenclature.
> 
> I'd really prefer to keep the file internally consistent -- not necessarily
with
> the server types, but at least with names that are implementation details
within
> the file.  I don't mind changing everything to use the generically correct
> nomenclature; but I'd really prefer not to have a mix.

Done.

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:91: if (name_tokens.size() > 2)
On 2014/06/04 00:13:45, Ilya Sherman wrote:
> On 2014/06/03 23:55:35, Evan Stade wrote:
> > On 2014/06/03 22:53:34, Ilya Sherman wrote:
> > > Why > 2?  Can suffixes ever be valid names?
> > 
> > Yes. John Ma.
> 
> Ok... in that case, is "Ma" more likely to be a suffix or a last name?  It
seems
> wrong to decide based just on the number of tokens in the name.  What if John
> has a middle name?

Well, if John has a middle name he's SOL. But this behavior is copied from code
that is running on millions of phones, so who am I to second guess it?

[Ilya Sherman, 2014-06-04 00:34:33]

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:44: base::TrimString(element,
base::ASCIIToUTF16("."), &trimmed_element);
On 2014/06/04 00:24:23, Evan Stade wrote:
> On 2014/06/04 00:13:45, Ilya Sherman wrote:
> > On 2014/06/03 23:55:35, Evan Stade wrote:
> > > On 2014/06/03 22:53:35, Ilya Sherman wrote:
> > > > Why not just remove all dots, rather than removing merely some of them? 
> It
> > > > would make the suffixes list look less strange.
> > > 
> > > I copied this behavior from the original code (although the original code
> does
> > > more complicated things as well). I guess that "Ph.D." and "M.D." are
> common,
> > as
> > > is "Jr.", but "J.R." should not be considered a suffix.
> > 
> > In that case, how about not stripping trailing dots at all?  "m.d" just
looks
> > really weird.
> 
> But then "John Smith Jr." wouldn't match against "jr".

*sigh*, alright, I cave :P

https://codereview.chromium.org/310463005/diff/30001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:91: if (name_tokens.size() > 2)
On 2014/06/04 00:24:23, Evan Stade wrote:
> On 2014/06/04 00:13:45, Ilya Sherman wrote:
> > On 2014/06/03 23:55:35, Evan Stade wrote:
> > > On 2014/06/03 22:53:34, Ilya Sherman wrote:
> > > > Why > 2?  Can suffixes ever be valid names?
> > > 
> > > Yes. John Ma.
> > 
> > Ok... in that case, is "Ma" more likely to be a suffix or a last name?  It
> seems
> > wrong to decide based just on the number of tokens in the name.  What if
John
> > has a middle name?
> 
> Well, if John has a middle name he's SOL. But this behavior is copied from
code
> that is running on millions of phones, so who am I to second guess it?

Well, sure, I suppose heuristics are always going to be imperfect.  I guess I'd
at least include a comment for why the if-stmt is here.

[Ilya Sherman, 2014-06-04 01:22:38]

https://chromiumcodereview.appspot.com/310463005/diff/110001/components/autof...
File components/autofill/core/browser/contact_info.cc (right):

https://chromiumcodereview.appspot.com/310463005/diff/110001/components/autof...
components/autofill/core/browser/contact_info.cc:194: given_ = value;
Btw, what happens if I call SetRawInfo(NAME_FIRST) when there is a full_name_
set?

[Evan Stade, 2014-06-04 01:26:29]

(still working on the autofill table update, so this patch is not ready for
re-review)

https://chromiumcodereview.appspot.com/310463005/diff/30001/components/autofi...
File components/autofill/core/browser/contact_info.cc (right):

https://chromiumcodereview.appspot.com/310463005/diff/30001/components/autofi...
components/autofill/core/browser/contact_info.cc:91: if (name_tokens.size() > 2)
On 2014/06/04 00:34:32, Ilya Sherman wrote:
> On 2014/06/04 00:24:23, Evan Stade wrote:
> > On 2014/06/04 00:13:45, Ilya Sherman wrote:
> > > On 2014/06/03 23:55:35, Evan Stade wrote:
> > > > On 2014/06/03 22:53:34, Ilya Sherman wrote:
> > > > > Why > 2?  Can suffixes ever be valid names?
> > > > 
> > > > Yes. John Ma.
> > > 
> > > Ok... in that case, is "Ma" more likely to be a suffix or a last name?  It
> > seems
> > > wrong to decide based just on the number of tokens in the name.  What if
> John
> > > has a middle name?
> > 
> > Well, if John has a middle name he's SOL. But this behavior is copied from
> code
> > that is running on millions of phones, so who am I to second guess it?
> 
> Well, sure, I suppose heuristics are always going to be imperfect.  I guess
I'd
> at least include a comment for why the if-stmt is here.

Done.

https://chromiumcodereview.appspot.com/310463005/diff/110001/components/autof...
File components/autofill/core/browser/contact_info.cc (right):

https://chromiumcodereview.appspot.com/310463005/diff/110001/components/autof...
components/autofill/core/browser/contact_info.cc:194: given_ = value;
On 2014/06/04 01:22:38, Ilya Sherman wrote:
> Btw, what happens if I call SetRawInfo(NAME_FIRST) when there is a full_name_
> set?

while working on updating the autofill table, I thought about this, and decided
we should clear the full name any time (new) raw info is set.

[Evan Stade, 2014-06-04 23:11:58]

+nick for sync/
+shess for components/webdata and
components/test/data/web_database/version_56.sql

[Scott Hess - ex-Googler, 2014-06-04 23:48:17]

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:173:
db->DoesColumnExist("autofill_profile_names", "full_name");
I may be missing something, here.  How does the code get to this point without
going through MigrateToVersion57AddFullNameField()?

[Evan Stade, 2014-06-05 18:16:28]

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:173:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/04 23:48:17, shess wrote:
> I may be missing something, here.  How does the code get to this point without
> going through MigrateToVersion57AddFullNameField()?

MigrateToVersion37MergeAndCullOlderProfiles calls this function

[Evan Stade, 2014-06-06 23:10:44]

ping isherman, nick, shess

https://codereview.chromium.org/310463005/diff/170001/components/autofill/cor...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/170001/components/autofill/cor...
components/autofill/core/browser/contact_info.cc:256: if (FullName() == full)
NB

[Scott Hess - ex-Googler, 2014-06-06 23:55:36]

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:173:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/05 18:16:28, Evan Stade wrote:
> On 2014/06/04 23:48:17, shess wrote:
> > I may be missing something, here.  How does the code get to this point
without
> > going through MigrateToVersion57AddFullNameField()?
> 
> MigrateToVersion37MergeAndCullOlderProfiles calls this function

Wow, you present me with a dilemma.  In general, I really dislike
conditionally-constructed SQL statements.  A lot.  Your's looks correct, but my
dislike stems from experience with such things, so I trust it.  I also dislike
ever modifying migration code, it's risky, once written it should be left alone.

How averse would you be to duplicating the current method to
AddAutofillProfileNamesToProfileForVersion36(), and then modify the existing
function in the obvious way?

One reason I suggest this is that Sqlite.Version.Web shows over 98% of your
databases in versions 52-56.  It would probably be reasonable to look at calling
sql::MetaTable::RazeIfDeprecated() with some suitable value.  Elsewhere I've
used anything older than about two years as the cut-off, but I think 18 or 12
months would be reasonable, too.  Some number of those old databases will
_never_ update, either because the users in question fell off the auto-update
train, or because there is a database or filesystem problem preventing it.  In
the latter case, Raze() might shake some of those people loose and let them make
progress.  Might not, the world is a crazy place.

[So I mean if you duplicated the function, then it would only stick around until
you circled back and deprecated a bunch of stuff.]

[Ilya Sherman, 2014-06-07 00:02:38]

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:173:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/06 23:55:36, shess wrote:
> On 2014/06/05 18:16:28, Evan Stade wrote:
> > On 2014/06/04 23:48:17, shess wrote:
> > > I may be missing something, here.  How does the code get to this point
> without
> > > going through MigrateToVersion57AddFullNameField()?
> > 
> > MigrateToVersion37MergeAndCullOlderProfiles calls this function
> 
> Wow, you present me with a dilemma.  In general, I really dislike
> conditionally-constructed SQL statements.  A lot.  Your's looks correct, but
my
> dislike stems from experience with such things, so I trust it.  I also dislike
> ever modifying migration code, it's risky, once written it should be left
alone.
> 
> How averse would you be to duplicating the current method to
> AddAutofillProfileNamesToProfileForVersion36(), and then modify the existing
> function in the obvious way?
> 
> One reason I suggest this is that Sqlite.Version.Web shows over 98% of your
> databases in versions 52-56.  It would probably be reasonable to look at
calling
> sql::MetaTable::RazeIfDeprecated() with some suitable value.  Elsewhere I've
> used anything older than about two years as the cut-off, but I think 18 or 12
> months would be reasonable, too.  Some number of those old databases will
> _never_ update, either because the users in question fell off the auto-update
> train, or because there is a database or filesystem problem preventing it.  In
> the latter case, Raze() might shake some of those people loose and let them
make
> progress.  Might not, the world is a crazy place.
> 
> [So I mean if you duplicated the function, then it would only stick around
until
> you circled back and deprecated a bunch of stuff.]

+1.  I have found that, despite the ugliness, migration code should pretty much
always be written to not call out to any methods that aren't specifically
intended for migration.  Otherwise, either (a) core methods get very complex to
support rare migration code paths, or (b) someone comes along and changes a
method not thinking about the migration case, and accidentally breaks migration
in a subtle way (which ideally tests would catch, yes).

I think it would be great to deprecate some of the really old migration code we
have around in this file.  We'd probably want to get buy-in from all the WebData
owners, but the costs of maintaining super old migration code don't really seem
to merit the expected payoff.  Plus, all the things that Scott said.

[ncarter (slow), 2014-06-07 00:03:11]

Sorry for not noticing this review sooner. -me, +maniscalo

[maniscalco, 2014-06-09 17:21:27]

On 2014/06/07 00:03:11, ncarter wrote:
> Sorry for not noticing this review sooner. -me, +maniscalo

Looking at sync/ changes only...

sync/ lgtm

[Evan Stade, 2014-06-09 17:48:40]

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/150001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:173:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/07 00:02:38, Ilya Sherman wrote:
> On 2014/06/06 23:55:36, shess wrote:
> > On 2014/06/05 18:16:28, Evan Stade wrote:
> > > On 2014/06/04 23:48:17, shess wrote:
> > > > I may be missing something, here.  How does the code get to this point
> > without
> > > > going through MigrateToVersion57AddFullNameField()?
> > > 
> > > MigrateToVersion37MergeAndCullOlderProfiles calls this function
> > 
> > Wow, you present me with a dilemma.  In general, I really dislike
> > conditionally-constructed SQL statements.  A lot.  Your's looks correct, but
> my
> > dislike stems from experience with such things, so I trust it.  I also
dislike
> > ever modifying migration code, it's risky, once written it should be left
> alone.
> > 
> > How averse would you be to duplicating the current method to
> > AddAutofillProfileNamesToProfileForVersion36(), and then modify the existing
> > function in the obvious way?
> > 
> > One reason I suggest this is that Sqlite.Version.Web shows over 98% of your
> > databases in versions 52-56.  It would probably be reasonable to look at
> calling
> > sql::MetaTable::RazeIfDeprecated() with some suitable value.  Elsewhere I've
> > used anything older than about two years as the cut-off, but I think 18 or
12
> > months would be reasonable, too.  Some number of those old databases will
> > _never_ update, either because the users in question fell off the
auto-update
> > train, or because there is a database or filesystem problem preventing it. 
In
> > the latter case, Raze() might shake some of those people loose and let them
> make
> > progress.  Might not, the world is a crazy place.
> > 
> > [So I mean if you duplicated the function, then it would only stick around
> until
> > you circled back and deprecated a bunch of stuff.]
> 
> +1.  I have found that, despite the ugliness, migration code should pretty
much
> always be written to not call out to any methods that aren't specifically
> intended for migration.  Otherwise, either (a) core methods get very complex
to
> support rare migration code paths, or (b) someone comes along and changes a
> method not thinking about the migration case, and accidentally breaks
migration
> in a subtle way (which ideally tests would catch, yes).
> 
> I think it would be great to deprecate some of the really old migration code
we
> have around in this file.  We'd probably want to get buy-in from all the
WebData
> owners, but the costs of maintaining super old migration code don't really
seem
> to merit the expected payoff.  Plus, all the things that Scott said.

I agree, so I've filed crbug.com/382562

In the meantime, I took Scott's advice of making two separate functions.

[Scott Hess - ex-Googler, 2014-06-09 21:50:50]

I could support either direction WRT the other function's column-based
decisions.  I still don't like dynamic queries, but at some point you're
replicating a tree of functions, and I don't want to go there.

https://codereview.chromium.org/310463005/diff/190001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/190001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:295:
db->DoesColumnExist("autofill_profile_names", "full_name");
Sigh - digging around, I don't see a reasonable chance that this could be
handled the same way as the other function.  So ... could you pull this
invariant out of the loop, and put a TODO() referencing the new bug, so that
someone might realize they can simplify this code a bit when things get
deprecated?

[Though maybe others will disagree.  I think you'd need a versionN variant of
this function, plus a versionN variant of AddAutofillProfilePieces().  I guess
that's not obscenely hairy, and both functions are mostly glue so they wouldn't
be huge wastes of code space.]

[Evan Stade, 2014-06-12 01:55:33]

https://codereview.chromium.org/310463005/diff/190001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/190001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:295:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/09 21:50:50, shess wrote:
> Sigh - digging around, I don't see a reasonable chance that this could be
> handled the same way as the other function.  So ... could you pull this
> invariant out of the loop, and put a TODO() referencing the new bug, so that
> someone might realize they can simplify this code a bit when things get
> deprecated?
> 
> [Though maybe others will disagree.  I think you'd need a versionN variant of
> this function, plus a versionN variant of AddAutofillProfilePieces().  I guess
> that's not obscenely hairy, and both functions are mostly glue so they
wouldn't
> be huge wastes of code space.]

Done.

[Ilya Sherman, 2014-06-12 23:25:26]

Thanks, Evan.

https://codereview.chromium.org/310463005/diff/210001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/310463005/diff/210001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:45: // First and last name
are parsed from full name.
nit: And middle name as well, it looks like.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/contact_info.cc:255: // SetRawInfo.
Please file a bug for this pair of TODOs, and link to it from them.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:921:
TEST_F(AutofillProfileSyncableServiceTest, NoLanguageCodeNoSync) {
Please add similar test coverage for the full name: If there is no full name
explicitly set on the profile, we shouldn't generate any Sync updates. 
Otherwise, as soon as this change lands, everyone's Chromes would generate Sync
updates for every profile they have stored, which would make the Sync server
rather unhappy.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1045:
TEST_F(AutofillProfileSyncableServiceTest, EmptySyncPreservesLanguageCode) {
Please add similar test coverage: Data from older versions of Chrome -- before
full names were stored as a separate field -- shouldn't clobber data from newer
versions.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:199: }
Optional nit: I'd prefer that you inline this code, but given the very clear
name and doc string, it's fine if you prefer to keep it as a separate function. 
(My preference is to try not to mix migration code with production code as much
as possible.)

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:296:
db->DoesColumnExist("autofill_profile_names", "full_name");
Are we calling this function from the migration code?  If so, please update the
code not to do so.  Migration code shouldn't depend on non-migration code, since
non-migration code changes over time, but migration code needs to be frozen.

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2687:
connection.DoesColumnExist("autofill_profile_names", "full_name"));
I'd prefer that you also verify that the table starts with exactly one name,
"Jon Smith".  Otherwise, the test could be dropping names and still passing.

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2703: // table.
nit: Please update this comment.

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2719:
EXPECT_EQ(ASCIIToUTF16(""), s_names.ColumnString16(4));
nit: "base::string16()" is generally preferred over "ASCIIToUTF16("")".

[Scott Hess - ex-Googler, 2014-06-13 17:43:27]

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:199: }
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> Optional nit: I'd prefer that you inline this code, but given the very clear
> name and doc string, it's fine if you prefer to keep it as a separate
function. 
> (My preference is to try not to mix migration code with production code as
much
> as possible.)

I support making the minimal changes necessary to maintain things.  Inlining the
code is almost certainly safe, but doesn't really gain you anything (AFAICT the
compiler will already inline this if it makes sense on that front).  And that
migration function is already pretty big as things stand...

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:296:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> Are we calling this function from the migration code?  If so, please update
the
> code not to do so.  Migration code shouldn't depend on non-migration code,
since
> non-migration code changes over time, but migration code needs to be frozen.

AFAICT, doing the same thing here would require copying this function, and
copying the caller, which pushed me on the fence as to whether I think it's
worthwhile.  I'm willing to support either direction at this point, there are
advantages to keeping the migration code constant, and there are advantages to
keeping the production functions simple, but there are also advantages to not
bloating up the code that people have to look at.

[When I dream, I imagine database code having a _migration.cc file to contain
the migration subset, or some other mechanism of segregating it.]

[Ilya Sherman, 2014-06-13 19:33:00]

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:199: }
On 2014/06/13 17:43:27, shess wrote:
> On 2014/06/12 23:25:25, Ilya Sherman wrote:
> > Optional nit: I'd prefer that you inline this code, but given the very clear
> > name and doc string, it's fine if you prefer to keep it as a separate
> function. 
> > (My preference is to try not to mix migration code with production code as
> much
> > as possible.)
> 
> I support making the minimal changes necessary to maintain things.  Inlining
the
> code is almost certainly safe, but doesn't really gain you anything (AFAICT
the
> compiler will already inline this if it makes sense on that front).  And that
> migration function is already pretty big as things stand...

Fair enough.  I'm not worried about inlining for performance reasons -- I just
want all of the migration code to be disentangled from the non-migration code. 
I think your suggestion of having a _migration.cc file would be a better way to
achieve this goal, though it might not make sense given that the migration
functions access private state of the class.  Maybe we could have two anonymous
namespaces in this file -- one for the production code, and one specifically for
migration-only helpers...

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:296:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/13 17:43:27, shess wrote:
> On 2014/06/12 23:25:25, Ilya Sherman wrote:
> > Are we calling this function from the migration code?  If so, please update
> the
> > code not to do so.  Migration code shouldn't depend on non-migration code,
> since
> > non-migration code changes over time, but migration code needs to be frozen.
> 
> AFAICT, doing the same thing here would require copying this function, and
> copying the caller, which pushed me on the fence as to whether I think it's
> worthwhile.  I'm willing to support either direction at this point, there are
> advantages to keeping the migration code constant, and there are advantages to
> keeping the production functions simple, but there are also advantages to not
> bloating up the code that people have to look at.
> 
> [When I dream, I imagine database code having a _migration.cc file to contain
> the migration subset, or some other mechanism of segregating it.]

This is one case where I prefer code duplication.  My reasoning is that the
migration code we write once, test, and then basically never touch again.  The
production code, on the other hand, changes (at least) with each revision to the
DB.  Thus, I have a strong preference for keeping the production code clean and
simple, even if it comes at cost to the DRYness of the combined production and
migration code.

[Scott Hess - ex-Googler, 2014-06-13 19:42:47]

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:296:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/13 19:32:59, Ilya Sherman wrote:
> On 2014/06/13 17:43:27, shess wrote:
> > On 2014/06/12 23:25:25, Ilya Sherman wrote:
> > > Are we calling this function from the migration code?  If so, please
update
> > the
> > > code not to do so.  Migration code shouldn't depend on non-migration code,
> > since
> > > non-migration code changes over time, but migration code needs to be
frozen.
> > 
> > AFAICT, doing the same thing here would require copying this function, and
> > copying the caller, which pushed me on the fence as to whether I think it's
> > worthwhile.  I'm willing to support either direction at this point, there
are
> > advantages to keeping the migration code constant, and there are advantages
to
> > keeping the production functions simple, but there are also advantages to
not
> > bloating up the code that people have to look at.
> > 
> > [When I dream, I imagine database code having a _migration.cc file to
contain
> > the migration subset, or some other mechanism of segregating it.]
> 
> This is one case where I prefer code duplication.  My reasoning is that the
> migration code we write once, test, and then basically never touch again.  The
> production code, on the other hand, changes (at least) with each revision to
the
> DB.  Thus, I have a strong preference for keeping the production code clean
and
> simple, even if it comes at cost to the DRYness of the combined production and
> migration code.

OK, then, since I think I have a general preference for separation of this sort
from the sql/ side, and Ilya appears to have a strong preference for separation
from the autofill side, I think that adds up to a preference for separation.  A
series of nested namespaces and the like would be awesome, and crazy, I'm
completely satisfied with the in-place approach of making inconveniently-long
function names that include their intended user.

[Evan Stade, 2014-06-14 01:17:24]

https://codereview.chromium.org/310463005/diff/210001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/310463005/diff/210001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:45: // First and last name
are parsed from full name.
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> nit: And middle name as well, it looks like.

Done.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/contact_info.cc:255: // SetRawInfo.
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> Please file a bug for this pair of TODOs, and link to it from them.

Done.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:921:
TEST_F(AutofillProfileSyncableServiceTest, NoLanguageCodeNoSync) {
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> Please add similar test coverage for the full name: If there is no full name
> explicitly set on the profile, we shouldn't generate any Sync updates. 
> Otherwise, as soon as this change lands, everyone's Chromes would generate
Sync
> updates for every profile they have stored, which would make the Sync server
> rather unhappy.

Done.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1045:
TEST_F(AutofillProfileSyncableServiceTest, EmptySyncPreservesLanguageCode) {
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> Please add similar test coverage: Data from older versions of Chrome -- before
> full names were stored as a separate field -- shouldn't clobber data from
newer
> versions.

Done.

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:199: }
On 2014/06/13 19:32:59, Ilya Sherman wrote:
> On 2014/06/13 17:43:27, shess wrote:
> > On 2014/06/12 23:25:25, Ilya Sherman wrote:
> > > Optional nit: I'd prefer that you inline this code, but given the very
clear
> > > name and doc string, it's fine if you prefer to keep it as a separate
> > function. 
> > > (My preference is to try not to mix migration code with production code as
> > much
> > > as possible.)
> > 
> > I support making the minimal changes necessary to maintain things.  Inlining
> the
> > code is almost certainly safe, but doesn't really gain you anything (AFAICT
> the
> > compiler will already inline this if it makes sense on that front).  And
that
> > migration function is already pretty big as things stand...
> 
> Fair enough.  I'm not worried about inlining for performance reasons -- I just
> want all of the migration code to be disentangled from the non-migration code.

> I think your suggestion of having a _migration.cc file would be a better way
to
> achieve this goal, though it might not make sense given that the migration
> functions access private state of the class.  Maybe we could have two
anonymous
> namespaces in this file -- one for the production code, and one specifically
for
> migration-only helpers...

putting "ForVersionN" at the end is kind of like a namespace

https://codereview.chromium.org/310463005/diff/210001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:296:
db->DoesColumnExist("autofill_profile_names", "full_name");
On 2014/06/13 19:42:47, shess wrote:
> On 2014/06/13 19:32:59, Ilya Sherman wrote:
> > On 2014/06/13 17:43:27, shess wrote:
> > > On 2014/06/12 23:25:25, Ilya Sherman wrote:
> > > > Are we calling this function from the migration code?  If so, please
> update
> > > the
> > > > code not to do so.  Migration code shouldn't depend on non-migration
code,
> > > since
> > > > non-migration code changes over time, but migration code needs to be
> frozen.
> > > 
> > > AFAICT, doing the same thing here would require copying this function, and
> > > copying the caller, which pushed me on the fence as to whether I think
it's
> > > worthwhile.  I'm willing to support either direction at this point, there
> are
> > > advantages to keeping the migration code constant, and there are
advantages
> to
> > > keeping the production functions simple, but there are also advantages to
> not
> > > bloating up the code that people have to look at.
> > > 
> > > [When I dream, I imagine database code having a _migration.cc file to
> contain
> > > the migration subset, or some other mechanism of segregating it.]
> > 
> > This is one case where I prefer code duplication.  My reasoning is that the
> > migration code we write once, test, and then basically never touch again. 
The
> > production code, on the other hand, changes (at least) with each revision to
> the
> > DB.  Thus, I have a strong preference for keeping the production code clean
> and
> > simple, even if it comes at cost to the DRYness of the combined production
and
> > migration code.
> 
> OK, then, since I think I have a general preference for separation of this
sort
> from the sql/ side, and Ilya appears to have a strong preference for
separation
> from the autofill side, I think that adds up to a preference for separation. 
A
> series of nested namespaces and the like would be awesome, and crazy, I'm
> completely satisfied with the in-place approach of making inconveniently-long
> function names that include their intended user.

Done.

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2687:
connection.DoesColumnExist("autofill_profile_names", "full_name"));
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> I'd prefer that you also verify that the table starts with exactly one name,
> "Jon Smith".  Otherwise, the test could be dropping names and still passing.

Done.

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2703: // table.
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://codereview.chromium.org/310463005/diff/210001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2719:
EXPECT_EQ(ASCIIToUTF16(""), s_names.ColumnString16(4));
On 2014/06/12 23:25:25, Ilya Sherman wrote:
> nit: "base::string16()" is generally preferred over "ASCIIToUTF16("")".

Done.

[Ilya Sherman, 2014-06-16 21:25:05]

LGTM.  Thanks, Evan.

[Scott Hess - ex-Googler, 2014-06-16 21:55:55]

lgtm, minor nit.  No need for add'l review if you make that change.  Thanks for
putting up with me.

https://codereview.chromium.org/310463005/diff/250001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/250001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:332: "VALUES
(?,?,?,?,?)"));
Get this all the way back to the default constructor, rather than Assign().

[Evan Stade, 2014-06-16 22:57:26]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-16 22:59:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/310463005/270001

[Evan Stade, 2014-06-16 23:04:08]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2014-06-16 23:06:05]

thanks for the reviews.

https://codereview.chromium.org/310463005/diff/250001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/310463005/diff/250001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:332: "VALUES
(?,?,?,?,?)"));
On 2014/06/16 21:55:54, shess wrote:
> Get this all the way back to the default constructor, rather than Assign().

Done.

[Evan Stade, 2014-06-16 23:06:15]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-16 23:07:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/310463005/290001

[commit-bot: I haz the power, 2014-06-17 11:59:00]

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

[commit-bot: I haz the power, 2014-06-17 11:59:01]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Evan Stade, 2014-06-18 21:47:13]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-18 21:48:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/310463005/330001

[Evan Stade, 2014-06-18 23:38:27]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-18 23:39:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/310463005/350001

[Ilya Sherman, 2014-06-19 00:03:00]

Note: You'll probably need to update some tests since
https://codereview.chromium.org/261993006/ has landed.

[Evan Stade, 2014-06-19 01:50:18]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-19 01:53:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/310463005/370001

[commit-bot: I haz the power, 2014-06-19 15:01:30]

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

[commit-bot: I haz the power, 2014-06-19 15:01:31]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[Evan Stade, 2014-06-19 19:07:21]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-19 19:09:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/310463005/390001

[commit-bot: I haz the power, 2014-06-20 01:09:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-20 07:18:37]

Change committed as 278641