UI changes to Autofill dialogs according to the latest mocks...

UI changes to Autofill dialogs according to the latest mocks
Also added inferred labels implementation. Label inferred from the person name + differences in other fields to distinguish entries.
BUG=47120, 46634, 46635, 47435, 48881, 47430
TEST=in the mocks: http://folder/roma/design/chrome/autofill/ and unit tested.

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

[GeorgeY, 2010-06-25 03:43:39]

Latest UI changes

[James Hawkins, 2010-06-25 20:19:58]

http://codereview.chromium.org/2801020/diff/1/5
File chrome/browser/views/autofill_profiles_view_win.h (left):

http://codereview.chromium.org/2801020/diff/1/5#oldcode274
chrome/browser/views/autofill_profiles_view_win.h:274: TEXT_LABEL,
We shouldn't remove labels until we actually implement inferred labels.

[GeorgeY, 2010-06-25 20:33:29]

On 2010/06/25 20:19:58, James Hawkins wrote:
> http://codereview.chromium.org/2801020/diff/1/5
> File chrome/browser/views/autofill_profiles_view_win.h (left):
> 
> http://codereview.chromium.org/2801020/diff/1/5#oldcode274
> chrome/browser/views/autofill_profiles_view_win.h:274: TEXT_LABEL,
> We shouldn't remove labels until we actually implement inferred labels.

So, do we follow the latest mocks, or don't we? Currently I use preview string
(Name + first line of address as an address identifier)

[James Hawkins, 2010-06-25 20:36:06]

On 2010/06/25 20:33:29, GeorgeY wrote:
> On 2010/06/25 20:19:58, James Hawkins wrote:
> > http://codereview.chromium.org/2801020/diff/1/5
> > File chrome/browser/views/autofill_profiles_view_win.h (left):
> > 
> > http://codereview.chromium.org/2801020/diff/1/5#oldcode274
> > chrome/browser/views/autofill_profiles_view_win.h:274: TEXT_LABEL,
> > We shouldn't remove labels until we actually implement inferred labels.
> 
> So, do we follow the latest mocks, or don't we? Currently I use preview string
> (Name + first line of address as an address identifier)

We can't follow the mocks until we implement inferred labels.  The information
shown in the dialog comes from the inferred labels, which won't necessarily be
the address.  What if a profile doesn't have an address, etc?

[GeorgeY, 2010-06-25 20:41:12]

On 2010/06/25 20:36:06, James Hawkins wrote:
> On 2010/06/25 20:33:29, GeorgeY wrote:
> > On 2010/06/25 20:19:58, James Hawkins wrote:
> > > http://codereview.chromium.org/2801020/diff/1/5
> > > File chrome/browser/views/autofill_profiles_view_win.h (left):
> > > 
> > > http://codereview.chromium.org/2801020/diff/1/5#oldcode274
> > > chrome/browser/views/autofill_profiles_view_win.h:274: TEXT_LABEL,
> > > We shouldn't remove labels until we actually implement inferred labels.
> > 
> > So, do we follow the latest mocks, or don't we? Currently I use preview
string
> > (Name + first line of address as an address identifier)
> 
> We can't follow the mocks until we implement inferred labels.  The information
> shown in the dialog comes from the inferred labels, which won't necessarily be
> the address.  What if a profile doesn't have an address, etc?
It has to have *something*: either name or address, or it does not make much
sense. Preview includes *both*

[GeorgeY, 2010-06-29 19:49:01]

Updated CL

[James Hawkins, 2010-06-29 21:07:01]

http://codereview.chromium.org/2801020/diff/31001/32004
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/2801020/diff/31001/32004#newcode58
chrome/browser/autofill/autofill_profile.h:58: // Adjusts the labels according
to profile data.
Remove the |label_| member and make this method return a vector of strings.  A
profile's label is ephemeral in that it is based on the other profiles that are
being compared against, so there's no use storing it.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode59
chrome/browser/autofill/autofill_profile.h:59: // Empty labels remain *only* for
completely empty profiles.
I don't understand this sentence about empty profiles.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode60
chrome/browser/autofill/autofill_profile.h:60: // For the rest of the profiles,
they are:
What is 'they'?

http://codereview.chromium.org/2801020/diff/31001/32004#newcode61
chrome/browser/autofill/autofill_profile.h:61: // 1. Full name, including
middle.
Full name implies middle; no need to call it out.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode70
chrome/browser/autofill/autofill_profile.h:70: static bool
AdjustInferredLabels(std::vector<AutoFillProfile*> *profiles);
GetInferredLabels(profiles, std::vector<string16>* labels);

