[Autofill] Consolidate two implementations of IsValidPhoneNumber |
||
Issue descriptionInvestigate if we still need two: https://cs.chromium.org/search/?q=%22bool+IsValidPhoneNumber%22+file:.cc$&sq=package:chromium&type=cs
,
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
,
Jan 3 2018
fixed?
,
Jan 3 2018
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.
,
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
,
Jan 10 2018
consolidated call into libphonenumber into one place.
,
Jan 10 2018
Thank you!! |
||
►
Sign in to add a comment |
||
Comment 1 by wuandy@chromium.org
, Dec 13 2017Status: Assigned (was: Available)