Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2619423002:
[Payments] Preserve Wallet's card to billing address link after sync. (Closed)
|
Created:
3 years, 11 months ago by sebsg Modified:
3 years, 11 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, rouslan+autofill_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] Preserve Wallet's card to billing address link after sync.
Uses the relationship between a card and its billing address pulled
from sync to populate the billing_address_id of the generated
CreditCard object.
Overwrites the billing_address_id saved on disk only if it is not 36
characters long, which would represent a local profile. This only
happens if the user manually associated the Card to a local Autofill
Profile in PaymentRequest.
BUG=679466
Review-Url: https://codereview.chromium.org/2619423002
Cr-Commit-Position: refs/heads/master@{#442610}
Committed: https://chromium.googlesource.com/chromium/src/+/d46ba3e386640479b40c62ce9e7e53d61f4baa65
Patch Set 1 #
Total comments: 30
Patch Set 2 : Addressed comments #
Total comments: 2
Patch Set 3 : Fixed comment #
Messages
Total messages: 30 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
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...
Description was changed from ========== [Payments] Preserve Wallet's card to billing address link after sync. BUG=679466 ========== to ========== [Payments] Preserve Wallet's card to billing address link after sync. Uses the relationship between a card and its billing address pulled from sync to populate the billing_address_id of the generated CreditCard object. Overwrites the billing_address_id saved on disk only if it is not 36 characters long, which would represent a local profile. This only happens if the user manually associated the Card to a local Autofill Profile in PaymentRequest. BUG=679466 ==========
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL Thanks!
Description was changed from ========== [Payments] Preserve Wallet's card to billing address link after sync. Uses the relationship between a card and its billing address pulled from sync to populate the billing_address_id of the generated CreditCard object. Overwrites the billing_address_id saved on disk only if it is not 36 characters long, which would represent a local profile. This only happens if the user manually associated the Card to a local Autofill Profile in PaymentRequest. BUG=679466 ========== to ========== [Payments] Preserve Wallet's card to billing address link after sync. Uses the relationship between a card and its billing address pulled from sync to populate the billing_address_id of the generated CreditCard object. Overwrites the billing_address_id saved on disk only if it is not 36 characters long, which would represent a local profile. This only happens if the user manually associated the Card to a local Autofill Profile in PaymentRequest. BUG=679466 ==========
Good stuff! https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc (right): https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:24: const int kLocalGuidSize = 36; Comment. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:77: result.set_billing_address_id(card.billing_address_id()); Update the definition of billing_address_id to include this format as well: https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata... https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:121: std::string GetAddressId(const sync_pb::WalletPostalAddress& address) { Seems wasteful to call GetAddressId() with 14 characters and +3 lines for this function definition instead of calling .id() with 4 characters and no extra function. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:289: if (it != ids.end()) { nit: no need for {} https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:290: card.set_billing_address_id(it->second); Update the definition of the billing_address_id to include a server profile's server_id as well: https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata... https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h (right): https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:13: #include "components/autofill/core/browser/webdata/autofill_table.h" Forward declare AutofillTable instead. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:60: static void PopulateWalletCardsAndAddresses( Move to private and use FRIEND_TEST_ALL_PREFIXES() macro to let the tests access it. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:63: const syncer::SyncDataList& data_list); Input (data_list) should appear before output (wallet_cads, wallet_addresses) parameters. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:69: static void CopyRelevantBillingAddressesFromDisk( Ditto https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:70: AutofillTable* table, const-ref? You can put a "const" on AutofillTable::GetServerCreditCards() IMHO. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc (right): https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:19: std::string id, Here and elsewhere in this test: const char* or const-ref https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:53: TestAutofillTable(std::vector<CreditCard> cards_on_disk) explicit Also need to override the destructor. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:58: for (auto card_on_disk : cards_on_disk_) { const auto& https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:60: std::unique_ptr<CreditCard>{new CreditCard{card_on_disk}}); cards->push_back(base::MakeUnique(card_on_disk)); (should do the trick) https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:66: }; private: DISALLOW_COPY_AND_ASSIGN
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 sebsg@chromium.org to run a CQ dry run
Patchset #2 (id:60001) 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 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 #2 (id:80001) has been deleted
Thanks! Another look? https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc (right): https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:24: const int kLocalGuidSize = 36; On 2017/01/09 22:28:16, rouslan wrote: > Comment. Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:77: result.set_billing_address_id(card.billing_address_id()); On 2017/01/09 22:28:16, rouslan wrote: > Update the definition of billing_address_id to include this format as well: > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata... Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:121: std::string GetAddressId(const sync_pb::WalletPostalAddress& address) { On 2017/01/09 22:28:16, rouslan wrote: > Seems wasteful to call GetAddressId() with 14 characters and +3 lines for this > function definition instead of calling .id() with 4 characters and no extra > function. Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:289: if (it != ids.end()) { On 2017/01/09 22:28:16, rouslan wrote: > nit: no need for {} Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc:290: card.set_billing_address_id(it->second); On 2017/01/09 22:28:16, rouslan wrote: > Update the definition of the billing_address_id to include a server profile's > server_id as well: > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata... Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h (right): https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:13: #include "components/autofill/core/browser/webdata/autofill_table.h" On 2017/01/09 22:28:16, rouslan wrote: > Forward declare AutofillTable instead. Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:60: static void PopulateWalletCardsAndAddresses( On 2017/01/09 22:28:16, rouslan wrote: > Move to private and use FRIEND_TEST_ALL_PREFIXES() macro to let the tests access > it. Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:63: const syncer::SyncDataList& data_list); On 2017/01/09 22:28:16, rouslan wrote: > Input (data_list) should appear before output (wallet_cads, wallet_addresses) > parameters. Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:69: static void CopyRelevantBillingAddressesFromDisk( On 2017/01/09 22:28:16, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service.h:70: AutofillTable* table, On 2017/01/09 22:28:16, rouslan wrote: > const-ref? You can put a "const" on AutofillTable::GetServerCreditCards() IMHO. Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc (right): https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:19: std::string id, On 2017/01/09 22:28:16, rouslan wrote: > Here and elsewhere in this test: > > const char* or const-ref Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:53: TestAutofillTable(std::vector<CreditCard> cards_on_disk) On 2017/01/09 22:28:16, rouslan wrote: > explicit > > Also need to override the destructor. Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:58: for (auto card_on_disk : cards_on_disk_) { On 2017/01/09 22:28:16, rouslan wrote: > const auto& Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:60: std::unique_ptr<CreditCard>{new CreditCard{card_on_disk}}); On 2017/01/09 22:28:16, rouslan wrote: > cards->push_back(base::MakeUnique(card_on_disk)); > > > (should do the trick) Done. https://codereview.chromium.org/2619423002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_syncable_service_unittest.cc:66: }; On 2017/01/09 22:28:16, rouslan wrote: > private: > DISALLOW_COPY_AND_ASSIGN Done.
lgtm https://codereview.chromium.org/2619423002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2619423002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.h:170: // billing_address_id The guid string that identifies the local or server Remove the word "guid", because server profiles don't use their guids for identification.
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: This issue passed the CQ dry run.
Thanks! sending to CQ https://codereview.chromium.org/2619423002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2619423002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.h:170: // billing_address_id The guid string that identifies the local or server On 2017/01/10 15:24:21, rouslan wrote: > Remove the word "guid", because server profiles don't use their guids for > identification. Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2619423002/#ps120001 (title: "Fixed comment")
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": 1484065910093690,
"parent_rev": "caa5c484578fa649c62a44e90a08f921eae03e96", "commit_rev":
"d46ba3e386640479b40c62ce9e7e53d61f4baa65"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Preserve Wallet's card to billing address link after sync. Uses the relationship between a card and its billing address pulled from sync to populate the billing_address_id of the generated CreditCard object. Overwrites the billing_address_id saved on disk only if it is not 36 characters long, which would represent a local profile. This only happens if the user manually associated the Card to a local Autofill Profile in PaymentRequest. BUG=679466 ========== to ========== [Payments] Preserve Wallet's card to billing address link after sync. Uses the relationship between a card and its billing address pulled from sync to populate the billing_address_id of the generated CreditCard object. Overwrites the billing_address_id saved on disk only if it is not 36 characters long, which would represent a local profile. This only happens if the user manually associated the Card to a local Autofill Profile in PaymentRequest. BUG=679466 Review-Url: https://codereview.chromium.org/2619423002 Cr-Commit-Position: refs/heads/master@{#442610} Committed: https://chromium.googlesource.com/chromium/src/+/d46ba3e386640479b40c62ce9e7e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d46ba3e386640479b40c62ce9e7e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
