PaymentRequest: Add payer name field to payer info editor. (android)

PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

Intent to implement and ship:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

BUG=648051
Committed: https://crrev.com/04dad8e7c11b7d2e44ca5d36bf6167278ed3e2b2
Cr-Commit-Position: refs/heads/master@{#426718}

[zino, 2016-09-25 16:33:51]

Description was changed from

==========
Rename contact editor to payer info editor.

BUG=
==========

to

==========
Rename contact editor to payer info editor.

BUG=
==========

[zino, 2016-10-03 14:12:32]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 14:12:42]

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

[zino, 2016-10-03 16:58:53]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 16:59:24]

Dry run: 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-10-03 17:28:50]

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

[commit-bot: I haz the power, 2016-10-03 17:28:51]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[zino, 2016-10-03 17:49:20]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 17:49:57]

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

[zino, 2016-10-03 18:12:18]

Description was changed from

==========
Rename contact editor to payer info editor.

BUG=
==========

to

==========
PaymentRequest: Implement payer info editor for payer name.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

BUG=648051
==========

[zino, 2016-10-03 18:12:18]

jinho.bang@samsung.com changed reviewers:
+ rouslan@chromium.org

[zino, 2016-10-03 18:15:35]

Description was changed from

==========
PaymentRequest: Implement payer info editor for payer name.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

BUG=648051
==========

to

==========
PaymentRequest: Implement payer info editor for payer name.

Rename existing ContactEditor to PayerInfoEditor and then add a name field to
collect payer name from user on UI.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

BUG=648051
==========

[zino, 2016-10-03 18:21:40]

PTAL.

Most of things is just renaming. BUT
If this CL is too large to review, I can split this CL into smaller pieces and
then reupload them :)

Thank you!

[zino, 2016-10-03 18:56:54]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 18:57:26]

Dry run: 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-10-03 21:43:12]

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

[commit-bot: I haz the power, 2016-10-03 21:43:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zino, 2016-10-04 15:33:14]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 15:33:30]

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

[zino, 2016-10-04 16:25:34]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 16:25:50]

Dry run: 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-10-04 16:35:55]

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

[commit-bot: I haz the power, 2016-10-04 16:35:56]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zino, 2016-10-08 04:08:40]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-08 04:08:44]

Dry run: 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-10-08 04:11:14]

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

[commit-bot: I haz the power, 2016-10-08 04:11:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[zino, 2016-10-08 06:51:40]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-08 06:51:46]

Dry run: 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-10-08 07:02:09]

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

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

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[please use gerrit instead, 2016-10-13 00:29:39]

I think the renaming was not necessary, because "ContactInfo" can include "name"
as well. Please undo "ContactInfo"->"PayerInfo" rename. I hope this will reduce
the size of the patch slightly as well.

[zino, 2016-10-13 19:08:12]

Description was changed from

==========
PaymentRequest: Implement payer info editor for payer name.

Rename existing ContactEditor to PayerInfoEditor and then add a name field to
collect payer name from user on UI.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

BUG=648051
==========

to

==========
PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

BUG=648051
==========

[zino, 2016-10-13 19:08:41]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 19:09:00]

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

[zino, 2016-10-13 19:11:03]

jinho.bang@samsung.com changed reviewers:
+ haraken@chromium.org

[zino, 2016-10-13 19:12:33]

jinho.bang@samsung.com changed reviewers:
+ dfalcantara@chromium.org

[zino, 2016-10-13 19:14:22]

PTAL.

Payment review for rouslan@

RuntimeEnabledFeatures.in for haraken@

