Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775553004:
[WebPayments] Implementing Profile filter and dedupe (Closed)
|
Created:
3 years, 9 months ago by tmartino Modified:
3 years, 8 months ago CC:
browser-components-watch_chromium.org, chromium-reviews, darin-cc_chromium.org, estade+watch_chromium.org, gogerald+paymentswatch_chromium.org, jam, mahmadi+paymentswatch_chromium.org, mathp+autofillwatch_chromium.org, rogerm+autofillwatch_chromium.org, rouslan+autofill_chromium.org, rouslan+payments_chromium.org, sebsg+autofillwatch_chromium.org, sebsg+paymentswatch_chromium.org, vabr+watchlistautofill_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[WebPayments] Implementing Profile filter and dedupe logic in components
Still to-do:
- Backport this logic to Android in a forthcoming CL
- Consider filtering shipping addresses
BUG=706117
Review-Url: https://codereview.chromium.org/2775553004
Cr-Commit-Position: refs/heads/master@{#462603}
Committed: https://chromium.googlesource.com/chromium/src/+/a6eb22f51aa4a79ecfd8e61527280658aef1e089
Patch Set 1 #Patch Set 2 : Adding omitted files #
Total comments: 20
Patch Set 3 : updating to use options #Patch Set 4 : rebase #Patch Set 5 : fix tests #Patch Set 6 : include algo #
Total comments: 9
Patch Set 7 : now with tests #Patch Set 8 : now with tests that hopefully pass #
Total comments: 6
Patch Set 9 : mathp comments #Patch Set 10 : rebasing #
Messages
Total messages: 67 (50 generated)
Description was changed from ========== [WebPayments] Implementing Profile filter and dedupe logic in components BUG= ========== to ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Figure out a way to give iOS access to this (move to core) - Consider filtering shipping addresses BUG= ==========
tmartino@chromium.org changed reviewers: + rogerm@chromium.org
+rogerm for a first pass. In particular, I'm looking for some feedback on how I've used the comparator.
tmartino@chromium.org changed reviewers: + sebsg@chromium.org
+sebsg, who is familiar with the Android implementation that will eventually be replaced by this code.
mathp@chromium.org changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.h:14: // TODO(tmartino): Remove dependency on PaymentRequestSpec and move to core. If it's just for the three requestPayer* bool, I would do the change in this CL. Android doesn't have the spec anyway, so it will be out-of-the-box usable.
Description was changed from ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Figure out a way to give iOS access to this (move to core) - Consider filtering shipping addresses BUG= ========== to ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Figure out a way to give iOS access to this (move to core) - Consider filtering shipping addresses BUG= ==========
mathp@chromium.org changed reviewers: - mathp@chromium.org
https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.h:14: // TODO(tmartino): Remove dependency on PaymentRequestSpec and move to core. On 2017/03/24 at 16:16:02, Mathieu (slow - travel) wrote: > If it's just for the three requestPayer* bool, I would do the change in this CL. Android doesn't have the spec anyway, so it will be out-of-the-box usable. One thing anthonyvd and I discussed was potentially extracting a Spec interface into core--there's a lot of data encapsulated in that structure that's potentially relevant to business logic that doesn't necessarily need any of the Mojo stuff. Our existing Mojo-dependent class would implement the interface without any further changes, and for Android and iOS we would essentially implement a "dumb" version where the values are provided at construct-time.
mathp@chromium.org changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.h:14: // TODO(tmartino): Remove dependency on PaymentRequestSpec and move to core. On 2017/03/24 16:28:52, tmartino wrote: > On 2017/03/24 at 16:16:02, Mathieu (slow - travel) wrote: > > If it's just for the three requestPayer* bool, I would do the change in this > CL. Android doesn't have the spec anyway, so it will be out-of-the-box usable. > > One thing anthonyvd and I discussed was potentially extracting a Spec interface > into core--there's a lot of data encapsulated in that structure that's > potentially relevant to business logic that doesn't necessarily need any of the > Mojo stuff. Our existing Mojo-dependent class would implement the interface > without any further changes, and for Android and iOS we would essentially > implement a "dumb" version where the values are provided at construct-time. I would use the booleans for now and re-evaluate later if there's a need for a common Spec interface in core? That may be an interesting thing to do, but for now iOS will benefit directly from this change if we use the booleans.
https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.cc (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:18: std::vector<autofill::AutofillProfile*> processed = profiles; Note, but don't actually apply, that this could have been accomplished by taking profiles by value... which would of course confuse the next person who looked at the code. :) https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:46: processed.erase(it); this isn't safe (i.e., you're venturing off into undefined behavior). Maybe structure this as a std::partition then erase the tail? Alternatively, use integer indices not iterators. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:62: bool PaymentsProfileComparator::IsContactEqualOrSuperset( Alas, the merge-ability tests don't really yield an ordering. They're not giving you the superset condition. For example: John Public & J Q Public are mergeable but not orderable. This is also true for phone numbers. Email addresses are kind of a degenerate case as it's just a case-insensitive comparison. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:67: !super->HasInfo(autofill::NAME_FULL)) nit: multi-line if requires curly braces https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:99: return score; could be made a bit more succinct? return (spec_->request_foo() && profile->HasInfo(foo)) + (spec_->request_bar() && profile->HasInfo(bar)) + (spec_->request_baz() && profile->HasInfo(baz)); Design/Spec question: Should there be any difference in weight between the items. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:104: int score = spec_->request_payer_name() + spec_->request_payer_phone() + s/score/desired_score|full_score/ ? https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.h:56: bool CompareContactCompleteness(autofill::AutofillProfile* p1, nit: this name doesn't really suggest a boolean return value to me... i'd expect negative, 0, positive from a "Compare" func that lacks a relation in the name. maybe IsMoreComplete(p1, p2)?
Nice work!!! Some comments inline. Also I think the tests for your CL would be a great match for the INSTANTIATE_TEST_CASE_P test macro. If you don't know about it just ping me, it's very neat :) https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.cc (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:46: processed.erase(it); On 2017/03/24 18:23:16, Roger McFarlane (Chromium) wrote: > this isn't safe (i.e., you're venturing off into undefined behavior). > > Maybe structure this as a std::partition then erase the tail? > > Alternatively, use integer indices not iterators. +1. What I did in Java is have add them to a new vector... again... Not sure efficient but the dataset is very small. You could also keep the index of the ones you want to remove, and remove starting from the back to reduce the number of profiles that will be moved. But this is not very efficient either :P https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:62: bool PaymentsProfileComparator::IsContactEqualOrSuperset( On 2017/03/24 18:23:16, Roger McFarlane (Chromium) wrote: > Alas, the merge-ability tests don't really yield an ordering. They're not giving > you the superset condition. > > For example: John Public & J Q Public are mergeable but not orderable. > > This is also true for phone numbers. > > Email addresses are kind of a degenerate case as it's just a case-insensitive > comparison. > Yeah that's a good point Roger. In the Java implementation I just check if they are the same (minus case, maybe we could add punctuation too). It's not perfect, but I think it's fine to show both profiles in Roger's example. What we really want to avoid is showing all for cases like 1- Jon Snow, jon@snow.com, 123-123-1234 2- Jon Snow, jon@snow.com 3- Jon Snow, 123-123-1234 4- Jon Snow etc. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:99: return score; On 2017/03/24 18:23:16, Roger McFarlane (Chromium) wrote: > could be made a bit more succinct? > > return > (spec_->request_foo() && profile->HasInfo(foo)) + > (spec_->request_bar() && profile->HasInfo(bar)) + > (spec_->request_baz() && profile->HasInfo(baz)); > > Design/Spec question: > > Should there be any difference in weight between the items. For the spec question: Not really, they all need to be there. before the user can do the transaction.
The CQ bit was checked by tmartino@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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
Description was changed from ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Figure out a way to give iOS access to this (move to core) - Consider filtering shipping addresses BUG= ========== to ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Figure out a way to give iOS access to this (move to core) - Consider filtering shipping addresses BUG=706117 ==========
The CQ bit was checked by tmartino@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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Figure out a way to give iOS access to this (move to core) - Consider filtering shipping addresses BUG=706117 ========== to ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Consider filtering shipping addresses BUG=706117 ==========
The CQ bit was checked by tmartino@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_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 tmartino@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_chromeos_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 tmartino@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...
https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.cc (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:46: processed.erase(it); On 2017/03/27 at 17:13:28, sebsg wrote: > On 2017/03/24 18:23:16, Roger McFarlane (Chromium) wrote: > > this isn't safe (i.e., you're venturing off into undefined behavior). > > > > Maybe structure this as a std::partition then erase the tail? > > > > Alternatively, use integer indices not iterators. > > +1. What I did in Java is have add them to a new vector... again... Not sure efficient but the dataset is very small. > > You could also keep the index of the ones you want to remove, and remove starting from the back to reduce the number of profiles that will be moved. But this is not very efficient either :P As discussed offline, erase returns an iterator to the next element. Therefore this is safe if we add an assignment: it = processed.erase(it); We also discussed optimizations which would reduce the number of things being shifted around during these removes, but IMO the extra complexity (either an alg. which involves shifting these around in-place, or making additional vectors in-memory) is not justified w/r/t the expected small number of elements. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:62: bool PaymentsProfileComparator::IsContactEqualOrSuperset( On 2017/03/27 at 17:13:28, sebsg wrote: > On 2017/03/24 18:23:16, Roger McFarlane (Chromium) wrote: > > Alas, the merge-ability tests don't really yield an ordering. They're not giving > > you the superset condition. > > > > For example: John Public & J Q Public are mergeable but not orderable. > > > > This is also true for phone numbers. > > > > Email addresses are kind of a degenerate case as it's just a case-insensitive > > comparison. > > > > Yeah that's a good point Roger. In the Java implementation I just check if they are the same (minus case, maybe we could add punctuation too). > > It's not perfect, but I think it's fine to show both profiles in Roger's example. What we really want to avoid is showing all for cases like > > 1- Jon Snow, jon@snow.com, 123-123-1234 > 2- Jon Snow, jon@snow.com > 3- Jon Snow, 123-123-1234 > 4- Jon Snow > > etc. As resolved offline we've decided to accept this. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:67: !super->HasInfo(autofill::NAME_FULL)) On 2017/03/24 at 18:23:16, Roger McFarlane (Chromium) wrote: > nit: multi-line if requires curly braces Believe it or not, I've been told elsewhere that this is not required! (And it's clearly not enforced by git cl format, which I ran here.) However, I don't understand Chrome's bizarre war on "unnecessary brackets" so I will happily take your comment as an indication that the code is difficult for a reviewer to read, and therefore as permission to put them in. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:99: return score; On 2017/03/27 at 17:13:28, sebsg wrote: > On 2017/03/24 18:23:16, Roger McFarlane (Chromium) wrote: > > could be made a bit more succinct? > > > > return > > (spec_->request_foo() && profile->HasInfo(foo)) + > > (spec_->request_bar() && profile->HasInfo(bar)) + > > (spec_->request_baz() && profile->HasInfo(baz)); > > > > Design/Spec question: > > > > Should there be any difference in weight between the items. > > For the spec question: Not really, they all need to be there. before the user can do the transaction. Done re: succinctness. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.cc:104: int score = spec_->request_payer_name() + spec_->request_payer_phone() + On 2017/03/24 at 18:23:16, Roger McFarlane (Chromium) wrote: > s/score/desired_score|full_score/ > > ? Done https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... File components/payments/content/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.h:14: // TODO(tmartino): Remove dependency on PaymentRequestSpec and move to core. On 2017/03/24 at 16:37:27, Mathieu wrote: > On 2017/03/24 16:28:52, tmartino wrote: > > On 2017/03/24 at 16:16:02, Mathieu (slow - travel) wrote: > > > If it's just for the three requestPayer* bool, I would do the change in this > > CL. Android doesn't have the spec anyway, so it will be out-of-the-box usable. > > > > One thing anthonyvd and I discussed was potentially extracting a Spec interface > > into core--there's a lot of data encapsulated in that structure that's > > potentially relevant to business logic that doesn't necessarily need any of the > > Mojo stuff. Our existing Mojo-dependent class would implement the interface > > without any further changes, and for Android and iOS we would essentially > > implement a "dumb" version where the values are provided at construct-time. > > I would use the booleans for now and re-evaluate later if there's a need for a common Spec interface in core? That may be an interesting thing to do, but for now iOS will benefit directly from this change if we use the booleans. Looking at iOS, and talking to mahmadi@, actually reaffirmed my faith in this plan--we basically get a PaymentOptionsProvider implementation for free on iOS, as the corresponding class already exists. I also got buy-in from sebsg@ for using this approach on Android. I've updated this CL to patch in that CL and use the class accordingly. This code will now be available on iOS immediately after landing. https://codereview.chromium.org/2775553004/diff/20001/components/payments/con... components/payments/content/profile_util.h:56: bool CompareContactCompleteness(autofill::AutofillProfile* p1, On 2017/03/24 at 18:23:16, Roger McFarlane (Chromium) wrote: > nit: this name doesn't really suggest a boolean return value to me... i'd expect negative, 0, positive from a "Compare" func that lacks a relation in the name. > > maybe IsMoreComplete(p1, p2)? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey this is great! This would really benefit from unit tests though, wdyt? https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... File components/payments/core/profile_util.cc (right): https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.cc:96: autofill::AutofillProfile* profile) { const ref? https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... File components/payments/core/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.h:13: // Utility functions used for processing and filtering profiles. *address profiles (AutofillProfile) https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.h:34: PaymentOptionsProvider* options); can this be const PaymentOptionsProvider& options? https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.h:40: PaymentOptionsProvider* options); same
The CQ bit was checked by tmartino@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 tmartino@chromium.org
The CQ bit was checked by tmartino@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 ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Include unit-tests. I will add these to this CL, but I wanted some feedback on structure, etc. beforehand. - Backport this logic to Android in a forthcoming CL - Consider filtering shipping addresses BUG=706117 ========== to ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Backport this logic to Android in a forthcoming CL - Consider filtering shipping addresses BUG=706117 ==========
Added tests! PTAL. https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... File components/payments/core/profile_util.cc (right): https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.cc:96: autofill::AutofillProfile* profile) { On 2017/04/03 at 19:11:53, Mathieu wrote: > const ref? I thought about it, but considering the need to use pointers elsewhere in the file (and its callers), I think internal consistency is more desirable here. https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... File components/payments/core/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.h:13: // Utility functions used for processing and filtering profiles. On 2017/04/03 at 19:11:53, Mathieu wrote: > *address profiles (AutofillProfile) Done https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.h:34: PaymentOptionsProvider* options); On 2017/04/03 at 19:11:53, Mathieu wrote: > can this be const PaymentOptionsProvider& options? I'm open to it. However, my search found lots of examples of PaymentRequestSpec (the first subclass of Provider) being passed around as a pointer, and no examples of it being passed as a const-ref. My gut said it was more important to be consistent with that usage. Another option might be to make this a const pointer. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Small comments :) nice tests! https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... File components/payments/core/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... components/payments/core/profile_util.h:53: // Returns true every contact field requested in |options| is nonempty in missing an "if" in the sentence. https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... File components/payments/core/profile_util_unittest.cc (right): https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... components/payments/core/profile_util_unittest.cc:186: Can you add one for where the profile only has either name or phone missing, so that it has a score of one, and one where it has only the email, so a score of 0? Thanks! https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... components/payments/core/profile_util_unittest.cc:190: Can you also add a test for IsContactInfoComplete? thanks!
https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... File components/payments/core/profile_util.cc (right): https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.cc:96: autofill::AutofillProfile* profile) { On 2017/04/04 21:00:22, tmartino wrote: > On 2017/04/03 at 19:11:53, Mathieu wrote: > > const ref? > > I thought about it, but considering the need to use pointers elsewhere in the > file (and its callers), I think internal consistency is more desirable here. I think as much as possible we should use const ref. It makes it an explicit contract with the callers that we are not going to modify the object being passed in. Just by looking at the signature we can know that PaymentsProfileComparator will never modify AutofillProfiles. It's fine if the callers (say PaymentRequestState) handle a pointer or a vector of pointers. When using this class, it can simply do GetContactCompletenessScore(*profile) and so forth. https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... File components/payments/core/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/100001/components/payments/co... components/payments/core/profile_util.h:34: PaymentOptionsProvider* options); On 2017/04/04 21:00:22, tmartino wrote: > On 2017/04/03 at 19:11:53, Mathieu wrote: > > can this be const PaymentOptionsProvider& options? > > I'm open to it. However, my search found lots of examples of PaymentRequestSpec > (the first subclass of Provider) being passed around as a pointer, and no > examples of it being passed as a const-ref. My gut said it was more important to > be consistent with that usage. Another option might be to make this a const > pointer. WDYT? The difference is that there are non-const methods on PaymentRequestSpec, whereas PaymentOptionsProvider ideally would only have const methods (it does!).
re: mathp's comments on constref, I'll just reply in one place. I went back and changed: - the PaymentOptionsProvider to be passed by constref - IsContactEqualOrSuperset to take constrefs I did not change: - IsContactMoreComplete, because it needs to accept pointers to match the std::sort interface - GetContactCompletenessScore or IsContactInfoComplete, because we want them to correctly handle the null case However, in those latter cases, I did change the interface to use pointer-to-const. https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... File components/payments/core/profile_util.h (right): https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... components/payments/core/profile_util.h:53: // Returns true every contact field requested in |options| is nonempty in On 2017/04/04 at 22:17:17, sebsg wrote: > missing an "if" in the sentence. Done https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... File components/payments/core/profile_util_unittest.cc (right): https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... components/payments/core/profile_util_unittest.cc:186: On 2017/04/04 at 22:17:17, sebsg wrote: > Can you add one for where the profile only has either name or phone missing, so that it has a score of one, and one where it has only the email, so a score of 0? > > Thanks! Done https://codereview.chromium.org/2775553004/diff/140001/components/payments/co... components/payments/core/profile_util_unittest.cc:190: On 2017/04/04 at 22:17:17, sebsg wrote: > Can you also add a test for IsContactInfoComplete? thanks! Done
The CQ bit was checked by tmartino@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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm, very cool tests!
LGTM, Ship it! :)
The CQ bit was checked by tmartino@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.
The CQ bit was checked by tmartino@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/2775553004/#ps180001 (title: "rebasing")
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": 180001, "attempt_start_ts": 1491508229156170,
"parent_rev": "235452ff9ef544dde26e1b8b3609cbda3bed5709", "commit_rev":
"a6eb22f51aa4a79ecfd8e61527280658aef1e089"}
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491508229156170,
"parent_rev": "235452ff9ef544dde26e1b8b3609cbda3bed5709", "commit_rev":
"a6eb22f51aa4a79ecfd8e61527280658aef1e089"}
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Backport this logic to Android in a forthcoming CL - Consider filtering shipping addresses BUG=706117 ========== to ========== [WebPayments] Implementing Profile filter and dedupe logic in components Still to-do: - Backport this logic to Android in a forthcoming CL - Consider filtering shipping addresses BUG=706117 Review-Url: https://codereview.chromium.org/2775553004 Cr-Commit-Position: refs/heads/master@{#462603} Committed: https://chromium.googlesource.com/chromium/src/+/a6eb22f51aa4a79ecfd8e6152728... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a6eb22f51aa4a79ecfd8e6152728... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
