Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681853002:
[Autofill] In sync for Wallet Card billing keep local over Wallet. (Closed)
|
Created:
3 years, 10 months ago by sebsg Modified:
3 years, 10 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[Autofill] In sync for Wallet Card billing keep local over Wallet.
When syncing Wallet cards metadata, always keep a billing address id
refering to a local profile over one refering to a Wallet address.
BUG=689600
Review-Url: https://codereview.chromium.org/2681853002
Cr-Commit-Position: refs/heads/master@{#449085}
Committed: https://chromium.googlesource.com/chromium/src/+/75de00b7d1a421049d6132ae569822a7be094beb
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added more tests #
Messages
Total messages: 26 (15 generated)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (id:1) 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, rouslan@chromium.org
Hi Math and Rouslan, could you please take a look at this CL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2681853002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc (right): https://codereview.chromium.org/2681853002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:1146: } Additional test cases needed: 1) kLocalAddr1 and kLocalAddr2, where more recent should be kept. 2) kAddr1 and kAddr2, where the more recent should be kept as well. 3) kLocalAddr1 and kLocalAddr2 with the same timestamp and usage stats. What should happen here?
Patchset #2 (id:40001) has been deleted
Thanks, those were good tests to add! https://codereview.chromium.org/2681853002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc (right): https://codereview.chromium.org/2681853002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:1146: } On 2017/02/07 22:04:10, rouslan wrote: > Additional test cases needed: > 1) kLocalAddr1 and kLocalAddr2, where more recent should be kept. > 2) kAddr1 and kAddr2, where the more recent should be kept as well. > 3) kLocalAddr1 and kLocalAddr2 with the same timestamp and usage stats. What > should happen here? Those are good ideas. Added tests for 1) The tests for 2) were already present (DifferentServerBillingAddressId_RemoteMostRecent and DifferentServerBillingAddressId_LocalMostRecent) For 3): This should never happen in practice, but to be safe, it should be that if the timestamps are equal the remote will win. Because this will result is a consistent state, whereas if you keep local, you would send an update to remote, which would keep local, and etc etc over and over :) Thanks!
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...
lgtm
lgtm
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
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": 60001, "attempt_start_ts": 1486586477890630,
"parent_rev": "ef3aedc71c86337d5cd5ef18b56fbac78c2a5482", "commit_rev":
"75de00b7d1a421049d6132ae569822a7be094beb"}
Message was sent while issue was closed.
Description was changed from ========== [Autofill] In sync for Wallet Card billing keep local over Wallet. When syncing Wallet cards metadata, always keep a billing address id refering to a local profile over one refering to a Wallet address. BUG=689600 ========== to ========== [Autofill] In sync for Wallet Card billing keep local over Wallet. When syncing Wallet cards metadata, always keep a billing address id refering to a local profile over one refering to a Wallet address. BUG=689600 Review-Url: https://codereview.chromium.org/2681853002 Cr-Commit-Position: refs/heads/master@{#449085} Committed: https://chromium.googlesource.com/chromium/src/+/75de00b7d1a421049d6132ae5698... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/75de00b7d1a421049d6132ae5698...
Message was sent while issue was closed.
nsphu.cnht@gmail.com changed reviewers: + nsphu.cnht@gmail.com
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
