New issue
Advanced search Search tips

Issue 792471 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 3
Type: Bug


Show other hotlists

Hotlists containing this issue:
Autofill-Fixit


Sign in to add a comment

[Autofill] Consolidate two implementations of IsValidPhoneNumber

Project Member Reported by ma...@chromium.org, Dec 6 2017

Issue description

Comment 1 by wuandy@chromium.org, Dec 13 2017

Owner: wuandy@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6fbc587c75d06db1aa803b9a036a265a1b383b23

commit 6fbc587c75d06db1aa803b9a036a265a1b383b23
Author: Hui(Andy) Wu <wuandy@chromium.org>
Date: Mon Dec 18 15:52:24 2017

[Autofill] Replace usage of libphonenumber.IsValidNumber with IsPossibleNumber.

Autofill calls libphonenumber.IsValidNumber for number validation. This method
validates carrier code(among other things) and it's a bit too strict for autofill's
usage.

For example, it would reject 6501231234 as a valida US number because the first
number after area code cannot be 1.

This CL switches to use IsPossibleNumber instead, so autofill will take above number
and parse it as if it is a valid number.

Benefits:
1. metadata of libphonenumber might not be the latest due to chrome's
release cycle, therefore IsValidaNumber might reject valid numbers, while
IsPossibleNumber will not.

2. Might allow libphonenumber to use a lite metadata to save a little of binary
size.

Bug:  792471 
Change-Id: I25d3b21b81010b92a69b16f91f7d072e86ac7839
Reviewed-on: https://chromium-review.googlesource.com/827163
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Hui Wu <wuandy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524712}
[modify] https://crrev.com/6fbc587c75d06db1aa803b9a036a265a1b383b23/components/autofill/core/browser/phone_number_i18n.cc
[modify] https://crrev.com/6fbc587c75d06db1aa803b9a036a265a1b383b23/components/autofill/core/browser/phone_number_i18n_unittest.cc
[modify] https://crrev.com/6fbc587c75d06db1aa803b9a036a265a1b383b23/components/autofill/core/browser/phone_number_unittest.cc

Comment 3 by ma...@chromium.org, Jan 3 2018

fixed?
not yet. autofill part of change finished, still working on PR.

Moe and Anthony pointed out it might be useful to use IsValidPhoneNumber from PR, before formatting a number, so I am making the change for that.
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1f97b49eabdff3d27109ea7f5bf319dbecdd960

commit a1f97b49eabdff3d27109ea7f5bf319dbecdd960
Author: Hui(Andy) Wu <wuandy@chromium.org>
Date: Wed Jan 10 14:20:53 2018

[PaymentRequest] Change phone number validation to use IsPossibleNumber from libphonenumber.

Right now payment request calls to libphonenumber's IsValidPhoneNumber for number validation,
this might be too strict for our use case. IsValidPhoneNumber requires that
area code/carrier code must exists in the real world for it to consider as a valid number. Due
to chrome's release cycle, it might not have the latest metadata to make a correct call.

More importantly, PaymentRequest probably should not care if a number is valid or not, because
user can always input a fake number if they really want to. We should take any number that our
code can work with(parsing into different components for example).

Autofill has switched to use IsPossibleNumber in cl 827163.

Bug:  792471 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ie2d52c8c6fc904206a275ffa9e13e2cf507ec31c
Reviewed-on: https://chromium-review.googlesource.com/836813
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Commit-Queue: Hui Wu <wuandy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528303}
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestRemoveBillingAddressTest.java
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/chrome/browser/ui/views/payments/contact_info_editor_view_controller.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/autofill/core/browser/address_normalizer_impl_unittest.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/autofill/core/browser/phone_number_i18n.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/autofill/core/browser/phone_number_i18n.h
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/autofill/core/browser/phone_number_i18n_unittest.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/autofill/core/browser/validation.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/autofill/core/browser/validation.h
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/payments/content/payment_response_helper_unittest.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/components/payments/core/payments_profile_comparator.cc
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/ios/chrome/browser/payments/payment_response_helper_unittest.mm
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/ios/chrome/browser/ui/payments/address_edit_mediator.mm
[modify] https://crrev.com/a1f97b49eabdff3d27109ea7f5bf319dbecdd960/ios/chrome/browser/ui/payments/contact_info_edit_mediator.mm

Comment 6 by wuandy@chromium.org, Jan 10 2018

Status: Fixed (was: Assigned)
consolidated call into libphonenumber into one place.

Comment 7 by ma...@chromium.org, Jan 10 2018

Thank you!!

Sign in to add a comment