|
|
Created:
3 years, 7 months ago by tmartino Modified:
3 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, gogerald+paymentswatch_chromium.org, jam, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[WebPayments] Adding FilterProfilesForShipping to profile comparator
BUG=722949
Review-Url: https://codereview.chromium.org/2884393002
Cr-Original-Commit-Position: refs/heads/master@{#473929}
Committed: https://chromium.googlesource.com/chromium/src/+/eb8d574b98adfcbd3b5ce2eb6e521b83efbb428a
Review-Url: https://codereview.chromium.org/2884393002
Cr-Commit-Position: refs/heads/master@{#474392}
Committed: https://chromium.googlesource.com/chromium/src/+/cd131b3025061c950f9e406129d5703532f153be
Patch Set 1 #
Total comments: 8
Patch Set 2 : mathp feedback #Patch Set 3 : adding browsertest #
Total comments: 16
Patch Set 4 : further feedback #
Total comments: 2
Patch Set 5 : test comments #Patch Set 6 : fix flaky #Patch Set 7 : rebase #Messages
Total messages: 49 (37 generated)
Description was changed from ========== [WebPayments] Adding FilterProfilesForShipping to profile comparator BUG= ========== to ========== [WebPayments] Adding FilterProfilesForShipping to profile comparator BUG=722949 ==========
The CQ bit was checked by tmartino@google.com 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...
tmartino@chromium.org changed reviewers: + mathp@chromium.org - tmartino@google.com
Looks great, just a few things. Also, can we have a browsertest that adds two addresses, one used more than the other but also not complete, and a complete address, and test that the latter one is selected in the UI? Having this test will help us catch regressions later. https://codereview.chromium.org/2884393002/diff/1/components/payments/content... File components/payments/content/payment_request_state.cc (right): https://codereview.chromium.org/2884393002/diff/1/components/payments/content... components/payments/content/payment_request_state.cc:243: shipping_profiles_ = profile_comparator()->FilterProfilesForShipping( can you also call this in payment_request.mm (https://cs.chromium.org/chromium/src/ios/chrome/browser/payments/payment_requ...) and address the TODO on line 139 over there? Thanks https://codereview.chromium.org/2884393002/diff/1/components/payments/content... components/payments/content/payment_request_state.cc:261: for (autofill::AutofillProfile* profile : shipping_profiles_) { can you remove this code? I think we should be good with line 260 now? https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... File components/payments/core/payments_profile_comparator.cc (right): https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... components/payments/core/payments_profile_comparator.cc:161: // TODO(tmartino): Remove profiles with no relevant information, or which TODO(crbug.com/xxxxxx) please https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... File components/payments/core/payments_profile_comparator.h (right): https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... components/payments/core/payments_profile_comparator.h:85: int GetShippingCompletenessScore( can some of these functions be private?
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 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...
tmartino@chromium.org changed reviewers: + mahmadi@chromium.org
Resolved comments, added iOS code, added browser test. PTAL https://codereview.chromium.org/2884393002/diff/1/components/payments/content... File components/payments/content/payment_request_state.cc (right): https://codereview.chromium.org/2884393002/diff/1/components/payments/content... components/payments/content/payment_request_state.cc:243: shipping_profiles_ = profile_comparator()->FilterProfilesForShipping( On 2017/05/16 at 20:16:51, Mathieu wrote: > can you also call this in payment_request.mm (https://cs.chromium.org/chromium/src/ios/chrome/browser/payments/payment_requ...) and address the TODO on line 139 over there? > > Thanks Done https://codereview.chromium.org/2884393002/diff/1/components/payments/content... components/payments/content/payment_request_state.cc:261: for (autofill::AutofillProfile* profile : shipping_profiles_) { On 2017/05/16 at 20:16:51, Mathieu wrote: > can you remove this code? I think we should be good with line 260 now? Done https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... File components/payments/core/payments_profile_comparator.cc (right): https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... components/payments/core/payments_profile_comparator.cc:161: // TODO(tmartino): Remove profiles with no relevant information, or which On 2017/05/16 at 20:16:51, Mathieu wrote: > TODO(crbug.com/xxxxxx) please Done https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... File components/payments/core/payments_profile_comparator.h (right): https://codereview.chromium.org/2884393002/diff/1/components/payments/core/pa... components/payments/core/payments_profile_comparator.h:85: int GetShippingCompletenessScore( On 2017/05/16 at 20:16:51, Mathieu wrote: > can some of these functions be private? FilterProfilesForShipping: used by State GetShippingCompletenessScore: public for testing IsShippingComplete: used by State IsShippingMoreComplete: Yes, done GetStringForMissingContactFields: Used by UI Same applies to Contact equivalents.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This is cool! One remaining question on desktop and iOS https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc (right): https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:16: autofill::AutofillProfile CreateProfileWithCompleteAddress() { Could we use autofill::test::GetFullProfile? https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:26: autofill::AutofillProfile profile(base::GenerateGUID(), could use GetFullProfile and remove things https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:40: // PersonalDataLoadedObserverMock personal_data_observer_; fix https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:44: autofill::PersonalDataManager* personal_data_manager = GetDataManager(); bring closer to where it's used https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:45: // personal_data_manager->AddObserver(&personal_data_observer_); fix https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... File components/payments/content/payment_request_state.cc (right): https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:7: #include <algorithm> was that for std::transform? https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:264: // shipping option, and the top profile is complete. Assumes that profiles Did we check with Zach/UX? Perhaps we should select the first one even if it's not complete (it will appear select with a string saying what's missing, I think)? https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:274: profile_comparator()->IsContactInfoComplete(contact_profiles_[0])) same question here
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...
PTAL https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc (right): https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:16: autofill::AutofillProfile CreateProfileWithCompleteAddress() { On 2017/05/19 at 01:10:41, Mathieu wrote: > Could we use autofill::test::GetFullProfile? Yup, done https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:26: autofill::AutofillProfile profile(base::GenerateGUID(), On 2017/05/19 at 01:10:40, Mathieu wrote: > could use GetFullProfile and remove things Done https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:40: // PersonalDataLoadedObserverMock personal_data_observer_; On 2017/05/19 at 01:10:40, Mathieu wrote: > fix Done https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:44: autofill::PersonalDataManager* personal_data_manager = GetDataManager(); On 2017/05/19 at 01:10:41, Mathieu wrote: > bring closer to where it's used Done https://codereview.chromium.org/2884393002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/profile_list_view_controller_browsertest.cc:45: // personal_data_manager->AddObserver(&personal_data_observer_); On 2017/05/19 at 01:10:40, Mathieu wrote: > fix Done https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... File components/payments/content/payment_request_state.cc (right): https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:7: #include <algorithm> On 2017/05/19 at 01:10:41, Mathieu wrote: > was that for std::transform? std::find_if is still used https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:264: // shipping option, and the top profile is complete. Assumes that profiles On 2017/05/19 at 01:10:41, Mathieu wrote: > Did we check with Zach/UX? Perhaps we should select the first one even if it's not complete (it will appear select with a string saying what's missing, I think)? Just checked. This is preferred, so that the "preview" data (e.g., "1253 McGill Avenue... and 2 others") will appear. https://codereview.chromium.org/2884393002/diff/40001/components/payments/con... components/payments/content/payment_request_state.cc:274: profile_comparator()->IsContactInfoComplete(contact_profiles_[0])) On 2017/05/19 at 01:10:41, Mathieu wrote: > same question here ack
lgtm https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc (left): https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc:240: EXPECT_EQ(request->state()->contact_profiles().front(), // No contact profiles are selected because both are incomplete. EXPECT_EQ(nullptr, request->state()->selected_contact_profile()); https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc (right): https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc:269: // Expect the now-complete profile to be selected.
On 2017/05/19 18:31:00, Mathieu wrote: > lgtm > > https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... > File > chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc > (left): > > https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc:240: > EXPECT_EQ(request->state()->contact_profiles().front(), > // No contact profiles are selected because both are incomplete. > EXPECT_EQ(nullptr, request->state()->selected_contact_profile()); > > https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... > File > chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc > (right): > > https://codereview.chromium.org/2884393002/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc:269: > > // Expect the now-complete profile to be selected. ios/chrome/browser/payments/payment_request.mm lgtm
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 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, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2884393002/#ps80001 (title: "test comments")
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": 80001, "attempt_start_ts": 1495556179763190, "parent_rev": "1110a9e45de75f356e90f1568f6353240432cdcd", "commit_rev": "eb8d574b98adfcbd3b5ce2eb6e521b83efbb428a"}
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Adding FilterProfilesForShipping to profile comparator BUG=722949 ========== to ========== [WebPayments] Adding FilterProfilesForShipping to profile comparator BUG=722949 Review-Url: https://codereview.chromium.org/2884393002 Cr-Commit-Position: refs/heads/master@{#473929} Committed: https://chromium.googlesource.com/chromium/src/+/eb8d574b98adfcbd3b5ce2eb6e52... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/eb8d574b98adfcbd3b5ce2eb6e52...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2897133002/ by mek@chromium.org. The reason for reverting is: Appears to be causing test failures in https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin10_Tes... RUN ] PaymentRequestProfileListTest.PrioritizeCompleteness [640:3644:0523/102806.488:WARNING:embedded_test_server.cc(219)] Request not handled. Returning 404: /favicon.ico c:\b\c\b\win\src\chrome\browser\ui\views\payments\profile_list_view_controller_browsertest.cc(46): error: Expected: partial Which is: b5e544d1-6a66-49bf-bd4c-f2eb38671fd3 https://www.example.com/ Jane A. Smith jsmith@example.com ACME 48838 US 13105557889 To be equal to: *profiles[0] Which is: 63b73e74-aacb-4a0e-bde5-a3260918586c http://www.example.com/ John H. Doe johndoe@hades.com Underworld 666 Erebus St. Apt 8 Elysium CA 91111 US 16502111111 c:\b\c\b\win\src\chrome\browser\ui\views\payments\profile_list_view_controller_browsertest.cc(47): error: Expected: complete Which is: 63b73e74-aacb-4a0e-bde5-a3260918586c http://www.example.com/ John H. Doe johndoe@hades.com Underworld 666 Erebus St. Apt 8 Elysium CA 91111 US 16502111111 To be equal to: *profiles[1] Which is: b5e544d1-6a66-49bf-bd4c-f2eb38671fd3 https://www.example.com/ Jane A. Smith jsmith@example.com ACME 48838 US 13105557889 [ FAILED ] PaymentRequestProfileListTest.PrioritizeCompleteness, where TypeParam = and GetParam() = (1292 ms).
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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_chromeos_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_...) linux_chromium_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...
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, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2884393002/#ps120001 (title: "rebase")
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": 120001, "attempt_start_ts": 1495654502487950, "parent_rev": "571ab4cdcf663497ece493e2139a51ddf7283ae7", "commit_rev": "cd131b3025061c950f9e406129d5703532f153be"}
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Adding FilterProfilesForShipping to profile comparator BUG=722949 Review-Url: https://codereview.chromium.org/2884393002 Cr-Commit-Position: refs/heads/master@{#473929} Committed: https://chromium.googlesource.com/chromium/src/+/eb8d574b98adfcbd3b5ce2eb6e52... ========== to ========== [WebPayments] Adding FilterProfilesForShipping to profile comparator BUG=722949 Review-Url: https://codereview.chromium.org/2884393002 Cr-Original-Commit-Position: refs/heads/master@{#473929} Committed: https://chromium.googlesource.com/chromium/src/+/eb8d574b98adfcbd3b5ce2eb6e52... Review-Url: https://codereview.chromium.org/2884393002 Cr-Commit-Position: refs/heads/master@{#474392} Committed: https://chromium.googlesource.com/chromium/src/+/cd131b3025061c950f9e406129d5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/cd131b3025061c950f9e406129d5... |