chrome/android/* for dfalcantara@

[commit-bot: I haz the power, 2016-10-13 19:39:22]

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

[commit-bot: I haz the power, 2016-10-13 19:39:23]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[zino, 2016-10-13 19:56:01]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 19:56:30]

Dry run: 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-10-13 20:09:08]

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

[commit-bot: I haz the power, 2016-10-13 20:09:10]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[please use gerrit instead, 2016-10-13 20:20:34]

haraken@: Can we remove "PaymentRequestPayerName" flag once the feature is
stable?

jinho.bang@: Great work!

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java
(right):

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:33:
@Nullable private EditorFieldValidator mNameValidator;
No need for a fancy format validator for names, because we only check for
non-empty names.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:66:
return (!mRequestPayerName || getNameValidator().isValid(name))
Use the built-in !TextUtils.isEmpty(payerName) instead of the custom
getNameValidator().isValid(payerName). We only check that the name is not empty.
There's no fancy format validation, like for phones and emails.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:77:
if (getNameValidator().isValid(payerName)) mPayerNames.add(payerName);
Use the built-in !TextUtils.isEmpty(payerName) instead of the custom
getNameValidator().isValid(payerName).

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:110:
mContext.getString(R.string.payments_name_invalid_validation_message),
The only validation for payer names is that it's not empty. Therefore, you can
specify "null" for the error message about invalid names. The
"payments_field_required_validation_message" will take care of showing an error
for empty field.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:187:
}
There's no need for an EditorFieldValidator for names. We don't do any type of
fancy validation for names, unlike phone numbers and emails below. The editor
will take care of checking for empty names on its own.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:281:
false /* includeName */);
s/includeName/includeNameInLabel/g

It's confusing that requestPayerName is true, but includeName is false. This is
OK semantically, however, because includeName should apply only to labels, not
to data in the profile itself. To avoid confusion for the reader, please rename
includeName parameter into includeNameInLabel. The renaming needs to happen in
PersonalDataManager.java and personal_data_manager_android.h/cc. The .h/cc file
uses underscores for parameter naming, so the same parameter is called
include_name in there. These should be renamed into include_name_in_label.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:286: <message
name="IDS_AUTOFILL_PROFILE_EDITOR_NAME" desc="Label for text input field
containing a name. [CHAR-LIMIT=32]">
s/a name/the full name of a person/

Translators need to disambiguate between different ways to translate the same
word into their language. For example, "name" has 2 different words in Russian:
one for inanimate objects and one for people. The more information you can
provide in a comment, the better.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:288: </message>
Please move this down to <!-- Payments --> section below line 2506.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2519: <message
name="IDS_PAYMENTS_ADD_CONTACT_DETAILS_LABEL" desc="The title of the dialog for
user to add new contact information, such as payer name or email address and
phone number.">
"...such as the user's full name, an email address, or a phone number."

"payer name" makes sense only to you and me, payment folks. Translators would
probably prefer to know that this name is for a person.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2522: <message
name="IDS_PAYMENTS_EDIT_CONTACT_DETAILS_LABEL" desc="The title of the dialog for
user to edit their contact information, such as payer name or email address or
phone number.">
Ditto

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2557: </message>
The only validation for names is that it's not empty. If the name is empty, then
the user sees "field required" message. So there's no need to "invalid name"
message. Please remove it.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2567: <message
name="IDS_PAYMENTS_ADD_CONTACT" desc="Text on a button that lets a user add new
contact details, like payer name or phone number or email address.">
s/payer name/user's full name/

https://codereview.chromium.org/2368073002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2368073002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:168:
PaymentRequestPayerName status=stable
haraken@: Can we remove "PaymentRequestPayerName" flag once the feature is
stable?

[haraken, 2016-10-14 00:08:43]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2016-10-14 00:08:45]

This will need an API owner review.

+tkent

https://codereview.chromium.org/2368073002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2368073002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:168:
PaymentRequestPayerName status=stable
On 2016/10/13 20:20:34, rouslan (slow 10.10-10.17) wrote:
> haraken@: Can we remove "PaymentRequestPayerName" flag once the feature is
> stable?

No, we can drop "stable" once the feature has been in the stable channel for a
long period.

[tkent, 2016-10-14 04:17:15]

Did you send intent-to-ship for this feature?

[zino, 2016-10-14 04:46:21]

On 2016/10/14 04:17:15, tkent wrote:
> Did you send intent-to-ship for this feature?

