Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808633002:
[Payments] Move PaymentResponse logic to PaymentResponseHelper. (Closed)
|
Created:
3 years, 8 months ago by sebsg Modified:
3 years, 8 months ago Reviewers:
Mathieu CC:
chromium-reviews, jam, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, darin-cc_chromium.org, mahmadi+paymentswatch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Move PaymentResponse logic to PaymentResponseHelper.
BUG=709807
Review-Url: https://codereview.chromium.org/2808633002
Cr-Commit-Position: refs/heads/master@{#463652}
Committed: https://chromium.googlesource.com/chromium/src/+/695799a2220abd7a20ad75e4d6d5024a73c56834
Patch Set 1 #
Total comments: 17
Patch Set 2 : Addressed Mathp's comments #
Total comments: 2
Patch Set 3 : Moved tests #
Total comments: 10
Patch Set 4 : Rebase #Patch Set 5 : Addressed comments #
Total comments: 1
Patch Set 6 : Nit #
Messages
Total messages: 47 (37 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Should the tests from payment_request_state be transferred to the helper? https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... File components/payments/content/payment_request_state.cc (right): https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:72: response_helper_.reset( response_helper_ = base::MakeUnique... https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:73: new PaymentResponseHelper(app_locale_, spec_, selected_shipping_profile_, should probably pass the selected_instrument_ here as well and let the PaymentResponseHelper call InvokePaymentApp? https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... File components/payments/content/payment_response_helper.cc (right): https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.cc:110: delegate_->OnPaymentResponseReady(std::move(payment_response)); I'm curious why you are not getting an error here, because the std::move should be implied by copy ellision. http://en.cppreference.com/w/cpp/language/copy_elision https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... File components/payments/content/payment_response_helper.h (right): https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:10: #include "components/payments/core/autofill_payment_instrument.h" payment_instrument.h ? https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:33: PaymentResponseHelper(const std::string& app_locale, Mention that |delegate| can't be nullptr, but selected profiles can https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:40: static mojom::PaymentAddressPtr GetMojomPaymentAddressFromAutofillProfile( Let's add a function comment https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:51: const std::string app_locale_; nit: could keep as a const std::string& https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:53: // Not owned. Never null. All outlive this object. actually the profiles can be null?
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #2 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Another look? I created a bug to move the tests, but since this CL is required to do the normalization and the tests are still valid (still exercise all the relevant code) I'd rather do it in a separate CL. WDYT? https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... File components/payments/content/payment_request_state.cc (right): https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:72: response_helper_.reset( On 2017/04/10 00:03:22, Mathieu wrote: > response_helper_ = base::MakeUnique... Done. https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:73: new PaymentResponseHelper(app_locale_, spec_, selected_shipping_profile_, On 2017/04/10 00:03:22, Mathieu wrote: > should probably pass the selected_instrument_ here as well and let the > PaymentResponseHelper call InvokePaymentApp? Done. https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... File components/payments/content/payment_response_helper.cc (right): https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.cc:110: delegate_->OnPaymentResponseReady(std::move(payment_response)); On 2017/04/10 00:03:22, Mathieu wrote: > I'm curious why you are not getting an error here, because the std::move should > be implied by copy ellision. > http://en.cppreference.com/w/cpp/language/copy_elision I applied when I returned the PaymentAddress above, but here I pass it as parameter, so I think it does not apply? https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... File components/payments/content/payment_response_helper.h (right): https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:10: #include "components/payments/core/autofill_payment_instrument.h" On 2017/04/10 00:03:22, Mathieu wrote: > payment_instrument.h ? Done. https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:33: PaymentResponseHelper(const std::string& app_locale, On 2017/04/10 00:03:22, Mathieu wrote: > Mention that |delegate| can't be nullptr, but selected profiles can Done. https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:40: static mojom::PaymentAddressPtr GetMojomPaymentAddressFromAutofillProfile( On 2017/04/10 00:03:22, Mathieu wrote: > Let's add a function comment Done. https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:51: const std::string app_locale_; On 2017/04/10 00:03:22, Mathieu wrote: > nit: could keep as a const std::string& Done. https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.h:53: // Not owned. Never null. All outlive this object. On 2017/04/10 00:03:22, Mathieu wrote: > actually the profiles can be null? Done.
I think you should move the tests now because it may inform the design of the interface. I don't think it will be a big task, if it is, let me know and I'd be more comfortable with deferring. https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... File components/payments/content/payment_response_helper.cc (right): https://codereview.chromium.org/2808633002/diff/40001/components/payments/con... components/payments/content/payment_response_helper.cc:110: delegate_->OnPaymentResponseReady(std::move(payment_response)); On 2017/04/10 20:48:24, sebsg wrote: > On 2017/04/10 00:03:22, Mathieu wrote: > > I'm curious why you are not getting an error here, because the std::move > should > > be implied by copy ellision. > > http://en.cppreference.com/w/cpp/language/copy_elision > > I applied when I returned the PaymentAddress above, but here I pass it as > parameter, so I think it does not apply? Ah yes ok https://codereview.chromium.org/2808633002/diff/100001/components/payments/co... File components/payments/content/payment_response_helper.h (right): https://codereview.chromium.org/2808633002/diff/100001/components/payments/co... components/payments/content/payment_response_helper.h:66: Delegate* delegate_; nit: can bring this up with the other non-nulls
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #3 (id:120001) has been deleted
Moved the tests, another look? https://codereview.chromium.org/2808633002/diff/100001/components/payments/co... File components/payments/content/payment_response_helper.h (right): https://codereview.chromium.org/2808633002/diff/100001/components/payments/co... components/payments/content/payment_response_helper.h:66: Delegate* delegate_; On 2017/04/10 20:55:58, Mathieu wrote: > nit: can bring this up with the other non-nulls Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks for moving it! A few comments https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... File components/payments/content/payment_request_state_unittest.cc (right): https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_request_state_unittest.cc:28: credit_card_visa_(autofill::test::GetCreditCard()), See if we can clean up some of these members? https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_request_state_unittest.cc:38: credit_card_jcb_ = autofill::test::GetCreditCard(); I think this is never used? https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... File components/payments/content/payment_response_helper_unittest.cc (right): https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_response_helper_unittest.cc:70: "visa", credit_card_visa_, billing_addresses_, "en-US", since the instrument takes a copy, I'm not sure credit_card_visa_ needs to be a member https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_response_helper_unittest.cc:90: // Convenience method to create a PaymentRequestState with default details Spec? https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_response_helper_unittest.cc:124: std::unique_ptr<FakePaymentRequestDelegate> payment_request_delegate_; this can be a regular member
Patchset #5 (id:180001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Thanks another look? https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... File components/payments/content/payment_request_state_unittest.cc (right): https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_request_state_unittest.cc:28: credit_card_visa_(autofill::test::GetCreditCard()), On 2017/04/11 14:35:24, Mathieu wrote: > See if we can clean up some of these members? Done. https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_request_state_unittest.cc:38: credit_card_jcb_ = autofill::test::GetCreditCard(); On 2017/04/11 14:35:24, Mathieu wrote: > I think this is never used? Done. https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... File components/payments/content/payment_response_helper_unittest.cc (right): https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_response_helper_unittest.cc:70: "visa", credit_card_visa_, billing_addresses_, "en-US", On 2017/04/11 14:35:24, Mathieu wrote: > since the instrument takes a copy, I'm not sure credit_card_visa_ needs to be a > member Done. https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_response_helper_unittest.cc:90: // Convenience method to create a PaymentRequestState with default details On 2017/04/11 14:35:24, Mathieu wrote: > Spec? Done. https://codereview.chromium.org/2808633002/diff/140001/components/payments/co... components/payments/content/payment_response_helper_unittest.cc:124: std::unique_ptr<FakePaymentRequestDelegate> payment_request_delegate_; On 2017/04/11 14:35:24, Mathieu wrote: > this can be a regular member Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks! https://codereview.chromium.org/2808633002/diff/200001/components/payments/co... File components/payments/content/payment_response_helper_unittest.cc (right): https://codereview.chromium.org/2808633002/diff/200001/components/payments/co... components/payments/content/payment_response_helper_unittest.cc:80: // Convenience method to create a PaymentRequestSpec with spcified |details| *specified
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2808633002/#ps220001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491923537896600,
"parent_rev": "0ef03f39669e04007be7fbd529e47cbc053fc0c5", "commit_rev":
"695799a2220abd7a20ad75e4d6d5024a73c56834"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Move PaymentResponse logic to PaymentResponseHelper. BUG=709807 ========== to ========== [Payments] Move PaymentResponse logic to PaymentResponseHelper. BUG=709807 Review-Url: https://codereview.chromium.org/2808633002 Cr-Commit-Position: refs/heads/master@{#463652} Committed: https://chromium.googlesource.com/chromium/src/+/695799a2220abd7a20ad75e4d6d5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://chromium.googlesource.com/chromium/src/+/695799a2220abd7a20ad75e4d6d5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
