Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1110833002:
[autofill] Sync server card and address metadata. (Closed)
|
Created:
5 years, 8 months ago by please use gerrit instead Modified:
5 years, 6 months ago CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, albertb+watch_chromium.org, maniscalco+watch_chromium.org, brettw Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[autofill] Sync server card and address metadata.
BUG=481595
Committed: https://crrev.com/7c5be3bcf846d4de1d552342f46abb793127f5c5
Cr-Commit-Position: refs/heads/master@{#333202}
Patch Set 1 : Work #
Total comments: 2
Patch Set 2 : Rebase on top of https://crrev.com/1125143006 #
Total comments: 2
Patch Set 3 : Rewrite CreditCardChange::operator==() more clearly #
Total comments: 4
Patch Set 4 : Ditch ternary operator #
Total comments: 4
Patch Set 5 : One line DCHECK #
Total comments: 10
Patch Set 6 : Work #
Total comments: 11
Patch Set 7 : Rename change_list to changes_from_sync. Explain more in comments. Use callback instead of function… #
Total comments: 4
Patch Set 8 : Class comment and one more expectation in test. #Messages
Total messages: 65 (41 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1110833002/260001
The CQ bit was unchecked by rouslan@chromium.org
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
Patchset #1 (id:400001) has been deleted
Patchset #1 (id:420001) has been deleted
Patchset #1 (id:440001) has been deleted
Patchset #1 (id:460001) has been deleted
Patchset #1 (id:480001) has been deleted
Patchset #1 (id:500001) has been deleted
Patchset #1 (id:520001) has been deleted
Patchset #1 (id:540001) has been deleted
Patchset #1 (id:560001) has been deleted
Patchset #1 (id:580001) has been deleted
Patchset #1 (id:600001) has been deleted
Patchset #1 (id:620001) has been deleted
rouslan@chromium.org changed reviewers: + estade@chromium.org
Evan, PTAL. This is ready for a review. You added the metadata tables, so you may be the best reviewer. If you think Brett would be better because he added Wallet sync, please let me know. Although you don't mind large patches, do you think this patch would be better if it's split up along these categories? 1) Fix ups in Wallet sync. 2) Adding notifications that a CreditCard has been updated. 3) Moving server ID up to AutofillDataModel. 4) Adding WalletMetadataSpecifics protobuf. 5) Adding AutofillWalletMetadataSyncableService, which is the meat of the feature.
On 2015/05/18 20:01:12, Rouslan wrote: > Evan, PTAL. This is ready for a review. You added the metadata tables, so you > may be the best reviewer. If you think Brett would be better because he added > Wallet sync, please let me know. > > Although you don't mind large patches, do you think this patch would be better > if it's split up along these categories? > > 1) Fix ups in Wallet sync. > 2) Adding notifications that a CreditCard has been updated. > 3) Moving server ID up to AutofillDataModel. > 4) Adding WalletMetadataSpecifics protobuf. > 5) Adding AutofillWalletMetadataSyncableService, which is the meat of the > feature. Yes, you should split this up and have zea@ review the sync-specific stuff.
https://codereview.chromium.org/1110833002/diff/640001/components/autofill/co... File components/autofill/core/browser/autofill_data_model.h (right): https://codereview.chromium.org/1110833002/diff/640001/components/autofill/co... components/autofill/core/browser/autofill_data_model.h:49: void set_server_id(const std::string& server_id) { server_id_ = server_id; } can you make this protected
rouslan@chromium.org changed reviewers: + pkasting@chromium.org, zea@chromium.org
Patch Set 2 is rebased on top of https://crrev.com/1125143006 to avoid too much sync boilerplate in this review. Evan, PTAL in Patch Set 2: components/autofill.gypi components/autofill/core/browser/BUILD.gn components/autofill/core/browser/webdata/autofill_change.h components/autofill/core/browser/webdata/autofill_change.cc components/autofill/core/browser/webdata/autofill_webdata_backend.h components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc components/autofill/core/browser/webdata/autofill_webdata_service_observer.h components/components_tests.gyp Nicolas, PTAL in Patch Set 2: chrome/browser/sync/profile_sync_components_factory_impl.cc components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc Peter, PTAL in Patch Set 2: components/webdata_services/web_data_service_wrapper.cc https://codereview.chromium.org/1110833002/diff/640001/components/autofill/co... File components/autofill/core/browser/autofill_data_model.h (right): https://codereview.chromium.org/1110833002/diff/640001/components/autofill/co... components/autofill/core/browser/autofill_data_model.h:49: void set_server_id(const std::string& server_id) { server_id_ = server_id; } On 2015/05/18 22:02:59, Evan Stade wrote: > can you make this protected No longer applicable as we decided to go without this change.
LGTM
I looked at all the components/autofill/core/browser/webdata/ files but don't feel super-qualified to review. Nicolas should probably look at them all, rather than just the ones Rouslan listed above. https://codereview.chromium.org/1110833002/diff/660001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_change.cc (right): https://codereview.chromium.org/1110833002/diff/660001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:52: return type() == change.type() && key() == change.key() && (type() != REMOVE) I actually can't figure out what this is supposed to do. Can you rewrite it more clearly
Nicolas, PTAL in Patch Set 3: chrome/browser/sync/profile_sync_components_factory_impl.cc components/autofill.gypi components/autofill/core/browser/BUILD.gn components/autofill/core/browser/webdata/autofill_change.h components/autofill/core/browser/webdata/autofill_change.cc components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc components/autofill/core/browser/webdata/autofill_webdata_backend.h components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc components/autofill/core/browser/webdata/autofill_webdata_service_observer.h components/components_tests.gyp As Evan requested, that's everything except components/webdata_services/web_data_service_wrapper.cc that Peter has reviewed. https://codereview.chromium.org/1110833002/diff/660001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_change.cc (right): https://codereview.chromium.org/1110833002/diff/660001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:52: return type() == change.type() && key() == change.key() && (type() != REMOVE) On 2015/05/20 21:07:37, Evan Stade wrote: > I actually can't figure out what this is supposed to do. Can you rewrite it more > clearly Done.
https://codereview.chromium.org/1110833002/diff/680001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_change.cc (right): https://codereview.chromium.org/1110833002/diff/680001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:45: DCHECK(type == REMOVE ? !card : true); change to boolean logic, ditch ternary operator https://codereview.chromium.org/1110833002/diff/680001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:54: (type() != REMOVE) ? *card() == *change.card() : true; whitespace update didn't help me understand
https://codereview.chromium.org/1110833002/diff/680001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_change.cc (right): https://codereview.chromium.org/1110833002/diff/680001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:45: DCHECK(type == REMOVE ? !card : true); On 2015/05/20 21:35:37, Evan Stade wrote: > change to boolean logic, ditch ternary operator Ditched ternary operator in Patch Set 4. https://codereview.chromium.org/1110833002/diff/680001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:54: (type() != REMOVE) ? *card() == *change.card() : true; On 2015/05/20 21:35:36, Evan Stade wrote: > whitespace update didn't help me understand Ditched ternary operator in Patch Set 4.
I agree the previous ternaries were ugly, but I think one can use them to do even better than what you have here. https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_change.cc (right): https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:26: (type == REMOVE && !profile)); Simpler: DCHECK((type == REMOVE) ? !profile : (profile && profile->guid() == key)); https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:44: (type == REMOVE && !card)); Simpler: DCHECK((type == REMOVE) ? !card : (card && card->guid() == key));
https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_change.cc (right): https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:26: (type == REMOVE && !profile)); On 2015/05/20 23:11:21, Peter Kasting wrote: > Simpler: > > DCHECK((type == REMOVE) ? !profile : (profile && profile->guid() == key)); That's a good one. I like that it's a one-liner. https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... components/autofill/core/browser/webdata/autofill_change.cc:44: (type == REMOVE && !card)); On 2015/05/20 23:11:21, Peter Kasting wrote: > Simpler: > > DCHECK((type == REMOVE) ? !card : (card && card->guid() == key)); Ditto.
On 2015/05/21 00:13:48, Rouslan wrote: > https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... > File components/autofill/core/browser/webdata/autofill_change.cc (right): > > https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... > components/autofill/core/browser/webdata/autofill_change.cc:26: (type == REMOVE > && !profile)); > On 2015/05/20 23:11:21, Peter Kasting wrote: > > Simpler: > > > > DCHECK((type == REMOVE) ? !profile : (profile && profile->guid() == key)); > > That's a good one. I like that it's a one-liner. > > https://codereview.chromium.org/1110833002/diff/700001/components/autofill/co... > components/autofill/core/browser/webdata/autofill_change.cc:44: (type == REMOVE > && !card)); > On 2015/05/20 23:11:21, Peter Kasting wrote: > > Simpler: > > > > DCHECK((type == REMOVE) ? !card : (card && card->guid() == key)); > > Ditto. autofill_change.cc lgtm
https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:96: syncer::SyncMergeResult result(syncer::AUTOFILL_WALLET_METADATA); It's useful for debugging purposes to fill out as much of result as you can. At the very least the num items before and after association will help gauge how much sync data people are actually moving around. Tracking changes at association can also help identifying bugs. https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:148: // deleted along with the corresponding Wallet data. One thing to keep in mind is that it's probably good to track which items have been deleted, so that when you undelete you know what type of sync change to use (ADD vs UPDATE). Sync will throw an error if you attempt to ADD something which think exists locally, or UPDATE something which doesn't. https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:164: UpdateServerAddressUsageStats(profile); Will these method invocations trigger the CreditCardChanged and co callbacks? It would be good to avoid that if at all possible. https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:212: sync_error_factory_->CreateAndUploadError(FROM_HERE, error.ToString()); This isn't needed. The error is returned in case the datatype wants to do something special, but because the error in this case was already generated by sync you don't need to reupload it. You can just ignore it here and below (sync will shut down the datatype asynchronously). https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:242: sync_processor_->GetAllSyncData(syncer::AUTOFILL_WALLET_METADATA)); Wasn't the plan to keep around the sync data in memory? This is fine to do for now, but it's important to keep in mind that this will attempt to acquire a lock, possibly blocking this thread on other threads doing similar work.
Patchset #6 (id:740001) has been deleted
Patchset #6 (id:760001) has been deleted
Patchset #6 (id:760001) has been deleted
Nicolas, PTAL Patch Set 6. https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:96: syncer::SyncMergeResult result(syncer::AUTOFILL_WALLET_METADATA); On 2015/05/22 15:29:43, Nicolas Zea wrote: > It's useful for debugging purposes to fill out as much of result as you can. At > the very least the num items before and after association will help gauge how > much sync data people are actually moving around. Tracking changes at > association can also help identifying bugs. Done, although num items after association, num items added, and num items deleted are not interesting, because SyncableService does not add or delete Wallet metadata on disk. (Wallet metadata is added or deleted when the corresponding Wallet data is added or deleted.) https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:148: // deleted along with the corresponding Wallet data. On 2015/05/22 15:29:43, Nicolas Zea wrote: > One thing to keep in mind is that it's probably good to track which items have > been deleted, so that when you undelete you know what type of sync change to use > (ADD vs UPDATE). Sync will throw an error if you attempt to ADD something which > think exists locally, or UPDATE something which doesn't. Acknowledged. https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:164: UpdateServerAddressUsageStats(profile); On 2015/05/22 15:29:43, Nicolas Zea wrote: > Will these method invocations trigger the CreditCardChanged and co callbacks? It > would be good to avoid that if at all possible. These calls go directly into AutofillTable and do not trigger CreditCardChanged or AutofillProfileChanged callbacks. I've added a comment to the declarations to that effect. https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:212: sync_error_factory_->CreateAndUploadError(FROM_HERE, error.ToString()); On 2015/05/22 15:29:43, Nicolas Zea wrote: > This isn't needed. The error is returned in case the datatype wants to do > something special, but because the error in this case was already generated by > sync you don't need to reupload it. You can just ignore it here and below (sync > will shut down the datatype asynchronously). Done, thank you for the tip. https://codereview.chromium.org/1110833002/diff/720001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:242: sync_processor_->GetAllSyncData(syncer::AUTOFILL_WALLET_METADATA)); On 2015/05/22 15:29:43, Nicolas Zea wrote: > Wasn't the plan to keep around the sync data in memory? This is fine to do for > now, but it's important to keep in mind that this will attempt to acquire a > lock, possibly blocking this thread on other threads doing similar work. Changed to avoid GetAllSyncData() by keeping the sync data in cache_.
Sync logic LG, couple comments https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:130: syncer::SyncChangeList remote_changes; nit: I wonder if there's a better name. Remote changes to me implies changes made remotely (as in the ones coming from sync). Maybe call the two sets of changes "changes_from_sync" and "changes_to_sync"? Or something else that's a bit clearer? https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:244: MergeData(syncer::SyncDataList(cache_)); Note that for the purposes of correctly setting update/add/delete you don't necessarily need to keep a full SyncDataList. You really just need to know what exists and doesn't exist in sync, so effectively a tag list. Up to you in that respect though. https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h (right): https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h:108: // true if the corresponding local metadata was found. nit: It would probably be good to explain a bit more some of the conditions for updating here. E.g. the metadata has to exist locally, and the counts or times in the remote data have to be higher/newer. https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h:112: bool (AutofillWalletMetadataSyncableService::*updater)(const DataType&), Use a callback instead?
Nicolas, PTAL Patch Set 7. https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc (right): https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:130: syncer::SyncChangeList remote_changes; On 2015/06/04 18:43:25, Nicolas Zea wrote: > nit: I wonder if there's a better name. Remote changes to me implies changes > made remotely (as in the ones coming from sync). Maybe call the two sets of > changes "changes_from_sync" and "changes_to_sync"? Or something else that's a > bit clearer? Done. Using "changes_from_sync" and "changes_to_sync". https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc:244: MergeData(syncer::SyncDataList(cache_)); On 2015/06/04 18:43:25, Nicolas Zea wrote: > Note that for the purposes of correctly setting update/add/delete you don't > necessarily need to keep a full SyncDataList. You really just need to know what > exists and doesn't exist in sync, so effectively a tag list. Up to you in that > respect though. I'd like to keep a full SyncDataList, because I prefer not to trigger an update message for unchanged metadata. https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h (right): https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h:57: syncer::SyncError ProcessSyncChanges( Overrides in Chrome usually have no comments, but I feel it would be useful to explain how ProcessSyncChanges and AutofillMultipleChanged interact with the sync server. WDYT? https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h:108: // true if the corresponding local metadata was found. On 2015/06/04 18:43:25, Nicolas Zea wrote: > nit: It would probably be good to explain a bit more some of the conditions for > updating here. E.g. the metadata has to exist locally, and the counts or times > in the remote data have to be higher/newer. Added explanation of the conditions for updating/adding/removing remote data (on the sync server). Should I also add explanation of when local data is updated? https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h:112: bool (AutofillWalletMetadataSyncableService::*updater)(const DataType&), On 2015/06/04 18:43:25, Nicolas Zea wrote: > Use a callback instead? Done.
Those are great tests btw, nice job! A couple nits, else LGTM https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h (right): https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h:57: syncer::SyncError ProcessSyncChanges( On 2015/06/04 22:59:21, Rouslan wrote: > Overrides in Chrome usually have no comments, but I feel it would be useful to > explain how ProcessSyncChanges and AutofillMultipleChanged interact with the > sync server. WDYT? Agreed. You don't necessarily have to have the comment here. It could be a comment for AutofillWalletMetadataSyncableService, just explaining how it works in general. https://codereview.chromium.org/1110833002/diff/800001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc (right): https://codereview.chromium.org/1110833002/diff/800001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:190: EXPECT_CALL(local, UpdateCardStats(_)).Times(0); Maybe check remote's methods as well? (although it looks like you're not doing that in most these tests. Is that covered by something else?) https://codereview.chromium.org/1110833002/diff/800001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:406: // These methods do not trigger notifications or sync: Is this comment accurate? These do trigger sync adds after all (here and below).
Sending to cq. https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h (right): https://codereview.chromium.org/1110833002/diff/780001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h:57: syncer::SyncError ProcessSyncChanges( On 2015/06/05 20:51:21, Nicolas Zea wrote: > On 2015/06/04 22:59:21, Rouslan wrote: > > Overrides in Chrome usually have no comments, but I feel it would be useful to > > explain how ProcessSyncChanges and AutofillMultipleChanged interact with the > > sync server. WDYT? > > Agreed. You don't necessarily have to have the comment here. It could be a > comment for AutofillWalletMetadataSyncableService, just explaining how it works > in general. > This class comment should be sufficient, I hope: // Syncs usage counts and last use dates (metadata) for Wallet cards and // addresses (data). Creation and deletion of metadata on the sync server // follows the creation and deletion of the local metadata. (Local metadata has // 1:1 relationship with Wallet data.) https://codereview.chromium.org/1110833002/diff/800001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc (right): https://codereview.chromium.org/1110833002/diff/800001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:190: EXPECT_CALL(local, UpdateCardStats(_)).Times(0); On 2015/06/05 20:51:21, Nicolas Zea wrote: > Maybe check remote's methods as well? (although it looks like you're not doing > that in most these tests. Is that covered by something else?) Good call. I've added that check as well. https://codereview.chromium.org/1110833002/diff/800001/components/autofill/co... components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:406: // These methods do not trigger notifications or sync: On 2015/06/05 20:51:21, Nicolas Zea wrote: > Is this comment accurate? These do trigger sync adds after all (here and below). It's accurate. UpdateAddressStats() and UpdateCardStats() write to disk without triggering notifications or sync. AutofillProfileChanged(), CreditCardChanged(), and AutofillMultipleChanged() are callbacks that fire when someone else writes metadata to disk and may cause sync "add", "update", and "delete" messages. These method names are easy to confuse, sorry about that.
On 2015/06/05 20:51:22, Nicolas Zea wrote: > Those are great tests btw, nice job! Thanks! :-D
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, estade@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/1110833002/#ps820001 (title: "Class comment and one more expectation in test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1110833002/820001
Message was sent while issue was closed.
Committed patchset #8 (id:820001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7c5be3bcf846d4de1d552342f46abb793127f5c5 Cr-Commit-Position: refs/heads/master@{#333202}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:820001) has been created in https://codereview.chromium.org/1170123002/ by dominicc@chromium.org. The reason for reverting is: Chrome Stability sheriff here--I suspect that this is causing crashes on Win, Mac and Android (maybe everything); see https://code.google.com/p/chromium/issues/detail?id=497659#c6. |
