[Payments] Format and validate phone number by using libphonenumber

Format and validate phone number by using libphonenumber

Refer details on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK01rA5UPBu3yqENY

BUG=689046
Review-Url: https://codereview.chromium.org/2743763003
Cr-Commit-Position: refs/heads/master@{#456897}
Committed: https://chromium.googlesource.com/chromium/src/+/7cede91629178ab5ac464e86323f17c24a522295

[gogerald1, 2017-03-10 16:51:37]

Patchset #1 (id:1) has been deleted

[gogerald1, 2017-03-10 16:52:26]

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

[commit-bot: I haz the power, 2017-03-10 16:52:55]

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

[gogerald1, 2017-03-10 16:53:34]

Description was changed from

==========
format phone number by using libphonenumber

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

to

==========
Format and validate phone number by using libphonenumber

BUG=689046
==========

[gogerald1, 2017-03-10 16:54:18]

Description was changed from

==========
Format and validate phone number by using libphonenumber

BUG=689046
==========

to

==========
Format and validate phone number by using libphonenumber

Refer discussion on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK...

BUG=689046
==========

[gogerald1, 2017-03-10 16:54:35]

Description was changed from

==========
Format and validate phone number by using libphonenumber

Refer discussion on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK...

BUG=689046
==========

to

==========
Format and validate phone number by using libphonenumber

Refer details on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK...

BUG=689046
==========

[gogerald1, 2017-03-10 16:56:43]

gogerald@chromium.org changed reviewers:
+ mathp@google.com, rouslan@chromium.org

[gogerald1, 2017-03-10 16:56:44]

Hi rouslan@ and mathp@, PTAL,

[please use gerrit instead, 2017-03-10 17:19:11]

Description was changed from

==========
Format and validate phone number by using libphonenumber

Refer details on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK...

BUG=689046
==========

to

==========
Format and validate phone number by using libphonenumber

Refer details on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK...

BUG=689046
==========

[commit-bot: I haz the power, 2017-03-10 17:53:02]

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

[commit-bot: I haz the power, 2017-03-10 17:53:06]

Dry run: 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...)

[please use gerrit instead, 2017-03-10 20:24:40]

rouslan@chromium.org changed reviewers:
+ mathp@chromium.org
- mathp@google.com

[please use gerrit instead, 2017-03-10 20:24:41]

Mathieu, please give it a pass first then put me back into reviewers. Thank you!

[please use gerrit instead, 2017-03-10 20:25:47]

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

[Mathieu, 2017-03-10 20:42:52]

high-level comment, let me know if possible

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

https://codereview.chromium.org/2743763003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:45:
mPhoneNumberUtilAndroid = nativeInit();
As mentioned, consider removing the class from the C++ side and calling Format
and Parse directly

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

https://codereview.chromium.org/2743763003/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:26:
phone_number_util_(PhoneNumberUtil::GetInstance()) {}
It looks like this is cheap? Perhaps we can make Format be a "static" method and
not wrap it in PhoneNumberUtilAndroid?

This is an example:
https://cs.chromium.org/chromium/src/components/payments/content/android/paym...

[gogerald1, 2017-03-10 23:16:47]

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

[commit-bot: I haz the power, 2017-03-10 23:17:12]

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

[gogerald1, 2017-03-10 23:17:36]

Good idea, did not pay attention to that, please take another look,

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

https://codereview.chromium.org/2743763003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:45:
mPhoneNumberUtilAndroid = nativeInit();
On 2017/03/10 20:42:51, Mathieu Perreault wrote:
> As mentioned, consider removing the class from the C++ side and calling Format
> and Parse directly 

Done.

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

https://codereview.chromium.org/2743763003/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:26:
phone_number_util_(PhoneNumberUtil::GetInstance()) {}
On 2017/03/10 20:42:51, Mathieu Perreault wrote:
> It looks like this is cheap? Perhaps we can make Format be a "static" method
and
> not wrap it in PhoneNumberUtilAndroid?
> 
> This is an example:
>
https://cs.chromium.org/chromium/src/components/payments/content/android/paym...

Done.

[commit-bot: I haz the power, 2017-03-11 01:11:22]

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

[commit-bot: I haz the power, 2017-03-11 01:11:23]

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...)

[gogerald1, 2017-03-13 15:24:02]

Fixed tests

[Mathieu, 2017-03-14 00:36:23]

Thanks sorry for the delay!

https://codereview.chromium.org/2743763003/diff/60001/chrome/DEPS
File chrome/DEPS (right):

