[sync] skeleton implementation of TypedURLSyncBridge

components/history/core/browser/typed_url_sync_bridge.h

Index: components/history/core/browser/typed_url_sync_bridge.h
diff --git a/components/history/core/browser/typed_url_sync_bridge.h b/components/history/core/browser/typed_url_sync_bridge.h
new file mode 100644
index 0000000000000000000000000000000000000000..4b62e620ed5a2c70cee09da177f35b7c190cf68e
--- /dev/null
+++ b/components/history/core/browser/typed_url_sync_bridge.h
@@ -0,0 +1,67 @@
+// Copyright 2017 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.
+
+#ifndef COMPONENTS_HISTORY_CORE_BROWSER_TYPED_URL_SYNC_BRIDGE_H_
+#define COMPONENTS_HISTORY_CORE_BROWSER_TYPED_URL_SYNC_BRIDGE_H_
+
+#include "components/history/core/browser/history_backend_observer.h"
+#include "components/sync/model/metadata_change_list.h"
+#include "components/sync/model/model_type_sync_bridge.h"
+#include "components/sync/model/sync_error.h"
+
+namespace history {
+
+class TypedURLSyncBridge : public syncer::ModelTypeSyncBridge,
+                           public history::HistoryBackendObserver {
+ public:
+  TypedURLSyncBridge(HistoryBackend* history_backend,
+                     const ChangeProcessorFactory& change_processor_factory);
+  ~TypedURLSyncBridge() override;
+
+  // syncer::ModelTypeService implementation.
+  std::unique_ptr<syncer::MetadataChangeList> CreateMetadataChangeList()
+      override;
+  base::Optional<syncer::ModelError> MergeSyncData(
+      std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
+      syncer::EntityDataMap entity_data_map) override;
+  base::Optional<syncer::ModelError> 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;
+  // Generate a tag that uniquely identifies |entity_data| across all data
+  // types. This is used to identify the entity on the server. The string will
+  // be the url (URLRow::url()::spec()).

[skym, 2017/03/07 18:08:04]

I'm still finding these comments kind of odd. I should have pushed back on these
from autocomplete's bridge's .h. "This is used to identify the entity on the
server" is not strictly true either.

What do you think of moving this to the definition of the method in the .cc and
changing to something like

// Must be exactly the value of GURL::spec() for backwards comparability with
the previous (Directory + SyncableService) iteration of sync integration. This
can be large but it is assumed that this is not held in memory at steady state.

[skym, 2017/03/07 19:25:22]

I just posted a CL to make autocomplete's comments a little better and more
inline of how i'm suggesting these comments should look.
https://codereview.chromium.org/2737023002/

[Gang Wu, 2017/03/31 19:35:58]

Done.

+  std::string GetClientTag(const syncer::EntityData& entity_data) override;
+  // Generate a string key uniquely identifying |entity_data| in the context of

[skym, 2017/03/07 18:08:04]

Same as above, lets move to .cc, and what do you think of the following text:

// Prefer to use URLRow::id() to uniquely identify entities when coordinating
with sync because it has a significantly low memory cost then a URL.

[skym, 2017/03/07 18:53:51]

This returns a string right.. Are we representing this number as base-10? Or
something else more efficient?

[Gang Wu, 2017/03/31 19:35:58]

base-10

+  // local storage. The string will be the id in the “urls” table
+  // (URLRow::id()).
+  std::string GetStorageKey(const syncer::EntityData& entity_data) override;
+
+  // history::HistoryBackendObserver:
+  void OnURLVisited(history::HistoryBackend* history_backend,
+                    ui::PageTransition transition,
+                    const history::URLRow& row,
+                    const history::RedirectList& redirects,
+                    base::Time visit_time) override;
+  void OnURLsModified(history::HistoryBackend* history_backend,
+                      const history::URLRows& changed_urls) override;
+  void OnURLsDeleted(history::HistoryBackend* history_backend,
+                     bool all_history,
+                     bool expired,
+                     const history::URLRows& deleted_rows,
+                     const std::set<GURL>& favicon_urls) override;
+
+ private:
+  // The backend we're syncing local changes from and sync changes to.
+  HistoryBackend* const history_backend_;
+
+  base::SequenceChecker sequence_checker_;

[skym, 2017/03/07 18:08:04]

Can you add a comment about why we're using a sequence checker and not a thread
checker?

[Gang Wu, 2017/03/31 19:35:58]

comment added, more detail here.
https://codereview.chromium.org/2591643004

+
+  DISALLOW_COPY_AND_ASSIGN(TypedURLSyncBridge);
+};
+
+}  // namespace history
+
+#endif  // COMPONENTS_HISTORY_CORE_BROWSER_TYPED_URL_SYNC_BRIDGE_H_