Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889983002:
Adds the AddressNormalizationManager and tests. (Closed)
|
Created:
3 years, 7 months ago by macourteau Modified:
3 years, 7 months ago CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, sebsg, Moe Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds the AddressNormalizationManager and tests.
This class makes it easy to manage multiple concurrent address normalization requests, and to have a callback executed when all address normalization requests have completed.
BUG=
R=mathp@chromium.org, sebsg@chromium.org
Review-Url: https://codereview.chromium.org/2889983002 .
Cr-Commit-Position: refs/heads/master@{#472918}
Committed: https://chromium.googlesource.com/chromium/src/+/0aa5c726da0324909f7d891f2ca033c115edc372
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 20
Patch Set 3 : Addresses comments from mathp@. #
Total comments: 2
Patch Set 4 : . #
Dependent Patchsets: Messages
Total messages: 32 (19 generated)
macourteau@chromium.org changed reviewers: + mathp@chromium.org
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...
Description was changed from ========== Adds the AddressNormalizationManager and tests. BUG= ========== to ========== Adds the AddressNormalizationManager and tests. This class makes it easy to manage multiple concurrent address normalization requests, and to have a callback executed when all address normalization requests have completed. BUG= ==========
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
mathp@chromium.org changed reviewers: + sebsg@chromium.org
Looks good, few comments! It's seb's code would like him to have a look too https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... File components/payments/core/address_normalization_manager.cc (right): https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:23: : public payments::AddressNormalizer::Delegate { no need for payments:: https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:82: delegates_.push_back(base::MakeUnique<NormalizerDelegate>( can we use emplace_back and remove the std::unique_ptr for NormalizerDelegate? https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:111: if (!autofill::data_util::IsValidCountryCode(country_code)) { nit: no curlies https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:119: bool AddressNormalizationManager::NormalizerDelegate::HasCompleted() const { has_completed() can be used https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... File components/payments/core/address_normalization_manager.h (right): https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:30: base::OnceCallback<void(const AddressNormalizationManager&)>; can this be a base::OnceClosure()? If so, perhaps you don't need the typedef at all https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:36: std::unique_ptr<AddressNormalizer> address_normalizer, can we remove the std::unique_ptr? https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:44: // last pending request completes. you should mention how to get the normalized addresses from the manager. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:53: class NormalizerDelegate; why not declare the whole class here? https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:63: std::string default_country_code_; nit: const?
https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... File components/payments/core/address_normalization_manager.cc (right): https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:23: : public payments::AddressNormalizer::Delegate { On 2017/05/17 16:45:14, Mathieu wrote: > no need for payments:: Done. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:82: delegates_.push_back(base::MakeUnique<NormalizerDelegate>( On 2017/05/17 16:45:14, Mathieu wrote: > can we use emplace_back and remove the std::unique_ptr for NormalizerDelegate? NormalizerDelegate is noncopyable (DISALLOW_COPY_AND_ASSIGN), which is by design, since the AddressNormalizer requires a pointer to its delegate (i.e. it can't change). Storing the objects directly in the vector (w/o a pointer) doesn't work if the object is noncopyable. move construction/assignment wouldn't work either since the delegate pointer passed to AddressNormalizer would no longer be valid. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:111: if (!autofill::data_util::IsValidCountryCode(country_code)) { On 2017/05/17 16:45:14, Mathieu wrote: > nit: no curlies Done. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.cc:119: bool AddressNormalizationManager::NormalizerDelegate::HasCompleted() const { On 2017/05/17 16:45:14, Mathieu wrote: > has_completed() can be used Done. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... File components/payments/core/address_normalization_manager.h (right): https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:30: base::OnceCallback<void(const AddressNormalizationManager&)>; On 2017/05/17 16:45:14, Mathieu wrote: > can this be a base::OnceClosure()? If so, perhaps you don't need the typedef at > all Done. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:36: std::unique_ptr<AddressNormalizer> address_normalizer, On 2017/05/17 16:45:14, Mathieu wrote: > can we remove the std::unique_ptr? The alternative would be for the caller to own the AddressNormalizer, but it would still have to be a pointer, since AddressNormalizer is a pure virtual class, and actual objects are either AddressNormalizerImpl or TestAddressNormalizer instances. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:44: // last pending request completes. On 2017/05/17 16:45:14, Mathieu wrote: > you should mention how to get the normalized addresses from the manager. This is mentioned in the documentation for StartNormalizingAddress below ("On completion, the address in |profile| will be updated with the normalized address."). https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:53: class NormalizerDelegate; On 2017/05/17 16:45:14, Mathieu wrote: > why not declare the whole class here? Figured it was an implementation detail, and users of this class don't need to know (also: shorter header file). I can move it here if you'd prefer, I don't have a strong opinion. https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:63: std::string default_country_code_; On 2017/05/17 16:45:14, Mathieu wrote: > nit: const? Done.
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...
Very nice :) The code LGTM with a small concern for the use. https://codereview.chromium.org/2889983002/diff/40001/components/payments/cor... File components/payments/core/address_normalization_manager.h (right): https://codereview.chromium.org/2889983002/diff/40001/components/payments/cor... components/payments/core/address_normalization_manager.h:45: void StartNormalizingAddress(autofill::AutofillProfile* profile); My only concern is this modification of the profile, and is more related to the use of this class than the class itself. If you were planning to make a copy of the profile and pass the pointer to the copy in this function then everything is perfect :) Otherwise here is my concern: The reason why the original code sends a copy is that I don't want to have the original profile be normalized. For example, The phone number will be normalized in E.164 format which is not the format we want to save or show to the user.
https://codereview.chromium.org/2889983002/diff/40001/components/payments/cor... File components/payments/core/address_normalization_manager.h (right): https://codereview.chromium.org/2889983002/diff/40001/components/payments/cor... components/payments/core/address_normalization_manager.h:45: void StartNormalizingAddress(autofill::AutofillProfile* profile); On 2017/05/17 19:30:23, sebsg wrote: > My only concern is this modification of the profile, and is more related to the > use of this class than the class itself. > > If you were planning to make a copy of the profile and pass the pointer to the > copy in this function then everything is perfect :) > > Otherwise here is my concern: > > The reason why the original code sends a copy is that I don't want to have the > original profile be normalized. For example, The phone number will be normalized > in E.164 format which is not the format we want to save or show to the user. Ok, good to know. Will make sure the code using this makes a copy (not relevant to this CL though).
lgtm https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... File components/payments/core/address_normalization_manager.h (right): https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:53: class NormalizerDelegate; On 2017/05/17 18:38:40, macourteau wrote: > On 2017/05/17 16:45:14, Mathieu wrote: > > why not declare the whole class here? > > Figured it was an implementation detail, and users of this class don't need to > know (also: shorter header file). I can move it here if you'd prefer, I don't > have a strong opinion. Yeah, I don't see this pattern a lot so I would prefer having it declared here, thanks
https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... File components/payments/core/address_normalization_manager.h (right): https://codereview.chromium.org/2889983002/diff/20001/components/payments/cor... components/payments/core/address_normalization_manager.h:53: class NormalizerDelegate; On 2017/05/17 19:47:33, Mathieu wrote: > On 2017/05/17 18:38:40, macourteau wrote: > > On 2017/05/17 16:45:14, Mathieu wrote: > > > why not declare the whole class here? > > > > Figured it was an implementation detail, and users of this class don't need to > > know (also: shorter header file). I can move it here if you'd prefer, I don't > > have a strong opinion. > > Yeah, I don't see this pattern a lot so I would prefer having it declared here, > thanks Done.
The CQ bit was checked by macourteau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, sebsg@chromium.org Link to the patchset: https://codereview.chromium.org/2889983002/#ps60001 (title: ".")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by macourteau@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by macourteau@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 macourteau@chromium.org
Description was changed from ========== Adds the AddressNormalizationManager and tests. This class makes it easy to manage multiple concurrent address normalization requests, and to have a callback executed when all address normalization requests have completed. BUG= ========== to ========== Adds the AddressNormalizationManager and tests. This class makes it easy to manage multiple concurrent address normalization requests, and to have a callback executed when all address normalization requests have completed. BUG= R=mathp@chromium.org, sebsg@chromium.org Review-Url: https://codereview.chromium.org/2889983002 . Cr-Commit-Position: refs/heads/master@{#472918} Committed: https://chromium.googlesource.com/chromium/src/+/0aa5c726da0324909f7d891f2ca0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 0aa5c726da0324909f7d891f2ca033c115edc372 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
