[sync] skeleton implementation of AutocompleteSyncBridge

components/autofill/core/browser/webdata/autocomplete_sync_bridge.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.

[maxbogue, 2016/11/18 22:22:36]

line below

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+#ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_WEBDATA_AUTOCOMPLETE_SYNC_BRIDGE_H_
+#define COMPONENTS_AUTOFILL_CORE_BROWSER_WEBDATA_AUTOCOMPLETE_SYNC_BRIDGE_H_
+
+#include "base/scoped_observer.h"
+#include "base/supports_user_data.h"
+#include "base/threading/non_thread_safe.h"
+#include "components/autofill/core/browser/webdata/autofill_change.h"
+#include "components/autofill/core/browser/webdata/autofill_webdata_service_observer.h"
+#include "components/sync/model/model_type_sync_bridge.h"
+
+namespace syncer {
+class SyncError;
+}
+
+namespace autofill {

[maxbogue, 2016/11/18 22:22:36]

line below

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+class AutofillTable;
+class AutofillWebDataBackend;
+class AutofillWebDataService;
+
+class AutocompleteSyncBridge : public base::SupportsUserData::Data,
+                               public syncer::ModelTypeSyncBridge,
+                               public AutofillWebDataServiceObserverOnDBThread,
+                               public base::NonThreadSafe {

[maxbogue, 2016/11/18 22:22:36]

It's discouraged to use this class and encouraged to have a ThreadChecker member
variable instead. Something something composition over inheritance.

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+ public:
+  ~AutocompleteSyncBridge() override;

[maxbogue, 2016/11/18 22:22:36]

destructor generally goes below constructors

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+  AutocompleteSyncBridge();
+  AutocompleteSyncBridge(
+      AutofillWebDataBackend* backend,
+      const ChangeProcessorFactory& change_processor_factory);
+
+  static void CreateForWebDataServiceAndBackend(
+      AutofillWebDataService* web_data_service,
+      AutofillWebDataBackend* web_data_backend);
+
+  static AutocompleteSyncBridge* FromWebDataService(
+      AutofillWebDataService* web_data_service);
+
+  static syncer::ModelType model_type() { return syncer::AUTOFILL; }

[maxbogue, 2016/11/18 22:22:36]

I don't think you need this.

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+
+  // syncer::ModelTypeService implementation.
+  std::unique_ptr<syncer::MetadataChangeList> CreateMetadataChangeList()
+      override;
+  syncer::SyncError MergeSyncData(
+      std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
+      syncer::EntityDataMap entity_data_map) override;
+  syncer::SyncError ApplySyncChanges(
+      std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
+      syncer::EntityChangeList entity_changes) override;
+  void GetData(StorageKeyList storage_keys, DataCallback callback) override;
+  void GetAllData(DataCallback callback) override;
+  std::string GetClientTag(const syncer::EntityData& entity_data) override;

[pavely, 2016/11/21 22:54:19]

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.

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+  std::string GetStorageKey(const syncer::EntityData& entity_data) override;
+  syncer::ConflictResolution ResolveConflict(
+      const syncer::EntityData& local_data,
+      const syncer::EntityData& remote_data) const override;
+
+  // AutofillWebDataServiceObserverOnDBThread implementation.
+  void AutofillEntriesChanged(const AutofillChangeList& changes) override;
+
+ private:
+  // The |web_data_backend_| is expected to outlive |this|.

[maxbogue, 2016/11/18 22:22:36]

Generally you want to use stronger language ("is guaranteed to outlive |this|"),
because if you can't then you have a problem.

[pavely, 2016/11/21 22:54:19]

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.

[Patrick Noland, 2016/11/22 18:51:04]

Done.

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+  AutofillWebDataBackend* const web_data_backend_;
+
+  ScopedObserver<AutofillWebDataBackend, AutocompleteSyncBridge>
+      scoped_observer_;
+
+  static std::string KeyToTag(std::string storage_key);

[maxbogue, 2016/11/18 22:22:36]

There isn't really a reason to have a private static function instead of an
anonymous one in the cc file.

[Patrick Noland, 2016/11/22 18:51:04]

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.

+};
+}  // namespace autofill

[maxbogue, 2016/11/18 22:22:36]

Add blank line above this.

[Patrick Noland, 2016/11/22 18:51:04]

Done.

+
+#endif  // COMPONENTS_AUTOFILL_CORE_BROWSER_WEBDATA_AUTOCOMPLETE_SYNC_BRIDGE_H_