http://codereview.chromium.org/2801020/diff/31001/32004#newcode85
chrome/browser/autofill/autofill_profile.h:85: string16 BuildFullName(bool
use_middle_name) const;
No need for this method: GetFieldText(NAME_FULL).

http://codereview.chromium.org/2801020/diff/31001/32004#newcode87
chrome/browser/autofill/autofill_profile.h:87: // Builds inferred label,
includes full name at the beginning.
'includes full name at the beginning': While name should be at the top of the
importance rankings, you can't assume that any fields are non-empty.

[GeorgeY, 2010-06-29 21:36:15]

Will re-upload when compiles...

http://codereview.chromium.org/2801020/diff/31001/32004
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/2801020/diff/31001/32004#newcode58
chrome/browser/autofill/autofill_profile.h:58: // Adjusts the labels according
to profile data.
On 2010/06/29 21:07:01, James Hawkins wrote:
> Remove the |label_| member and make this method return a vector of strings.  A
> profile's label is ephemeral in that it is based on the other profiles that
are
> being compared against, so there's no use storing it.

Actually no. These labels are *not* changed until user edits the data. They are
going to be used for:
1. Indexing in Credit Card
2. Possibly some previews.

In addition this is a fairly long operation O(N*M), where N is a number of
profiles and M is number of indexable properties, so it makes sense to cache it
anyway.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode59
chrome/browser/autofill/autofill_profile.h:59: // Empty labels remain *only* for
completely empty profiles.
On 2010/06/29 21:07:01, James Hawkins wrote:
> I don't understand this sentence about empty profiles.

It means Label will include *some* data, even if it is only a company name

http://codereview.chromium.org/2801020/diff/31001/32004#newcode60
chrome/browser/autofill/autofill_profile.h:60: // For the rest of the profiles,
they are:
On 2010/06/29 21:07:01, James Hawkins wrote:
> What is 'they'?

Labels. Reworded.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode61
chrome/browser/autofill/autofill_profile.h:61: // 1. Full name, including
middle.
On 2010/06/29 21:07:01, James Hawkins wrote:
> Full name implies middle; no need to call it out.

Done.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode70
chrome/browser/autofill/autofill_profile.h:70: static bool
AdjustInferredLabels(std::vector<AutoFillProfile*> *profiles);
On 2010/06/29 21:07:01, James Hawkins wrote:
> GetInferredLabels(profiles, std::vector<string16>* labels);

See above.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode85
chrome/browser/autofill/autofill_profile.h:85: string16 BuildFullName(bool
use_middle_name) const;
On 2010/06/29 21:07:01, James Hawkins wrote:
> No need for this method: GetFieldText(NAME_FULL).

Done.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode87
chrome/browser/autofill/autofill_profile.h:87: // Builds inferred label,
includes full name at the beginning.
On 2010/06/29 21:07:01, James Hawkins wrote:
> 'includes full name at the beginning': While name should be at the top of the
> importance rankings, you can't assume that any fields are non-empty.

changed to 'includes full name (if present) at the beginning'

[James Hawkins, 2010-06-29 21:57:26]

