Chromium Code Reviews
Help | Chromium Project | Sign in
(18)

Issue 2829503002: [Payments] Normalize Shipping Address sent to merchant on Desktop. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by sebsg
Modified:
1 week, 1 day ago
Reviewers:
ಠ_ಠ, anthonyvd
CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, gogerald+paymentswatch_chromium.org, jam, mahmadi+paymentswatch_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, xjz+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Addressed Anthony's comment #

Total comments: 14

Patch Set 3 : Addressed Rouslan's comments #

Total comments: 2

Patch Set 4 : Be more strict on country code #

Unified diffs Side-by-side diffs Delta from patch set Stats (+231 lines, -62 lines) Patch
M chrome/browser/payments/chrome_payment_request_delegate.h View 1 2 4 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/payments/chrome_payment_request_delegate.cc View 1 2 3 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/payments/payment_request_payment_response_browsertest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/payments/test_chrome_payment_request_delegate.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/payments/test_chrome_payment_request_delegate.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/autofill/core/browser/address.cc View 1 2 3 3 chunks +5 lines, -3 lines 0 comments Download
M components/autofill/core/browser/autofill_data_util.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M components/autofill/core/browser/autofill_data_util.cc View 1 2 3 2 chunks +12 lines, -0 lines 0 comments Download
M components/autofill/core/browser/autofill_data_util_unittest.cc View 1 2 3 1 chunk +41 lines, -0 lines 0 comments Download
M components/payments/content/payment_request_state.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M components/payments/content/payment_response_helper.h View 1 2 3 chunks +23 lines, -6 lines 0 comments Download
M components/payments/content/payment_response_helper.cc View 1 2 3 3 chunks +70 lines, -29 lines 0 comments Download
M components/payments/content/payment_response_helper_unittest.cc View 10 chunks +40 lines, -7 lines 0 comments Download
M components/payments/core/address_normalizer.h View 2 chunks +6 lines, -5 lines 0 comments Download
M components/payments/core/payment_request_delegate.h View 2 chunks +6 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 56 (44 generated)
sebsg
Hi Anthony, could you please take a look? Thanks!
1 week, 1 day ago (2017-04-20 15:36:39 UTC) #34
anthonyvd
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/payments/chrome_payment_request_delegate.cc ...
1 week, 1 day ago (2017-04-20 16:59:22 UTC) #35
sebsg
Thanks Anthony! Rouslan, could you please take a look? https://codereview.chromium.org/2829503002/diff/160001/chrome/browser/payments/chrome_payment_request_delegate.cc File chrome/browser/payments/chrome_payment_request_delegate.cc (right): https://codereview.chromium.org/2829503002/diff/160001/chrome/browser/payments/chrome_payment_request_delegate.cc#newcode25 chrome/browser/payments/chrome_payment_request_delegate.cc:25: ...
1 week, 1 day ago (2017-04-20 19:18:39 UTC) #39
ಠ_ಠ
https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payments/chrome_payment_request_delegate.cc File chrome/browser/payments/chrome_payment_request_delegate.cc (right): https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payments/chrome_payment_request_delegate.cc#newcode26 chrome/browser/payments/chrome_payment_request_delegate.cc:26: address_normalizer_( address_normalizer_(GetAddressInputSource(), GetAddressInputStorage()) {} https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payments/chrome_payment_request_delegate.cc#newcode93 chrome/browser/payments/chrome_payment_request_delegate.cc:93: return address_normalizer_.get(); return ...
1 week, 1 day ago (2017-04-20 20:15:20 UTC) #40
sebsg
Thanks! Another look? https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payments/chrome_payment_request_delegate.cc File chrome/browser/payments/chrome_payment_request_delegate.cc (right): https://codereview.chromium.org/2829503002/diff/180001/chrome/browser/payments/chrome_payment_request_delegate.cc#newcode26 chrome/browser/payments/chrome_payment_request_delegate.cc:26: address_normalizer_( On 2017/04/20 20:15:20, ಠ_ಠ wrote: ...
1 week, 1 day ago (2017-04-20 21:06:19 UTC) #44
ಠ_ಠ
LGTM % comment https://codereview.chromium.org/2829503002/diff/220001/components/payments/content/payment_response_helper.cc File components/payments/content/payment_response_helper.cc (right): https://codereview.chromium.org/2829503002/diff/220001/components/payments/content/payment_response_helper.cc#newcode50 components/payments/content/payment_response_helper.cc:50: if (region_code.empty()) Can you check that ...
1 week, 1 day ago (2017-04-20 21:13:06 UTC) #45
sebsg
Thanks, could you take one last look? Thanks! https://codereview.chromium.org/2829503002/diff/220001/components/payments/content/payment_response_helper.cc File components/payments/content/payment_response_helper.cc (right): https://codereview.chromium.org/2829503002/diff/220001/components/payments/content/payment_response_helper.cc#newcode50 components/payments/content/payment_response_helper.cc:50: if ...
1 week, 1 day ago (2017-04-20 22:58:27 UTC) #48
ಠ_ಠ
Still LGTM
1 week, 1 day ago (2017-04-21 00:46:16 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2829503002/240001
1 week, 1 day ago (2017-04-21 00:49:17 UTC) #51
commit-bot: I haz the power
Committed patchset #4 (id:240001) as https://chromium.googlesource.com/chromium/src/+/64bdeb2244044a245c98c092075f750345acd9a0
1 week, 1 day ago (2017-04-21 02:05:38 UTC) #54
keishi
A revert of this CL (patchset #4 id:240001) has been created in https://codereview.chromium.org/2832003003/ by keishi@chromium.org. ...
1 week, 1 day ago (2017-04-21 03:41:47 UTC) #55
findit-for-me
1 week, 1 day ago (2017-04-21 05:25:45 UTC) #56
Message was sent while issue was closed.
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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46