Adjust autofill phone number format.

Adjust autofill phone number format.
- Use '+' sign for out of country call, except US.
- Keep valid national number input as it is.

R=isherman@chromium.org
BUG=98911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284226

[ziran.sun, 2014-07-02 08:53:13]

Please review. Thanks!

[Ilya Sherman, 2014-07-02 22:23:54]

https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_browsertest.cc:632: // Test Autofill does not
appends country when the phone number contains the
nit: "appends country" -> "append a country code"

https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_browsertest.cc:653: // Test Autofill uses '+'
sign for international numbers.
nit: "Test Autofill" -> "Test that Autofill"

https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_browsertest.cc:655: //   The phone number with
a leading '+'.
nit: "The phone number with" -> "The phone number has"

https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_browsertest.cc:657: //   The phone number with
a leading IDD.
I'm not sure what "IDD" stands for.  Could you please expand out the acronym?

https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
components/autofill/core/browser/phone_number_i18n.cc:73: //   The phone number
with a leading IDD.
nit: Please spell out the acronym "IDD", and clarify what it means to have "a
leading international direct dialing", perhaps by appending "code" to the end of
the sentence.

https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
components/autofill/core/browser/phone_number_i18n.cc:82:
phone_util->FormatInOriginalFormat (number, region_code, &processed_number);
nit: Please omit the space before the paren.

https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
components/autofill/core/browser/phone_number_i18n.cc:86: *formatted_number =
base::UTF8ToUTF16(processed_number);
Where is the |formatted_number| used?  Should the '+' be dropped for US numbers
here as well?

