Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844783002:
[Payment Request] Adds address normalization on iOS. (Closed)
|
Created:
3 years, 8 months ago by macourteau Modified:
3 years, 7 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, pkl (ping after 24h if needed), browser-components-watch_chromium.org, vabr+watchlistautofill_chromium.org, noyau+watch_chromium.org, mathp+autofillwatch_chromium.org, marq+watch_chromium.org, estade+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payment Request] Adds address normalization on iOS.
BUG=602666
Patch Set 1 #Patch Set 2 : Adds missing new files. #Patch Set 3 : Cleanup. #
Total comments: 27
Patch Set 4 : Rebase. #Patch Set 5 : Addresses code review comments. #Patch Set 6 : Adds missing dependency. #
Total comments: 12
Patch Set 7 : Addresses comments from mahmadi@. #
Total comments: 4
Patch Set 8 : Rebase. #
Messages
Total messages: 25 (15 generated)
macourteau@chromium.org changed reviewers: + mahmadi@chromium.org, sebsg@chromium.org
Please take a first look through this before I add other owners. Thanks :)
Nice job! Just a couple of small comments :) https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:237: } I don't know if you have access to that here but it could also be useful to load the rules of the app locale. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:309: if (autofill::data_util::IsValidCountryCode(country_code)) { If the country code is not valid, you could fallback to the one inferred from the app_locale. To get the country code from the app_locale: autofill::AutofillCountry::CountryCodeForLocale(app_locale_); https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:377: _paymentRequest->billing_profiles(), Just a heads-up I just landed a CL which adds a bit more behavior here, basically finding the associated billing address and only sending that one to GetBasicCardResponseFromAutofillCreditCard. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:377: _paymentRequest->billing_profiles(), Also, you will eventually want to normalize that billing address too, shouldn't be complicated with the nice framework for normalization you created.
Thank you MAC! I left a few comments. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... File ios/chrome/browser/autofill/validation_rules_storage_factory.h (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... ios/chrome/browser/autofill/validation_rules_storage_factory.h:26: // chrome/browser/autofill/validation_rules_storage_factory.{cc,h}. I'm not an ios/.../autofill owner but these two files are almost identical. I recommend finding a way to pass the right blocking pool and the use data directory to the factory in each platform instead. Also it wasn't clear from the comments what was adapted. I had to patch the CL and diff the files to see what had changed. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:53: class AddressNormalizerDelegate : public payments::AddressNormalizer::Delegate { Please move this to its own files in i/c/b/payments. We should avoid putting non-UI stuff under ui moving forward. Could you construct this with a reference to the coordinator similar to FullCardRequester? it can call a function on the coordinator once the delegate method is called. That way, calling the normalizer for a profile is just a single line and you'll have named functions in the coordinators that receive the normalized profiles which you can copy or keep a reference to. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:66: DCHECK(callback_ != nil); nit? DCHECK(callback_) https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:79: Callback callback_ = nil; s/nil/nullptr for C++ objects https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:85: class PRCardUnmaskPromptViewBridge I'll move PRCardUnmaskPromptViewBridge and FullCardRequester to i/c/b/payments myself. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:203: } _fullCard; please typedef this. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:219: base::WrapUnique(new autofill::ChromeMetadataSource( MakeUnique is preferred. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:356: - (void)maybeDone { nit: please change to something more descriptive like sendPaymentResponseIfAllAsyncCallsCompleted dont' be shy to be verbose in Objective-C :)
PTAL. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... File ios/chrome/browser/autofill/validation_rules_storage_factory.h (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... ios/chrome/browser/autofill/validation_rules_storage_factory.h:26: // chrome/browser/autofill/validation_rules_storage_factory.{cc,h}. On 2017/04/27 12:19:52, moe wrote: > I'm not an ios/.../autofill owner but these two files are almost identical. I > recommend finding a way to pass the right blocking pool and the use data > directory to the factory in each platform instead. Also it wasn't clear from the > comments what was adapted. I had to patch the CL and diff the files to see what > had changed. Yep, had discussed this with mathp@ and sebsg@, and we figured doing this was a good way of doing things. See personal_data_manager_factory.{cc,h}, which is also forked in ios/. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:53: class AddressNormalizerDelegate : public payments::AddressNormalizer::Delegate { On 2017/04/27 12:19:53, moe wrote: > Please move this to its own files in i/c/b/payments. We should avoid putting > non-UI stuff under ui moving forward. This is just a helper class, I think it makes sense to keep it near where it's used, and in the anonymous namespace. > Could you construct this with a reference to the coordinator similar to > FullCardRequester? it can call a function on the coordinator once the delegate > method is called. That way, calling the normalizer for a profile is just a > single line and you'll have named functions in the coordinators that receive the > normalized profiles which you can copy or keep a reference to. Doing that would render this helper class useless: if I'm calling a named function on an object, might as well get rid of this class and just implement OnAddressNormalized/OnCouldNotNormalize on the coordinator directly. The problem this helper solves is that we can't know, when these delegate methods are called, which address it was called for (shipping? contact? other?). Using a block (or could be a base::Callback) lets us keep state to know exactly which address we're getting called back for. I suggested that sebsg@ refactor the AddressNormalizer to receive two base::Callback's instead of a delegate pointer, so that might change and then we can get rid of this altogether. We'll be able to base::Bind a pointer to the AutofillProfile in the callback directly, and update that with the normalized version. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:66: DCHECK(callback_ != nil); On 2017/04/27 12:19:53, moe wrote: > nit? DCHECK(callback_) Done. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:79: Callback callback_ = nil; On 2017/04/27 12:19:53, moe wrote: > s/nil/nullptr for C++ objects Done. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:85: class PRCardUnmaskPromptViewBridge On 2017/04/27 12:19:53, moe wrote: > I'll move PRCardUnmaskPromptViewBridge and FullCardRequester to i/c/b/payments > myself. Acknowledged. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:203: } _fullCard; On 2017/04/27 12:19:52, moe wrote: > please typedef this. Done. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:219: base::WrapUnique(new autofill::ChromeMetadataSource( On 2017/04/27 12:19:53, moe wrote: > MakeUnique is preferred. Done. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:309: if (autofill::data_util::IsValidCountryCode(country_code)) { On 2017/04/26 20:52:51, sebsg wrote: > If the country code is not valid, you could fallback to the one inferred from > the app_locale. > > To get the country code from the app_locale: > autofill::AutofillCountry::CountryCodeForLocale(app_locale_); > Done (with iOS APIs). https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:356: - (void)maybeDone { On 2017/04/27 12:19:53, moe wrote: > nit: please change to something more descriptive like > sendPaymentResponseIfAllAsyncCallsCompleted > dont' be shy to be verbose in Objective-C :) Done. That *is* quite verbose 😳 https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:377: _paymentRequest->billing_profiles(), On 2017/04/26 20:52:51, sebsg wrote: > Also, you will eventually want to normalize that billing address too, shouldn't > be complicated with the nice framework for normalization you created. Done. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:377: _paymentRequest->billing_profiles(), On 2017/04/26 20:52:51, sebsg wrote: > Just a heads-up I just landed a CL which adds a bit more behavior here, > basically finding the associated billing address and only sending that one to > GetBasicCardResponseFromAutofillCreditCard. Acknowledged, taken into account.
The CQ bit was checked by macourteau@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by macourteau@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Great work MAC! ios/.../payments lgtm % nits https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... File ios/chrome/browser/autofill/validation_rules_storage_factory.h (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... ios/chrome/browser/autofill/validation_rules_storage_factory.h:26: // chrome/browser/autofill/validation_rules_storage_factory.{cc,h}. On 2017/04/27 18:07:51, macourteau wrote: > On 2017/04/27 12:19:52, moe wrote: > > I'm not an ios/.../autofill owner but these two files are almost identical. I > > recommend finding a way to pass the right blocking pool and the use data > > directory to the factory in each platform instead. Also it wasn't clear from > the > > comments what was adapted. I had to patch the CL and diff the files to see > what > > had changed. > > Yep, had discussed this with mathp@ and sebsg@, and we figured doing this was a > good way of doing things. See personal_data_manager_factory.{cc,h}, which is > also forked in ios/. sure. it's up to you and the owners. It's just that more owners (who are currently mostly OOO) have to lgtm because of the added ios dependencies. But PLEASE comment what was adapted for iOS for posterity. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:53: class AddressNormalizerDelegate : public payments::AddressNormalizer::Delegate { On 2017/04/27 18:07:52, macourteau wrote: > On 2017/04/27 12:19:53, moe wrote: > > Please move this to its own files in i/c/b/payments. We should avoid putting > > non-UI stuff under ui moving forward. > > This is just a helper class, I think it makes sense to keep it near where it's > used, and in the anonymous namespace. > > > Could you construct this with a reference to the coordinator similar to > > FullCardRequester? it can call a function on the coordinator once the delegate > > method is called. That way, calling the normalizer for a profile is just a > > single line and you'll have named functions in the coordinators that receive > the > > normalized profiles which you can copy or keep a reference to. > > Doing that would render this helper class useless: if I'm calling a named > function on an object, might as well get rid of this class and just implement > OnAddressNormalized/OnCouldNotNormalize on the coordinator directly. The problem > this helper solves is that we can't know, when these delegate methods are > called, which address it was called for (shipping? contact? other?). Using a > block (or could be a base::Callback) lets us keep state to know exactly which > address we're getting called back for. > > I suggested that sebsg@ refactor the AddressNormalizer to receive two > base::Callback's instead of a delegate pointer, so that might change and then we > can get rid of this altogether. We'll be able to base::Bind a pointer to the > AutofillProfile in the callback directly, and update that with the normalized > version. Ah I see. Forgot about the need to keep track of the normalized profile. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:52: // of which address this delegate was created for. nit: please update the comment to reflect "... the callback mechanism is needed in order to keep track of ...". The class itself is needed because we can't have an objective-C class inherit from a C++ class. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:172: struct FullCard { nit: please move to anonymous namespace. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:219: - (void)initAddressNormalizer { nit: s/initAddressNormalizer/startAddressNormalizer initializer methods start with init. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:294: flag:(BOOL*)normalizationRequestPendingFlag { nit: this function has side effects. Please declare it in @implementation PaymentRequestCoordinator { ... } and provide comments on what it does. nit: s/flag:/normalizationRequestPendingFlag: and s/normalizationRequestPendingFlag/flag for improved descriptiveness. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:305: return; I think you should be able to say "if (!weakSelf) return;". I don't know why the assignment was needed in the first place incl. other instances in the code. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:324: - (void)sendPaymentResponse { nit: s/sendPaymentResponse/requestFullCardAndNormalizeProfiles or something like that.
What do you think about my suggestions for the app locale? If you don't want to do it in this CL, could you create a bug? Thanks! Excited to see normalization working on all platforms!
Patchset #7 (id:120001) has been deleted
On 2017/04/28 13:53:29, sebsg wrote: > What do you think about my suggestions for the app locale? > > If you don't want to do it in this CL, could you create a bug? Thanks! > > Excited to see normalization working on all platforms! Already done, see -normalizeProfile:delegate:flag:, which has been updated to get the app locale from NSLocale. :) https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... File ios/chrome/browser/autofill/validation_rules_storage_factory.h (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/auto... ios/chrome/browser/autofill/validation_rules_storage_factory.h:26: // chrome/browser/autofill/validation_rules_storage_factory.{cc,h}. On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > On 2017/04/27 18:07:51, macourteau wrote: > > On 2017/04/27 12:19:52, moe wrote: > > > I'm not an ios/.../autofill owner but these two files are almost identical. > I > > > recommend finding a way to pass the right blocking pool and the use data > > > directory to the factory in each platform instead. Also it wasn't clear from > > the > > > comments what was adapted. I had to patch the CL and diff the files to see > > what > > > had changed. > > > > Yep, had discussed this with mathp@ and sebsg@, and we figured doing this was > a > > good way of doing things. See personal_data_manager_factory.{cc,h}, which is > > also forked in ios/. > > sure. it's up to you and the owners. It's just that more owners (who are > currently mostly OOO) have to lgtm because of the added ios dependencies. But > PLEASE comment what was adapted for iOS for posterity. Done. https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2844783002/diff/40001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:53: class AddressNormalizerDelegate : public payments::AddressNormalizer::Delegate { On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > On 2017/04/27 18:07:52, macourteau wrote: > > On 2017/04/27 12:19:53, moe wrote: > > > Please move this to its own files in i/c/b/payments. We should avoid putting > > > non-UI stuff under ui moving forward. > > > > This is just a helper class, I think it makes sense to keep it near where it's > > used, and in the anonymous namespace. > > > > > Could you construct this with a reference to the coordinator similar to > > > FullCardRequester? it can call a function on the coordinator once the > delegate > > > method is called. That way, calling the normalizer for a profile is just a > > > single line and you'll have named functions in the coordinators that receive > > the > > > normalized profiles which you can copy or keep a reference to. > > > > Doing that would render this helper class useless: if I'm calling a named > > function on an object, might as well get rid of this class and just implement > > OnAddressNormalized/OnCouldNotNormalize on the coordinator directly. The > problem > > this helper solves is that we can't know, when these delegate methods are > > called, which address it was called for (shipping? contact? other?). Using a > > block (or could be a base::Callback) lets us keep state to know exactly which > > address we're getting called back for. > > > > I suggested that sebsg@ refactor the AddressNormalizer to receive two > > base::Callback's instead of a delegate pointer, so that might change and then > we > > can get rid of this altogether. We'll be able to base::Bind a pointer to the > > AutofillProfile in the callback directly, and update that with the normalized > > version. > > Ah I see. Forgot about the need to keep track of the normalized profile. Acknowledged. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:52: // of which address this delegate was created for. On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > nit: please update the comment to reflect "... the callback mechanism is needed > in order to keep track of ...". > The class itself is needed because we can't have an objective-C class inherit > from a C++ class. Done. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:172: struct FullCard { On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > nit: please move to anonymous namespace. Done. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:219: - (void)initAddressNormalizer { On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > nit: s/initAddressNormalizer/startAddressNormalizer > initializer methods start with init. Done. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:294: flag:(BOOL*)normalizationRequestPendingFlag { On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > nit: this function has side effects. Please declare it in @implementation > PaymentRequestCoordinator { ... } and provide comments on what it does. > > nit: s/flag:/normalizationRequestPendingFlag: and > s/normalizationRequestPendingFlag/flag for improved descriptiveness. Done. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:305: return; On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > I think you should be able to say "if (!weakSelf) return;". I don't know why the > assignment was needed in the first place incl. other instances in the code. Done. https://codereview.chromium.org/2844783002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:324: - (void)sendPaymentResponse { On 2017/04/28 09:04:44, moe (OOO until May 8) wrote: > nit: s/sendPaymentResponse/requestFullCardAndNormalizeProfiles > or something like that. Done.
macourteau@chromium.org changed reviewers: + marq@chromium.org
marq@chromium.org: Please provide OWNERS review. :)
The CQ bit was checked by macourteau@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...
Sweet! LGTM for the normalization logic
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
I've made a few implementation comments, but I have two significant
observations:
- You are adding another 250+ lines of implementation to an already-large file
(payment_request_coordinator). Generally speaking the role of a coordinator
object should be to create view controllers and the model-layer objects they
need and connect them together. Coordinators themselves shouldn't need to
implement significant model-layer code. So I would strongly suggest that you
precede this CL with another one that moves as much of the model-layer business
logic as possible out of the coordinator into another object
("payment_request_mediator") ; this CL can just add code to that object.
- You are adding 250 lines of implementation and zero new unit tests. In
addition to the changes I suggest above, please break this CL into smaller
increments with unit tests at each stage.
Please feel free to contact me directly for further clarifications.
https://codereview.chromium.org/2844783002/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right):
https://codereview.chromium.org/2844783002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/payments/payment_request_coordinator.mm:61: void
SetCallback(Callback callback) { callback_ = callback; }
Since there is only one callback that is used, why not just have this delegate
directly update the coordinator properties instead of making the implementation
more complicated with a callback?
https://codereview.chromium.org/2844783002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/payments/payment_request_coordinator.mm:231: -
(void)startAddressNormalizer {
Here and elsewhere: Every method requires a comment describing what the method
does, pre/post conditions, etc.
https://codereview.chromium.org/2844783002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/payments/payment_request_coordinator.mm:312: // Check that
PaymentRequestCoordinator still exists as |profileToNormalize|
But the delegate is a property of this coordinator object, so if the delegate
exists to execute this callback, the coordinator must as well.
https://codereview.chromium.org/2844783002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/payments/payment_request_coordinator.mm:318:
*profileToNormalize = normalizedProfile;
Instead of updating instance variables passed in by reference, please just
directly set properties of self.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