https://codereview.chromium.org/2743763003/diff/60001/chrome/DEPS#newcode43
chrome/DEPS:43: "+third_party/libphonenumber",
couldn't this be in chrome/browser/autofill/android/DEPS instead?

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

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:30:
String formatedNumber = format(s.toString());
*formattedNumber

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:48:
public static TextWatcher getFormatTextWatcher() {
Is this function needed? Sounds like callers could do new
PhoneNumberUtil.FormatTextWatcher()? If it works, the contract is a bit clearer
for the caller than "get"

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:53:
* Formats the given phone number.
I think examples would make this clearer. "Formats any string into a phone
number in international format [link], returning the original string if no
formatting can be made. Example: ..."

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:63:
* Checks whether the given phone number is valid.
Valid according to what?

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

https://codereview.chromium.org/2743763003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:24: //
i18n::phonenumbers::PhoneNumberUtil::Format.
Mention that we are choosing INTERNATIONAL and provide a link.

https://codereview.chromium.org/2743763003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:42: std::string
formtted_number;
*formatted

[gogerald1, 2017-03-14 15:32:59]

Thanks, another look?

https://codereview.chromium.org/2743763003/diff/60001/chrome/DEPS
File chrome/DEPS (right):

https://codereview.chromium.org/2743763003/diff/60001/chrome/DEPS#newcode43
chrome/DEPS:43: "+third_party/libphonenumber",
On 2017/03/14 00:36:22, Mathieu Perreault wrote:
> couldn't this be in chrome/browser/autofill/android/DEPS instead?

Done.

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

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:30:
String formatedNumber = format(s.toString());
On 2017/03/14 00:36:23, Mathieu Perreault wrote:
> *formattedNumber

Done.

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:48:
public static TextWatcher getFormatTextWatcher() {
On 2017/03/14 00:36:23, Mathieu Perreault wrote:
> Is this function needed? Sounds like callers could do new
> PhoneNumberUtil.FormatTextWatcher()? If it works, the contract is a bit
clearer
> for the caller than "get"

Done.

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:53:
* Formats the given phone number.
On 2017/03/14 00:36:22, Mathieu Perreault wrote:
> I think examples would make this clearer. "Formats any string into a phone
> number in international format [link], returning the original string if no
> formatting can be made. Example: ..."

Done.

https://codereview.chromium.org/2743763003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:63:
* Checks whether the given phone number is valid.
On 2017/03/14 00:36:23, Mathieu Perreault wrote:
> Valid according to what?

Done.

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

https://codereview.chromium.org/2743763003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:24: //
i18n::phonenumbers::PhoneNumberUtil::Format.
On 2017/03/14 00:36:23, Mathieu Perreault wrote:
> Mention that we are choosing INTERNATIONAL and provide a link.

Done.

https://codereview.chromium.org/2743763003/diff/60001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:42: std::string
formtted_number;
On 2017/03/14 00:36:23, Mathieu Perreault wrote:
> *formatted

Done.

[Mathieu, 2017-03-14 15:36:59]

lgtm, thanks

[gogerald1, 2017-03-14 15:40:50]

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

[gogerald1, 2017-03-14 15:40:51]

Hi rouslan@, PTAL,

[gogerald1, 2017-03-14 15:40:59]

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

[commit-bot: I haz the power, 2017-03-14 15:41:47]

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, 2017-03-14 16:40:36]

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

[commit-bot: I haz the power, 2017-03-14 16:40:37]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-03-14 18:17:16]

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

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:24:
private boolean mSelfChange = false;
'= false' is redundant, because 'false' is the default value for booleans in
Java. Also, The resulting binary will be ever so slightly bigger if you specify
the redundant initialization. I don't know why. It's silly. But we have to live
with it.

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

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:154:
if (mPhoneInput != null) {
mPhoneInput is guaranteed to be null in constructor, so no need for this if
statement.

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java
(right):

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:41:
"650-253-0000", "jon.doe@gmail.com", "en-US"));
What was wrong with 310-310-6000?

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

https://codereview.chromium.org/2743763003/diff/80001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:14: using
base::android::ConvertJavaStringToUTF8;
Prepend "base" with "::" and put these "using" statements inside of anonymous
namespace, which should be inside of the "autofill" namespace. That prevents
polluting the namespaces of the header files that you have included above.

namespace autofill {
namespace {
using ::base::android::ConvertJavaStringToUTF8;
...

[gogerald1, 2017-03-14 19:32:10]

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

[gogerald1, 2017-03-14 19:32:33]

Hi rouslan@, another look?

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

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:24:
private boolean mSelfChange = false;
On 2017/03/14 18:17:15, ಠ_ಠ wrote:
> '= false' is redundant, because 'false' is the default value for booleans in
> Java. Also, The resulting binary will be ever so slightly bigger if you
specify
> the redundant initialization. I don't know why. It's silly. But we have to
live
> with it.

Done.

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

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:154:
if (mPhoneInput != null) {
On 2017/03/14 18:17:15, ಠ_ಠ wrote:
> mPhoneInput is guaranteed to be null in constructor, so no need for this if
> statement.

Done.

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java
(right):

https://codereview.chromium.org/2743763003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:41:
"650-253-0000", "jon.doe@gmail.com", "en-US"));
On 2017/03/14 18:17:15, ಠ_ಠ wrote:
> What was wrong with 310-310-6000?

Nothing wrong, just want to be consistent in this file,

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

https://codereview.chromium.org/2743763003/diff/80001/chrome/browser/autofill...
chrome/browser/autofill/android/phone_number_util_android.cc:14: using
base::android::ConvertJavaStringToUTF8;
On 2017/03/14 18:17:15, ಠ_ಠ wrote:
> Prepend "base" with "::" and put these "using" statements inside of anonymous
> namespace, which should be inside of the "autofill" namespace. That prevents
> polluting the namespaces of the header files that you have included above.
> 
> namespace autofill {
> namespace {
> using ::base::android::ConvertJavaStringToUTF8;
> ...

Done.

[commit-bot: I haz the power, 2017-03-14 19:33:48]

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-03-14 19:38:29]

lgtm % comments

https://codereview.chromium.org/2743763003/diff/100001/chrome/browser/autofil...
File chrome/browser/autofill/android/phone_number_util_android.cc (right):

https://codereview.chromium.org/2743763003/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/android/phone_number_util_android.cc:17: using
::base::android::ConvertJavaStringToUTF8;
No indent.

https://codereview.chromium.org/2743763003/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/android/phone_number_util_android.cc:23: }
}  // namespace

[gogerald1, 2017-03-14 19:52:46]

https://codereview.chromium.org/2743763003/diff/100001/chrome/browser/autofil...
File chrome/browser/autofill/android/phone_number_util_android.cc (right):

https://codereview.chromium.org/2743763003/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/android/phone_number_util_android.cc:17: using
::base::android::ConvertJavaStringToUTF8;
On 2017/03/14 19:38:29, ಠ_ಠ wrote:
> No indent.

Done.

https://codereview.chromium.org/2743763003/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/android/phone_number_util_android.cc:23: }
On 2017/03/14 19:38:28, ಠ_ಠ wrote:
> }  // namespace

Done.

[gogerald1, 2017-03-14 19:52:51]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2017-03-14 19:52:51]

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/2743763003/#ps120001
(title: "formats")

[commit-bot: I haz the power, 2017-03-14 19:53:28]

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-03-14 20:07:23]

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

[commit-bot: I haz the power, 2017-03-14 20:07:25]

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...)

[gogerald1, 2017-03-14 20:13:54]

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

[gogerald1, 2017-03-14 20:13:55]

Hi dfalcantara@, PTAL of the changes in
chrome/browser/android/chrome_jni_registrar.cc

[gogerald1, 2017-03-14 20:14:17]

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

[commit-bot: I haz the power, 2017-03-14 20:14:53]

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, 2017-03-14 21:31:22]

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

[commit-bot: I haz the power, 2017-03-14 21:31:23]

Dry run: This issue passed the CQ dry run.

[gone, 2017-03-14 23:03:29]

chrome/jni_registrar.cc lgtm

[Mathieu, 2017-03-14 23:45:20]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:46:16]

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-03-14 23:53:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489535120828180,
"parent_rev": "075901f78f9b2ca03a66dcd8620fcbfcfd7941fc", "commit_rev":
"7cede91629178ab5ac464e86323f17c24a522295"}

[commit-bot: I haz the power, 2017-03-14 23:54:29]

Description was changed from

==========
Format and validate phone number by using libphonenumber

Refer details on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK...

BUG=689046
==========

to

==========
Format and validate phone number by using libphonenumber

Refer details on bug and
https://docs.google.com/a/google.com/document/d/1H8U486cj5Y1gyWObWToyIp_v50LK...

BUG=689046

Review-Url: https://codereview.chromium.org/2743763003
Cr-Commit-Position: refs/heads/master@{#456897}
Committed:
https://chromium.googlesource.com/chromium/src/+/7cede91629178ab5ac464e86323f...
==========

[commit-bot: I haz the power, 2017-03-14 23:54:30]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/7cede91629178ab5ac464e86323f...