[Payments] Format shipping and billing phone number in normalizer.

[Payments] Format shipping and billing phone number in normalizer.

BUG=710545
Review-Url: https://codereview.chromium.org/2808983003
Cr-Commit-Position: refs/heads/master@{#464514}
Committed: https://chromium.googlesource.com/chromium/src/+/bcb00124e73304f8d44ec9a78ce21bea2f10a1fa

[sebsg, 2017-04-11 18:26:16]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-11 18:26:25]

Patchset #1 (id:1) has been deleted

[sebsg, 2017-04-11 18:26:53]

sebsg@chromium.org changed reviewers:
+ mathp@chromium.org

[sebsg, 2017-04-11 18:26:53]

Hi Math, PTAL?

[commit-bot: I haz the power, 2017-04-11 18:27:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-04-11 18:53:31]

just a couple of questions, looks good otherwise!

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
File components/payments/core/address_normalizer.cc (right):

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
components/payments/core/address_normalizer.cc:68: FormatPhoneNumber();
is there a possibility we early return above and never normalize the phone?

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
components/payments/core/address_normalizer.cc:98: void FormatPhoneNumber() {
Add a function comment, mentioning the format and so forth

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
components/payments/core/address_normalizer.cc:100:
profile_.GetRawInfo(autofill::PHONE_HOME_WHOLE_NUMBER));
GetRawInfo vs GetInfo? I seem to remember it matters for phone numbers

[commit-bot: I haz the power, 2017-04-11 19:01:40]

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

[commit-bot: I haz the power, 2017-04-11 19:01:41]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sebsg, 2017-04-11 21:33:59]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-11 21:34:12]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-04-11 21:34:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sebsg, 2017-04-11 21:34:54]

sebsg@chromium.org changed reviewers:
+ rouslan@chromium.org

[sebsg, 2017-04-11 21:34:55]

Thanks Math, Rouslan could you please take a look?

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
File components/payments/core/address_normalizer.cc (right):

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
components/payments/core/address_normalizer.cc:68: FormatPhoneNumber();
On 2017/04/11 18:53:31, Mathieu wrote:
> is there a possibility we early return above and never normalize the phone?

It should not, the first time this method is called the phone will be formatted.

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
components/payments/core/address_normalizer.cc:98: void FormatPhoneNumber() {
On 2017/04/11 18:53:31, Mathieu wrote:
> Add a function comment, mentioning the format and so forth

Done.

https://codereview.chromium.org/2808983003/diff/20001/components/payments/cor...
components/payments/core/address_normalizer.cc:100:
profile_.GetRawInfo(autofill::PHONE_HOME_WHOLE_NUMBER));
On 2017/04/11 18:53:31, Mathieu wrote:
> GetRawInfo vs GetInfo? I seem to remember it matters for phone numbers

I changed it to GetInfo to be safe, but it seems like you cannot set phone parts
with SetRawInfo or SetInfo, you can only Get*Raw*Info the parts. Thanks for
spotting this

[please use gerrit instead, 2017-04-11 21:41:27]

Good stuff! LGTM % comment.

https://codereview.chromium.org/2808983003/diff/60001/components/payments/cor...
File components/payments/core/address_normalizer_unittest.cc (right):

https://codereview.chromium.org/2808983003/diff/60001/components/payments/cor...
components/payments/core/address_normalizer_unittest.cc:52: AutofillProfile
profile() { return profile_; }
The return value should be const-ref to avoid copying the |profile_| object on
every profile() call. All accessor methods should be const.

[sebsg, 2017-04-11 21:46:10]

Thanks, sending to CQ.

https://codereview.chromium.org/2808983003/diff/60001/components/payments/cor...
File components/payments/core/address_normalizer_unittest.cc (right):

https://codereview.chromium.org/2808983003/diff/60001/components/payments/cor...
components/payments/core/address_normalizer_unittest.cc:52: AutofillProfile
profile() { return profile_; }
On 2017/04/11 21:41:27, ಠ_ಠ wrote:
> The return value should be const-ref to avoid copying the |profile_| object on
> every profile() call. All accessor methods should be const.

Done.

[sebsg, 2017-04-11 21:46:17]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2017-04-11 21:46:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2808983003/#ps80001
(title: "Addressed rouslan's comments")

[commit-bot: I haz the power, 2017-04-11 21:46:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 22:38:48]

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

