Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686613003:
[Payments] Move address normalization code from android to native. (Closed)
|
Created:
3 years, 10 months ago by sebsg Modified:
3 years, 10 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, jam, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Move address normalization code from android to native.
BUG=689612
Review-Url: https://codereview.chromium.org/2686613003
Cr-Commit-Position: refs/heads/master@{#450964}
Committed: https://chromium.googlesource.com/chromium/src/+/a4a7c2d66abfbba4a24db8a67ec23755ecef6f8b
Patch Set 1 #
Total comments: 36
Patch Set 2 : Addressed comments #
Total comments: 6
Patch Set 3 : Renamed file and addressed comments #Messages
Total messages: 47 (33 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (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...
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...) 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_compile_dbg_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...
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Description was changed from ========== WIP [Payments] Move address normalization code from android to native. BUG=689612 ========== to ========== [Payments] Move address normalization code from android to native. BUG=689612 ==========
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: + rouslan@chromium.org
Hi Rouslan, can you please take a look?
https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:275: class AndroidAddressNormalizationRequester nit: AddressNormalizerDelegate https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:285: void OnAddressNormalized(AutofillProfile normalized_profile) override { const-ref for the normalized proifle. https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:293: virtual ~AndroidAddressNormalizationRequester() {} override instead of virtual https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (left): https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:329: void CancelPendingAddressNormalizations( Is it OK to remove this functionality? https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... File components/payments/address_validator_helper.cc (right): https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:29: const std::string& region_code, const-ref https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:31: autofill::AddressValidator* address_validator) { nit: Use initializer list, i.e., ClassName(type param_name) : parame_name_(param_name) {} https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:46: const std::string& region_code) { nit: move the body of this function inside of OnRulesSuccessfullyLoaded. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:71: autofill::AddressValidator* address_validator_; Please use the same the order for member variables and constructor parameters. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:127: bool success) { Add a note about the reason for ignoring |success| parameter. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:131: for (size_t i = 0; i < it->second.size(); ++i) nit {} for the "for" loops. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... File components/payments/address_validator_helper.h (right): https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:18: class AddressNormalizationRequester { nit: AddressNormalizer::Delegate https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:19: public: Need a virtual destructor with empty body. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:25: class NormalizationRequest { nit: AddressNormalizer::Request https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:32: class AddressValidatorHelper nit: AddressNormalizer, because it does not validate anything at the moment. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:50: bool StartAddressNormalization(autofill::AutofillProfile profile, const-ref for profile You seem to have changed the PersonalDataManager API to return void instead of boolean. Please be consistent. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:51: const std::string region_code, const-ref for region_code https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:55: void OnAddressValidationRulesLoaded(const std::string& region_code, Specify the interface being implemented. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:64: }; DISALLOW_COPY_AND_ASSIGN
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:180001) has been deleted
Thanks, another look? :) https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:275: class AndroidAddressNormalizationRequester On 2017/02/14 21:31:08, rouslan wrote: > nit: AddressNormalizerDelegate Done. https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:285: void OnAddressNormalized(AutofillProfile normalized_profile) override { On 2017/02/14 21:31:08, rouslan wrote: > const-ref for the normalized proifle. Done. https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:293: virtual ~AndroidAddressNormalizationRequester() {} On 2017/02/14 21:31:08, rouslan wrote: > override instead of virtual Done. https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (left): https://codereview.chromium.org/2686613003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:329: void CancelPendingAddressNormalizations( On 2017/02/14 21:31:08, rouslan wrote: > Is it OK to remove this functionality? It was not currently used so I think it's ok to remove now. However, that feature is required in the long run. I'm thinking of moving the timeout from the caller to the normalization task itself. So when asking for a normalization, the caller would specify the time limit after which it would like to be called back with a non normalized profile. Since this is adding some more complex logic, and the current code works as is, do you mind if I do it in another CL (Starting right now)? https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... File components/payments/address_validator_helper.cc (right): https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:29: const std::string& region_code, On 2017/02/14 21:31:08, rouslan wrote: > const-ref Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:31: autofill::AddressValidator* address_validator) { On 2017/02/14 21:31:08, rouslan wrote: > nit: Use initializer list, i.e., ClassName(type param_name) : > parame_name_(param_name) {} Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:46: const std::string& region_code) { On 2017/02/14 21:31:08, rouslan wrote: > nit: move the body of this function inside of OnRulesSuccessfullyLoaded. Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:71: autofill::AddressValidator* address_validator_; On 2017/02/14 21:31:08, rouslan wrote: > Please use the same the order for member variables and constructor parameters. Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:127: bool success) { On 2017/02/14 21:31:08, rouslan wrote: > Add a note about the reason for ignoring |success| parameter. I was thinking of using it and adding the onCouldNotNormalize in a future CL but it makes much more sense to add it here. Thanks! https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.cc:131: for (size_t i = 0; i < it->second.size(); ++i) On 2017/02/14 21:31:08, rouslan wrote: > nit {} for the "for" loops. Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... File components/payments/address_validator_helper.h (right): https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:18: class AddressNormalizationRequester { On 2017/02/14 21:31:09, rouslan wrote: > nit: AddressNormalizer::Delegate Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:19: public: On 2017/02/14 21:31:08, rouslan wrote: > Need a virtual destructor with empty body. Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:25: class NormalizationRequest { On 2017/02/14 21:31:09, rouslan wrote: > nit: AddressNormalizer::Request Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:32: class AddressValidatorHelper On 2017/02/14 21:31:09, rouslan wrote: > nit: AddressNormalizer, because it does not validate anything at the moment. Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:50: bool StartAddressNormalization(autofill::AutofillProfile profile, On 2017/02/14 21:31:09, rouslan wrote: > const-ref for profile > > You seem to have changed the PersonalDataManager API to return void instead of > boolean. Please be consistent. Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:51: const std::string region_code, On 2017/02/14 21:31:09, rouslan wrote: > const-ref for region_code Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:55: void OnAddressValidationRulesLoaded(const std::string& region_code, On 2017/02/14 21:31:09, rouslan wrote: > Specify the interface being implemented. Done. https://codereview.chromium.org/2686613003/diff/160001/components/payments/ad... components/payments/address_validator_helper.h:64: }; On 2017/02/14 21:31:09, rouslan wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/2686613003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2686613003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:159: onAddressNormalized(null); Why ignore the profile parameter? https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... File components/payments/address_validator_helper.cc (right): https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... components/payments/address_validator_helper.cc:28: AddressNormalizationRequest(const AutofillProfile& profile, Please add a comment that delegate and address_validator need to outlive this object. https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... File components/payments/address_validator_helper.h (right): https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... components/payments/address_validator_helper.h:18: class AddressNormalizer : public autofill::LoadRulesListener, Please rename the file to address_normalizer.h
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...
Thanks, another look? https://codereview.chromium.org/2686613003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2686613003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:159: onAddressNormalized(null); On 2017/02/15 21:27:45, rouslan wrote: > Why ignore the profile parameter? Right, it seems really weird now but this is because I just added this call to the interface, and this class managed to handle the case of a timeout in that way. This will all be made better in my followup where the time is handled in native code :) https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... File components/payments/address_validator_helper.cc (right): https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... components/payments/address_validator_helper.cc:28: AddressNormalizationRequest(const AutofillProfile& profile, On 2017/02/15 21:27:45, rouslan wrote: > Please add a comment that delegate and address_validator need to outlive this > object. Done. https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... File components/payments/address_validator_helper.h (right): https://codereview.chromium.org/2686613003/diff/200001/components/payments/ad... components/payments/address_validator_helper.h:18: class AddressNormalizer : public autofill::LoadRulesListener, On 2017/02/15 21:27:45, rouslan wrote: > Please rename the file to address_normalizer.h Done.
lgtm
The CQ bit was unchecked by sebsg@chromium.org
The CQ bit was checked by sebsg@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sebsg@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sebsg@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sebsg@chromium.org
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": 1487256962218210,
"parent_rev": "f8969afdf8bb49522af51123fda1d2e4c5050444", "commit_rev":
"a4a7c2d66abfbba4a24db8a67ec23755ecef6f8b"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Move address normalization code from android to native. BUG=689612 ========== to ========== [Payments] Move address normalization code from android to native. BUG=689612 Review-Url: https://codereview.chromium.org/2686613003 Cr-Commit-Position: refs/heads/master@{#450964} Committed: https://chromium.googlesource.com/chromium/src/+/a4a7c2d66abfbba4a24db8a67ec2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:220001) as https://chromium.googlesource.com/chromium/src/+/a4a7c2d66abfbba4a24db8a67ec2... |