https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
components/autofill/core/browser/phone_number_i18n.cc:90: if
(processed_number[0] == '+' && region_code.compare("US")) {
nit: Please write the second condition as |region_code != "US"|.

[ziran.sun, 2014-07-03 15:45:57]

On 2014/07/02 22:23:54, Ilya Sherman wrote:
>
https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
> File chrome/browser/autofill/autofill_browsertest.cc (right):
> 
>
https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
> chrome/browser/autofill/autofill_browsertest.cc:632: // Test Autofill does not
> appends country when the phone number contains the
> nit: "appends country" -> "append a country code"
> 
>
https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
> chrome/browser/autofill/autofill_browsertest.cc:653: // Test Autofill uses '+'
> sign for international numbers.
> nit: "Test Autofill" -> "Test that Autofill"
> 
>
https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
> chrome/browser/autofill/autofill_browsertest.cc:655: //   The phone number
with
> a leading '+'.
> nit: "The phone number with" -> "The phone number has"
> 
>
https://codereview.chromium.org/355823007/diff/40001/chrome/browser/autofill/...
> chrome/browser/autofill/autofill_browsertest.cc:657: //   The phone number
with
> a leading IDD.
> I'm not sure what "IDD" stands for.  Could you please expand out the acronym?
> 
>
https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
> File components/autofill/core/browser/phone_number_i18n.cc (right):
> 
>
https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
> components/autofill/core/browser/phone_number_i18n.cc:73: //   The phone
number
> with a leading IDD.
> nit: Please spell out the acronym "IDD", and clarify what it means to have "a
> leading international direct dialing", perhaps by appending "code" to the end
of
> the sentence.
> 
>
https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
> components/autofill/core/browser/phone_number_i18n.cc:82:
> phone_util->FormatInOriginalFormat (number, region_code, &processed_number);
> nit: Please omit the space before the paren.
> 
>
https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
> components/autofill/core/browser/phone_number_i18n.cc:86: *formatted_number =
> base::UTF8ToUTF16(processed_number);
> Where is the |formatted_number| used?  Should the '+' be dropped for US
numbers
> here as well?
> 

There are two other places that might be relevant:
- 
autofill::i18n::ConstructPhoneNumber->autofill::PhoneNumber::PhoneCombineHelper::ParseNumber
   This is called by autofill::ImportSingleProfile(), it uses constructed_number
to set raw info for the profile.
   It is also called by autofill::PersonalDataManager::ImportFormData, this is
to use constructed_number to set info for the profile
   
   I'm not sure that we need to drop '+' for US numbers at these places.

-  autofill::i18n::GetFormattedNumber
   This is called from autofill::AutofillProfileWrapper::GetInfoForDisplay and
autofill::PhoneNumber::SetInfo
   Both are related to AutofillProfie. And the same I'm not sure that for
autofill profile, we need to check '+' since we care the end result that
displays on the autofill UI. Is it right?

Do please let me have your suggestion on this.

>
https://codereview.chromium.org/355823007/diff/40001/components/autofill/core...
> components/autofill/core/browser/phone_number_i18n.cc:90: if
> (processed_number[0] == '+' && region_code.compare("US")) {
> nit: Please write the second condition as |region_code != "US"|.

[ziran.sun, 2014-07-03 15:51:33]

Updated code as per review comments. All comments have been addressed. Please
review. Thanks!

[Ilya Sherman, 2014-07-08 04:07:01]

Thanks, Ziran!

> > Where is the |formatted_number| used?  Should the '+' be dropped for US
> > numbers here as well?
> 
> There are two other places that might be relevant:
> - 
>
autofill::i18n::ConstructPhoneNumber->autofill::PhoneNumber::PhoneCombineHelper::ParseNumber
>    This is called by autofill::ImportSingleProfile(), it uses
constructed_number
> to set raw info for the profile.
>    It is also called by autofill::PersonalDataManager::ImportFormData, this is
> to use constructed_number to set info for the profile
>    
>    I'm not sure that we need to drop '+' for US numbers at these places.

IMO ImportSingleProfile() should import a pretty-formatted number.

> -  autofill::i18n::GetFormattedNumber
>    This is called from autofill::AutofillProfileWrapper::GetInfoForDisplay and
> autofill::PhoneNumber::SetInfo
>    Both are related to AutofillProfie. And the same I'm not sure that for
> autofill profile, we need to check '+' since we care the end result that
> displays on the autofill UI. Is it right?

I think the value that we set here is what is displayed if the user goes to edit
their Autofill data via chrome://settings/autofill.  I think we should set a
correctly formatted phone number for these.

While looking into the details of where these phone number routines are used, I
also noticed one more issue:  The requestAutocomplete UI currently treats a
number as unformatted if it contains only digits.  This should be updated to
allow for the '+' sign as well.  This fix, of course, doesn't need to be part of
this CL; but it's easy and closely related, so it might make sense to roll it
in.  Code link:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/355823007/diff/60001/chrome/browser/autofill/...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://codereview.chromium.org/355823007/diff/60001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_browsertest.cc:632: // Test that Autofill does
not appends a country code when the phone number
nit: "appends" -> "append"

[ziran.sun, 2014-07-10 17:23:19]

Updated code as per review comments. All comments have been addressed. Please
review. Thanks!

[Ilya Sherman, 2014-07-11 03:43:40]

Thanks, Ziran.  LGTM % the remaining nits.

https://codereview.chromium.org/355823007/diff/140001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/140001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:76: //   written in the
nation format.
nit: "nation" -> "national"

https://codereview.chromium.org/355823007/diff/140001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:89: *formatted_number =
(processed_number[0] == '+' && region_code == "US" ?
Please add comments to explain why the US is special-cased.

https://codereview.chromium.org/355823007/diff/140001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:92:
base::UTF8ToUTF16(processed_number));
Hmm, this ternary expression is a bit hard to read.  I think it would be a
little cleaner if written as an if/else.  WDYT?

[ziran.sun, 2014-07-11 09:28:55]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-07-11 09:29:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/355823007/160001

[ziran.sun, 2014-07-11 09:48:08]

Ben, is it okay to review the code, especially the *.out files? Many thanks!

[blundell, 2014-07-11 09:57:26]

On 2014/07/11 09:48:08, ziran.sun wrote:
> Ben, is it okay to review the code, especially the *.out files? Many thanks!

What are you looking for? You haven't added anyone named Ben. Also, Ilya's
review should be good enough for OWNERS.

[blundell, 2014-07-11 09:58:22]

On 2014/07/11 09:57:26, blundell wrote:
> On 2014/07/11 09:48:08, ziran.sun wrote:
> > Ben, is it okay to review the code, especially the *.out files? Many thanks!
> 
> What are you looking for? You haven't added anyone named Ben. Also, Ilya's
> review should be good enough for OWNERS.

oh I see...you need //components/test/data/autofill to have an OWNERS review?
How about instead you add an OWNERS file there that has the //component/autofill
OWNERS, and then I'll lgtm that addition? Thanks!

[ziran.sun, 2014-07-11 10:09:52]

On 2014/07/11 09:58:22, blundell wrote:
> On 2014/07/11 09:57:26, blundell wrote:
> > On 2014/07/11 09:48:08, ziran.sun wrote:
> > > Ben, is it okay to review the code, especially the *.out files? Many
thanks!
> > 
> > What are you looking for? You haven't added anyone named Ben. Also, Ilya's
> > review should be good enough for OWNERS.
> 
> oh I see...you need //components/test/data/autofill to have an OWNERS review?
> How about instead you add an OWNERS file there that has the
//component/autofill
> OWNERS, and then I'll lgtm that addition? Thanks!

Just added OWNERS file. Thanks.

I thought Ben is you as it's popped up when I run "git-cl owners" command. Sorry
about it :).

[ziran.sun, 2014-07-11 10:12:34]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-07-11 10:12:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/355823007/200001

[blundell, 2014-07-11 10:13:21]

lgtm

[commit-bot: I haz the power, 2014-07-11 13:23:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 14:49:06]

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

[commit-bot: I haz the power, 2014-07-11 14:49:07]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Ilya Sherman, 2014-07-11 19:15:47]

