[Payments] Show name and address when selecting a CC billing address.

Also make the dropdown selection text be multiline.

BUG=637332
Committed: https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
Cr-Commit-Position: refs/heads/master@{#415286}

[sebsg, 2016-08-26 14:55:06]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-08-26 14:55:54]

sebsg@chromium.org changed reviewers:
+ rouslan@chromium.org

[sebsg, 2016-08-26 14:55:54]

Hi Rouslan, PTAL?

[please use gerrit instead, 2016-08-26 16:46:15]

Looking good!

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/multiline_spinner_item.xml (right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:1: <?xml version="1.0"
encoding="utf-8"?>
Since you decided to go with ellipsis instead of multiline for spinner items, do
we still need this file?

If we need it, it should be called something like "ellipsis_spinner_item.xml"
not "multiline_spinner_item.xml", right? :-)

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:8: <TextView
xmlns:android="http://schemas.android.com/apk/res/android"
Is this a trailing whitespace? If so, remove it.

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:9:
android:id="@android:id/text1"
Remove the ID

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:11:
android:singleLine="false"
If you want a single-line with ellipsis, then you should set singleLine="true".

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java
(right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:541:
includeName),
This indentation looks funny. Something like this instead?

return getProfilesWithLabels(
        nativeGetProfileLabelsToSuggest(
                mPersonalDataManagerAndroid, includeName),
        nativeGetprofileGUIDstoSuggest(mPersonalDataManagerAndroid));

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java
(right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:67:
mDropdown = (Spinner) mLayout.findViewById(R.id.spinner);
How do you accomplish the folded dropdown look of multiple lines? Is that
determined by the adapter as well?

https://codereview.chromium.org/2287513002/diff/40001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.cc:668: if
(include_name) suggested_fields->push_back(NAME_FULL);
Single-line if statement is allowed in Java, but not in C++.

if (include_name)
  suggested_fields->push_back(NAME_FULL);

[sebsg, 2016-08-26 19:36:45]

Thanks, another look?

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/multiline_spinner_item.xml (right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2016/08/26 16:46:15, rouslan wrote:
> Since you decided to go with ellipsis instead of multiline for spinner items,
do
> we still need this file?
> 
> If we need it, it should be called something like "ellipsis_spinner_item.xml"
> not "multiline_spinner_item.xml", right? :-)

That resource file is for the text value of the drop down. The selections use
ellipsis but once you selected it we can show multiline :)

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:8: <TextView
xmlns:android="http://schemas.android.com/apk/res/android"
On 2016/08/26 16:46:15, rouslan wrote:
> Is this a trailing whitespace? If so, remove it.

Done.

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:9:
android:id="@android:id/text1"
On 2016/08/26 16:46:15, rouslan wrote:
> Remove the ID

Done.

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:11:
android:singleLine="false"
On 2016/08/26 16:46:15, rouslan wrote:
> If you want a single-line with ellipsis, then you should set
singleLine="true".

This one actually does multiline :)

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java
(right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:541:
includeName),
On 2016/08/26 16:46:15, rouslan wrote:
> This indentation looks funny. Something like this instead?
> 
> return getProfilesWithLabels(
>         nativeGetProfileLabelsToSuggest(
>                 mPersonalDataManagerAndroid, includeName),
>         nativeGetprofileGUIDstoSuggest(mPersonalDataManagerAndroid));

