Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2620783002:
[sync] Handle local changes in AutocompleteSyncBridge (Closed)
|
Created:
3 years, 11 months ago by Patrick Noland 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[sync] Handle local changes in AutocompleteSyncBridge
Implement AutofillEntriesChanged to listen for local autofill changes.
Add logic that propagates these changes to the processor.
Extract RecordingModelTypeChangeProcessor for general use.
R=pavely@chromium.org,maxbogue@chromium.org,mathp@chromium.org,pkasting@chromium.org
BUG=679517
Review-Url: https://codereview.chromium.org/2620783002
Cr-Commit-Position: refs/heads/master@{#444840}
Committed: https://chromium.googlesource.com/chromium/src/+/1e4b08621e36a68cc141e04b50a55f1a09cec9ce
Patch Set 1 #Patch Set 2 : FREEZE.unindexed #Patch Set 3 : self-review cleanups #
Total comments: 68
Patch Set 4 : Use specifics for storage key and verification #Patch Set 5 : rebase onto sky's changes #
Total comments: 48
Patch Set 6 : [sync] Handle local changes in AutocompleteSyncBridge #
Total comments: 11
Patch Set 7 : test assignment in conditional for warnings #Patch Set 8 : rebase #
Messages
Total messages: 70 (49 generated)
The CQ bit was checked by pnoland@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-...)
The CQ bit was checked by pnoland@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== [sync] Handle local changes in AutocompleteSyncBridge Implement AutofillEntriesChanged to listen for local autofill changes. Add logic that propagates these changes to the processor. Extract RecordingModelTypeChangeProcessor for general use. R=pavely@chromium.org,mbogue@chromium.org,mathp@chromium.org,pkasting@chromiu... BUG=679517 ========== to ========== [sync] Handle local changes in AutocompleteSyncBridge Implement AutofillEntriesChanged to listen for local autofill changes. Add logic that propagates these changes to the processor. Extract RecordingModelTypeChangeProcessor for general use. R=pavely@chromium.org,maxbogue@chromium.org,mathp@chromium.org,pkasting@chrom... BUG=679517 ==========
pnoland@chromium.org changed reviewers: + maxbogue@chromium.org - mbogue@chromium.org
The CQ bit was checked by pnoland@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...
mathp@ PTAL at autofill pkasting@ PTAL at web_data_service_wrapper pavely@ and maxbogue@ PTAL everything
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
skym@chromium.org changed reviewers: + skym@chromium.org
https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:174: DCHECK(change_processor()); I'd remove this. The bridge should always have a proc now. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:175: std::unique_ptr<syncer::MetadataChangeList> change_list = Need to call TakeError() on this thing. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:186: DCHECK(success); Is this really impossible? How about something like the following, WDYT? change_processor()->ReportError(FROM_HERE, "Failed reading from WebDatabase."); https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:191: WriteAutofillEntry(entry, &specifics); This feels more awkward than it needed to be. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:235: flare_.Run(syncer::AUTOFILL); It looks like we end up dropping the changes here, because if this isn't a first time, we no longer merge. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:98: return CreateAutofillEntryFromValues(name, value, time_shift, -1); I really dislike this -1 approach. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:310: processor()->put_multimap().find(GetStorageKeyFromModel(entry_key1)); What do you think of creating the model objects, making the changes, and then recreating the same expected objects manually here as specifics and verify we see those? Instead of calling this private method. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:380: struct TestForRun { Should this get moved to up above the test cases? https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/m... File components/sync/model/model_type_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/m... components/sync/model/model_type_sync_bridge.h:43: typedef base::Callback<void(ModelType)> StartSyncFlare; Can we curry the type when we hand these things to the model?
https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:98: return CreateAutofillEntryFromValues(name, value, time_shift, -1); On 2017/01/12 00:18:32, skym wrote: > I really dislike this -1 approach. Like, what do you think of maybe just passing time_shift as 3rd and 4th params and delete lines 87,88,90?
https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:43: entity_data->specifics.mutable_autofill(); PopulateAutofillSpecifics(entry, entity_data->specifics.mutable_autofill()); https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:60: void WriteAutofillEntry(const AutofillEntry& entry, A more idiomatic name for this function would be PopulateAutofillSpecifics. We discussed offline that this could potentially just return the specifics instead of using an out pointer, but I think leaving this form lets you use it efficiently above, which is desirable... https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:186: DCHECK(success); On 2017/01/12 00:18:32, skym wrote: > Is this really impossible? How about something like the following, WDYT? > > change_processor()->ReportError(FROM_HERE, "Failed reading from WebDatabase."); +1, please handle your errors :) https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:189: sync_pb::AutofillSpecifics specifics; You do not use this variable. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:191: WriteAutofillEntry(entry, &specifics); On 2017/01/12 00:18:32, skym wrote: > This feels more awkward than it needed to be. This line is populating a specifics variable that is never used, please remove it. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:224: const syncer::ModelTypeSyncBridge::StartSyncFlare& flare) { omit "syncer::ModelTypeSyncBridge::" https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:232: if (change_processor()->IsTrackingMetadata()) { I would prefer this just be at the top of ActOnLocalChanges, like: if (!change_processor()->IsTrackingMetadata()) return; https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:234: } else if (!flare_.is_null()) { The flare is simply an optimization right? We're asking sync to start up faster? But in USS world our ability to track changes isn't blocked by sync starting up, it's blocked on the model loading metadata, so I'm not certain why a flare is needed... Let's chat tomorrow morning about this. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:235: flare_.Run(syncer::AUTOFILL); On 2017/01/12 00:18:32, skym wrote: > It looks like we end up dropping the changes here, because if this isn't a first > time, we no longer merge. I don't think this comment is accurate; maybe Sky can clarify? https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:244: std::string AutocompleteSyncBridge::GetStorageKeyFromModel( "// static" on a line above https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:67: const syncer::ModelTypeSyncBridge::StartSyncFlare& flare); Since we are inside a ModelTypeSyncBridge implementation, you can omit the "syncer::ModelTypeSyncBridge::" from the type. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:76: std::string GetStorageKeyFromModel(const AutofillKey& key); Make static and put below ActOnChanges. Might not apply if you do what Sky suggests in the test file. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:78: void ActOnChanges(const AutofillChangeList& changes); comment + also rename to something with "local" in the name? ActOnLocalChanges or ApplyLocalChanges or somesuch. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:89: syncer::ModelTypeSyncBridge::StartSyncFlare flare_; omit "syncer::ModelTypeSyncBridge::" https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:91: friend class AutocompleteSyncBridgeTest; Friend things are always the first things under "private:". I'm not really sure why but that seems conventional. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:92: FRIEND_TEST_ALL_PREFIXES(AutocompleteSyncBridgeTest, GetStorageKeyFromModel); GetStorageKeyFromModel is not the name of a test case, so I don't believe this line is doing anything... https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:108: } // namespace Why'd you take the FakeAutofillBackend out of the anonymous namespace? https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:129: protected: I don't understand the protected thing in tests. Just have public and private IMO. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:132: syncer::RecordingModelTypeChangeProcessor* processor_ = nullptr; This variable has an accessor, it should just be private. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:133: AutofillTable table_; Give this variable an accessor and make it private. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:185: public: plz put public section above the protected one for my sanity. Actually since I'm also asking you to get rid of the protected section please put these things at the top of the new public section. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:380: struct TestForRun { On 2017/01/12 00:18:32, skym wrote: > Should this get moved to up above the test cases? (yes, inside the anonymous namespace) https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:410: }; // namespace autofill No semi-colon, that's just for classes because C++ hates consistency. https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/m... File components/sync/model/model_type_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/m... components/sync/model/model_type_sync_bridge.h:43: typedef base::Callback<void(ModelType)> StartSyncFlare; On 2017/01/12 00:18:32, skym wrote: > Can we curry the type when we hand these things to the model? Probably doable, and worth noting that this would reduce the type to just base::Closure (no typedef needed). https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... File components/sync/model/recording_model_type_change_processor.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... components/sync/model/recording_model_type_change_processor.cc:45: return put_multimap_; This function and the two below can be defined inline in the header. https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... File components/sync/model/recording_model_type_change_processor.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... components/sync/model/recording_model_type_change_processor.h:22: nit: generally we don't have blank lines between interface overrides and have a line above the block with a comment like "ModelTypeChangeProcessor implementation." Or in this case, perhaps "FakeModelTypeChangeProcessor overrides." would be more appropriate, since it's only a partial implementation of the original interface.
LGTM
https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:232: if (change_processor()->IsTrackingMetadata()) { On 2017/01/12 01:33:35, maxbogue wrote: > I would prefer this just be at the top of ActOnLocalChanges, like: > > if (!change_processor()->IsTrackingMetadata()) > return; So, this only works if we're not tracking because sync is disabled or we haven't finished the initial download yet. If we're not tracking because we haven't given the metadata to the proc, then we need to do this ourselves now, and then act on the changes. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:234: } else if (!flare_.is_null()) { On 2017/01/12 01:33:35, maxbogue wrote: > The flare is simply an optimization right? We're asking sync to start up faster? > But in USS world our ability to track changes isn't blocked by sync starting up, > it's blocked on the model loading metadata, so I'm not certain why a flare is > needed... Let's chat tomorrow morning about this. Agreed, lets remove the flare. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:235: flare_.Run(syncer::AUTOFILL); On 2017/01/12 01:33:35, maxbogue wrote: > On 2017/01/12 00:18:32, skym wrote: > > It looks like we end up dropping the changes here, because if this isn't a > first > > time, we no longer merge. > > I don't think this comment is accurate; maybe Sky can clarify? So I think in the Directory world, we were more concerned about consistency because metadata wasn't stored with our data. If there were many local changes before sync started up, we *hand wave hand wave* thought this would increase our chances of having data not line up. For a straight forward data type like AUTOFILL, I don't think this fear really lines up, so maybe my speculation is incorrect. However, in this USS world, we don't need to start up sync. Flaring here doesn't do any good. The proc is going to be able to track local changes just fine for us w/o sync being enabled. If we haven't done our first time sync yet, and we're going to merge, that's fine, we'll merge and life will be okay. You can probably remove the concept of flaring from this class. The only scenario I see flaring as useful going forward is if you want to do a round trip to the server really fast. Things like showing foreign session tabs on the NTP on startup :) However, this kind of exposes a different problem with our approach to implementing this class. No one is calling OnMetadataLoaded(). You're not technically allowed to Put()/Delete() to the proc before you do this. Which this method should probably worry about. I've created crbug.com/680603 for this. I'm fine with you fixing it as part of this CL or just leaving a TODO. But we'll want to do it soon. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:91: friend class AutocompleteSyncBridgeTest; On 2017/01/12 01:33:36, maxbogue wrote: > Friend things are always the first things under "private:". I'm not really sure > why but that seems conventional. I was hoping you'd be able to remove the friend class here. See https://codereview.chromium.org/2582713003/ . It seems like we should avoid friend classing anything for tests in components/autofill/*. vabr@ wrote: Please do not friend the test. Tests should only depend on the public API, making them depend on the internals makes them fragile. I added some concrete suggestions below for how to keep your current test without the need to friend it with the bridge.
The CQ bit was checked by pnoland@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.
The CQ bit was checked by pnoland@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 #5 (id:80001) has been deleted
The CQ bit was checked by pnoland@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...
sky, max, PTAL. I've removed the flaring logic and unit test usage of GetStorageKeyFromModel and added metadata loading on startup. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:60: void WriteAutofillEntry(const AutofillEntry& entry, On 2017/01/12 01:33:35, maxbogue wrote: > A more idiomatic name for this function would be PopulateAutofillSpecifics. We > discussed offline that this could potentially just return the specifics instead > of using an out pointer, but I think leaving this form lets you use it > efficiently above, which is desirable... Actually I don't think this is needed at all https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:174: DCHECK(change_processor()); On 2017/01/12 00:18:32, skym wrote: > I'd remove this. The bridge should always have a proc now. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:175: std::unique_ptr<syncer::MetadataChangeList> change_list = On 2017/01/12 00:18:32, skym wrote: > Need to call TakeError() on this thing. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:186: DCHECK(success); On 2017/01/12 00:18:32, skym wrote: > Is this really impossible? How about something like the following, WDYT? > > change_processor()->ReportError(FROM_HERE, "Failed reading from WebDatabase."); Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:189: sync_pb::AutofillSpecifics specifics; On 2017/01/12 01:33:35, maxbogue wrote: > You do not use this variable. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:191: WriteAutofillEntry(entry, &specifics); On 2017/01/12 01:33:35, maxbogue wrote: > On 2017/01/12 00:18:32, skym wrote: > > This feels more awkward than it needed to be. > > This line is populating a specifics variable that is never used, please remove > it. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:224: const syncer::ModelTypeSyncBridge::StartSyncFlare& flare) { On 2017/01/12 01:33:35, maxbogue wrote: > omit "syncer::ModelTypeSyncBridge::" n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:232: if (change_processor()->IsTrackingMetadata()) { On 2017/01/12 01:33:35, maxbogue wrote: > I would prefer this just be at the top of ActOnLocalChanges, like: > > if (!change_processor()->IsTrackingMetadata()) > return; Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:235: flare_.Run(syncer::AUTOFILL); On 2017/01/12 18:49:16, skym wrote: > On 2017/01/12 01:33:35, maxbogue wrote: > > On 2017/01/12 00:18:32, skym wrote: > > > It looks like we end up dropping the changes here, because if this isn't a > > first > > > time, we no longer merge. > > > > I don't think this comment is accurate; maybe Sky can clarify? > > So I think in the Directory world, we were more concerned about consistency > because metadata wasn't stored with our data. If there were many local changes > before sync started up, we *hand wave hand wave* thought this would increase our > chances of having data not line up. For a straight forward data type like > AUTOFILL, I don't think this fear really lines up, so maybe my speculation is > incorrect. > > However, in this USS world, we don't need to start up sync. Flaring here doesn't > do any good. The proc is going to be able to track local changes just fine for > us w/o sync being enabled. If we haven't done our first time sync yet, and we're > going to merge, that's fine, we'll merge and life will be okay. You can probably > remove the concept of flaring from this class. The only scenario I see flaring > as useful going forward is if you want to do a round trip to the server really > fast. Things like showing foreign session tabs on the NTP on startup :) > > However, this kind of exposes a different problem with our approach to > implementing this class. No one is calling OnMetadataLoaded(). You're not > technically allowed to Put()/Delete() to the proc before you do this. Which this > method should probably worry about. I've created crbug.com/680603 for this. I'm > fine with you fixing it as part of this CL or just leaving a TODO. But we'll > want to do it soon. I've gone ahead and added metadata loading https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:244: std::string AutocompleteSyncBridge::GetStorageKeyFromModel( On 2017/01/12 01:33:35, maxbogue wrote: > "// static" on a line above n/a now that unit tests don't use this https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:67: const syncer::ModelTypeSyncBridge::StartSyncFlare& flare); On 2017/01/12 01:33:36, maxbogue wrote: > Since we are inside a ModelTypeSyncBridge implementation, you can omit the > "syncer::ModelTypeSyncBridge::" from the type. n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:76: std::string GetStorageKeyFromModel(const AutofillKey& key); On 2017/01/12 01:33:35, maxbogue wrote: > Make static and put below ActOnChanges. Might not apply if you do what Sky > suggests in the test file. n/a now that unit tests don't use it https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:89: syncer::ModelTypeSyncBridge::StartSyncFlare flare_; On 2017/01/12 01:33:36, maxbogue wrote: > omit "syncer::ModelTypeSyncBridge::" n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:91: friend class AutocompleteSyncBridgeTest; On 2017/01/12 18:49:16, skym wrote: > On 2017/01/12 01:33:36, maxbogue wrote: > > Friend things are always the first things under "private:". I'm not really > sure > > why but that seems conventional. > > I was hoping you'd be able to remove the friend class here. See > https://codereview.chromium.org/2582713003/ . It seems like we should avoid > friend classing anything for tests in components/autofill/*. vabr@ wrote: > > Please do not friend the test. Tests should only depend on the public API, > making them depend on the internals makes them fragile. I added some concrete > suggestions below for how to keep your current test without the need to friend > it with the bridge. n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:91: friend class AutocompleteSyncBridgeTest; On 2017/01/12 01:33:36, maxbogue wrote: > Friend things are always the first things under "private:". I'm not really sure > why but that seems conventional. n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:92: FRIEND_TEST_ALL_PREFIXES(AutocompleteSyncBridgeTest, GetStorageKeyFromModel); On 2017/01/12 01:33:36, maxbogue wrote: > GetStorageKeyFromModel is not the name of a test case, so I don't believe this > line is doing anything... n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:98: return CreateAutofillEntryFromValues(name, value, time_shift, -1); On 2017/01/12 00:40:29, skym wrote: > On 2017/01/12 00:18:32, skym wrote: > > I really dislike this -1 approach. > > Like, what do you think of maybe just passing time_shift as 3rd and 4th params > and delete lines 87,88,90? Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:108: } // namespace On 2017/01/12 01:33:36, maxbogue wrote: > Why'd you take the FakeAutofillBackend out of the anonymous namespace? Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:129: protected: On 2017/01/12 01:33:36, maxbogue wrote: > I don't understand the protected thing in tests. Just have public and private > IMO. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:132: syncer::RecordingModelTypeChangeProcessor* processor_ = nullptr; On 2017/01/12 01:33:36, maxbogue wrote: > This variable has an accessor, it should just be private. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:133: AutofillTable table_; On 2017/01/12 01:33:36, maxbogue wrote: > Give this variable an accessor and make it private. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:185: public: On 2017/01/12 01:33:36, maxbogue wrote: > plz put public section above the protected one for my sanity. Actually since I'm > also asking you to get rid of the protected section please put these things at > the top of the new public section. Done. https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:310: processor()->put_multimap().find(GetStorageKeyFromModel(entry_key1)); On 2017/01/12 00:18:32, skym wrote: > What do you think of creating the model objects, making the changes, and then > recreating the same expected objects manually here as specifics and verify we > see those? Instead of calling this private method. Done. I've added a method that converts from an AutofillEntry to specifics to make this less tedious https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:380: struct TestForRun { On 2017/01/12 01:33:36, maxbogue wrote: > On 2017/01/12 00:18:32, skym wrote: > > Should this get moved to up above the test cases? > > (yes, inside the anonymous namespace) n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:410: }; // namespace autofill On 2017/01/12 01:33:36, maxbogue wrote: > No semi-colon, that's just for classes because C++ hates consistency. Done. https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/m... File components/sync/model/model_type_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/m... components/sync/model/model_type_sync_bridge.h:43: typedef base::Callback<void(ModelType)> StartSyncFlare; On 2017/01/12 01:33:36, maxbogue wrote: > On 2017/01/12 00:18:32, skym wrote: > > Can we curry the type when we hand these things to the model? > > Probably doable, and worth noting that this would reduce the type to just > base::Closure (no typedef needed). n/a now that flare is gone https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... File components/sync/model/recording_model_type_change_processor.cc (right): https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... components/sync/model/recording_model_type_change_processor.cc:45: return put_multimap_; On 2017/01/12 01:33:36, maxbogue wrote: > This function and the two below can be defined inline in the header. Done. https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... File components/sync/model/recording_model_type_change_processor.h (right): https://codereview.chromium.org/2620783002/diff/40001/components/sync/model/r... components/sync/model/recording_model_type_change_processor.h:22: On 2017/01/12 01:33:36, maxbogue wrote: > nit: generally we don't have blank lines between interface overrides and have a > line above the block with a comment like "ModelTypeChangeProcessor > implementation." Or in this case, perhaps "FakeModelTypeChangeProcessor > overrides." would be more appropriate, since it's only a partial implementation > of the original interface. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:77: void LoadMetadata(); I'll add a comment for this function.
lgtm https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:297: LoadMetadata(); As far as I can tell, no unit tests check that this was called. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:419: AutofillEntry entry(change.key(), date_created, date_last_used); If you're going to const the storage_key, might as well const this too, right? https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:420: std::unique_ptr<syncer::EntityData> entity_data = I'd personally inline this, you're only saving a single character switching from "CreateEntityData(entry)" to "std::move(entity_data)" https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:435: change_processor()->ReportError( You should give |error| (via error.value()) to ReportError() here. That way we preserve the original FROM_HERE and hopefully a more useful message. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:441: std::unique_ptr<syncer::MetadataBatch> batch = auto! https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:18: #include "base/strings/utf_string_conversions.h" Remove this, duplicate with line 17. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:21: #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h" Remove this, duplicate with line 5. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:102: AutofillSpecifics CreateAutofillSpecificsFromEntry(const AutofillEntry& entry) { Part of me wonders if maybe it'd be the most clean if all the tests start off creating either model or specifics objects. And then convert in a single direction? I feel like the tests are a bit inconsistent, WDYT? https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:118: // Time deep in the past would cause Autocomplete sync to discard the Technically not true right now. See line 144, we don't do anything. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:124: return AutofillEntry(AutofillKey(ASCIIToUTF16(name), ASCIIToUTF16(value)), Why ASCIIToUTF16 instead of base::UTF8ToUTF16? https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:164: table_.UpdateModelTypeState(syncer::AUTOFILL, model_type_state); Does this do anything? Do the tests pass if you take this out? https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:528: VerifyEqual(recorded_specifics, specifics); Seems like these couple lines keep repeating themselves. Can you create a helper method to verify the put_multimap looks good?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:401: std::unique_ptr<AutofillMetadataChangeList> change_list = auto mcl = base::MakeUnique... auto because MakeUnique makes the type clear, and mcl because to me it's actually more descriptive than change_list (less confuse-able with non-metadata changes) https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:420: std::unique_ptr<syncer::EntityData> entity_data = On 2017/01/14 00:04:33, skym wrote: > I'd personally inline this, you're only saving a single character switching from > "CreateEntityData(entry)" to "std::move(entity_data)" +1 https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:435: change_processor()->ReportError( On 2017/01/14 00:04:33, skym wrote: > You should give |error| (via error.value()) to ReportError() here. That way we > preserve the original FROM_HERE and hopefully a more useful message. +1 https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:175: AutocompleteSyncBridge* bridge() { return bridge_.get(); } nit: we almost always have these sorts of accessors at the bottom of the public section. just another little consistency thing. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:182: CreateModelTypeChangeProcessor(syncer::ModelType type, this can be private https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:272: AutofillTable table_; was there an ownership reason this one moved? https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:476: const AutofillEntry added_entry1( use = syntax (added_entry1 = CreateAutofillEntry...) https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:478: const AutofillEntry added_entry2( use = syntax https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:510: const AutofillEntry added_entry( = https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:530: const AutofillEntry updated_entry( = https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:552: const AutofillEntry deleted_entry( = https://codereview.chromium.org/2620783002/diff/100001/components/sync/model/... File components/sync/model/recording_model_type_change_processor.h (right): https://codereview.chromium.org/2620783002/diff/100001/components/sync/model/... components/sync/model/recording_model_type_change_processor.h:30: const inline std::multimap<std::string, std::unique_ptr<EntityData>>& you don't actually need (or want) the inline keyword here or the below two functions. the compiler will figure it out.
https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:401: std::unique_ptr<AutofillMetadataChangeList> change_list = On 2017/01/14 00:39:02, maxbogue wrote: > auto mcl = base::MakeUnique... > > auto because MakeUnique makes the type clear, and mcl because to me it's > actually more descriptive than change_list (less confuse-able with non-metadata > changes) I'm not a huge fan of abbreviations. I'd prefer metadata_change_list or metadata_changes, both of which we have precedent using. But I'm okay with whatever else is chosen.
The CQ bit was checked by pnoland@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.
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Patchset #6 (id:120001) 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...
Max, Mathieu, PTAL https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:297: LoadMetadata(); On 2017/01/14 00:04:33, skym wrote: > As far as I can tell, no unit tests check that this was called. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:401: std::unique_ptr<AutofillMetadataChangeList> change_list = On 2017/01/17 17:57:21, skym wrote: > On 2017/01/14 00:39:02, maxbogue wrote: > > auto mcl = base::MakeUnique... > > > > auto because MakeUnique makes the type clear, and mcl because to me it's > > actually more descriptive than change_list (less confuse-able with > non-metadata > > changes) > > I'm not a huge fan of abbreviations. I'd prefer metadata_change_list or > metadata_changes, both of which we have precedent using. But I'm okay with > whatever else is chosen. Done. I also don't love abbreviations whose meanings aren't obvious, so I went with metadata_change_list. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:419: AutofillEntry entry(change.key(), date_created, date_last_used); On 2017/01/14 00:04:33, skym wrote: > If you're going to const the storage_key, might as well const this too, right? Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:420: std::unique_ptr<syncer::EntityData> entity_data = On 2017/01/14 00:04:33, skym wrote: > I'd personally inline this, you're only saving a single character switching from > "CreateEntityData(entry)" to "std::move(entity_data)" Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:435: change_processor()->ReportError( On 2017/01/14 00:39:02, maxbogue wrote: > On 2017/01/14 00:04:33, skym wrote: > > You should give |error| (via error.value()) to ReportError() here. That way we > > preserve the original FROM_HERE and hopefully a more useful message. > > +1 Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:441: std::unique_ptr<syncer::MetadataBatch> batch = On 2017/01/14 00:04:33, skym wrote: > auto! Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:77: void LoadMetadata(); On 2017/01/13 23:31:53, Patrick Noland wrote: > I'll add a comment for this function. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:18: #include "base/strings/utf_string_conversions.h" On 2017/01/14 00:04:34, skym wrote: > Remove this, duplicate with line 17. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:21: #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h" On 2017/01/14 00:04:34, skym wrote: > Remove this, duplicate with line 5. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:102: AutofillSpecifics CreateAutofillSpecificsFromEntry(const AutofillEntry& entry) { On 2017/01/14 00:04:34, skym wrote: > Part of me wonders if maybe it'd be the most clean if all the tests start off > creating either model or specifics objects. And then convert in a single > direction? I feel like the tests are a bit inconsistent, WDYT? This adds a couple of lines to each test but is more consistent and makes the helper code cleaner so overall it's a net win. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:118: // Time deep in the past would cause Autocomplete sync to discard the On 2017/01/14 00:04:34, skym wrote: > Technically not true right now. See line 144, we don't do anything. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:124: return AutofillEntry(AutofillKey(ASCIIToUTF16(name), ASCIIToUTF16(value)), On 2017/01/14 00:04:34, skym wrote: > Why ASCIIToUTF16 instead of base::UTF8ToUTF16? Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:164: table_.UpdateModelTypeState(syncer::AUTOFILL, model_type_state); On 2017/01/14 00:04:34, skym wrote: > Does this do anything? Do the tests pass if you take this out? They all fail. LoadMetadata fails in the absence of stored ModelTypeState, which reports an unexpected error, causing FakeModelTypeChangeProcessor to fail the test. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:175: AutocompleteSyncBridge* bridge() { return bridge_.get(); } On 2017/01/14 00:39:02, maxbogue wrote: > nit: we almost always have these sorts of accessors at the bottom of the public > section. just another little consistency thing. Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:182: CreateModelTypeChangeProcessor(syncer::ModelType type, On 2017/01/14 00:39:02, maxbogue wrote: > this can be private Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:272: AutofillTable table_; On 2017/01/14 00:39:02, maxbogue wrote: > was there an ownership reason this one moved? No, I'll put it back where it was. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:476: const AutofillEntry added_entry1( On 2017/01/14 00:39:02, maxbogue wrote: > use = syntax (added_entry1 = CreateAutofillEntry...) Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:478: const AutofillEntry added_entry2( On 2017/01/14 00:39:02, maxbogue wrote: > use = syntax Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:510: const AutofillEntry added_entry( On 2017/01/14 00:39:02, maxbogue wrote: > = Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:528: VerifyEqual(recorded_specifics, specifics); On 2017/01/14 00:04:34, skym wrote: > Seems like these couple lines keep repeating themselves. Can you create a helper > method to verify the put_multimap looks good? Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:530: const AutofillEntry updated_entry( On 2017/01/14 00:39:02, maxbogue wrote: > = Done. https://codereview.chromium.org/2620783002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:552: const AutofillEntry deleted_entry( On 2017/01/14 00:39:02, maxbogue wrote: > = Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:431: if (error) { What do you think of inlining this? if (Optional<ModelError> error = metadata_change_list->TakeError()) { change_processor()->ReportError(error.value()); } I know, using the return value of an assignment is weird, but with optionals, suddenly I feel like it becomes kinda idiomatic.
lgtm w/ a few little nits! https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:256: CreateModelTypeChangeProcessor(syncer::ModelType type, Functions go above member variables: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:515: const auto& deleted_entries = processor()->delete_set().find(storage_key); This would read more clearly if it had iter in the name, like deleted_iter or something. Also plain auto is probably fine; it has to copy it anyways doesn't it? https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:516: EXPECT_NE(deleted_entries, processor()->delete_set().end()); switch order; (not) expected thing comes first. https://codereview.chromium.org/2620783002/diff/140001/components/sync/model/... File components/sync/model/recording_model_type_change_processor.h (right): https://codereview.chromium.org/2620783002/diff/140001/components/sync/model/... components/sync/model/recording_model_type_change_processor.h:30: const inline std::multimap<std::string, std::unique_ptr<EntityData>>& You missed removing these inline keywords.
https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:431: if (error) { On 2017/01/17 23:50:10, skym wrote: > What do you think of inlining this? > > if (Optional<ModelError> error = metadata_change_list->TakeError()) { > change_processor()->ReportError(error.value()); > } > > I know, using the return value of an assignment is weird, but with optionals, > suddenly I feel like it becomes kinda idiomatic. At least in the past, this could cause compiler warnings on some platforms. If the {} are removed from the one-line body here, I think it reads OK without doing this. I always struggle to read assignment-inside-conditional, FWIW.
The CQ bit was checked by pnoland@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.
On 2017/01/18 00:33:27, Peter Kasting wrote: > https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... > File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc > (right): > > https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... > components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:431: if > (error) { > On 2017/01/17 23:50:10, skym wrote: > > What do you think of inlining this? > > > > if (Optional<ModelError> error = metadata_change_list->TakeError()) { > > change_processor()->ReportError(error.value()); > > } > > > > I know, using the return value of an assignment is weird, but with optionals, > > suddenly I feel like it becomes kinda idiomatic. > > At least in the past, this could cause compiler warnings on some platforms. > > If the {} are removed from the one-line body here, I think it reads OK without > doing this. I always struggle to read assignment-inside-conditional, FWIW. The trybots didn't complain about assignment-in-conditional. I think I'll keep it, as it does seem to be idiomatic for optionals.
autofill lgtm, but mostly leaning on expertise from sync reviewers
https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:431: if (error) { On 2017/01/17 23:50:10, skym wrote: > What do you think of inlining this? > > if (Optional<ModelError> error = metadata_change_list->TakeError()) { > change_processor()->ReportError(error.value()); > } > > I know, using the return value of an assignment is weird, but with optionals, > suddenly I feel like it becomes kinda idiomatic. I like this structure for optionals. Done. https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:256: CreateModelTypeChangeProcessor(syncer::ModelType type, On 2017/01/18 00:01:42, maxbogue wrote: > Functions go above member variables: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:515: const auto& deleted_entries = processor()->delete_set().find(storage_key); On 2017/01/18 00:01:42, maxbogue wrote: > This would read more clearly if it had iter in the name, like deleted_iter or > something. Also plain auto is probably fine; it has to copy it anyways doesn't > it? I think I'll just inline it actually, since it's only used in the line below. https://codereview.chromium.org/2620783002/diff/140001/components/autofill/co... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:516: EXPECT_NE(deleted_entries, processor()->delete_set().end()); On 2017/01/18 00:01:42, maxbogue wrote: > switch order; (not) expected thing comes first. Done. https://codereview.chromium.org/2620783002/diff/140001/components/sync/model/... File components/sync/model/recording_model_type_change_processor.h (right): https://codereview.chromium.org/2620783002/diff/140001/components/sync/model/... components/sync/model/recording_model_type_change_processor.h:30: const inline std::multimap<std::string, std::unique_ptr<EntityData>>& On 2017/01/18 00:01:42, maxbogue wrote: > You missed removing these inline keywords. Done.
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, skym@chromium.org, maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/2620783002/#ps160001 (title: "test assignment in conditional for warnings")
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
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by pnoland@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 pnoland@chromium.org
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, maxbogue@chromium.org, pkasting@chromium.org, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2620783002/#ps180001 (title: "rebase")
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": 180001, "attempt_start_ts": 1484854868467780,
"parent_rev": "dd4298c2034b2f5cf6e58239eb2057e7f91046b4", "commit_rev":
"1e4b08621e36a68cc141e04b50a55f1a09cec9ce"}
Message was sent while issue was closed.
Description was changed from ========== [sync] Handle local changes in AutocompleteSyncBridge Implement AutofillEntriesChanged to listen for local autofill changes. Add logic that propagates these changes to the processor. Extract RecordingModelTypeChangeProcessor for general use. R=pavely@chromium.org,maxbogue@chromium.org,mathp@chromium.org,pkasting@chrom... BUG=679517 ========== to ========== [sync] Handle local changes in AutocompleteSyncBridge Implement AutofillEntriesChanged to listen for local autofill changes. Add logic that propagates these changes to the processor. Extract RecordingModelTypeChangeProcessor for general use. R=pavely@chromium.org,maxbogue@chromium.org,mathp@chromium.org,pkasting@chrom... BUG=679517 Review-Url: https://codereview.chromium.org/2620783002 Cr-Commit-Position: refs/heads/master@{#444840} Committed: https://chromium.googlesource.com/chromium/src/+/1e4b08621e36a68cc141e04b50a5... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/1e4b08621e36a68cc141e04b50a5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