(Still LGTM)

https://codereview.chromium.org/355823007/diff/200001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/200001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:93:
processed_number.length()));
nit: Please run git cl format to fix up the formatting of these two lines.

[ziran.sun, 2014-07-11 20:58:44]

The CQ bit was checked by ziran.sun@samsung.com

[ziran.sun, 2014-07-11 21:00:35]

The CQ bit was unchecked by ziran.sun@samsung.com

[Ilya Sherman, 2014-07-11 23:17:06]

Hi Ziran, please note that Evan is landing some related changes in [
https://codereview.chromium.org/374053007/ ].  You might want to check whether
there is any conflict between the two CLs.

[ziran.sun, 2014-07-16 16:00:44]

Rebase code and solve conflict with Evan's patch. Please review. Thanks!

[Ilya Sherman, 2014-07-16 17:50:18]

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:76:
processed_number.substr(1, processed_number.length()));
nit: foo.substr(1, foo.length()) is equivalent to just foo.substr(1)

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:83: // Keep the leading
'+' while normalising numbers, excluding US numbers.
nit: "normalising" -> "normalizing"

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:85: processed_number =
processed_number.substr(1, processed_number.length());
(here too)

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:92: }
It might be cleaner to factor out the code that splits off the leading plus
sign, like below.  WDYT?

// Drop the leading '+' for US numbers as some US sites can't handle the "+",
// and in the US dialing "+1..." is the same as dialing "1...".
std::string prefix;
if (processed_number[0] == '+') {
  process_number = process_number.substr(1);
  if (region_code == "US")
    prefix = "+";
}

if (formatted_number)
  *formatted_number = base::UTF8ToUTF16(prefix + processed_number);

if (normalized_number) {
  phone_util->NormalizeDigitsOnly(&processed_number);
  *normalized_number = base::UTF8ToUTF16(prefix + processed_number);
}

[Evan Stade, 2014-07-17 00:54:49]

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:85: processed_number =
processed_number.substr(1, processed_number.length());
why do you have to remove the '+' then re-add it? What happens if you don't
remove it?

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n_unittest.cc (right):

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n_unittest.cc:152:
EXPECT_EQ(number, ASCIIToUTF16("1 650-234-5678"));
these expectations are backwards (expected value goes in front)

[ziran.sun, 2014-07-17 12:35:36]

