Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2508263003:
[sync] skeleton implementation of AutocompleteSyncBridge (Closed)
|
Created:
4 years, 1 month ago by Patrick Noland Modified:
4 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, mac-reviews_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, sync-reviews_chromium.org, sdefresne+watch_chromium.org, Ilya Sherman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sync] skeleton implementation of AutocompleteSyncBridge
AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of
the AutocompleteSyncableService. The goal is for identical
functionality, but with reduced memory usage due to unifying the storage
of sync metadata and autofill data.
R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org
BUG=666406
Committed: https://crrev.com/aa02c72c403675f9b2f1e5636a6e1c820959accf
Cr-Commit-Position: refs/heads/master@{#434796}
Patch Set 1 : [sync] skeleton implementation of autocomplete_sync_bridge #
Total comments: 54
Patch Set 2 : Address Max and Pavel's comments #
Total comments: 20
Patch Set 3 : Max's comments; merged chrome_sync_client #Messages
Total messages: 55 (39 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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 #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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.
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [sync] skeleton implementation of autocomplete_sync_bridge R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 ========== to ========== [sync] skeleton implementation of AutocompleteSyncBridge AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of the AutocompleteSyncableService. The goal is for identical functionality, but with reduced memory usage due to unifying the storage of sync metadata and autofill data. R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 ==========
pavely, PTAL at everything isherman, PTAL at components/autofill/* pkasting, PTAL at components/webdata/*
isherman@chromium.org changed reviewers: + mathp@chromium.org
I haven't been actively working on Autofill for quite a while, and this change seems potentially subtle in terms of things to pay attention to while reviewing. So, +Mathieu to review instead of me. Thanks!
maxbogue@chromium.org changed reviewers: + maxbogue@chromium.org
Mostly looks good, just a bunch of nitpicky comments. Also, please manually line-wrap your description because for some reason Rietveld doesn't. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:3: // found in the LICENSE file. line below https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:15: no blank line; it's all one section. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:18: namespace { Add line below. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:30: } // namespace Add line above. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:34: const char kAutocompleteEntryNamespaceTag[] = "autofill_entry|"; The whole anonymous namespace can be inside the autofill namespace and can include this constant. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:73: return base::MakeUnique<syncer::AccumulatingMetadataChangeList>(); You want to make your own subclass of InMemoryMetadataChangeList; AMCL is for ModelTypeStore users specifically, which is why it is in model_impl (which you shouldn't be including from). You can leave that for another CL and just return nullptr here. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:118: syncer::ConflictResolution AutocompleteSyncBridge::ResolveConflict( This one is optional to override and should probably be omitted unless you know we're gonna want a custom implementation. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:3: // found in the LICENSE file. line below https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:18: namespace autofill { line below https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:26: public base::NonThreadSafe { It's discouraged to use this class and encouraged to have a ThreadChecker member variable instead. Something something composition over inheritance. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:28: ~AutocompleteSyncBridge() override; destructor generally goes below constructors https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:41: static syncer::ModelType model_type() { return syncer::AUTOFILL; } I don't think you need this. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:64: // The |web_data_backend_| is expected to outlive |this|. Generally you want to use stronger language ("is guaranteed to outlive |this|"), because if you can't then you have a problem. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:70: static std::string KeyToTag(std::string storage_key); There isn't really a reason to have a private static function instead of an anonymous one in the cc file. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:72: } // namespace autofill Add blank line above this. https://codereview.chromium.org/2508263003/diff/60001/components/browser_sync... File components/browser_sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/browser_sync... components/browser_sync/profile_sync_components_factory_impl.cc:163: // db_thread_, error_callback, sync_client_, web_data_service_)); Guessing you didn't mean to leave this line in. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... File components/sync/driver/model_type_controller.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:29: namespace { line below https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:30: static void CallOnSyncStartingHelper( static here and on the other two is actually redundant with the anonymous namespace: http://stackoverflow.com/questions/25724787/static-functions-outside-classes https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:57: } // namespace line above https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... File components/sync/driver/sync_driver_switches.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/sync_driver_switches.cc:40: // SyncableService based or ModelTypeBridge based implementation is used for ModelTypeSyncBridge https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/sync_driver_switches.cc:46: const base::Feature kSyncUSSAutocomplete{"EnableSyncUSSAutocomplete", I actually dislike that the DeviceInfo one has "Enable" in the string and would rather we omit it from this one. It's weird that the "EnableSyncUSSAutocomplete" flag can be enabled or disabled. Reads better to just say "SyncUSSAutocomplete" is enabled or disabled.
maxbogue@chromium.org changed reviewers: - maxbogue@chromium.org
maxbogue@chromium.org changed reviewers: - isherman@chromium.org
Moving Ilya to cc.
components/webdata_services/ LGTM
Description was changed from ========== [sync] skeleton implementation of AutocompleteSyncBridge AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of the AutocompleteSyncableService. The goal is for identical functionality, but with reduced memory usage due to unifying the storage of sync metadata and autofill data. R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 ========== to ========== [sync] skeleton implementation of AutocompleteSyncBridge AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of the AutocompleteSyncableService. The goal is for identical functionality, but with reduced memory usage due to unifying the storage of sync metadata and autofill data. R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 ==========
https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:107: return KeyToTag(storage_key); KeyToTag is just prepending storage_key with prefix. I would simply do it here inline. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:129: std::string AutocompleteSyncBridge::KeyToTag(std::string storage_key) { Could you pass parameter as const std::string&. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:54: std::string GetClientTag(const syncer::EntityData& entity_data) override; GetClientTag and GetStorageKey affect how entries are stored locally and how they are identified on the server. Unlike the code in other functions, formats of these strings will not easily change in the future. Could you call out formats of both of them in a comment. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:64: // The |web_data_backend_| is expected to outlive |this|. On 2016/11/18 22:22:36, maxbogue wrote: > Generally you want to use stronger language ("is guaranteed to outlive |this|"), > because if you can't then you have a problem. You can mention that AutocompleteSyncBridge is owned by AutofillWebDataBackend through SupportsUserData. This will explain why web_data_backend_ is valid throughout lifetime of the bridge. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... File components/sync/driver/model_type_controller.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:37: bridge->OnSyncStarting(std::move(error_handler), callback); You need to explicitly check if bridge is still valid: if (bridge) bridge->OnSyncStarting(...) Previously base::Bind & co was doing it for you. (same in the other two functions) https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:116: base::Bind(&CallGetStatusCountersHelper, sync_client_, type(), callback)); Unrelated to your change... Do we need BindToCurrentThread(callback) here as well?
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Pavel, Mathieu, PTAL https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:3: // found in the LICENSE file. On 2016/11/18 22:22:35, maxbogue wrote: > line below Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:15: On 2016/11/18 22:22:35, maxbogue wrote: > no blank line; it's all one section. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:18: namespace { On 2016/11/18 22:22:35, maxbogue wrote: > Add line below. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:30: } // namespace On 2016/11/18 22:22:35, maxbogue wrote: > Add line above. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:34: const char kAutocompleteEntryNamespaceTag[] = "autofill_entry|"; On 2016/11/18 22:22:35, maxbogue wrote: > The whole anonymous namespace can be inside the autofill namespace and can > include this constant. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:73: return base::MakeUnique<syncer::AccumulatingMetadataChangeList>(); On 2016/11/18 22:22:36, maxbogue wrote: > You want to make your own subclass of InMemoryMetadataChangeList; AMCL is for > ModelTypeStore users specifically, which is why it is in model_impl (which you > shouldn't be including from). You can leave that for another CL and just return > nullptr here. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:107: return KeyToTag(storage_key); On 2016/11/21 22:54:19, pavely wrote: > KeyToTag is just prepending storage_key with prefix. I would simply do it here > inline. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:118: syncer::ConflictResolution AutocompleteSyncBridge::ResolveConflict( On 2016/11/18 22:22:35, maxbogue wrote: > This one is optional to override and should probably be omitted unless you know > we're gonna want a custom implementation. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:129: std::string AutocompleteSyncBridge::KeyToTag(std::string storage_key) { On 2016/11/21 22:54:19, pavely wrote: > Could you pass parameter as const std::string&. Not necessary since I'm removing KeyToTag entirely. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:3: // found in the LICENSE file. On 2016/11/18 22:22:36, maxbogue wrote: > line below Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:18: namespace autofill { On 2016/11/18 22:22:36, maxbogue wrote: > line below Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:26: public base::NonThreadSafe { On 2016/11/18 22:22:36, maxbogue wrote: > It's discouraged to use this class and encouraged to have a ThreadChecker member > variable instead. Something something composition over inheritance. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:28: ~AutocompleteSyncBridge() override; On 2016/11/18 22:22:36, maxbogue wrote: > destructor generally goes below constructors Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:41: static syncer::ModelType model_type() { return syncer::AUTOFILL; } On 2016/11/18 22:22:36, maxbogue wrote: > I don't think you need this. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:54: std::string GetClientTag(const syncer::EntityData& entity_data) override; On 2016/11/21 22:54:19, pavely wrote: > GetClientTag and GetStorageKey affect how entries are stored locally and how > they are identified on the server. Unlike the code in other functions, formats > of these strings will not easily change in the future. Could you call out > formats of both of them in a comment. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:64: // The |web_data_backend_| is expected to outlive |this|. On 2016/11/18 22:22:36, maxbogue wrote: > Generally you want to use stronger language ("is guaranteed to outlive |this|"), > because if you can't then you have a problem. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:64: // The |web_data_backend_| is expected to outlive |this|. On 2016/11/21 22:54:19, pavely wrote: > On 2016/11/18 22:22:36, maxbogue wrote: > > Generally you want to use stronger language ("is guaranteed to outlive > |this|"), > > because if you can't then you have a problem. > > You can mention that AutocompleteSyncBridge is owned by AutofillWebDataBackend > through SupportsUserData. This will explain why web_data_backend_ is valid > throughout lifetime of the bridge. Done. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:70: static std::string KeyToTag(std::string storage_key); On 2016/11/18 22:22:36, maxbogue wrote: > There isn't really a reason to have a private static function instead of an > anonymous one in the cc file. It's possible that unit tests will need it(that's why it's private in the syncable service) but that bridge can be crossed when the tests are written. https://codereview.chromium.org/2508263003/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:72: } // namespace autofill On 2016/11/18 22:22:36, maxbogue wrote: > Add blank line above this. Done. https://codereview.chromium.org/2508263003/diff/60001/components/browser_sync... File components/browser_sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/browser_sync... components/browser_sync/profile_sync_components_factory_impl.cc:163: // db_thread_, error_callback, sync_client_, web_data_service_)); On 2016/11/18 22:22:36, maxbogue wrote: > Guessing you didn't mean to leave this line in. Done. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... File components/sync/driver/model_type_controller.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:29: namespace { On 2016/11/18 22:22:36, maxbogue wrote: > line below Done. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:30: static void CallOnSyncStartingHelper( On 2016/11/18 22:22:36, maxbogue wrote: > static here and on the other two is actually redundant with the anonymous > namespace: > http://stackoverflow.com/questions/25724787/static-functions-outside-classes Done. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:37: bridge->OnSyncStarting(std::move(error_handler), callback); On 2016/11/21 22:54:19, pavely wrote: > You need to explicitly check if bridge is still valid: > if (bridge) > bridge->OnSyncStarting(...) > > Previously base::Bind & co was doing it for you. > > (same in the other two functions) Done. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:57: } // namespace On 2016/11/18 22:22:36, maxbogue wrote: > line above Done. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/model_type_controller.cc:116: base::Bind(&CallGetStatusCountersHelper, sync_client_, type(), callback)); On 2016/11/21 22:54:19, pavely wrote: > Unrelated to your change... > Do we need BindToCurrentThread(callback) here as well? As far as I can tell, yes. Done. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... File components/sync/driver/sync_driver_switches.cc (right): https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/sync_driver_switches.cc:40: // SyncableService based or ModelTypeBridge based implementation is used for On 2016/11/18 22:22:36, maxbogue wrote: > ModelTypeSyncBridge Done. https://codereview.chromium.org/2508263003/diff/60001/components/sync/driver/... components/sync/driver/sync_driver_switches.cc:46: const base::Feature kSyncUSSAutocomplete{"EnableSyncUSSAutocomplete", On 2016/11/18 22:22:36, maxbogue wrote: > I actually dislike that the DeviceInfo one has "Enable" in the string and would > rather we omit it from this one. It's weird that the "EnableSyncUSSAutocomplete" > flag can be enabled or disabled. Reads better to just say "SyncUSSAutocomplete" > is enabled or disabled. Done.
Another little drive-by, sorry! https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:21: // static remove; not needed (it's obviously not a method since it's not on a class) https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:29: // static same https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:34: } // namespace line above https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:31: ~AutocompleteSyncBridge() override; under both constructors plz https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:77: } // namespace autofill line above this please. sorry to be so picky about spacing but since I've changed so many namespaces around I try to keep them consistent :S https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... File components/sync/driver/model_type_controller.cc (right): https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:31: // static remove https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:44: // static remove https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:55: // static remove https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:126: BindToCurrentThread(callback))); Whoops, I missed Pavel's comment before... it's actually not needed, as PSS binds it: https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_ser... it's also mentioned in the method comment: https://cs.chromium.org/chromium/src/components/sync/driver/data_type_control... I can't remember why the other one doesn't, but yeah. Don't need it here.
lgtm % max's comments
just nits, thanks lgtm https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... components/browser_sync/profile_sync_components_factory_impl.cc:155: // Autofill sync is enabled by default. Register unless explicitly I wish this was called "Autocomplete" now. What we think of Autofill is "Autofill profile", below. Can you add a comment?
On 2016/11/23 12:47:21, Mathieu Perreault wrote: > just nits, thanks lgtm > > https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... > File components/browser_sync/profile_sync_components_factory_impl.cc (right): > > https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... > components/browser_sync/profile_sync_components_factory_impl.cc:155: // Autofill > sync is enabled by default. Register unless explicitly > I wish this was called "Autocomplete" now. What we think of Autofill is > "Autofill profile", below. Can you add a comment? Could you share how you tested this? Is there a plan for tests?
On 2016/11/23 12:48:15, Mathieu Perreault wrote: > On 2016/11/23 12:47:21, Mathieu Perreault wrote: > > just nits, thanks lgtm > > > > > https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... > > File components/browser_sync/profile_sync_components_factory_impl.cc (right): > > > > > https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... > > components/browser_sync/profile_sync_components_factory_impl.cc:155: // > Autofill > > sync is enabled by default. Register unless explicitly > > I wish this was called "Autocomplete" now. What we think of Autofill is > > "Autofill profile", below. Can you add a comment? > > Could you share how you tested this? Is there a plan for tests? Most of this change isn't testable, since most of the functional surface area is unimplemented. However, I have tested a superset of this changelist that includes enough functionality to start the bridge and read/write data from/to the autofill database. The larger plan for testing the migration to USS includes unit tests for the AutocompleteSyncBridge and related new components as well as integration tests for the Autocomplete model type. The rollout will be controlled by a finch flag, and we'll do it gradually just as we have with the first type we migrated.
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.
https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:21: // static On 2016/11/22 19:07:23, maxbogue wrote: > remove; not needed (it's obviously not a method since it's not on a class) Done. https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:29: // static On 2016/11/22 19:07:23, maxbogue wrote: > same Done. https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:34: } // namespace On 2016/11/22 19:07:23, maxbogue wrote: > line above Done. https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:31: ~AutocompleteSyncBridge() override; On 2016/11/22 19:07:23, maxbogue wrote: > under both constructors plz Done. https://codereview.chromium.org/2508263003/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:77: } // namespace autofill On 2016/11/22 19:07:23, maxbogue wrote: > line above this please. sorry to be so picky about spacing but since I've > changed so many namespaces around I try to keep them consistent :S Done. https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/2508263003/diff/80001/components/browser_sync... components/browser_sync/profile_sync_components_factory_impl.cc:155: // Autofill sync is enabled by default. Register unless explicitly On 2016/11/23 12:47:21, Mathieu Perreault wrote: > I wish this was called "Autocomplete" now. What we think of Autofill is > "Autofill profile", below. Can you add a comment? Done. https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... File components/sync/driver/model_type_controller.cc (right): https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:31: // static On 2016/11/22 19:07:23, maxbogue wrote: > remove Done. https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:44: // static On 2016/11/22 19:07:23, maxbogue wrote: > remove Done. https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:55: // static On 2016/11/22 19:07:23, maxbogue wrote: > remove Done. https://codereview.chromium.org/2508263003/diff/80001/components/sync/driver/... components/sync/driver/model_type_controller.cc:126: BindToCurrentThread(callback))); On 2016/11/22 19:07:23, maxbogue wrote: > Whoops, I missed Pavel's comment before... it's actually not needed, as PSS > binds it: > > https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_ser... > > it's also mentioned in the method comment: > > https://cs.chromium.org/chromium/src/components/sync/driver/data_type_control... > > I can't remember why the other one doesn't, but yeah. Don't need it here. 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, mathp@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2508263003/#ps100001 (title: "Max's comments; merged chrome_sync_client")
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": 100001, "attempt_start_ts": 1480378202530770,
"parent_rev": "bd60c84ca0fb5daf9b09f21c50e63473f0553900", "commit_rev":
"8e9663356b64fe04e49e9babe5b7a83453a7f7ae"}
Message was sent while issue was closed.
Description was changed from ========== [sync] skeleton implementation of AutocompleteSyncBridge AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of the AutocompleteSyncableService. The goal is for identical functionality, but with reduced memory usage due to unifying the storage of sync metadata and autofill data. R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 ========== to ========== [sync] skeleton implementation of AutocompleteSyncBridge AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of the AutocompleteSyncableService. The goal is for identical functionality, but with reduced memory usage due to unifying the storage of sync metadata and autofill data. R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [sync] skeleton implementation of AutocompleteSyncBridge AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of the AutocompleteSyncableService. The goal is for identical functionality, but with reduced memory usage due to unifying the storage of sync metadata and autofill data. R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 ========== to ========== [sync] skeleton implementation of AutocompleteSyncBridge AutocompleteSyncBridge is the USS(unified sync and storage) equivalent of the AutocompleteSyncableService. The goal is for identical functionality, but with reduced memory usage due to unifying the storage of sync metadata and autofill data. R=pavely@chromium.org,isherman@chromium.org,pkasting@chromium.org BUG=666406 Committed: https://crrev.com/aa02c72c403675f9b2f1e5636a6e1c820959accf Cr-Commit-Position: refs/heads/master@{#434796} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aa02c72c403675f9b2f1e5636a6e1c820959accf Cr-Commit-Position: refs/heads/master@{#434796}
Message was sent while issue was closed.
Patchset #4 (id:120001) has been deleted |
