Change PhoneNumber::SetInfo to only apply formatting where there is none

Change PhoneNumber::SetInfo to only apply formatting where there is none

See discussion on https://codereview.chromium.org/347183005/

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282761

[Ilya Sherman, 2014-07-08 23:53:55]

https://codereview.chromium.org/374053007/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/phone_number.cc (right):

https://codereview.chromium.org/374053007/diff/1/components/autofill/core/bro...
components/autofill/core/browser/phone_number.cc:142:
base::ASCIIToUTF16("0123456789"))) {
The '+' sign should also be treated as an unformatted number, as it is actually
a required part of the number.  (The similar check in the requestAutocomplete
DataModelWrapper class should be updated as well.)

[Evan Stade, 2014-07-08 23:58:31]

https://codereview.chromium.org/374053007/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/phone_number.cc (right):

https://codereview.chromium.org/374053007/diff/1/components/autofill/core/bro...
components/autofill/core/browser/phone_number.cc:142:
base::ASCIIToUTF16("0123456789"))) {
On 2014/07/08 23:53:54, Ilya Sherman wrote:
> The '+' sign should also be treated as an unformatted number, as it is
actually
> a required part of the number.  (The similar check in the requestAutocomplete
> DataModelWrapper class should be updated as well.)

Done.

[Ilya Sherman, 2014-07-09 00:01:58]

LGTM, thanks.

[Evan Stade, 2014-07-10 23:22:48]

made one more change

https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (left):

https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:144: if
(normalized_number.length() <= national_significant_number.length() &&
I don't know what the purpose of this check was. The effect was that a German
number:

08 450 777-777

would have a country code prepended simply because the normalized number
contains the trunk code (0):

08450777777

and therefore the normalized number is longer than the
national_significant_number, which is just

8450777777

[Ilya Sherman, 2014-07-11 01:11:01]

https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (left):

https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:144: if
(normalized_number.length() <= national_significant_number.length() &&
On 2014/07/10 23:22:48, Evan Stade wrote:
> I don't know what the purpose of this check was. The effect was that a German
> number:
> 
> 08 450 777-777
> 
> would have a country code prepended simply because the normalized number
> contains the trunk code (0):
> 
> 08450777777
> 
> and therefore the normalized number is longer than the
> national_significant_number, which is just
> 
> 8450777777

I'm also not sure.  I guess if no tests fail, then it's hard to claim that this
change is not appropriate.  I wish the terminology around phone numbers were a
little more intuitively grokable... oh well.

[Evan Stade, 2014-07-11 18:08:13]

https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
File components/autofill/core/browser/phone_number_i18n.cc (left):

https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
components/autofill/core/browser/phone_number_i18n.cc:144: if
(normalized_number.length() <= national_significant_number.length() &&
On 2014/07/11 01:11:01, Ilya Sherman wrote:
> On 2014/07/10 23:22:48, Evan Stade wrote:
> > I don't know what the purpose of this check was. The effect was that a
German
> > number:
> > 
> > 08 450 777-777
> > 
> > would have a country code prepended simply because the normalized number
> > contains the trunk code (0):
> > 
> > 08450777777
> > 
> > and therefore the normalized number is longer than the
> > national_significant_number, which is just
> > 
> > 8450777777
> 
> I'm also not sure.  I guess if no tests fail, then it's hard to claim that
this
> change is not appropriate.  I wish the terminology around phone numbers were a
> little more intuitively grokable... oh well.

well, tests did fail, which caused me to look into this more and I found a
better way of checking where the country code came from.

[aruslan, 2014-07-11 19:20:24]

On 2014/07/11 18:08:13, Evan Stade wrote:
>
https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
> File components/autofill/core/browser/phone_number_i18n.cc (left):
> 
>
https://codereview.chromium.org/374053007/diff/100001/components/autofill/cor...
> components/autofill/core/browser/phone_number_i18n.cc:144: if
> (normalized_number.length() <= national_significant_number.length() &&
> On 2014/07/11 01:11:01, Ilya Sherman wrote:
> > On 2014/07/10 23:22:48, Evan Stade wrote:
> > > I don't know what the purpose of this check was. The effect was that a
> German
> > > number:
> > > 
> > > 08 450 777-777
> > > 
> > > would have a country code prepended simply because the normalized number
> > > contains the trunk code (0):
> > > 
> > > 08450777777
> > > 
> > > and therefore the normalized number is longer than the
> > > national_significant_number, which is just
> > > 
> > > 8450777777
> > 
> > I'm also not sure.  I guess if no tests fail, then it's hard to claim that
> this
> > change is not appropriate.  I wish the terminology around phone numbers were
a
> > little more intuitively grokable... oh well.
> 
> well, tests did fail, which caused me to look into this more and I found a
> better way of checking where the country code came from.

yay for good tests!

[Evan Stade, 2014-07-11 21:48:20]

The CQ bit was checked by estade@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/374053007/120001

[Ilya Sherman, 2014-07-11 23:15:58]

Nice, this does look much better.  Thanks, Evan!

[commit-bot: I haz the power, 2014-07-12 00:56:36]

Change committed as 282761