[Payments] Don't show error for intl phone from other country.

[Payments] Don't show error for intl phone from other country.

BUG=727432
Review-Url: https://codereview.chromium.org/2911973002
Cr-Commit-Position: refs/heads/master@{#476028}
Committed: https://chromium.googlesource.com/chromium/src/+/0efa58326c8a9203c0d44a5266b64742e9ad01ed

[sebsg, 2017-05-30 21:38:09]

Patchset #1 (id:1) has been deleted

[sebsg, 2017-05-30 21:38:51]

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

[sebsg, 2017-05-30 21:38:52]

Hi Math, PTAL?

[sebsg, 2017-05-30 21:39:22]

+tmartino@ in CC

[tmartino, 2017-05-30 21:52:57]

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

[tmartino, 2017-05-30 21:52:57]

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
File components/payments/core/payments_profile_comparator.cc (right):

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
components/payments/core/payments_profile_comparator.cc:211: if
(!(autofill::IsValidPhoneNumber(phone, country) ||
We should keep this consistent with what we do in the validation delegates for
Shipping + Contact editors

[sebsg, 2017-05-31 14:20:35]

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
File components/payments/core/payments_profile_comparator.cc (right):

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
components/payments/core/payments_profile_comparator.cc:211: if
(!(autofill::IsValidPhoneNumber(phone, country) ||
On 2017/05/30 21:52:57, tmartino wrote:
> We should keep this consistent with what we do in the validation delegates for
> Shipping + Contact editors

Yep, it should already be the case. I modified the
GetFormattedPhoneNumberForDisplay method to check with the added "+" as well. 

For the editors themselves, Rouslan made a Cl to format the phone numbers, so
the + is added automatically.

[Mathieu, 2017-05-31 14:29:21]

On 2017/05/31 14:20:35, sebsg wrote:
>
https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
> File components/payments/core/payments_profile_comparator.cc (right):
> 
>
https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
> components/payments/core/payments_profile_comparator.cc:211: if
> (!(autofill::IsValidPhoneNumber(phone, country) ||
> On 2017/05/30 21:52:57, tmartino wrote:
> > We should keep this consistent with what we do in the validation delegates
for
> > Shipping + Contact editors
> 
> Yep, it should already be the case. I modified the
> GetFormattedPhoneNumberForDisplay method to check with the added "+" as well. 
> 
> For the editors themselves, Rouslan made a Cl to format the phone numbers, so
> the + is added automatically.

I'll wait on Tommy's approval before looking.

[tmartino, 2017-05-31 14:48:15]

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
File components/payments/core/payments_profile_comparator.cc (right):

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
components/payments/core/payments_profile_comparator.cc:211: if
(!(autofill::IsValidPhoneNumber(phone, country) ||
On 2017/05/31 at 14:20:35, sebsg wrote:
> On 2017/05/30 21:52:57, tmartino wrote:
> > We should keep this consistent with what we do in the validation delegates
for
> > Shipping + Contact editors
> 
> Yep, it should already be the case. I modified the
GetFormattedPhoneNumberForDisplay method to check with the added "+" as well. 
> 
> For the editors themselves, Rouslan made a Cl to format the phone numbers, so
the + is added automatically.

So basically are you saying the + is always passed along to:

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/shippin...

?

[tmartino, 2017-05-31 17:44:35]

lgtm

[sebsg, 2017-05-31 17:50:26]

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
File components/payments/core/payments_profile_comparator.cc (right):

https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
components/payments/core/payments_profile_comparator.cc:211: if
(!(autofill::IsValidPhoneNumber(phone, country) ||
On 2017/05/31 14:48:14, tmartino wrote:
> On 2017/05/31 at 14:20:35, sebsg wrote:
> > On 2017/05/30 21:52:57, tmartino wrote:
> > > We should keep this consistent with what we do in the validation delegates
> for
> > > Shipping + Contact editors
> > 
> > Yep, it should already be the case. I modified the
> GetFormattedPhoneNumberForDisplay method to check with the added "+" as well. 
> > 
> > For the editors themselves, Rouslan made a Cl to format the phone numbers,
so
> the + is added automatically.
> 
> So basically are you saying the + is always passed along to:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/shippin...
> 
> ?

Not necessarily. Here is the distinction

When we get the number from disk, we want to add a + because it gets removed in
GetInfo and GetRawInfo for US and CA. That happens when we generate the labels
and when we fill editor fields with existing value.

When we validate what the user entered, we don't want to add a +. We validate
what the user entered as is.  

I know it's a bit convoluted :P But does that make sense?

[tmartino, 2017-05-31 17:52:01]

On 2017/05/31 at 17:50:26, sebsg wrote:
>
https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
> File components/payments/core/payments_profile_comparator.cc (right):
> 
>
https://codereview.chromium.org/2911973002/diff/20001/components/payments/cor...
> components/payments/core/payments_profile_comparator.cc:211: if
(!(autofill::IsValidPhoneNumber(phone, country) ||
> On 2017/05/31 14:48:14, tmartino wrote:
> > On 2017/05/31 at 14:20:35, sebsg wrote:
> > > On 2017/05/30 21:52:57, tmartino wrote:
> > > > We should keep this consistent with what we do in the validation
delegates
> > for
> > > > Shipping + Contact editors
> > > 
> > > Yep, it should already be the case. I modified the
> > GetFormattedPhoneNumberForDisplay method to check with the added "+" as
well. 
> > > 
> > > For the editors themselves, Rouslan made a Cl to format the phone numbers,
so
> > the + is added automatically.
> > 
> > So basically are you saying the + is always passed along to:
> > 
> >
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/shippin...
> > 
> > ?
> 
> Not necessarily. Here is the distinction
> 
> When we get the number from disk, we want to add a + because it gets removed
in GetInfo and GetRawInfo for US and CA. That happens when we generate the
labels and when we fill editor fields with existing value.
> 
> When we validate what the user entered, we don't want to add a +. We validate
what the user entered as is.  
> 
> I know it's a bit convoluted :P But does that make sense?

I feel comfortable based on what we talked about offline.

[Mathieu, 2017-05-31 18:30:15]

rs lgtm

[sebsg, 2017-05-31 18:31:06]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2017-05-31 18:31: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, 2017-05-31 18:54:29]

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

[commit-bot: I haz the power, 2017-05-31 18:54:30]

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

[sebsg, 2017-05-31 19:39:58]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2017-05-31 19:41:18]

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-05-31 21:25:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496259598601940,
"parent_rev": "ea3fb69a9bb9ce86d98fa6f9cabe6084a739fdf5", "commit_rev":
"0efa58326c8a9203c0d44a5266b64742e9ad01ed"}

[commit-bot: I haz the power, 2017-05-31 21:25:39]

Description was changed from

==========
[Payments] Don't show error for intl phone from other country.

BUG=727432
==========

to

==========
[Payments] Don't show error for intl phone from other country.

BUG=727432

Review-Url: https://codereview.chromium.org/2911973002
Cr-Commit-Position: refs/heads/master@{#476028}
Committed:
https://chromium.googlesource.com/chromium/src/+/0efa58326c8a9203c0d44a5266b6...
==========

[commit-bot: I haz the power, 2017-05-31 21:25:40]

Committed patchset #1 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0efa58326c8a9203c0d44a5266b6...