Updated code as per review comments from Ilya and Evan. All review comments have
been addressed. Please review. Thanks!

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:85: processed_number =
processed_number.substr(1, processed_number.length());
On 2014/07/17 00:54:48, Evan Stade wrote:
> why do you have to remove the '+' then re-add it? What happens if you don't
> remove it?
NormalizeDigitsOnly () will remove the '+' sign. For non-US international
numbers, we'd like to keep '+' on.

[Evan Stade, 2014-07-17 17:28:51]

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:85: processed_number =
processed_number.substr(1, processed_number.length());
On 2014/07/17 12:35:36, ziran.sun wrote:
> On 2014/07/17 00:54:48, Evan Stade wrote:
> > why do you have to remove the '+' then re-add it? What happens if you don't
> > remove it?
> NormalizeDigitsOnly () will remove the '+' sign. For non-US international
> numbers, we'd like to keep '+' on.

If NormalizeDigitsOnly () will remove the '+' sign, why do you have to remove it
on L85?

[Ilya Sherman, 2014-07-17 18:44:33]

LGTM, thanks.

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:85: processed_number =
processed_number.substr(1, processed_number.length());
On 2014/07/17 17:28:50, Evan Stade wrote:
> On 2014/07/17 12:35:36, ziran.sun wrote:
> > On 2014/07/17 00:54:48, Evan Stade wrote:
> > > why do you have to remove the '+' then re-add it? What happens if you
don't
> > > remove it?
> > NormalizeDigitsOnly () will remove the '+' sign. For non-US international
> > numbers, we'd like to keep '+' on.
> 
> If NormalizeDigitsOnly () will remove the '+' sign, why do you have to remove
it
> on L85?

Evan, the code is structured differently now.  Do you still have this concern?

[Evan Stade, 2014-07-17 18:53:50]

lgtm

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (right):

https://codereview.chromium.org/355823007/diff/280001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:85: processed_number =
processed_number.substr(1, processed_number.length());
On 2014/07/17 18:44:33, Ilya Sherman wrote:
> On 2014/07/17 17:28:50, Evan Stade wrote:
> > On 2014/07/17 12:35:36, ziran.sun wrote:
> > > On 2014/07/17 00:54:48, Evan Stade wrote:
> > > > why do you have to remove the '+' then re-add it? What happens if you
> don't
> > > > remove it?
> > > NormalizeDigitsOnly () will remove the '+' sign. For non-US international
> > > numbers, we'd like to keep '+' on.
> > 
> > If NormalizeDigitsOnly () will remove the '+' sign, why do you have to
remove
> it
> > on L85?
> 
> Evan, the code is structured differently now.  Do you still have this concern?

nope

[ziran.sun, 2014-07-17 20:08:08]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-07-17 20:12:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/355823007/300001

[commit-bot: I haz the power, 2014-07-17 23:44:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-18 00:02:57]

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

[commit-bot: I haz the power, 2014-07-18 00:02:58]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[ziran.sun, 2014-07-18 09:36:10]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-07-18 09:37:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/355823007/320001

[ziran.sun, 2014-07-18 09:37:15]

The CQ bit was unchecked by ziran.sun@samsung.com

[ziran.sun, 2014-07-18 09:43:43]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-07-18 09:44:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/355823007/320001

[ziran.sun, 2014-07-18 09:44:50]

The CQ bit was unchecked by ziran.sun@samsung.com

[ziran.sun, 2014-07-18 09:53:32]

Update reviewer list. Lei Zhang/Jam, is it okay to review the code, especially
changes on autofill/AutofillDialogControllerTest.java please? Thanks!

[Ilya Sherman, 2014-07-18 16:23:11]

-thestig, jam
+aruslan

[aruslan, 2014-07-18 19:27:04]

android test lgtm
I hope you guys know what you are doing: this affects not only heuristic-based
autofill, but the precise information returned from Wallet and
requestAutocomplete Autofill as well.

[ziran.sun, 2014-07-18 22:07:28]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-07-18 22:08:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/355823007/320001

[commit-bot: I haz the power, 2014-07-18 22:35:29]

Change committed as 284226