No, I didn't yet.

Before I start to work on this, I checked it from rouslan@ and foolip@.
 - https://codereview.chromium.org/2348103002/#msg16
 - https://codereview.chromium.org/2348103002/#msg20

But if we still need a intent to ship, I'll send it today.
(From my small experience, it's correct to send a intent even if the feature is
small change.)

[tkent, 2016-10-14 04:51:14]

tkent@chromium.org changed reviewers:
+ foolip@chromium.org

[tkent, 2016-10-14 04:53:03]

> > Did you send intent-to-ship for this feature?
> 
> No, I didn't yet.
> 
> Before I start to work on this, I checked it from rouslan@ and foolip@.
>  - https://codereview.chromium.org/2348103002/#msg16
>  - https://codereview.chromium.org/2348103002/#msg20
> 
> But if we still need a intent to ship, I'll send it today.
> (From my small experience, it's correct to send a intent even if the feature
is small change.)

Then, I delegate to foolip@.

[foolip, 2016-10-14 10:12:11]

On 2016/10/14 04:53:03, tkent wrote:
> > > Did you send intent-to-ship for this feature?
> > 
> > No, I didn't yet.
> > 
> > Before I start to work on this, I checked it from rouslan@ and foolip@.
> >  - https://codereview.chromium.org/2348103002/#msg16
> >  - https://codereview.chromium.org/2348103002/#msg20
> > 
> > But if we still need a intent to ship, I'll send it today.
> > (From my small experience, it's correct to send a intent even if the feature
> is small change.)
> 
> Then, I delegate to foolip@.

I had a hard time understanding the use of RuntimeEnabledFeatures in the
previous review, and now I've forgotten and will be confused again.

It's weird that this doesn't change
LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt, and
the reason is that PaymentRequest is still status=experimental while
PaymentRequestPayerName is made status=stable. That seems strange, should they
not both be flipped to stable now, while keeping them around for long enough for
other embedders to disable one or both while UI work is ongoing?

I think that effectively this is shipping a new member in PaymentOptions and
PaymentResponse. If you'll want a new chromestatus.com entry for it, then a new
Intent to Ship would be in order. Otherwise follow up on the original thread to
say that this change is now also shipping?

[foolip, 2016-10-14 10:17:19]

On 2016/10/14 10:12:11, foolip wrote:
> On 2016/10/14 04:53:03, tkent wrote:
> > > > Did you send intent-to-ship for this feature?
> > > 
> > > No, I didn't yet.
> > > 
> > > Before I start to work on this, I checked it from rouslan@ and foolip@.
> > >  - https://codereview.chromium.org/2348103002/#msg16
> > >  - https://codereview.chromium.org/2348103002/#msg20
> > > 
> > > But if we still need a intent to ship, I'll send it today.
> > > (From my small experience, it's correct to send a intent even if the
feature
> > is small change.)
> > 
> > Then, I delegate to foolip@.
> 
> I had a hard time understanding the use of RuntimeEnabledFeatures in the
> previous review, and now I've forgotten and will be confused again.
> 
> It's weird that this doesn't change
> LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt,
and
> the reason is that PaymentRequest is still status=experimental while
> PaymentRequestPayerName is made status=stable. That seems strange, should they
> not both be flipped to stable now, while keeping them around for long enough
for
> other embedders to disable one or both while UI work is ongoing?
> 
> I think that effectively this is shipping a new member in PaymentOptions and
> PaymentResponse. If you'll want a new http://chromestatus.com entry for it,
then a new
> Intent to Ship would be in order. Otherwise follow up on the original thread
to
> say that this change is now also shipping?

To make this a bit more actionable, if support for the Payment API itself and
this change isn't going to reach stable in the same milestone, which I think it
won't, then developers will need to understand that, so a chromestatus.com entry
and Intent to Ship is appropriate, though paperwork is boring.

(Before something has shipped, I think it's fine to throw in more and more
things, and say so in the eventual Intent to Ship.)

