[Payments] Normalize Shipping Address sent to merchant on Desktop.

[Payments] Normalize Shipping Address sent to merchant on Desktop.

BUG=707765
Review-Url: https://codereview.chromium.org/2829503002
Cr-Commit-Position: refs/heads/master@{#466226}
Committed: https://chromium.googlesource.com/chromium/src/+/64bdeb2244044a245c98c092075f750345acd9a0

[sebsg, 2017-04-18 22:28:04]

Description was changed from

==========
WIP Normalize Shipping Address on Desktop.

BUG=
==========

to

==========
WIP Normalize Shipping Address on Desktop.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

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

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

[commit-bot: I haz the power, 2017-04-19 14:19:46]

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-19 14:38:11]

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

[commit-bot: I haz the power, 2017-04-19 14:38:12]

Dry run: Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[sebsg, 2017-04-19 15:21:05]

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

[commit-bot: I haz the power, 2017-04-19 15:21:39]

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-19 15:29:15]

Patchset #1 (id:1) has been deleted

[sebsg, 2017-04-19 16:01:35]

Patchset #1 (id:20001) has been deleted

[sebsg, 2017-04-19 16:01:45]

Patchset #1 (id:40001) has been deleted

[sebsg, 2017-04-19 16:26:55]

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

[commit-bot: I haz the power, 2017-04-19 16:27:36]

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-19 17:05:36]

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

[commit-bot: I haz the power, 2017-04-19 17:05:37]

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...)

[sebsg, 2017-04-19 17:15:25]

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

[commit-bot: I haz the power, 2017-04-19 17:15:48]

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-19 17:24:51]

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

[commit-bot: I haz the power, 2017-04-19 17:24:52]

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

[sebsg, 2017-04-20 13:05:15]

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

[commit-bot: I haz the power, 2017-04-20 13:05:23]

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-20 13:57:51]

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

[commit-bot: I haz the power, 2017-04-20 13:57:51]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-04-20 14:41:56]

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

[commit-bot: I haz the power, 2017-04-20 14:42:14]

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-20 15:27:41]

Patchset #2 (id:80001) has been deleted

[sebsg, 2017-04-20 15:27:56]

Patchset #2 (id:100001) has been deleted

[sebsg, 2017-04-20 15:28:20]

Patchset #2 (id:120001) has been deleted

[sebsg, 2017-04-20 15:28:31]

Patchset #1 (id:60001) has been deleted

[sebsg, 2017-04-20 15:30:03]

Description was changed from

==========
WIP Normalize Shipping Address on Desktop.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[Payments] Normalize Shipping Address sent to merchant on Desktop.

BUG=707765
==========

[sebsg, 2017-04-20 15:34:54]

Patchset #1 (id:140001) has been deleted

[sebsg, 2017-04-20 15:35:07]

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

[commit-bot: I haz the power, 2017-04-20 15:35:32]

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-20 15:36:38]

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

[sebsg, 2017-04-20 15:36:39]

Hi Anthony, could you please take a look? Thanks!

[anthonyvd, 2017-04-20 16:59:22]

This generally looks good to me but I don't own c/b/payments. Minor comment
below.

https://codereview.chromium.org/2829503002/diff/160001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2829503002/diff/160001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:25:
address_normalizer_ = base::MakeUnique<AddressNormalizer>(
nit: initialize this in the initialization list above:
address_normalizer_(base::MakeUnique<AddressNormalizer>...)

[commit-bot: I haz the power, 2017-04-20 17:11:07]

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

[commit-bot: I haz the power, 2017-04-20 17:11:08]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-04-20 19:18:38]

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

[sebsg, 2017-04-20 19:18:39]

Thanks Anthony!

Rouslan, could you please take a look?

