[Payments] Normalize contact info phone# in PaymentResponse on Android.

[Payments] Normalize contact info phone# in PaymentResponse on Android.

BUG=707960
Review-Url: https://codereview.chromium.org/2791973003
Cr-Commit-Position: refs/heads/master@{#461908}
Committed: https://chromium.googlesource.com/chromium/src/+/5eab1195d0823cd85964c585afe8767fbd6d45cf

[sebsg, 2017-04-03 22:25:36]

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

[sebsg, 2017-04-03 22:25:36]

Hi Math, PTAL?

[Mathieu, 2017-04-04 00:40:52]

This looks good but should there be tests that have actual international numbers
in them? I would expect something like +16505551212 or some such formatting?

[sebsg, 2017-04-04 17:31:03]

Patchset #2 (id:20001) has been deleted

[sebsg, 2017-04-04 17:32:15]

PTAL?

[Mathieu, 2017-04-04 17:33:37]

Thanks :)

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

https://codereview.chromium.org/2791973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:52:
public static String formatForUser(String phoneNumber) {
formatForDisplay

https://codereview.chromium.org/2791973003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:65:
public static String formatForMerchant(String phoneNumber) {
formatForResponse

[Mathieu, 2017-04-04 17:35:12]

lgtm with name change

[Mathieu, 2017-04-04 17:35:58]

Description was changed from

==========
[Payments] Normalize contact info phone# in PaymentResponse on Android.

BUG=707960
==========

to

==========
[Payments] Normalize contact info phone# in PaymentResponse on Android.

BUG=707960
==========

[sebsg, 2017-04-04 17:44:00]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-04 17:44:36]

Hi Rouslan, PTAL?

[commit-bot: I haz the power, 2017-04-04 17:44:46]

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

[please use gerrit instead, 2017-04-04 18:28:22]

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

[please use gerrit instead, 2017-04-04 18:28:23]

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java
(right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:50:
* @return formatted phone number.
Capital F.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:57:
* Formats the given phone number in E.164 format
Link to spec section that mentions E.164.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:63:
* @return formatted phone number.
Capital F.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java
(right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:73:
if (selectedShippingAddress != null) {
Shipping address also has a phone number that needs formatting. It's never null.

Also please format the phone number of the billing address in
AutofillPaymentInstrument. That one is also never null.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java
(right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:53:
"Google", "340 Main St", "CA", "Los Angeles", "", "90291", "", "US",
"5555555555",
Why change the contents of the Autofill table?

https://codereview.chromium.org/2791973003/diff/60001/chrome/browser/autofill...
File chrome/browser/autofill/android/phone_number_util_android.cc (right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:26: std::string
FormatPhoneNumber(std::string phone_number,
const-ref the input phone_number.

https://codereview.chromium.org/2791973003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:69:
PhoneNumberUtil::PhoneNumberFormat::E164));
Link to spec text that mentions E164.

[sebsg, 2017-04-04 19:52:01]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-04 19:52:31]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-04-04 19:52:56]

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

[sebsg, 2017-04-04 19:54:35]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 19:55:11]

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

[sebsg, 2017-04-04 19:55:42]

Patchset #4 (id:100001) has been deleted

[sebsg, 2017-04-04 19:55:57]

Thanks, another look?

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java
(right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:50:
* @return formatted phone number.
On 2017/04/04 18:28:22, ಠ_ಠ wrote:
> Capital F.

Done.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:57:
* Formats the given phone number in E.164 format
On 2017/04/04 18:28:22, ಠ_ಠ wrote:
> Link to spec section that mentions E.164.

Done.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:63:
* @return formatted phone number.
On 2017/04/04 18:28:22, ಠ_ಠ wrote:
> Capital F.

Done.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java
(right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:73:
if (selectedShippingAddress != null) {
On 2017/04/04 18:28:22, ಠ_ಠ wrote:
> Shipping address also has a phone number that needs formatting. It's never
null.
> 
> Also please format the phone number of the billing address in
> AutofillPaymentInstrument. That one is also never null.

Yep, But I want to do it in the normalizer so it all happens at the same place
and at the same time :)

That way there is also only one round trip needed to the native code.

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java
(right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestEmailAndPhoneTest.java:53:
"Google", "340 Main St", "CA", "Los Angeles", "", "90291", "", "US",
"5555555555",
On 2017/04/04 18:28:22, ಠ_ಠ wrote:
> Why change the contents of the Autofill table?

Done.

https://codereview.chromium.org/2791973003/diff/60001/chrome/browser/autofill...
File chrome/browser/autofill/android/phone_number_util_android.cc (right):

https://codereview.chromium.org/2791973003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:26: std::string
FormatPhoneNumber(std::string phone_number,
On 2017/04/04 18:28:22, ಠ_ಠ wrote:
> const-ref the input phone_number.

Done.

https://codereview.chromium.org/2791973003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:69:
PhoneNumberUtil::PhoneNumberFormat::E164));
On 2017/04/04 18:28:22, ಠ_ಠ wrote:
> Link to spec text that mentions E164.

Done.

[please use gerrit instead, 2017-04-04 20:07:37]

lgtm

[sebsg, 2017-04-04 22:19:23]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2017-04-04 22:19:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2791973003/#ps140001
(title: "Updated ContactDetails tests expectations to reflect the new phone
formatting")

[commit-bot: I haz the power, 2017-04-04 22:20:00]

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, 2017-04-04 22:27:54]

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

[commit-bot: I haz the power, 2017-04-04 22:27:55]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sebsg, 2017-04-04 23:26:56]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2017-04-04 23:27:50]

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, 2017-04-05 00:10:56]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491348416738940,
"parent_rev": "62ad1c5bc2e538f2a57c2e16e06327414a6fbf7a", "commit_rev":
"5eab1195d0823cd85964c585afe8767fbd6d45cf"}

[commit-bot: I haz the power, 2017-04-05 00:12:03]

Description was changed from

==========
[Payments] Normalize contact info phone# in PaymentResponse on Android.

BUG=707960
==========

to

==========
[Payments] Normalize contact info phone# in PaymentResponse on Android.

BUG=707960

Review-Url: https://codereview.chromium.org/2791973003
Cr-Commit-Position: refs/heads/master@{#461908}
Committed:
https://chromium.googlesource.com/chromium/src/+/5eab1195d0823cd85964c585afe8...
==========

[commit-bot: I haz the power, 2017-04-05 00:12:05]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/5eab1195d0823cd85964c585afe8...