Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626263005:
[Autofill] Sync new wallet metadata fields through Chrome Sync. (Closed)
|
Created:
3 years, 11 months ago by sebsg Modified:
3 years, 11 months ago 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, sync-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Sync new wallet metadata fields through Chrome Sync.
Sync the new has_converted field for wallet addresses metadata and the
new billing_address_id field for the wallet card metadata through
Chrome Sync.
BUG=681131
Review-Url: https://codereview.chromium.org/2626263005
Cr-Commit-Position: refs/heads/master@{#444423}
Committed: https://chromium.googlesource.com/chromium/src/+/ca7d5cf7a234e2f4d085b3be0781bfd0765c0130
Patch Set 1 : Add tests #
Total comments: 12
Patch Set 2 : Addressed comments #Patch Set 3 : Merge fields individually #
Total comments: 6
Patch Set 4 : Addressed comments #
Messages
Total messages: 33 (18 generated)
Description was changed from ========== [Autofill] Sync new wallet metadata fields through Chrome Sync. BUG=681131 ========== to ========== [Autofill] Sync new wallet metadata fields through Chrome Sync. Sync the new has_converted field for wallet addresses metadata and the new billing_address_id field for the wallet card metadata through Chrome Sync. BUG=681131 ==========
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 #1 (id:1) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, could you please take a look?
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
Description was changed from ========== [Autofill] Sync new wallet metadata fields through Chrome Sync. Sync the new has_converted field for wallet addresses metadata and the new billing_address_id field for the wallet card metadata through Chrome Sync. BUG=681131 ========== to ========== [Autofill] Sync new wallet metadata fields through Chrome Sync. Sync the new has_converted field for wallet addresses metadata and the new billing_address_id field for the wallet card metadata through Chrome Sync. BUG=681131 ==========
Forgot to mention this is still slightly WIP for the merging logic. I'm thinking about always checking the has_converted and give priority for profiles that have converted and then fall back to last_use_date. The rest should stay the same so your review is appreciated :) Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:246: has_converted_ = profile.has_converted(); You need to initialize this boolean in every constructor. Otherwise it will be random. https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (left): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:168: size_t remote_use_count = You appear to have wiped out the comparison of remote use count to local use count. Any good reason for that? https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:43: // Sets the basic syncable |metadata| for the |local_data_model|. s/SetBasicData/SetCommonMetadata https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:74: // The strings must be in valid UTF-8 to sync. I can't think of a situation when this string would not be utf8. If neither can you, then DCHECK() for utf-ness instead of re-encoding the data. https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:96: BuildSyncData(metadata_type, server_id, *local_metadata))); Does this method call one of the BuildSyncData() methods based on whether |local_metadata| is a profile a or a card? In that case you don't need two identical AutofillDataModelChanged methods.
Thanks Rouslan, another look? https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:246: has_converted_ = profile.has_converted(); On 2017/01/16 18:20:12, rouslan wrote: > You need to initialize this boolean in every constructor. Otherwise it will be > random. Seems I did too much Java recently... thanks! https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (left): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:168: size_t remote_use_count = On 2017/01/16 18:20:12, rouslan wrote: > You appear to have wiped out the comparison of remote use count to local use > count. Any good reason for that? Before, the value of each individual field was checked and the bigger number was kept. However, with the addition of the new fields this is no longer possible, because for guids and bools, the is no "bigger" version. Therefore I opted to keep the values of the data model with the most recent use_date. Like I mentioned in the CL description, I'm still unsure about this. I'm thinking of checking the has_converted first and always keep an address that was converted over and address that was not. If the values are the same, then fallback to the most recent use_date. WDYT? https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:43: // Sets the basic syncable |metadata| for the |local_data_model|. On 2017/01/16 18:20:12, rouslan wrote: > s/SetBasicData/SetCommonMetadata Done. https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:74: // The strings must be in valid UTF-8 to sync. On 2017/01/16 18:20:12, rouslan wrote: > I can't think of a situation when this string would not be utf8. If neither can > you, then DCHECK() for utf-ness instead of re-encoding the data. In the tests, non UTF-8 values are used as ids for the addresses and cards, so if I remove this the tests will crash. I'll ask zea@ if this is still relevant and remove it if it's not. https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:96: BuildSyncData(metadata_type, server_id, *local_metadata))); On 2017/01/16 18:20:12, rouslan wrote: > Does this method call one of the BuildSyncData() methods based on whether > |local_metadata| is a profile a or a card? In that case you don't need two > identical AutofillDataModelChanged methods. Done.
https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (left): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:168: size_t remote_use_count = On 2017/01/16 18:57:39, sebsg wrote: > On 2017/01/16 18:20:12, rouslan wrote: > > You appear to have wiped out the comparison of remote use count to local use > > count. Any good reason for that? > > Before, the value of each individual field was checked and the bigger number was > kept. However, with the addition of the new fields this is no longer possible, > because for guids and bools, the is no "bigger" version. Therefore I opted to > keep the values of the data model with the most recent use_date. > > Like I mentioned in the CL description, I'm still unsure about this. I'm > thinking of checking the has_converted first and always keep an address that was > converted over and address that was not. If the values are the same, then > fallback to the most recent use_date. WDYT? You should keep the "max" of every field. The "max" of "has_converted" is "true", because it starts from "false" and eventually switches to "true". It should never go back. With your current logic, the following flow does not work correctly: Computer A: 1) User sets the billing address of card. 2) Server address gets converted to local address. 3) User uses the card twice on 2017/01/01. Computer B: 1) Before sync happens, user uses the same card once on 2017/01/02. 2) Sync happens. With your code, the billing address and "has_converted" would be wiped, because Computer B has later use_date and did not yet convert the address or set the billing address of the card. Also, the use count would drop from 2 to 1. Therefore, the best approach is to check every field. You can use the use_date as the supplement for resolving conflicts between different billing address identifiers.
https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/2626263005/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:74: // The strings must be in valid UTF-8 to sync. On 2017/01/16 18:57:39, sebsg wrote: > On 2017/01/16 18:20:12, rouslan wrote: > > I can't think of a situation when this string would not be utf8. If neither > can > > you, then DCHECK() for utf-ness instead of re-encoding the data. > > In the tests, non UTF-8 values are used as ids for the addresses and cards, so > if I remove this the tests will crash. I'll ask zea@ if this is still relevant > and remove it if it's not. Ah, you're right. Server ID is not guaranteed to be unicode. Only local IDs are unicode.
Thanks Rouslan! Another look?
sebsg@chromium.org changed reviewers: + zea@chromium.org
Hi zea@ could you please review the changes in components/sync/* Thanks!
lgtm % comments https://codereview.chromium.org/2626263005/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/2626263005/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:159: void MergeBasicMetadata(const sync_pb::WalletMetadataSpecifics& remote_metadata, s/Basic/Common/ https://codereview.chromium.org/2626263005/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:597: template <class DataType> Is template required or can you use the parent class instead? https://codereview.chromium.org/2626263005/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:613: void AutofillWalletMetadataSyncableService::SendUpdateIfMoreRecent( No need for separate method.
lgtm
lgtm
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/2626263005/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/2626263005/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:159: void MergeBasicMetadata(const sync_pb::WalletMetadataSpecifics& remote_metadata, On 2017/01/17 20:57:30, rouslan wrote: > s/Basic/Common/ Done. https://codereview.chromium.org/2626263005/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:597: template <class DataType> On 2017/01/17 20:57:30, rouslan wrote: > Is template required or can you use the parent class instead? I need the template if I want to continue passing a const ref. The parent class const ref cannot be implicitly converted to a profile or card const ref in the BuildSyncData parameters. I feel like this way is the most readable. WDYT? https://codereview.chromium.org/2626263005/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:613: void AutofillWalletMetadataSyncableService::SendUpdateIfMoreRecent( On 2017/01/17 20:57:30, rouslan wrote: > No need for separate method. 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, rouslan@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/2626263005/#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": 1484764434277690,
"parent_rev": "a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6", "commit_rev":
"ca7d5cf7a234e2f4d085b3be0781bfd0765c0130"}
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Sync new wallet metadata fields through Chrome Sync. Sync the new has_converted field for wallet addresses metadata and the new billing_address_id field for the wallet card metadata through Chrome Sync. BUG=681131 ========== to ========== [Autofill] Sync new wallet metadata fields through Chrome Sync. Sync the new has_converted field for wallet addresses metadata and the new billing_address_id field for the wallet card metadata through Chrome Sync. BUG=681131 Review-Url: https://codereview.chromium.org/2626263005 Cr-Commit-Position: refs/heads/master@{#444423} Committed: https://chromium.googlesource.com/chromium/src/+/ca7d5cf7a234e2f4d085b3be0781... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ca7d5cf7a234e2f4d085b3be0781... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
