Android: Get AutofillProfile summary labels from PersonalDataManagerAndroid

Android: Get AutofillProfile summary labels from PersonalDataManagerAndroid

This will be used for the Android preferences UI

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

[Evan Stade, 2014-08-06 00:24:39]

+isherman for components/autofill/core/browser/autofill_profile.cc

+aruslan for everything else

https://codereview.chromium.org/440283002/diff/1/chrome/browser/autofill/andr...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/440283002/diff/1/chrome/browser/autofill/andr...
chrome/browser/autofill/android/personal_data_manager_android.h:80:
base::android::ScopedJavaLocalRef<jobjectArray> GetProfileLabels(
Q: Why is this not just part of GetProfileBy*?

A: Because the labels are created in such a way to distinguish the profiles from
each other. You can only create the list of labels by looking at all the
profiles together.

[Ilya Sherman, 2014-08-06 01:28:17]

https://codereview.chromium.org/440283002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/440283002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_profile.cc:80: if (excluded_field ==
UNKNOWN_TYPE) {
Why might there be an excluded field without a list of suggested fields?  The
excluded field is supposed to be the one the user is interacting with; but if
there's a field that the user is interacting with, then the suggested_fields
should be all the other field types found in the form (and no types that aren't
found in the form).

[aruslan, 2014-08-06 16:06:52]

Please move autofill_profile.cc change to another CL (it doesn't seem to belong
here), and please add tests for the summary labels or extend existing ones in
PersonalDataManagerTest.java.

https://codereview.chromium.org/440283002/diff/1/chrome/browser/autofill/andr...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/440283002/diff/1/chrome/browser/autofill/andr...
chrome/browser/autofill/android/personal_data_manager_android.h:80:
base::android::ScopedJavaLocalRef<jobjectArray> GetProfileLabels(
On 2014/08/06 00:24:39, Evan Stade wrote:
> Q: Why is this not just part of GetProfileBy*?
> 
> A: Because the labels are created in such a way to distinguish the profiles
from
> each other. You can only create the list of labels by looking at all the
> profiles together.

Acknowledged.

[Evan Stade, 2014-08-06 17:59:19]

> Please move autofill_profile.cc change to another CL (it doesn't seem to
belong here)

It is in fact required for this CL to work.

> and please add tests for the summary labels or extend existing ones in
> PersonalDataManagerTest.java.

Will do.

https://codereview.chromium.org/440283002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/440283002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_profile.cc:80: if (excluded_field ==
UNKNOWN_TYPE) {
On 2014/08/06 01:28:17, Ilya Sherman wrote:
> Why might there be an excluded field without a list of suggested fields?  The
> excluded field is supposed to be the one the user is interacting with; but if
> there's a field that the user is interacting with, then the suggested_fields
> should be all the other field types found in the form (and no types that
aren't
> found in the form).

Because Android puts the full name on the first line and further distinguishing
info on the second line for the profile summaries. (In contrast, desktop puts
everything on one line.) Thus to get the second-line summary for Android, we
want to exclude FULL_NAME but otherwise rely on the default distinguishing
fields.

[Evan Stade, 2014-08-06 18:00:47]

https://codereview.chromium.org/440283002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/440283002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_profile.cc:80: if (excluded_field ==
UNKNOWN_TYPE) {
On 2014/08/06 17:59:19, Evan Stade wrote:
> On 2014/08/06 01:28:17, Ilya Sherman wrote:
> > Why might there be an excluded field without a list of suggested fields? 
The
> > excluded field is supposed to be the one the user is interacting with; but
if
> > there's a field that the user is interacting with, then the suggested_fields
> > should be all the other field types found in the form (and no types that
> aren't
> > found in the form).
> 
> Because Android puts the full name on the first line and further
distinguishing
> info on the second line for the profile summaries. (In contrast, desktop puts
> everything on one line.) Thus to get the second-line summary for Android, we
> want to exclude FULL_NAME but otherwise rely on the default distinguishing
> fields.

(I'm talking about the preferences UI on desktop and Android; perhaps that
wasn't clear)

[Ilya Sherman, 2014-08-06 19:15:35]

I see -- thanks for the explanation.  Could you please add test coverage for
this function being called with no suggested_fields, but a non-unknown excluded
field?  Other than that, LGTM.

https://codereview.chromium.org/440283002/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/440283002/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:79: suggested_fields =
&default_fields;
nit: Please move this to be after the early return.

[Evan Stade, 2014-08-06 21:04:27]

tests added for both AutofillProfile and PersonalDataManager.java

https://codereview.chromium.org/440283002/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/440283002/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:79: suggested_fields =
&default_fields;
On 2014/08/06 19:15:35, Ilya Sherman wrote:
> nit: Please move this to be after the early return.

Done.

[Ilya Sherman, 2014-08-06 21:07:36]

Thanks.  Still LGTM :)

[aruslan, 2014-08-06 21:10:41]

lgtm thanks!

[Evan Stade, 2014-08-06 21:13:28]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-06 21:15:31]

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

[commit-bot: I haz the power, 2014-08-07 07:08:10]

Change committed as 287978