[commit-bot: I haz the power, 2017-04-11 22:38:49]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sebsg, 2017-04-12 16:53:06]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 16:53:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 17:00:33]

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

[commit-bot: I haz the power, 2017-04-12 17:00:34]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sebsg, 2017-04-12 17:34:10]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 17:35:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 18:19:08]

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

[commit-bot: I haz the power, 2017-04-12 18:19:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sebsg, 2017-04-12 20:38:05]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 20:38:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sebsg, 2017-04-12 21:15:26]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 21:16:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 21:23:04]

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

[commit-bot: I haz the power, 2017-04-12 21:23:05]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sebsg, 2017-04-12 22:22:15]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 22:23:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 22:31:21]

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

[commit-bot: I haz the power, 2017-04-12 22:31:21]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sebsg, 2017-04-13 12:41:15]

Patchset #5 (id:120001) has been deleted

[sebsg, 2017-04-13 12:41:39]

Patchset #5 (id:140001) has been deleted

[sebsg, 2017-04-13 13:09:19]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 13:09:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 13:45:12]

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

[commit-bot: I haz the power, 2017-04-13 13:45:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sebsg, 2017-04-13 14:20:13]

Patchset #5 (id:120002) has been deleted

[sebsg, 2017-04-13 15:24:29]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 15:25:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sebsg, 2017-04-13 15:25:38]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-13 15:25:51]

Patchset #5 (id:170001) has been deleted

[sebsg, 2017-04-13 15:26:02]

Patchset #5 (id:190001) has been deleted

[commit-bot: I haz the power, 2017-04-13 15:26:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 15:35:57]

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

[commit-bot: I haz the power, 2017-04-13 15:35:59]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[sebsg, 2017-04-13 15:38:22]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 15:38:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sebsg, 2017-04-13 15:53:26]

Patchset #5 (id:210001) has been deleted

[commit-bot: I haz the power, 2017-04-13 16:44:13]

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

[commit-bot: I haz the power, 2017-04-13 16:44:14]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-04-13 17:43:15]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-13 17:43:32]

Patchset #5 (id:230001) has been deleted

[commit-bot: I haz the power, 2017-04-13 17:43:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sebsg, 2017-04-13 17:46:07]

Hi Math, I moved the libphonenumber use to a util file so it doesn't conflict
with libaddress input, it should compile on WIN now.

[Mathieu, 2017-04-13 17:59:48]

lgtm

https://codereview.chromium.org/2808983003/diff/250001/components/payments/co...
File components/payments/core/payment_request_data_util.cc (right):

https://codereview.chromium.org/2808983003/diff/250001/components/payments/co...
components/payments/core/payment_request_data_util.cc:159: // Formats the given
number |jphone_number| to
update comment

https://codereview.chromium.org/2808983003/diff/250001/components/payments/co...
components/payments/core/payment_request_data_util.cc:168: // Formats the given
number |jphone_number| to
same

[sebsg, 2017-04-13 18:02:48]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2017-04-13 18:02:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2808983003/#ps270001
(title: "Nits")

[commit-bot: I haz the power, 2017-04-13 18:03:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 19:48:36]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1492106568550000,
"parent_rev": "b62de227bd60c25f9a9b7421d628022fd2631a0c", "commit_rev":
"7a391b9ac3e2f696f0aeb943ab6f08afc31693a7"}

[commit-bot: I haz the power, 2017-04-13 19:53:47]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1492106568550000,
"parent_rev": "c472f8a3eb4e5fbc74c7007b4c4446f8cd80bcb9", "commit_rev":
"bcb00124e73304f8d44ec9a78ce21bea2f10a1fa"}

[commit-bot: I haz the power, 2017-04-13 19:54:40]

Description was changed from

==========
[Payments] Format shipping and billing phone number in normalizer.

BUG=710545
==========

to

==========
[Payments] Format shipping and billing phone number in normalizer.

BUG=710545

Review-Url: https://codereview.chromium.org/2808983003
Cr-Commit-Position: refs/heads/master@{#464514}
Committed:
https://chromium.googlesource.com/chromium/src/+/bcb00124e73304f8d44ec9a78ce2...
==========

[commit-bot: I haz the power, 2017-04-13 19:54:43]

Committed patchset #6 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/bcb00124e73304f8d44ec9a78ce2...