[zino, 2016-10-14 15:42:05]

On 2016/10/14 10:17:19, foolip wrote:
> On 2016/10/14 10:12:11, foolip wrote:
> > On 2016/10/14 04:53:03, tkent wrote:
> > > > > Did you send intent-to-ship for this feature?
> > > > 
> > > > No, I didn't yet.
> > > > 
> > > > Before I start to work on this, I checked it from rouslan@ and foolip@.
> > > >  - https://codereview.chromium.org/2348103002/#msg16
> > > >  - https://codereview.chromium.org/2348103002/#msg20
> > > > 
> > > > But if we still need a intent to ship, I'll send it today.
> > > > (From my small experience, it's correct to send a intent even if the
> feature
> > > is small change.)
> > > 
> > > Then, I delegate to foolip@.
> > 
> > I had a hard time understanding the use of RuntimeEnabledFeatures in the
> > previous review, and now I've forgotten and will be confused again.
> > 
> > It's weird that this doesn't change
> > LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt,
> and
> > the reason is that PaymentRequest is still status=experimental while
> > PaymentRequestPayerName is made status=stable. That seems strange, should
they
> > not both be flipped to stable now, while keeping them around for long enough
> for
> > other embedders to disable one or both while UI work is ongoing?
> > 
> > I think that effectively this is shipping a new member in PaymentOptions and
> > PaymentResponse. If you'll want a new http://chromestatus.com entry for it,
> then a new
> > Intent to Ship would be in order. Otherwise follow up on the original thread
> to
> > say that this change is now also shipping?
> 
> To make this a bit more actionable, if support for the Payment API itself and
> this change isn't going to reach stable in the same milestone, which I think
it
> won't, then developers will need to understand that, so a
http://chromestatus.com entry
> and Intent to Ship is appropriate, though paperwork is boring.
> 
> (Before something has shipped, I think it's fine to throw in more and more
> things, and say so in the eventual Intent to Ship.)

I agree with you overall. Thank you for inputs.

I addded a chromestatus entry as follows:
  - https://www.chromestatus.com/feature/5674107207155712

I think this change is small but it's useful in the some of cases.
So, I'd like to ship this feature asap. So, I'll send a intent to ship now.
and I'll explain why it is useful :)

Thanks.

[zino, 2016-10-14 17:28:08]

I sent a intent to implement and ship for this.
 - https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

rouslan@,
I sent a pull request to your repo. PTAL
 - https://github.com/rsolomakhin/rsolomakhin.github.io/pull/6

[zino, 2016-10-14 18:10:18]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 18:10:58]

Dry run: 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-10-14 18:22:34]

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

[commit-bot: I haz the power, 2016-10-14 18:22:35]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zino, 2016-10-14 19:01:56]

rouslan@

Really thank you for detailed review.
I addressed all your comments.

Could you please review again?

Thanks.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java
(right):

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:33:
@Nullable private EditorFieldValidator mNameValidator;
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> No need for a fancy format validator for names, because we only check for
> non-empty names.

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:66:
return (!mRequestPayerName || getNameValidator().isValid(name))
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> Use the built-in !TextUtils.isEmpty(payerName) instead of the custom
> getNameValidator().isValid(payerName). We only check that the name is not
empty.
> There's no fancy format validation, like for phones and emails.

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:77:
if (getNameValidator().isValid(payerName)) mPayerNames.add(payerName);
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> Use the built-in !TextUtils.isEmpty(payerName) instead of the custom
> getNameValidator().isValid(payerName).

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:110:
mContext.getString(R.string.payments_name_invalid_validation_message),
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> The only validation for payer names is that it's not empty. Therefore, you can
> specify "null" for the error message about invalid names. The
> "payments_field_required_validation_message" will take care of showing an
error
> for empty field.

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:187:
}
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> There's no need for an EditorFieldValidator for names. We don't do any type of
> fancy validation for names, unlike phone numbers and emails below. The editor
> will take care of checking for empty names on its own.

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:281:
false /* includeName */);
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> s/includeName/includeNameInLabel/g
> 
> It's confusing that requestPayerName is true, but includeName is false. This
is
> OK semantically, however, because includeName should apply only to labels, not
> to data in the profile itself. To avoid confusion for the reader, please
rename
> includeName parameter into includeNameInLabel. The renaming needs to happen in
> PersonalDataManager.java and personal_data_manager_android.h/cc. The .h/cc
file
> uses underscores for parameter naming, so the same parameter is called
> include_name in there. These should be renamed into include_name_in_label.

