Added inferred labels implementation. Label inferred from the person name + d...

Added inferred labels implementation. Label inferred from the person name + differences in other fields to distinguish entries.
Also added it to generated previews.
TODO: adjust indices in the Credit Cards.
TEST=unit tested.
BUG=none

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

[GeorgeY, 2010-06-29 17:58:12]

Inferred labels implementation, so I could submit UI changes as well :)

[GeorgeY, 2010-07-02 00:48:43]

New version, with suggestions

[James Hawkins, 2010-07-07 01:06:33]

http://codereview.chromium.org/2835026/diff/37001/1008
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/2835026/diff/37001/1008#newcode416
chrome/browser/autofill/autofill_profile.cc:416: if (included_fields) {
DCHECK on |included_fields| since this is an internal method and the var should
never be NULL. Comment this requirement at the declaration.

http://codereview.chromium.org/2835026/diff/37001/1009
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/2835026/diff/37001/1009#newcode101
chrome/browser/autofill/autofill_profile.h:101: // Builds inferred label,
includes full name (if present) at the beginning.
Please take this comment about about the full name. It's implied by the
algorithm and makes it seems as though it's special, when it's not.

http://codereview.chromium.org/2835026/diff/37001/1010
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/37001/1010#newcode130
chrome/browser/autofill/autofill_profile_unittest.cc:130:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe")), profiles[0]->Label());
The label for the UI summary should include two pieces of profile data, so even
though the addresses are different, we still display the name and address.

http://codereview.chromium.org/2835026/diff/37001/1010#newcode154
chrome/browser/autofill/autofill_profile_unittest.cc:154:
EXPECT_EQ(string16(ASCIIToUTF16("Jane Doe")), profiles[1]->Label());
The 1st and 3rd are good, second needs address as above.

http://codereview.chromium.org/2835026/diff/37001/1010#newcode181
chrome/browser/autofill/autofill_profile_unittest.cc:181:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe, 22222222222 fax")),
Can we change this format to:

"fax:#"?

It's much easier to read.

http://codereview.chromium.org/2835026/diff/37001/1010#newcode206
chrome/browser/autofill/autofill_profile_unittest.cc:206:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe, 11111111111, 22222222222 fax")),
Hmm, we shouldn't be displaying more than two pieces of profile data:

John Doe, fax:22222222222
John Doe, fax:33333333333
John Doe, 444444444444

[James Hawkins, 2010-07-07 15:57:15]

Also, I just tried this out using one profile, and the suggestion for the name
field was:

James (James Hawkins)

which seems a bit redundant. We should modify the algorithm to exclude the field
type of the field the user is currently typing in.

[James Hawkins, 2010-07-07 15:58:37]

On 2010/07/07 15:57:15, James Hawkins wrote:
> Also, I just tried this out using one profile, and the suggestion for the name
> field was:
> 
> James (James Hawkins)
> 
> which seems a bit redundant. We should modify the algorithm to exclude the
field
> type of the field the user is currently typing in.

I added another profile which only changed the address fields (the name is the
same), and the label still showed the full name. Why? The label is one piece of
profile data only.

[GeorgeY, 2010-07-07 17:34:06]

3 - fixed, 3 - I do not agree: see my comments.

http://codereview.chromium.org/2835026/diff/37001/1008
File chrome/browser/autofill/autofill_profile.cc (right):

http://codereview.chromium.org/2835026/diff/37001/1008#newcode416
chrome/browser/autofill/autofill_profile.cc:416: if (included_fields) {
On 2010/07/07 01:06:33, James Hawkins wrote:
> DCHECK on |included_fields| since this is an internal method and the var
should
> never be NULL. Comment this requirement at the declaration.

Old check, fixed.

http://codereview.chromium.org/2835026/diff/37001/1009
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/2835026/diff/37001/1009#newcode101
chrome/browser/autofill/autofill_profile.h:101: // Builds inferred label,
includes full name (if present) at the beginning.
On 2010/07/07 01:06:33, James Hawkins wrote:
> Please take this comment about about the full name. It's implied by the
> algorithm and makes it seems as though it's special, when it's not.
Fixed comment

http://codereview.chromium.org/2835026/diff/37001/1010
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/37001/1010#newcode130
chrome/browser/autofill/autofill_profile_unittest.cc:130:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe")), profiles[0]->Label());
On 2010/07/07 01:06:33, James Hawkins wrote:
> The label for the UI summary should include two pieces of profile data, so
even
> though the addresses are different, we still display the name and address.
I do *not* agree. We need to show data enough to differentiate between he
profiles, but show as little of redundant data as needed. If the first field is
redundant it still needs to be shown to indicate similarity of profiles.

http://codereview.chromium.org/2835026/diff/37001/1010#newcode154
chrome/browser/autofill/autofill_profile_unittest.cc:154:
EXPECT_EQ(string16(ASCIIToUTF16("Jane Doe")), profiles[1]->Label());
On 2010/07/07 01:06:33, James Hawkins wrote:
> The 1st and 3rd are good, second needs address as above.

See above.

http://codereview.chromium.org/2835026/diff/37001/1010#newcode181
chrome/browser/autofill/autofill_profile_unittest.cc:181:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe, 22222222222 fax")),
On 2010/07/07 01:06:33, James Hawkins wrote:
> Can we change this format to:
> 
> "fax:#"?
> 
> It's much easier to read.

No problem - done.

http://codereview.chromium.org/2835026/diff/37001/1010#newcode206
chrome/browser/autofill/autofill_profile_unittest.cc:206:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe, 11111111111, 22222222222 fax")),
On 2010/07/07 01:06:33, James Hawkins wrote:
> Hmm, we shouldn't be displaying more than two pieces of profile data:
> 
> John Doe, fax:22222222222
> John Doe, fax:33333333333
> John Doe, 444444444444

And is #2 in your list profiles[2] or profiles[3]? It is not clear. I think we
need to display complete differentiating key.

[GeorgeY, 2010-07-07 17:35:51]

On 2010/07/07 15:57:15, James Hawkins wrote:
> Also, I just tried this out using one profile, and the suggestion for the name
> field was:
> 
> James (James Hawkins)
> 
> which seems a bit redundant. We should modify the algorithm to exclude the
field
> type of the field the user is currently typing in.

I think this is correct. We need to display info to identify profile completely.

[GeorgeY, 2010-07-07 17:37:01]

On 2010/07/07 15:58:37, James Hawkins wrote:
> On 2010/07/07 15:57:15, James Hawkins wrote:
> > Also, I just tried this out using one profile, and the suggestion for the
name
> > field was:
> > 
> > James (James Hawkins)
> > 
> > which seems a bit redundant. We should modify the algorithm to exclude the
> field
> > type of the field the user is currently typing in.
> 
> I added another profile which only changed the address fields (the name is the
> same), and the label still showed the full name. Why? The label is one piece
of
> profile data only.

Because of our previous discussion we *always* need to show first relevant field
to ease differentiating of profiles.

[James Hawkins, 2010-07-07 18:27:20]

http://codereview.chromium.org/2835026/diff/37001/1010
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/37001/1010#newcode130
chrome/browser/autofill/autofill_profile_unittest.cc:130:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe")), profiles[0]->Label());
On 2010/07/07 17:34:06, GeorgeY wrote:
> On 2010/07/07 01:06:33, James Hawkins wrote:
> > The label for the UI summary should include two pieces of profile data, so
> even
> > though the addresses are different, we still display the name and address.
> I do *not* agree. We need to show data enough to differentiate between he
> profiles, but show as little of redundant data as needed. If the first field
is
> redundant it still needs to be shown to indicate similarity of profiles.
> 
> 

I'm fine with using the complete differentiating key, but I'm insisting on
having at least 2 pieces of profile data in the profile preview.  We're not
*only* differentiating profiles in the dialog; we're also previewing the
profile. Having just the name shown is not enough preview.

http://codereview.chromium.org/2835026/diff/37001/1010#newcode206
chrome/browser/autofill/autofill_profile_unittest.cc:206:
EXPECT_EQ(string16(ASCIIToUTF16("John Doe, 11111111111, 22222222222 fax")),
On 2010/07/07 17:34:06, GeorgeY wrote:
> On 2010/07/07 01:06:33, James Hawkins wrote:
> > Hmm, we shouldn't be displaying more than two pieces of profile data:
> > 
> > John Doe, fax:22222222222
> > John Doe, fax:33333333333
> > John Doe, 444444444444
> 
> And is #2 in your list profiles[2] or profiles[3]? It is not clear. I think we
> need to display complete differentiating key.

Hmm, good point.  Yes, I think you're right in that we'll need the complete
differentiating key; however, two points:
a) We should not display just the diff key if the key contains only one value. 
See my previous comment.
b) The complete differentiating key for the first profile is only "John Doe,
fax:22222222222" because that fax number exists nowhere else.

[GeorgeY, 2010-07-07 19:36:59]

On 2010/07/07 18:27:20, James Hawkins wrote:
> http://codereview.chromium.org/2835026/diff/37001/1010
> File chrome/browser/autofill/autofill_profile_unittest.cc (right):
> 
> http://codereview.chromium.org/2835026/diff/37001/1010#newcode130
> chrome/browser/autofill/autofill_profile_unittest.cc:130:
> EXPECT_EQ(string16(ASCIIToUTF16("John Doe")), profiles[0]->Label());
> On 2010/07/07 17:34:06, GeorgeY wrote:
> > On 2010/07/07 01:06:33, James Hawkins wrote:
> > > The label for the UI summary should include two pieces of profile data, so
> > even
> > > though the addresses are different, we still display the name and address.
> > I do *not* agree. We need to show data enough to differentiate between he
> > profiles, but show as little of redundant data as needed. If the first field
> is
> > redundant it still needs to be shown to indicate similarity of profiles.
> > 
> > 
> 
> I'm fine with using the complete differentiating key, but I'm insisting on
> having at least 2 pieces of profile data in the profile preview.  We're not
> *only* differentiating profiles in the dialog; we're also previewing the
> profile. Having just the name shown is not enough preview.
> 
adding...
> http://codereview.chromium.org/2835026/diff/37001/1010#newcode206
> chrome/browser/autofill/autofill_profile_unittest.cc:206:
> EXPECT_EQ(string16(ASCIIToUTF16("John Doe, 11111111111, 22222222222 fax")),
> On 2010/07/07 17:34:06, GeorgeY wrote:
> > On 2010/07/07 01:06:33, James Hawkins wrote:
> > > Hmm, we shouldn't be displaying more than two pieces of profile data:
> > > 
> > > John Doe, fax:22222222222
> > > John Doe, fax:33333333333
> > > John Doe, 444444444444
> > 
> > And is #2 in your list profiles[2] or profiles[3]? It is not clear. I think
we
> > need to display complete differentiating key.
> 
> Hmm, good point.  Yes, I think you're right in that we'll need the complete
> differentiating key; however, two points:
> a) We should not display just the diff key if the key contains only one value.

> See my previous comment.
> b) The complete differentiating key for the first profile is only "John Doe,
> fax:22222222222" because that fax number exists nowhere else.
I think this will be not always clear: why we shown full key for other profiles,
and only partial for this. In addition my current algorithm is O(N*M) for N
profiles with M significant fields, what you suggesting is O(N*N*M)