http://codereview.chromium.org/2801020/diff/31001/32003
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/2801020/diff/31001/32003#newcode186
chrome/browser/autofill/autofill_profile.cc:186: const AutoFillFieldType
distinguishing_fields[] = {
The order is:

NAME_FULL,
ADDRESS_HOME_LINE1,
ADDRESS_HOME_CITY,
ADDRESS_HOME_STATE,
ADDRESS_HOME_ZIP,
ADDRESS_HOME_COUNTRY,
EMAIL_ADDRESS,
PHONE_HOME_WHOLE_NUMBER,
PHONE_FAX_WHOLE_NUMBER
COMPANY_NAME

http://codereview.chromium.org/2801020/diff/31001/32004
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/2801020/diff/31001/32004#newcode58
chrome/browser/autofill/autofill_profile.h:58: // Adjusts the labels according
to profile data.
On 2010/06/29 21:36:15, GeorgeY wrote:
> On 2010/06/29 21:07:01, James Hawkins wrote:
> > Remove the |label_| member and make this method return a vector of strings. 
A
> > profile's label is ephemeral in that it is based on the other profiles that
> are
> > being compared against, so there's no use storing it.
> 
> Actually no. These labels are *not* changed until user edits the data. They
are
> going to be used for:
> 1. Indexing in Credit Card
> 2. Possibly some previews.
> 
> In addition this is a fairly long operation O(N*M), where N is a number of
> profiles and M is number of indexable properties, so it makes sense to cache
it
> anyway.

For inferred labels, the set of profiles sent in will not always be the entire
set of available profiles.  Please think about the fact that we're using
inferred labels for the form UI.  If the user has three profiles, and only two
match the typed value 'Ge', the inferred labels are only based on two profiles.

You could potentially cache the inferred labels for the sake of the AF dialog,
where the entire set of profiles is used to infer labels, but you'd have to
invalidate all of the labels any time the user edits any profile data.  It's
safe to assume that the user is visiting the dialog to modify data.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode59
chrome/browser/autofill/autofill_profile.h:59: // Empty labels remain *only* for
completely empty profiles.
On 2010/06/29 21:36:15, GeorgeY wrote:
> On 2010/06/29 21:07:01, James Hawkins wrote:
> > I don't understand this sentence about empty profiles.
> 
> It means Label will include *some* data, even if it is only a company name

You refer to 'Empty labels'. That's what I don't understand.

http://codereview.chromium.org/2801020/diff/31001/32004#newcode60
chrome/browser/autofill/autofill_profile.h:60: // For the rest of the profiles,
they are:
On 2010/06/29 21:36:15, GeorgeY wrote:
> On 2010/06/29 21:07:01, James Hawkins wrote:
> > What is 'they'?
> 
> Labels. Reworded.

Gotcha. Because it's confusing. can you please be more specific in the comment?

[GeorgeY, 2010-06-29 23:01:32]

On 2010/06/29 21:57:26, James Hawkins wrote:
> http://codereview.chromium.org/2801020/diff/31001/32003
> File chrome/browser/autofill/autofill_profile.cc (right):
> 
> http://codereview.chromium.org/2801020/diff/31001/32003#newcode186
> chrome/browser/autofill/autofill_profile.cc:186: const AutoFillFieldType
> distinguishing_fields[] = {
> The order is:
> 
> NAME_FULL,
> ADDRESS_HOME_LINE1,
> ADDRESS_HOME_CITY,
> ADDRESS_HOME_STATE,
> ADDRESS_HOME_ZIP,
> ADDRESS_HOME_COUNTRY,
> EMAIL_ADDRESS,
> PHONE_HOME_WHOLE_NUMBER,
> PHONE_FAX_WHOLE_NUMBER
> COMPANY_NAME
done.
> 
> http://codereview.chromium.org/2801020/diff/31001/32004
> File chrome/browser/autofill/autofill_profile.h (right):
> 
> http://codereview.chromium.org/2801020/diff/31001/32004#newcode58
> chrome/browser/autofill/autofill_profile.h:58: // Adjusts the labels according
> to profile data.
> On 2010/06/29 21:36:15, GeorgeY wrote:
> > On 2010/06/29 21:07:01, James Hawkins wrote:
> > > Remove the |label_| member and make this method return a vector of
strings. 
> A
> > > profile's label is ephemeral in that it is based on the other profiles
that
> > are
> > > being compared against, so there's no use storing it.
> > 
> > Actually no. These labels are *not* changed until user edits the data. They
> are
> > going to be used for:
> > 1. Indexing in Credit Card
> > 2. Possibly some previews.
> > 
> > In addition this is a fairly long operation O(N*M), where N is a number of
> > profiles and M is number of indexable properties, so it makes sense to cache
> it
> > anyway.
> 
> For inferred labels, the set of profiles sent in will not always be the entire
> set of available profiles.  Please think about the fact that we're using
> inferred labels for the form UI.  If the user has three profiles, and only two
> match the typed value 'Ge', the inferred labels are only based on two
profiles.
> 
> You could potentially cache the inferred labels for the sake of the AF dialog,
> where the entire set of profiles is used to infer labels, but you'd have to
> invalidate all of the labels any time the user edits any profile data.  It's
> safe to assume that the user is visiting the dialog to modify data.
added such function
CreateInferredLabels(const std::vector<AutoFillProfile*>* profiles,
                     std::vector<string16>* created_labels);

> 
> http://codereview.chromium.org/2801020/diff/31001/32004#newcode59
> chrome/browser/autofill/autofill_profile.h:59: // Empty labels remain *only*
for
> completely empty profiles.
> On 2010/06/29 21:36:15, GeorgeY wrote:
> > On 2010/06/29 21:07:01, James Hawkins wrote:
> > > I don't understand this sentence about empty profiles.
> > 
> > It means Label will include *some* data, even if it is only a company name
> 
> You refer to 'Empty labels'. That's what I don't understand.
removed.
> 
> http://codereview.chromium.org/2801020/diff/31001/32004#newcode60
> chrome/browser/autofill/autofill_profile.h:60: // For the rest of the
profiles,
> they are:
> On 2010/06/29 21:36:15, GeorgeY wrote:
> > On 2010/06/29 21:07:01, James Hawkins wrote:
> > > What is 'they'?
> > 
> > Labels. Reworded.
> 
> Gotcha. Because it's confusing. can you please be more specific in the
comment?
Done.

[James Hawkins, 2010-06-30 00:36:14]

http://codereview.chromium.org/2801020/diff/12008/9006
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/2801020/diff/12008/9006#newcode220
chrome/browser/autofill/autofill_profile.cc:220: // NAME_FULL is a special
field, that always appears in the inferred label.
This is not true.  There's nothing to stop a profile from having an empty full
name field.

[GeorgeY, 2010-06-30 18:35:59]

http://codereview.chromium.org/2801020/diff/12008/9006
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/2801020/diff/12008/9006#newcode220
chrome/browser/autofill/autofill_profile.cc:220: // NAME_FULL is a special
field, that always appears in the inferred label.
On 2010/06/30 00:36:15, James Hawkins wrote:
> This is not true.  There's nothing to stop a profile from having an empty full
> name field.

Added:
    // if it present in the profile.

[James Hawkins, 2010-06-30 18:53:03]

http://codereview.chromium.org/2801020/diff/12008/9006
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/2801020/diff/12008/9006#newcode220
chrome/browser/autofill/autofill_profile.cc:220: // NAME_FULL is a special
field, that always appears in the inferred label.
On 2010/06/30 18:36:00, GeorgeY wrote:
> On 2010/06/30 00:36:15, James Hawkins wrote:
> > This is not true.  There's nothing to stop a profile from having an empty
full
> > name field.
> 
> Added:
>     // if it present in the profile.
> 

It's not a special field, so why label it as one?

http://codereview.chromium.org/2801020/diff/48001/41004#newcode220
chrome/browser/autofill/autofill_profile.cc:220: // NAME_FULL is a special
field, that always appears in the inferred label.
I still don't see why you're special casing NAME_FULL. Just put it at the top of
the list and let the algorithm work its magic.

[GeorgeY, 2010-06-30 19:42:37]

http://codereview.chromium.org/2801020/diff/48001/41004
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/2801020/diff/48001/41004#newcode220
chrome/browser/autofill/autofill_profile.cc:220: // NAME_FULL is a special
field, that always appears in the inferred label,
On 2010/06/30 18:53:03, James Hawkins wrote:
> I still don't see why you're special casing NAME_FULL. Just put it at the top
of
> the list and let the algorithm work its magic.

Why I left it: The name is a necessary hint in the preview, if it present. Phone
number out of context is hardly helpful, especially if you have a lot of
profiles auto saved. For example my phone and my mother phone are different only
by one digit, and it is easy to put wrong number without the name, which will be
omitted if I do what you suggesting - the phones *are* different after all.

[James Hawkins, 2010-06-30 19:49:57]

On 2010/06/30 19:42:37, GeorgeY wrote:
> http://codereview.chromium.org/2801020/diff/48001/41004
> File chrome/browser/autofill/autofill_profile.cc (right):
> 
> http://codereview.chromium.org/2801020/diff/48001/41004#newcode220
> chrome/browser/autofill/autofill_profile.cc:220: // NAME_FULL is a special
> field, that always appears in the inferred label,
> On 2010/06/30 18:53:03, James Hawkins wrote:
> > I still don't see why you're special casing NAME_FULL. Just put it at the
top
> of
> > the list and let the algorithm work its magic.
> 
> Why I left it: The name is a necessary hint in the preview, if it present.
Phone
> number out of context is hardly helpful, especially if you have a lot of
> profiles auto saved. For example my phone and my mother phone are different
only
> by one digit, and it is easy to put wrong number without the name, which will
be
> omitted if I do what you suggesting - the phones *are* different after all.

I see what you're saying.  You're saying that if there are two profiles with the
same name, we should display the name anyway, even though they're equal.

Taking this offline, as we need to hash out this algorithm.

[GeorgeY, 2010-07-16 22:47:57]

Based on new inferredLabels

[James Hawkins, 2010-07-16 22:56:07]

LGTM