I agree with you. I thought it was confusing!
I uploaded a separate CL for this (because we don't want to add more reviewers.
:))
 - Rename CL : https://codereview.chromium.org/2417353002/

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:286: <message
name="IDS_AUTOFILL_PROFILE_EDITOR_NAME" desc="Label for text input field
containing a name. [CHAR-LIMIT=32]">
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> s/a name/the full name of a person/
> 
> Translators need to disambiguate between different ways to translate the same
> word into their language. For example, "name" has 2 different words in
Russian:
> one for inanimate objects and one for people. The more information you can
> provide in a comment, the better.

Oh, you're right.
Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:288: </message>
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> Please move this down to <!-- Payments --> section below line 2506.

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2519: <message
name="IDS_PAYMENTS_ADD_CONTACT_DETAILS_LABEL" desc="The title of the dialog for
user to add new contact information, such as payer name or email address and
phone number.">
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> "...such as the user's full name, an email address, or a phone number."
> 
> "payer name" makes sense only to you and me, payment folks. Translators would
> probably prefer to know that this name is for a person.

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2522: <message
name="IDS_PAYMENTS_EDIT_CONTACT_DETAILS_LABEL" desc="The title of the dialog for
user to edit their contact information, such as payer name or email address or
phone number.">
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> Ditto

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2557: </message>
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> The only validation for names is that it's not empty. If the name is empty,
then
> the user sees "field required" message. So there's no need to "invalid name"
> message. Please remove it.

Done.

https://codereview.chromium.org/2368073002/diff/240001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2567: <message
name="IDS_PAYMENTS_ADD_CONTACT" desc="Text on a button that lets a user add new
contact details, like payer name or phone number or email address.">
On 2016/10/13 20:20:33, rouslan (slow 10.10-10.17) wrote:
> s/payer name/user's full name/

Done.

[zino, 2016-10-14 19:02:48]

Description was changed from

==========
PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

BUG=648051
==========

to

==========
PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

Intent to implement and ship:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

BUG=648051
==========

[please use gerrit instead, 2016-10-17 15:21:52]

On 2016/10/14 10:12:11, foolip wrote:
> It's weird that this doesn't change
> LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt,
and
> the reason is that PaymentRequest is still status=experimental while
> PaymentRequestPayerName is made status=stable. That seems strange, should they
> not both be flipped to stable now, while keeping them around for long enough
for
> other embedders to disable one or both while UI work is ongoing?

PaymentRequest is status=experimental because only Android has implemented the
UI required for this API so far. We selectively enable it on Android:

https://cs.chromium.org/chromium/src/content/child/runtime_features.cc?rcl=0&...

> I think that effectively this is shipping a new member in PaymentOptions and
> PaymentResponse. If you'll want a new http://chromestatus.com entry for it,
then a new
> Intent to Ship would be in order. Otherwise follow up on the original thread
to
> say that this change is now also shipping?

IMHO, an additive change to the PaymentRequest API is non-breaking and does not
need a a new http://chromestatus.com entry.

[foolip, 2016-10-18 12:13:40]

On 2016/10/17 15:21:52, rouslan (slow 10.10-10.17) wrote:
> On 2016/10/14 10:12:11, foolip wrote:
> > It's weird that this doesn't change
> > LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt,
> and
> > the reason is that PaymentRequest is still status=experimental while
> > PaymentRequestPayerName is made status=stable. That seems strange, should
they
> > not both be flipped to stable now, while keeping them around for long enough
> for
> > other embedders to disable one or both while UI work is ongoing?
> 
> PaymentRequest is status=experimental because only Android has implemented the
> UI required for this API so far. We selectively enable it on Android:
> 
>
https://cs.chromium.org/chromium/src/content/child/runtime_features.cc?rcl=0&...

