Commit 6fbc587c authored by Hui(Andy) Wu's avatar Hui(Andy) Wu Committed by Commit Bot

[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/827163Reviewed-by: default avatarRoger McFarlane <rogerm@chromium.org>
Reviewed-by: default avatarSebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Hui Wu <wuandy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524712}
parent f60a94af
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "components/autofill/core/browser/autofill_country.h" #include "components/autofill/core/browser/autofill_country.h"
#include "components/autofill/core/browser/autofill_data_util.h" #include "components/autofill/core/browser/autofill_data_util.h"
#include "components/autofill/core/browser/autofill_profile.h" #include "components/autofill/core/browser/autofill_profile.h"
#include "components/autofill/core/browser/validation.h"
#include "third_party/libphonenumber/phonenumber_api.h" #include "third_party/libphonenumber/phonenumber_api.h"
namespace autofill { namespace autofill {
...@@ -47,20 +46,22 @@ std::string SanitizeRegion(const std::string& region, ...@@ -47,20 +46,22 @@ std::string SanitizeRegion(const std::string& region,
return AutofillCountry::CountryCodeForLocale(app_locale); return AutofillCountry::CountryCodeForLocale(app_locale);
} }
// Returns true if |phone_number| is valid. // Returns true if |phone_number| is a possible number.
bool IsValidPhoneNumber(const ::i18n::phonenumbers::PhoneNumber& phone_number) { bool IsPossiblePhoneNumber(
const ::i18n::phonenumbers::PhoneNumber& phone_number) {
PhoneNumberUtil* phone_util = PhoneNumberUtil::GetInstance(); PhoneNumberUtil* phone_util = PhoneNumberUtil::GetInstance();
if (!phone_util->IsPossibleNumber(phone_number)) return phone_util->IsPossibleNumber(phone_number);
return false; }
// Verify that the number has a valid area code (that in some cases could be bool IsPossiblePhoneNumber(const std::string& phone_number,
// empty) for the parsed country code. Also verify that this is a valid const std::string& country_code) {
// number (for example, in the US 1234567 is not valid, because numbers do not ::i18n::phonenumbers::PhoneNumber parsed_number;
// start with 1). PhoneNumberUtil* phone_util = PhoneNumberUtil::GetInstance();
if (!phone_util->IsValidNumber(phone_number)) auto result = phone_util->ParseAndKeepRawInput(phone_number, country_code,
return false; &parsed_number);
return true; return result == ::i18n::phonenumbers::PhoneNumberUtil::NO_PARSING_ERROR &&
phone_util->IsPossibleNumber(parsed_number);
} }
// Formats the given |number| as a human-readable string, and writes the result // Formats the given |number| as a human-readable string, and writes the result
...@@ -76,9 +77,8 @@ void FormatValidatedNumber(const ::i18n::phonenumbers::PhoneNumber& number, ...@@ -76,9 +77,8 @@ void FormatValidatedNumber(const ::i18n::phonenumbers::PhoneNumber& number,
base::string16* formatted_number, base::string16* formatted_number,
base::string16* normalized_number) { base::string16* normalized_number) {
PhoneNumberUtil::PhoneNumberFormat format = PhoneNumberUtil::PhoneNumberFormat format =
country_code.empty() ? country_code.empty() ? PhoneNumberUtil::NATIONAL
PhoneNumberUtil::NATIONAL : : PhoneNumberUtil::INTERNATIONAL;
PhoneNumberUtil::INTERNATIONAL;
PhoneNumberUtil* phone_util = PhoneNumberUtil::GetInstance(); PhoneNumberUtil* phone_util = PhoneNumberUtil::GetInstance();
std::string processed_number; std::string processed_number;
...@@ -133,13 +133,13 @@ bool ParsePhoneNumber(const base::string16& value, ...@@ -133,13 +133,13 @@ bool ParsePhoneNumber(const base::string16& value,
// The |default_region| should already be sanitized. // The |default_region| should already be sanitized.
DCHECK_EQ(2U, default_region.size()); DCHECK_EQ(2U, default_region.size());
if (phone_util->ParseAndKeepRawInput( if (phone_util->ParseAndKeepRawInput(number_text, default_region,
number_text, default_region, i18n_number) != i18n_number) !=
PhoneNumberUtil::NO_PARSING_ERROR) { PhoneNumberUtil::NO_PARSING_ERROR) {
return false; return false;
} }
if (!IsValidPhoneNumber(*i18n_number)) if (!IsPossiblePhoneNumber(*i18n_number))
return false; return false;
std::string national_significant_number; std::string national_significant_number;
...@@ -171,8 +171,8 @@ bool ParsePhoneNumber(const base::string16& value, ...@@ -171,8 +171,8 @@ bool ParsePhoneNumber(const base::string16& value,
if (i18n_number->has_country_code() && if (i18n_number->has_country_code() &&
i18n_number->country_code_source() != i18n_number->country_code_source() !=
::i18n::phonenumbers::PhoneNumber::FROM_DEFAULT_COUNTRY) { ::i18n::phonenumbers::PhoneNumber::FROM_DEFAULT_COUNTRY) {
*country_code = base::UTF8ToUTF16( *country_code =
base::IntToString(i18n_number->country_code())); base::UTF8ToUTF16(base::IntToString(i18n_number->country_code()));
} }
// The region might be different from what we started with. // The region might be different from what we started with.
...@@ -230,16 +230,14 @@ bool PhoneNumbersMatch(const base::string16& number_a, ...@@ -230,16 +230,14 @@ bool PhoneNumbersMatch(const base::string16& number_a,
// Parse phone numbers based on the region // Parse phone numbers based on the region
::i18n::phonenumbers::PhoneNumber i18n_number1; ::i18n::phonenumbers::PhoneNumber i18n_number1;
if (phone_util->Parse( if (phone_util->Parse(base::UTF16ToUTF8(number_a), region.c_str(),
base::UTF16ToUTF8(number_a), region.c_str(), &i18n_number1) != &i18n_number1) != PhoneNumberUtil::NO_PARSING_ERROR) {
PhoneNumberUtil::NO_PARSING_ERROR) {
return false; return false;
} }
::i18n::phonenumbers::PhoneNumber i18n_number2; ::i18n::phonenumbers::PhoneNumber i18n_number2;
if (phone_util->Parse( if (phone_util->Parse(base::UTF16ToUTF8(number_b), region.c_str(),
base::UTF16ToUTF8(number_b), region.c_str(), &i18n_number2) != &i18n_number2) != PhoneNumberUtil::NO_PARSING_ERROR) {
PhoneNumberUtil::NO_PARSING_ERROR) {
return false; return false;
} }
...@@ -274,8 +272,7 @@ base::string16 GetFormattedPhoneNumberForDisplay(const AutofillProfile& profile, ...@@ -274,8 +272,7 @@ base::string16 GetFormattedPhoneNumberForDisplay(const AutofillProfile& profile,
// being a valid number. // being a valid number.
const std::string country_code = const std::string country_code =
autofill::data_util::GetCountryCodeWithFallback(profile, locale); autofill::data_util::GetCountryCodeWithFallback(profile, locale);
if (IsValidPhoneNumber(base::UTF8ToUTF16(tentative_intl_phone), if (IsPossiblePhoneNumber(tentative_intl_phone, country_code)) {
country_code)) {
return base::UTF8ToUTF16( return base::UTF8ToUTF16(
FormatPhoneForDisplay(tentative_intl_phone, country_code)); FormatPhoneForDisplay(tentative_intl_phone, country_code));
} }
...@@ -318,7 +315,9 @@ PhoneObject::PhoneObject(const base::string16& number, ...@@ -318,7 +315,9 @@ PhoneObject::PhoneObject(const base::string16& number,
} }
} }
PhoneObject::PhoneObject(const PhoneObject& other) { *this = other; } PhoneObject::PhoneObject(const PhoneObject& other) {
*this = other;
}
PhoneObject::PhoneObject() {} PhoneObject::PhoneObject() {}
......
...@@ -127,15 +127,17 @@ TEST(PhoneNumberTest, SetInfo) { ...@@ -127,15 +127,17 @@ TEST(PhoneNumberTest, SetInfo) {
ASCIIToUTF16("650111111"), "US")); ASCIIToUTF16("650111111"), "US"));
EXPECT_EQ(base::string16(), phone.GetRawInfo(PHONE_HOME_WHOLE_NUMBER)); EXPECT_EQ(base::string16(), phone.GetRawInfo(PHONE_HOME_WHOLE_NUMBER));
// If the stored number is invalid, we still try to respond to queries // If the stored number is invalid due to metadata mismatch(non-existing
// for a complete number with what user has entered. // carrier code for example), but otherwise is a possible number and can be
EXPECT_FALSE(phone.SetInfo(AutofillType(PHONE_HOME_WHOLE_NUMBER), // parsed into different components, we should respond to queries with best
ASCIIToUTF16("5141231234"), "US")); // effort as if it is a valid number.
EXPECT_TRUE(phone.SetInfo(AutofillType(PHONE_HOME_WHOLE_NUMBER),
ASCIIToUTF16("5141231234"), "US"));
EXPECT_EQ(ASCIIToUTF16("5141231234"), EXPECT_EQ(ASCIIToUTF16("5141231234"),
phone.GetInfo(PHONE_HOME_CITY_AND_NUMBER, "CA")); phone.GetInfo(PHONE_HOME_CITY_AND_NUMBER, "CA"));
EXPECT_EQ(ASCIIToUTF16("5141231234"), EXPECT_EQ(ASCIIToUTF16("5141231234"),
phone.GetInfo(PHONE_HOME_WHOLE_NUMBER, "CA")); phone.GetInfo(PHONE_HOME_WHOLE_NUMBER, "CA"));
EXPECT_EQ(base::string16(), phone.GetInfo(PHONE_HOME_CITY_CODE, "CA")); EXPECT_EQ(ASCIIToUTF16("514"), phone.GetInfo(PHONE_HOME_CITY_CODE, "CA"));
} }
// Test that cached phone numbers are correctly invalidated and updated. // Test that cached phone numbers are correctly invalidated and updated.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment