Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2734463004:
[Payments] Update server card billing if the address has already converted (Closed)
|
Created:
3 years, 9 months ago by sebsg Modified:
3 years, 9 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Update server card billing if the address has already converted
Before this patch, if you added a new server card where the billing
address was already converted (with an older card for example), the
billing address id for the new card was not updated.
With this patch, these cards will be updated too.
For server cards with a billing address that was already converted,
look for a local profile that matches it and update the id in the card
BUG=698364
Review-Url: https://codereview.chromium.org/2734463004
Cr-Commit-Position: refs/heads/master@{#454976}
Committed: https://chromium.googlesource.com/chromium/src/+/1eeee730395362b15e304fa65fa36c5a0bc5e204
Patch Set 1 #Patch Set 2 : Refactored #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 2
Messages
Total messages: 24 (17 generated)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #2 (id:20001) 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...
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Payments] Update server card billing if the address has already convert Before this patch, if you added a new server card where the billing address was already converted (with an older card for example), the billing address id for the new card was not updated. With this patch, these cards will be updated too. For server cards with a billing address that was already converted, look for a local profile that matches it and update the id in the card BUG=698364 ========== to ========== [Payments] Update server card billing if the address has already converted Before this patch, if you added a new server card where the billing address was already converted (with an older card for example), the billing address id for the new card was not updated. With this patch, these cards will be updated too. For server cards with a billing address that was already converted, look for a local profile that matches it and update the id in the card BUG=698364 ==========
lgtm https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:58: const int LOCAL_GUID_LENGTH = 36; comment? https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1251: // If new data was synced, try to convert new server profiles. update comment https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1867: // address and we will need to access them by guid later to update the Wallet *addresses https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1869: std::unordered_map<std::string, AutofillProfile*> server_profiles_map; |server_id_profile_map| is clearer? https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1901: server_profiles_map->insert(std::pair<std::string, AutofillProfile*>( std::make_pair?
Patchset #3 (id:60001) has been deleted
Thanks for the comments, sending to CQ https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:58: const int LOCAL_GUID_LENGTH = 36; On 2017/03/06 20:40:34, Mathieu Perreault wrote: > comment? Done. https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1251: // If new data was synced, try to convert new server profiles. On 2017/03/06 20:40:34, Mathieu Perreault wrote: > update comment Done. https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1867: // address and we will need to access them by guid later to update the Wallet On 2017/03/06 20:40:34, Mathieu Perreault wrote: > *addresses Done. https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1869: std::unordered_map<std::string, AutofillProfile*> server_profiles_map; On 2017/03/06 20:40:34, Mathieu Perreault wrote: > |server_id_profile_map| is clearer? Done. https://codereview.chromium.org/2734463004/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1901: server_profiles_map->insert(std::pair<std::string, AutofillProfile*>( On 2017/03/06 20:40:34, Mathieu Perreault wrote: > std::make_pair? Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2734463004/#ps80001 (title: "Addressed 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": 1488835608624060,
"parent_rev": "2eabcd9262347c01e1c5d8366891a8435420276d", "commit_rev":
"1eeee730395362b15e304fa65fa36c5a0bc5e204"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Update server card billing if the address has already converted Before this patch, if you added a new server card where the billing address was already converted (with an older card for example), the billing address id for the new card was not updated. With this patch, these cards will be updated too. For server cards with a billing address that was already converted, look for a local profile that matches it and update the id in the card BUG=698364 ========== to ========== [Payments] Update server card billing if the address has already converted Before this patch, if you added a new server card where the billing address was already converted (with an older card for example), the billing address id for the new card was not updated. With this patch, these cards will be updated too. For server cards with a billing address that was already converted, look for a local profile that matches it and update the id in the card BUG=698364 Review-Url: https://codereview.chromium.org/2734463004 Cr-Commit-Position: refs/heads/master@{#454976} Committed: https://chromium.googlesource.com/chromium/src/+/1eeee730395362b15e304fa65fa3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1eeee730395362b15e304fa65fa3...
Message was sent while issue was closed.
twellington@chromium.org changed reviewers: + twellington@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2734463004/diff/80001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2734463004/diff/80001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1951: DCHECK(it != server_id_profiles_map->end()); I synced this morning and now I'm getting crashes on this DCHECK: [FATAL:personal_data_manager.cc(1955)] Check failed: it != server_id_profiles_map->end(). /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:1955 00951be5 ConvertWalletAddressesAndUpdateWalletCards /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:1883 0022897b NotifyPersonalDataChanged /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:1260 0022840d OnWebDataServiceRequestDone /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:410 00004659 RequestCompletedOnThread /work/clankium/src/components/webdata/common/web_data_request_manager.cc:177 00004eff Invoke<const scoped_refptr<WebDataRequestManager> &, std::__ndk1::unique_ptr<WebDataRequest, std::__ndk1::default_delete<WebDataRequest> >, std::__ndk1::unique_ptr<WDTypedResult, std::__ndk1::default_delete<WDTypedResult> > > /work/clankium/src/base/bind_internal.h:214 v------> MakeItSo<void (WebDataRequestManager::*const &)(std::__ndk1::unique_ptr<WebDataRequest, std::__ndk1::default_delete<WebDataRequest> >, std::__ndk1::unique_ptr<WDTypedResult, std::__ndk1::default_delete<WDTypedResult> >), const scoped_refptr<WebDataRequestManager> &, std::__ndk1::unique_ptr<WebDataRequest, std::__ndk1::default_delete<WebDataRequest> >, std::__ndk1::unique_ptr<WDTypedResult, std::__ndk1::default_delete<WDTypedResult> > > /work/clankium/src/base/bind_internal.h:285 v------> RunImpl<void (WebDataRequestManager::*const &)(std::__ndk1::unique_ptr<WebDataRequest, std::__ndk1::default_delete<WebDataRequest> >, std::__ndk1::unique_ptr<WDTypedResult, std::__ndk1::default_delete<WDTypedResult> >), const std::__ndk1::tuple<scoped_refptr<WebDataRequestManager>, base::internal::PassedWrapper<std::__ndk1::unique_ptr<WebDataRequest, std::__ndk1::default_delete<WebDataRequest> > >, base::internal::PassedWrapper<std::__ndk1::unique_ptr<WDTypedResult, std::__ndk1::default_delete<WDTypedResult> > > > &, 0, 1, 2> /work/clankium/src/base/bind_internal.h:361 00004e9f Run /work/clankium/src/base/bind_internal.h:339 0008d523 Run /work/clankium/src/base/callback.h:68 0008d409 RunTask /work/clankium/src/base/debug/task_annotator.cc:59 000a7f61 RunTask /work/clankium/src/base/message_loop/message_loop.cc:423 000a81bf DeferOrRunPendingTask /work/clankium/src/base/message_loop/message_loop.cc:434 000a8389 DoWork /work/clankium/src/base/message_loop/message_loop.cc:527 v------> DoRunLoopOnce /work/clankium/src/base/message_loop/message_pump_android.cc:44 000a94d1 Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce /work/clankium/src/out-gn/Debug/gen/base/base_jni_headers/base/jni/SystemMessageHandler_jni.h:44 000161bd offset 0x36000 /data/data/com.google.android.apps.chrome/incremental-install-files/optimized-dexes/base.base_java.dex.dex
Message was sent while issue was closed.
https://codereview.chromium.org/2734463004/diff/80001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2734463004/diff/80001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1951: DCHECK(it != server_id_profiles_map->end()); On 2017/03/07 21:58:17, Theresa wrote: > I synced this morning and now I'm getting crashes on this DCHECK: > > [FATAL:personal_data_manager.cc(1955)] Check failed: it != > server_id_profiles_map->end(). > > > /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:1955 > 00951be5 ConvertWalletAddressesAndUpdateWalletCards > > > > > > > /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:1883 > 0022897b NotifyPersonalDataChanged > > > > > > > /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:1260 > 0022840d OnWebDataServiceRequestDone > > > > > > > /work/clankium/src/components/autofill/core/browser/personal_data_manager.cc:410 > 00004659 RequestCompletedOnThread > > > > > > > /work/clankium/src/components/webdata/common/web_data_request_manager.cc:177 > 00004eff Invoke<const scoped_refptr<WebDataRequestManager> &, > std::__ndk1::unique_ptr<WebDataRequest, > std::__ndk1::default_delete<WebDataRequest> >, > std::__ndk1::unique_ptr<WDTypedResult, > std::__ndk1::default_delete<WDTypedResult> > > > > > > /work/clankium/src/base/bind_internal.h:214 > v------> MakeItSo<void (WebDataRequestManager::*const > &)(std::__ndk1::unique_ptr<WebDataRequest, > std::__ndk1::default_delete<WebDataRequest> >, > std::__ndk1::unique_ptr<WDTypedResult, > std::__ndk1::default_delete<WDTypedResult> >), const > scoped_refptr<WebDataRequestManager> &, std::__ndk1::unique_ptr<WebDataRequest, > std::__ndk1::default_delete<WebDataRequest> >, > std::__ndk1::unique_ptr<WDTypedResult, > std::__ndk1::default_delete<WDTypedResult> > > > > /work/clankium/src/base/bind_internal.h:285 > v------> RunImpl<void (WebDataRequestManager::*const > &)(std::__ndk1::unique_ptr<WebDataRequest, > std::__ndk1::default_delete<WebDataRequest> >, > std::__ndk1::unique_ptr<WDTypedResult, > std::__ndk1::default_delete<WDTypedResult> >), const > std::__ndk1::tuple<scoped_refptr<WebDataRequestManager>, > base::internal::PassedWrapper<std::__ndk1::unique_ptr<WebDataRequest, > std::__ndk1::default_delete<WebDataRequest> > >, > base::internal::PassedWrapper<std::__ndk1::unique_ptr<WDTypedResult, > std::__ndk1::default_delete<WDTypedResult> > > > &, 0, 1, 2> > /work/clankium/src/base/bind_internal.h:361 > 00004e9f Run > > > > > > > /work/clankium/src/base/bind_internal.h:339 > 0008d523 Run > > > > > > > /work/clankium/src/base/callback.h:68 > 0008d409 RunTask > > > > > > > /work/clankium/src/base/debug/task_annotator.cc:59 > 000a7f61 RunTask > > > > > > > /work/clankium/src/base/message_loop/message_loop.cc:423 > 000a81bf DeferOrRunPendingTask > > > > > > > /work/clankium/src/base/message_loop/message_loop.cc:434 > 000a8389 DoWork > > > > > > > /work/clankium/src/base/message_loop/message_loop.cc:527 > v------> DoRunLoopOnce > > > > > > > /work/clankium/src/base/message_loop/message_pump_android.cc:44 > 000a94d1 Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce > > > > > > > /work/clankium/src/out-gn/Debug/gen/base/base_jni_headers/base/jni/SystemMessageHandler_jni.h:44 > 000161bd offset 0x36000 > > > > > > > /data/data/com.google.android.apps.chrome/incremental-install-files/optimized-dexes/base.base_java.dex.dex > I am very curious on how it could trigger. Can we move the discussion to crbug.com/698364 ? I would like to try to repro |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