Right. What I found weird is that the default statuses of PaymentRequest and
PaymentRequestPayerName don't make sense together.

It doesn't really matter though. The intent thread needs one more LGTM, but rs
LGTM for this if you need it to eventually land.

[zino, 2016-10-18 16:20:09]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 16:20:29]

Dry run: 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-10-18 16:28:55]

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

[commit-bot: I haz the power, 2016-10-18 16:28:57]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zino, 2016-10-18 17:01:56]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 17:02:21]

Dry run: 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-10-18 17:11:07]

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

[commit-bot: I haz the power, 2016-10-18 17:11:08]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zino, 2016-10-19 00:32:19]

tkent@,

Could you review a intent for this CL? It still needs a one more lgtm from API
owner.
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

[zino, 2016-10-19 00:33:41]

rouslan@

Please take a look.
I addressed all your comments.

[please use gerrit instead, 2016-10-19 14:47:58]

lgtm

Great work!

[zino, 2016-10-19 17:14:14]

jinho.bang@samsung.com changed reviewers:
+ mathp@chromium.org

[zino, 2016-10-19 17:15:21]

jinho.bang@samsung.com changed reviewers:
+ tedchoc@chromium.org

[zino, 2016-10-19 17:16:39]

+ Ted C for chrome/android/*

+ Mathieu Perreault for chrome/browser/autofill/*

PTAL

[zino, 2016-10-19 17:20:30]

On 2016/10/19 17:16:39, zino wrote:
> + Ted C for chrome/android/*
> 
> + Mathieu Perreault for chrome/browser/autofill/*
> 
> PTAL

Ah, Sorry..

There is no change in chrome/browser/autofill/*.

Only needs a stamp for chrome/android/*

[zino, 2016-10-19 17:21:18]

jinho.bang@samsung.com changed reviewers:
- mathp@chromium.org

[zino, 2016-10-20 12:46:48]

Ping Ted C@

[Ted C, 2016-10-20 16:46:03]

On 2016/10/20 12:46:48, zino wrote:
> Ping Ted C@

lgtm

[zino, 2016-10-20 16:54:52]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2016-10-20 16:54:53]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2368073002/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-20 16:55:19]

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-10-20 17:31:18]

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

[commit-bot: I haz the power, 2016-10-20 17:31:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zino, 2016-10-21 01:34:25]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2016-10-21 01:34:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, tedchoc@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2368073002/#ps320001
(title: "PaymentRequest: Add payer name field to payer info editor. (android)")

[commit-bot: I haz the power, 2016-10-21 01:35:19]

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-10-21 03:47:48]

Description was changed from

==========
PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

Intent to implement and ship:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

BUG=648051
==========

to

==========
PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

Intent to implement and ship:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

BUG=648051
==========

[commit-bot: I haz the power, 2016-10-21 03:47:50]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-10-21 13:27:05]

Description was changed from

==========
PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

Intent to implement and ship:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

BUG=648051
==========

to

==========
PaymentRequest: Add payer name field to payer info editor. (android)

This CL is adding a name field to collect payer name from user on Payment UI.

Blink side change:
  https://codereview.chromium.org/2348103002/

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/270

Intent to implement and ship:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_L5nKV3SSYw

BUG=648051

Committed: https://crrev.com/04dad8e7c11b7d2e44ca5d36bf6167278ed3e2b2
Cr-Commit-Position: refs/heads/master@{#426718}
==========

[commit-bot: I haz the power, 2016-10-21 13:27:06]

Patchset 17 (id:??) landed as
https://crrev.com/04dad8e7c11b7d2e44ca5d36bf6167278ed3e2b2
Cr-Commit-Position: refs/heads/master@{#426718}