Done.

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java
(right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:67:
mDropdown = (Spinner) mLayout.findViewById(R.id.spinner);
On 2016/08/26 16:46:15, rouslan wrote:
> How do you accomplish the folded dropdown look of multiple lines? Is that
> determined by the adapter as well?

It's determined by the "multiline_spinner_item" resource. For the selection
dropdown we would have to change the "simple_spinner_dropdown_item" to a new
"multiline_spinner_dropdown_item".

https://codereview.chromium.org/2287513002/diff/40001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/2287513002/diff/40001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.cc:668: if
(include_name) suggested_fields->push_back(NAME_FULL);
On 2016/08/26 16:46:15, rouslan wrote:
> Single-line if statement is allowed in Java, but not in C++.
> 
> if (include_name)
>   suggested_fields->push_back(NAME_FULL);

Done.

[please use gerrit instead, 2016-08-26 19:49:05]

lgtm

[sebsg, 2016-08-26 20:21:44]

sebsg@chromium.org changed reviewers:
+ bauerb@chromium.org, vabr@chromium.org

[sebsg, 2016-08-26 20:21:44]

vabr@chromium.org: Could you please review the changes in chrome/browser/*?
Thanks!

bauerb@chromium.org: Could you please review the changes in
chrome/android/javatests/* PersonalDataManager.java and
multiline_spinner_item.xml? Thanks!

[vabr (Chromium), 2016-08-29 08:47:04]

//chrome/browser/autofill LGTM!

https://codereview.chromium.org/2287513002/diff/60001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/2287513002/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.h:65: // included
or not based on the |include_name| parameter. All other fields are
optional nit: The current phrasing is ambiguous (had to force myself not to read
it as "The name is included, or it is not based on...". What about:

// The |include_name| argument controls whether the name is included.

[sebsg, 2016-08-29 13:18:28]

Description was changed from

==========
[Payments] Show name and address when selecting a CC billing address.

BUG=637332
==========

to

==========
Also make the dropdown selection text be multiline.

BUG=637332
==========

[sebsg, 2016-08-29 13:22:42]

sebsg@chromium.org changed reviewers:
- bauerb@chromium.org

[sebsg, 2016-08-29 13:24:08]

sebsg@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[sebsg, 2016-08-29 13:24:09]

dfalcantara@chromium.org: Could you please review the changes in
chrome/android/javatests/* PersonalDataManager.java and
multiline_spinner_item.xml? Thanks!

https://codereview.chromium.org/2287513002/diff/60001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/2287513002/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.h:65: // included
or not based on the |include_name| parameter. All other fields are
On 2016/08/29 08:47:04, vabr (Chromium) wrote:
> optional nit: The current phrasing is ambiguous (had to force myself not to
read
> it as "The name is included, or it is not based on...". What about:
> 
> // The |include_name| argument controls whether the name is included.

Done.

[gone, 2016-08-29 18:00:36]

https://chromiumcodereview.appspot.com/2287513002/diff/80001/chrome/android/j...
File chrome/android/java/res/layout/multiline_spinner_item.xml (right):

https://chromiumcodereview.appspot.com/2287513002/diff/80001/chrome/android/j...
chrome/android/java/res/layout/multiline_spinner_item.xml:3: 
Might as well just remove the newline here.

https://chromiumcodereview.appspot.com/2287513002/diff/80001/chrome/android/j...
chrome/android/java/res/layout/multiline_spinner_item.xml:13:
android:ellipsize="marquee"
Does marquee do anything in this case?  It's marked as being multiline, so it'll
just spill over onto new lines when it gets too long.

https://chromiumcodereview.appspot.com/2287513002/diff/80001/chrome/android/j...
chrome/android/java/res/layout/multiline_spinner_item.xml:14:
android:textAlignment="inherit"/>
space between " and />

https://chromiumcodereview.appspot.com/2287513002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java
(right):

https://chromiumcodereview.appspot.com/2287513002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:65:
adapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
Shouldn't the spinner item also be multiline?  The mock shows that it breaks
into multiple lines when it's too large:

https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/add%20...

[sebsg, 2016-08-29 20:08:50]

Thanks for the comments, another look?

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/multiline_spinner_item.xml (right):

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:3: 
On 2016/08/29 18:00:35, dfalcantara wrote:
> Might as well just remove the newline here.

Done.

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:13:
android:ellipsize="marquee"
On 2016/08/29 18:00:35, dfalcantara wrote:
> Does marquee do anything in this case?  It's marked as being multiline, so
it'll
> just spill over onto new lines when it gets too long.

Done.

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/multiline_spinner_item.xml:14:
android:textAlignment="inherit"/>
On 2016/08/29 18:00:35, dfalcantara wrote:
> space between " and />

Done.

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java
(right):

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:65:
adapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
On 2016/08/29 18:00:35, dfalcantara wrote:
> Shouldn't the spinner item also be multiline?  The mock shows that it breaks
> into multiple lines when it's too large:
> 
>
https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/add%20...

Right, they should eventually be multiline. However, it seems that change is not
as simple as setting "singleLine=false" in a resource file :(. Since other
things have come up, I got these changes approved in the bug. This CL can then
be used as a stepping stone toward the mocks and I will come back to it when the
other pressing stuff has been dealt with. Thanks! Btw I'd gladly take your input
and ideas of how to fix this when I get back to it :)

[gone, 2016-08-29 20:11:13]

lgtm

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java
(right):

https://codereview.chromium.org/2287513002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:65:
adapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
On 2016/08/29 20:08:50, sebsg wrote:
> On 2016/08/29 18:00:35, dfalcantara wrote:
> > Shouldn't the spinner item also be multiline?  The mock shows that it breaks
> > into multiple lines when it's too large:
> > 
> >
>
https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/add%20...
> 
> Right, they should eventually be multiline. However, it seems that change is
not
> as simple as setting "singleLine=false" in a resource file :(. Since other
> things have come up, I got these changes approved in the bug. This CL can then
> be used as a stepping stone toward the mocks and I will come back to it when
the
> other pressing stuff has been dealt with. Thanks! Btw I'd gladly take your
input
> and ideas of how to fix this when I get back to it :)

Yeah, sounds good.

[sebsg, 2016-08-29 20:12:12]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-08-29 20:12:13]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2287513002/#ps100001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-08-29 20:54:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:41:28]

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

[commit-bot: I haz the power, 2016-08-30 03:41:29]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[sebsg, 2016-08-30 12:37:25]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-08-30 12:37:26]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
rouslan@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2287513002/#ps120001
(title: "Fixed error")

[commit-bot: I haz the power, 2016-08-30 12:37:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 12:45:15]

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

[commit-bot: I haz the power, 2016-08-30 12:45:17]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[sebsg, 2016-08-30 12:52:53]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2016-08-30 12:53:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 13:48:15]

Description was changed from

==========
Also make the dropdown selection text be multiline.

BUG=637332
==========

to

==========
Also make the dropdown selection text be multiline.

BUG=637332
==========

[commit-bot: I haz the power, 2016-08-30 13:48:16]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-08-30 13:51:18]

Description was changed from

==========
Also make the dropdown selection text be multiline.

BUG=637332
==========

to

==========
Also make the dropdown selection text be multiline.

BUG=637332

Committed: https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
Cr-Commit-Position: refs/heads/master@{#415286}
==========

[commit-bot: I haz the power, 2016-08-30 13:51:19]

Patchset 6 (id:??) landed as
https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
Cr-Commit-Position: refs/heads/master@{#415286}

[Justin Donnelly, 2016-08-30 21:58:07]

Description was changed from

==========
Also make the dropdown selection text be multiline.

BUG=637332

Committed: https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
Cr-Commit-Position: refs/heads/master@{#415286}
==========

to

==========
Also make the dropdown selection text be multiline.

BUG=637332

Committed: https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
Cr-Commit-Position: refs/heads/master@{#415286}
==========

[Justin Donnelly, 2016-08-30 21:58:07]

jdonnelly@chromium.org changed reviewers:
+ jdonnelly@chromium.org, mdjones@chromium.org

[Justin Donnelly, 2016-08-30 21:59:55]

On 2016/08/30 13:51:19, commit-bot: I haz the power wrote:
> Patchset 6 (id:??) landed as
> https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
> Cr-Commit-Position: refs/heads/master@{#415286}

Hi Sebastien,

My change (https://codereview.chromium.org/2285523002/) was reverted for
breaking testMultilineStreetAddress*. I'm wondering if this change had something
to do with it, though. Would you mind taking a quick look?

Thanks,
Justin


* See
https://build.chromium.org/p/chromium.android/builders/KitKat%20Tablet%20Test....

[sebsg, 2016-08-31 13:57:09]

On 2016/08/30 21:59:55, Justin Donnelly wrote:
> On 2016/08/30 13:51:19, commit-bot: I haz the power wrote:
> > Patchset 6 (id:??) landed as
> > https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
> > Cr-Commit-Position: refs/heads/master@{#415286}
> 
> Hi Sebastien,
> 
> My change (https://codereview.chromium.org/2285523002/) was reverted for
> breaking testMultilineStreetAddress*. I'm wondering if this change had
something
> to do with it, though. Would you mind taking a quick look?
> 
> Thanks,
> Justin
> 
> 
> * See
>
https://build.chromium.org/p/chromium.android/builders/KitKat%20Tablet%20Test....

Interesting. I still had my patch so I ran that test alone 20 time and it didn't
fail a single time. However if I run all the tests in PersonalDataManagerTest I
can get it to fail sometimes, but sometimes it's also another test that fails.
It made me think of this CL Rouslan is currently doing to fix the flakes in this
test class (https://codereview.chromium.org/2296943002/). I think it might fix
the problem.

[chromium-reviews, 2016-08-31 14:03:28]

Interesting. I still had my patch so I ran that test alone 20 time and it
didn't fail a single time. However if I run all the tests in
PersonalDataManagerTest I can get it to fail sometimes, but sometimes it's
also another test that fails. It made me think of this CL Rouslan is
currently doing to fix the flakes in this test class (
https://codereview.chromium.org/2296943002/). I think it might fix the
problem.

On Tue, Aug 30, 2016 at 5:59 PM, <jdonnelly@chromium.org> wrote:

> On 2016/08/30 13:51:19, commit-bot: I haz the power wrote:
> > Patchset 6 (id:??) landed as
> > https://crrev.com/1ededfa5db7e0abf94b0e3d89e101b2df3767b5d
> > Cr-Commit-Position: refs/heads/master@{#415286}
>
> Hi Sebastien,
>
> My change (https://codereview.chromium.org/2285523002/) was reverted for
> breaking testMultilineStreetAddress*. I'm wondering if this change had
> something
> to do with it, though. Would you mind taking a quick look?
>
> Thanks,
> Justin
>
>
> * See
> https://build.chromium.org/p/chromium.android/builders/
> KitKat%20Tablet%20Tester/builds/5296/steps/chrome_public_test_apk.
>
> https://codereview.chromium.org/2287513002/
>



-- 

Sebastien Seguin-Gagnon |  Software Developer |  sebsg@google.com |

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Donnelly, 2016-09-02 19:16:45]

On 2016/08/31 14:03:28, chromium-reviews wrote:
> Interesting. I still had my patch so I ran that test alone 20 time and it
> didn't fail a single time. However if I run all the tests in
> PersonalDataManagerTest I can get it to fail sometimes, but sometimes it's
> also another test that fails. It made me think of this CL Rouslan is
> currently doing to fix the flakes in this test class (
> https://codereview.chromium.org/2296943002/). I think it might fix the
> problem.

Thanks. Rouslan just landed that change so I'm going to land mine again.