https://codereview.chromium.org/2829503002/diff/160001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2829503002/diff/160001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:25:
address_normalizer_ = base::MakeUnique<AddressNormalizer>(
On 2017/04/20 16:59:22, anthonyvd wrote:
> nit: initialize this in the initialization list above:
> address_normalizer_(base::MakeUnique<AddressNormalizer>...)

Done.

[please use gerrit instead, 2017-04-20 20:15:20]

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:26:
address_normalizer_(
address_normalizer_(GetAddressInputSource(), GetAddressInputStorage()) {}

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:93: return
address_normalizer_.get();
return &address_normaliser_;

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.h:57:
std::unique_ptr<AddressNormalizer> address_normalizer_;
No point of using std::unique_ptr if the variable is initialized in the
constructor and never reset. This works just well:

AddressNormalizer address_normalizer_;

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.cc:47:
autofill::AutofillCountry::CountryCodeForLocale(app_locale_);
Why not use the country code from the profile and fall back to app_locale_?

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.cc:50: default_region_code,
5, this);
Add a comment about the meaning of "5".

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.cc:104: if
(!is_waiting_for_shipping_address_normalization_) {
no {}

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
File components/payments/content/payment_response_helper.h (right):

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.h:15: class AutofillProfile;
Remove the forward decl.

[sebsg, 2017-04-20 21:05:37]

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

[sebsg, 2017-04-20 21:05:45]

Patchset #3 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-04-20 21:06:19]

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-20 21:06:19]

Thanks! Another look?

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:26:
address_normalizer_(
On 2017/04/20 20:15:20, ಠ_ಠ wrote:
> address_normalizer_(GetAddressInputSource(), GetAddressInputStorage()) {}

Done.

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:93: return
address_normalizer_.get();
On 2017/04/20 20:15:20, ಠ_ಠ wrote:
> return &address_normaliser_;

Done.

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.h:57:
std::unique_ptr<AddressNormalizer> address_normalizer_;
On 2017/04/20 20:15:20, ಠ_ಠ wrote:
> No point of using std::unique_ptr if the variable is initialized in the
> constructor and never reset. This works just well:
> 
> AddressNormalizer address_normalizer_;

Done.

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.cc:47:
autofill::AutofillCountry::CountryCodeForLocale(app_locale_);
On 2017/04/20 20:15:20, ಠ_ಠ wrote:
> Why not use the country code from the profile and fall back to app_locale_?
> 
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

That was my plan but I thought it would be more complex, turns out if the value
is set using SetInfo, it will save the countrycode. Thanks for the heads up

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.cc:50: default_region_code,
5, this);
On 2017/04/20 20:15:20, ಠ_ಠ wrote:
> Add a comment about the meaning of "5".

Done.

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.cc:104: if
(!is_waiting_for_shipping_address_normalization_) {
On 2017/04/20 20:15:20, ಠ_ಠ wrote:
> no {}

Done.

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
File components/payments/content/payment_response_helper.h (right):

https://codereview.chromium.org/2829503002/diff/180001/components/payments/co...
components/payments/content/payment_response_helper.h:15: class AutofillProfile;
On 2017/04/20 20:15:20, ಠ_ಠ wrote:
> Remove the forward decl.

Done.

[please use gerrit instead, 2017-04-20 21:13:06]

LGTM % comment

https://codereview.chromium.org/2829503002/diff/220001/components/payments/co...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2829503002/diff/220001/components/payments/co...
components/payments/content/payment_response_helper.cc:50: if
(region_code.empty())
Can you check that it matches ^[A-Z]{2}$ regex instead? Either use
third_party/re2/src/re2/re2.h or write your own simple function.

[commit-bot: I haz the power, 2017-04-20 22:17:02]

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

[commit-bot: I haz the power, 2017-04-20 22:17:03]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-04-20 22:58:27]

Thanks, could you take one last look? Thanks!

https://codereview.chromium.org/2829503002/diff/220001/components/payments/co...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2829503002/diff/220001/components/payments/co...
components/payments/content/payment_response_helper.cc:50: if
(region_code.empty())
On 2017/04/20 21:13:06, ಠ_ಠ wrote:
> Can you check that it matches ^[A-Z]{2}$ regex instead? Either use
> third_party/re2/src/re2/re2.h or write your own simple function.

Done.

[please use gerrit instead, 2017-04-21 00:46:16]

Still LGTM

[sebsg, 2017-04-21 00:48:55]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2017-04-21 00:49:17]

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-21 02:04:48]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492735735742930,
"parent_rev": "126cb0da53b0f4b172f4acf9c30bf7b540545478", "commit_rev":
"64bdeb2244044a245c98c092075f750345acd9a0"}

[commit-bot: I haz the power, 2017-04-21 02:05:36]

Description was changed from

==========
[Payments] Normalize Shipping Address sent to merchant on Desktop.

BUG=707765
==========

to

==========
[Payments] Normalize Shipping Address sent to merchant on Desktop.

BUG=707765

Review-Url: https://codereview.chromium.org/2829503002
Cr-Commit-Position: refs/heads/master@{#466226}
Committed:
https://chromium.googlesource.com/chromium/src/+/64bdeb2244044a245c98c092075f...
==========

[commit-bot: I haz the power, 2017-04-21 02:05:38]

Committed patchset #4 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/64bdeb2244044a245c98c092075f...

[keishi, 2017-04-21 03:41:47]

A revert of this CL (patchset #4 id:240001) has been created in
https://codereview.chromium.org/2832003003/ by keishi@chromium.org.

The reason for reverting is: Might have caused
PaymentResponseHelperTest.GeneratePaymentResponse_* tests to fail on Mac10.9
Tests (dbg)
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%....

[findit-for-me, 2017-04-21 05:25:45]

Findit(https://goo.gl/kROfz5) identified this CL at revision 466226 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...