[GeorgeY, 2010-07-13 18:04:10]

Done what we decided :)

[James Hawkins, 2010-07-13 18:15:52]

http://codereview.chromium.org/2835026/diff/47001/48003
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/47001/48003#newcode304
chrome/browser/autofill/autofill_manager_unittest.cc:304:
EXPECT_EQ(ASCIIToUTF16("Elvis Aaron Presley"), labels[0]);
The user is filling in the name field, so we shouldn't be showing the name as
the inferred label.

http://codereview.chromium.org/2835026/diff/47001/48006
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/47001/48006#newcode153
chrome/browser/autofill/autofill_profile_unittest.cc:153: ASCIIToUTF16("John
Doe, 666 Erebus St., johndoe@hades.com")),
Why is the address included for [0] and [2]? It's not a unique field for those
two profiles.

[GeorgeY, 2010-07-13 20:10:49]

http://codereview.chromium.org/2835026/diff/47001/48003
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/47001/48003#newcode304
chrome/browser/autofill/autofill_manager_unittest.cc:304:
EXPECT_EQ(ASCIIToUTF16("Elvis Aaron Presley"), labels[0]);
On 2010/07/13 18:15:52, James Hawkins wrote:
> The user is filling in the name field, so we shouldn't be showing the name as
> the inferred label.

Done.

http://codereview.chromium.org/2835026/diff/47001/48006
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/47001/48006#newcode153
chrome/browser/autofill/autofill_profile_unittest.cc:153: ASCIIToUTF16("John
Doe, 666 Erebus St., johndoe@hades.com")),
On 2010/07/13 18:15:52, James Hawkins wrote:
> Why is the address included for [0] and [2]? It's not a unique field for those
> two profiles.

As we agreed first *two* significant fields, regardless if they match or not

[James Hawkins, 2010-07-13 20:15:44]

http://codereview.chromium.org/2835026/diff/47001/48006
File chrome/browser/autofill/autofill_profile_unittest.cc (right):

http://codereview.chromium.org/2835026/diff/47001/48006#newcode153
chrome/browser/autofill/autofill_profile_unittest.cc:153: ASCIIToUTF16("John
Doe, 666 Erebus St., johndoe@hades.com")),
On 2010/07/13 20:10:50, GeorgeY wrote:
> On 2010/07/13 18:15:52, James Hawkins wrote:
> > Why is the address included for [0] and [2]? It's not a unique field for
those
> > two profiles.
> 
> As we agreed first *two* significant fields, regardless if they match or not

OK. I thought about it and I'm fine with that.

[James Hawkins, 2010-07-13 20:48:59]

Looks like you'll need to fix up the tests.

[James Hawkins, 2010-07-16 18:30:12]

LGTM