Hook up client side of per-datatype GetUpdates.

Which datatypes are fetched is dictated by the ModelSafeRoutingInfo.  We change the semantic of the ModelSafeRoutingInfo so that datatypes which should not be synced are not in the map.  We will do GetUpdates for GROUP_PASSIVE datatypes.

BUG=29905
TEST=included unit tests

Review URL: http://codereview.chromium.org/594024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@38726 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 215 insertions, 64 deletions

diff --git a/chrome/browser/sync/engine/build_commit_command.cc b/chrome/browser/sync/engine/build_commit_command.cc
index b945fd7..1cf0ae6 100755
--- a/chrome/browser/sync/engine/build_commit_command.cc
+++ b/chrome/browser/sync/engine/build_commit_command.cc
@@ -20,9 +20,11 @@ using std::set;
 using std::string;
 using std::vector;
 using syncable::ExtendedAttribute;
+using syncable::IS_DEL;
 using syncable::Id;
 using syncable::MutableEntry;
 using syncable::SPECIFICS;
+using syncable::UNSPECIFIED;
 
 namespace browser_sync {
 
@@ -96,6 +98,9 @@ void BuildCommitCommand::ExecuteImpl(SyncSession* session) {
     // This is the only change we make to the entry in this function.
     meta_entry.Put(syncable::SYNCING, true);
 
+    DCHECK(0 != session->routing_info().count(meta_entry.GetModelType()))
+        << "Committing change to datatype that's not actively enabled.";
+
     string name = meta_entry.Get(syncable::NON_UNIQUE_NAME);
     CHECK(!name.empty());  // Make sure this isn't an update.
     sync_entry->set_name(name);
@@ -159,18 +164,21 @@ void BuildCommitCommand::ExecuteImpl(SyncSession* session) {
     }
 
     // Deletion is final on the server, let's move things and then delete them.
-    if (meta_entry.Get(syncable::IS_DEL)) {
+    if (meta_entry.Get(IS_DEL)) {
       sync_entry->set_deleted(true);
-    } else if (meta_entry.Get(SPECIFICS).HasExtension(sync_pb::bookmark)) {
-      // Common data in both new and old protocol.
-      const Id& prev_id = meta_entry.Get(syncable::PREV_ID);
-      string prev_string = prev_id.IsRoot() ? string() : prev_id.GetServerId();
-      sync_entry->set_insert_after_item_id(prev_string);
-
-      // TODO(ncarter): In practice we won't want to send this data twice
-      // over the wire; instead, when deployed servers are able to accept
-      // the new-style scheme, we should abandon the old way.
-      SetOldStyleBookmarkData(&meta_entry, sync_entry);
+    } else {
+      if (meta_entry.Get(SPECIFICS).HasExtension(sync_pb::bookmark)) {
+        // Common data in both new and old protocol.
+        const Id& prev_id = meta_entry.Get(syncable::PREV_ID);
+        string prev_id_string =
+            prev_id.IsRoot() ? string() : prev_id.GetServerId();
+        sync_entry->set_insert_after_item_id(prev_id_string);
+
+        // TODO(ncarter): In practice we won't want to send this data twice
+        // over the wire; instead, when deployed servers are able to accept
+        // the new-style scheme, we should abandon the old way.
+        SetOldStyleBookmarkData(&meta_entry, sync_entry);
+      }
       SetEntrySpecifics(&meta_entry, sync_entry);
     }
   }
diff --git a/chrome/browser/sync/engine/download_updates_command.cc b/chrome/browser/sync/engine/download_updates_command.cc
index 9b75ec2..cc58874 100644
--- a/chrome/browser/sync/engine/download_updates_command.cc
+++ b/chrome/browser/sync/engine/download_updates_command.cc
@@ -42,6 +42,15 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
   LOG(INFO) << "Getting updates from ts " << dir->last_sync_timestamp();
   get_updates->set_from_timestamp(dir->last_sync_timestamp());
 
+  // We want folders for our associated types, always.  If we were to set
+  // this to false, the server would send just the non-container items
+  // (e.g. Bookmark URLs but not their containing folders).
+  get_updates->set_fetch_folders(true);
+
+  // Set the requested_types protobuf field so that we fetch all enabled types.
+  SetRequestedTypes(session->routing_info(),
+                    get_updates->mutable_requested_types());
+
   // Set GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(session->TestAndSetSource());
   get_updates->mutable_caller_info()->set_notifications_enabled(
@@ -62,4 +71,20 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
   status->mutable_updates_response()->CopyFrom(update_response);
 }
 
+void DownloadUpdatesCommand::SetRequestedTypes(
+    const ModelSafeRoutingInfo& routing_info,
+    sync_pb::EntitySpecifics* types) {
+  // The datatypes which should be synced are dictated by the value of the
+  // ModelSafeRoutingInfo.  If a datatype is in the routing info map, it
+  // should be synced (even if it's GROUP_PASSIVE).
+  int requested_type_count = 0;
+  for (ModelSafeRoutingInfo::const_iterator i = routing_info.begin();
+       i != routing_info.end();
+       ++i) {
+    requested_type_count++;
+    syncable::AddDefaultExtensionValue(i->first, types);
+  }
+  DCHECK_LT(0, requested_type_count) << "Doing GetUpdates with empty filter.";
+}
+
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/engine/download_updates_command.h b/chrome/browser/sync/engine/download_updates_command.h
index 6169bb7..cef42fb 100644
--- a/chrome/browser/sync/engine/download_updates_command.h
+++ b/chrome/browser/sync/engine/download_updates_command.h
@@ -6,8 +6,13 @@
 #define CHROME_BROWSER_SYNC_ENGINE_DOWNLOAD_UPDATES_COMMAND_H_
 
 #include "base/basictypes.h"
+#include "chrome/browser/sync/engine/model_safe_worker.h"
 #include "chrome/browser/sync/engine/syncer_command.h"
 
+namespace sync_pb {
+class EntitySpecifics;
+}
+
 namespace browser_sync {
 
 // Downloads updates from the server and places them in the SyncSession.
@@ -19,6 +24,9 @@ class DownloadUpdatesCommand : public SyncerCommand {
   // SyncerCommand implementation.
   virtual void ExecuteImpl(sessions::SyncSession* session);
 
+  void SetRequestedTypes(const ModelSafeRoutingInfo& routing_info,
+                         sync_pb::EntitySpecifics* types);
+
  private:
   DISALLOW_COPY_AND_ASSIGN(DownloadUpdatesCommand);
 };
diff --git a/chrome/browser/sync/engine/download_updates_command_unittest.cc b/chrome/browser/sync/engine/download_updates_command_unittest.cc
new file mode 100755
index 0000000..f66232a
--- /dev/null
+++ b/chrome/browser/sync/engine/download_updates_command_unittest.cc
@@ -0,0 +1,89 @@
+// Copyright (c) 2010 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.
+
+#include "chrome/browser/sync/engine/download_updates_command.h"
+#include "chrome/browser/sync/protocol/autofill_specifics.pb.h"
+#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h"
+#include "chrome/browser/sync/protocol/preference_specifics.pb.h"
+#include "chrome/browser/sync/protocol/sync.pb.h"
+#include "chrome/test/sync/engine/proto_extension_validator.h"
+#include "chrome/test/sync/engine/syncer_command_test.h"
+
+namespace browser_sync {
+
+// A test fixture for tests exercising DownloadUpdatesCommandTest.
+class DownloadUpdatesCommandTest : public SyncerCommandTest {
+ protected:
+  DownloadUpdatesCommandTest() {}
+  DownloadUpdatesCommand command_;
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DownloadUpdatesCommandTest);
+};
+
+TEST_F(DownloadUpdatesCommandTest, SetRequestedTypes) {
+  {
+    SCOPED_TRACE("Several enabled datatypes, spread out across groups.");
+    ModelSafeRoutingInfo routing_info;
+    routing_info[syncable::BOOKMARKS] = GROUP_UI;
+    routing_info[syncable::AUTOFILL] = GROUP_DB;
+    routing_info[syncable::PREFERENCES] = GROUP_PASSIVE;
+    sync_pb::EntitySpecifics get_updates_filter;
+    command_.SetRequestedTypes(routing_info, &get_updates_filter);
+    ProtoExtensionValidator<sync_pb::EntitySpecifics> v(get_updates_filter);
+    v.ExpectHasExtension(sync_pb::autofill);
+    v.ExpectHasExtension(sync_pb::preference);
+    v.ExpectHasExtension(sync_pb::bookmark);
+    v.ExpectNoOtherFieldsOrExtensions();
+  }
+
+  {
+    SCOPED_TRACE("Several enabled datatypes, one work group.");
+    ModelSafeRoutingInfo routing_info;
+    routing_info[syncable::BOOKMARKS] = GROUP_UI;
+    routing_info[syncable::AUTOFILL] = GROUP_UI;
+    routing_info[syncable::PREFERENCES] = GROUP_UI;
+    sync_pb::EntitySpecifics get_updates_filter;
+    command_.SetRequestedTypes(routing_info, &get_updates_filter);
+    ProtoExtensionValidator<sync_pb::EntitySpecifics> v(get_updates_filter);
+    v.ExpectHasExtension(sync_pb::autofill);
+    v.ExpectHasExtension(sync_pb::preference);
+    v.ExpectHasExtension(sync_pb::bookmark);
+    v.ExpectNoOtherFieldsOrExtensions();
+  }
+
+  {
+    SCOPED_TRACE("Bookmarks only.");
+    ModelSafeRoutingInfo routing_info;
+    routing_info[syncable::BOOKMARKS] = GROUP_PASSIVE;
+    sync_pb::EntitySpecifics get_updates_filter;
+    command_.SetRequestedTypes(routing_info, &get_updates_filter);
+    ProtoExtensionValidator<sync_pb::EntitySpecifics> v(get_updates_filter);
+    v.ExpectHasExtension(sync_pb::bookmark);
+    v.ExpectNoOtherFieldsOrExtensions();
+  }
+
+  {
+    SCOPED_TRACE("Autofill only.");
+    ModelSafeRoutingInfo routing_info;
+    routing_info[syncable::AUTOFILL] = GROUP_PASSIVE;
+    sync_pb::EntitySpecifics get_updates_filter;
+    command_.SetRequestedTypes(routing_info, &get_updates_filter);
+    ProtoExtensionValidator<sync_pb::EntitySpecifics> v(get_updates_filter);
+    v.ExpectHasExtension(sync_pb::autofill);
+    v.ExpectNoOtherFieldsOrExtensions();
+  }
+
+  {
+    SCOPED_TRACE("Preferences only.");
+    ModelSafeRoutingInfo routing_info;
+    routing_info[syncable::PREFERENCES] = GROUP_PASSIVE;
+    sync_pb::EntitySpecifics get_updates_filter;
+    command_.SetRequestedTypes(routing_info, &get_updates_filter);
+    ProtoExtensionValidator<sync_pb::EntitySpecifics> v(get_updates_filter);
+    v.ExpectHasExtension(sync_pb::preference);
+    v.ExpectNoOtherFieldsOrExtensions();
+  }
+}
+
+}  // namespace browser_sync
diff --git a/chrome/browser/sync/engine/model_safe_worker.h b/chrome/browser/sync/engine/model_safe_worker.h
index 84a6a2f..e44e342 100644
--- a/chrome/browser/sync/engine/model_safe_worker.h
+++ b/chrome/browser/sync/engine/model_safe_worker.h
@@ -74,7 +74,9 @@ class ModelSafeWorkerRegistrar {
   // Get the current list of active ModelSafeWorkers.  Should be threadsafe.
   virtual void GetWorkers(std::vector<ModelSafeWorker*>* out) = 0;
 
-  // Get the current routing information for all model types.
+  // Get the current routing information for all enabled model types.
+  // If a model type is not enabled (that is, if the syncer should not
+  // be trying to sync it), it is not in this map.
   virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) = 0;
  protected:
   virtual ~ModelSafeWorkerRegistrar() {}
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index 0fdf3a2..4ae26a2 100755
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -568,17 +568,9 @@ bool WriteNode::InitByClientTagLookup(const std::string& tag) {
 void WriteNode::PutModelType(syncable::ModelType model_type) {
   // Set an empty specifics of the appropriate datatype.  The presence
   // of the specific extension will identify the model type.
-  switch (model_type) {
-    case syncable::BOOKMARKS:
-      PutBookmarkSpecificsAndMarkForSyncing(
-          sync_pb::BookmarkSpecifics::default_instance());
-      break;
-    case syncable::PREFERENCES:
-      PutPreferenceSpecificsAndMarkForSyncing(
-          sync_pb::PreferenceSpecifics::default_instance());
-    default:
-      NOTREACHED();
-  }
+  sync_pb::EntitySpecifics specifics;
+  syncable::AddDefaultExtensionValue(model_type, &specifics);
+  PutSpecificsAndMarkForSyncing(specifics);
   DCHECK(GetModelType() == model_type);
 }
 
diff --git a/chrome/browser/sync/engine/syncer_proto_util.h b/chrome/browser/sync/engine/syncer_proto_util.h
index af31251..5bf7931 100755
--- a/chrome/browser/sync/engine/syncer_proto_util.h
+++ b/chrome/browser/sync/engine/syncer_proto_util.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "chrome/browser/sync/syncable/blob.h"
+#include "chrome/browser/sync/syncable/model_type.h"
 #include "chrome/browser/sync/util/sync_types.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"  // For FRIEND_TEST
 
@@ -20,6 +21,7 @@ class SyncName;
 
 namespace sync_pb {
 class ClientToServerResponse;
+class EntitySpecifics;
 }  // namespace sync_pb
 
 namespace browser_sync {
@@ -70,6 +72,12 @@ class SyncerProtoUtil {
   static const std::string& NameFromCommitEntryResponse(
       const CommitResponse_EntryResponse& entry);
 
+  // EntitySpecifics is used as a filter for the GetUpdates message to tell
+  // the server which datatypes to send back.  This adds a datatype so that
+  // it's included in the filter.
+  static void AddToEntitySpecificDatatypesFilter(syncable::ModelType datatype,
+      sync_pb::EntitySpecifics* filter);
+
  private:
   SyncerProtoUtil() {}
 
diff --git a/chrome/browser/sync/engine/syncer_thread_unittest.cc b/chrome/browser/sync/engine/syncer_thread_unittest.cc
index d9da657..5487e07 100644
--- a/chrome/browser/sync/engine/syncer_thread_unittest.cc
+++ b/chrome/browser/sync/engine/syncer_thread_unittest.cc
@@ -43,6 +43,9 @@ class SyncerThreadWithSyncerTest : public testing::Test,
             &SyncerThreadWithSyncerTest::HandleSyncerEvent));
     allstatus_->WatchSyncerThread(syncer_thread_);
     syncer_thread_->SetConnected(true);
+    std::bitset<syncable::MODEL_TYPE_COUNT> expected_types;
+    expected_types[syncable::BOOKMARKS] = true;
+    connection_->ExpectGetUpdatesRequestTypes(expected_types);
   }
   virtual void TearDown() {
     syncer_thread_ = NULL;
diff --git a/chrome/browser/sync/engine/syncer_unittest.cc b/chrome/browser/sync/engine/syncer_unittest.cc
index 0d78832..a2956c5 100755
--- a/chrome/browser/sync/engine/syncer_unittest.cc
+++ b/chrome/browser/sync/engine/syncer_unittest.cc
@@ -129,8 +129,12 @@ class SyncerTest : public testing::Test,
 
   virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {
     // We're just testing the sync engine here, so we shunt everything to
-    // the SyncerThread.
-    (*out)[syncable::BOOKMARKS] = GROUP_PASSIVE;
+    // the SyncerThread.  Datatypes which aren't enabled aren't in the map.
+    for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) {
+      if (enabled_datatypes_[i]) {
+        (*out)[syncable::ModelTypeFromInt(i)] = GROUP_PASSIVE;
+      }
+    }
   }
 
   void HandleSyncerEvent(SyncerEvent event) {
@@ -168,6 +172,7 @@ class SyncerTest : public testing::Test,
 
     mock_server_.reset(
         new MockConnectionManager(syncdb_.manager(), syncdb_.name()));
+    EnableDatatype(syncable::BOOKMARKS);
     worker_ = new ModelSafeWorker();
     context_.reset(new SyncSessionContext(mock_server_.get(), syncdb_.manager(),
        this));
@@ -380,6 +385,16 @@ class SyncerTest : public testing::Test,
     return entry.Get(META_HANDLE);
   }
 
+  void EnableDatatype(syncable::ModelType model_type) {
+    enabled_datatypes_[model_type] = true;
+    mock_server_->ExpectGetUpdatesRequestTypes(enabled_datatypes_);
+  }
+
+  void DisableDatatype(syncable::ModelType model_type) {
+    enabled_datatypes_[model_type] = false;
+    mock_server_->ExpectGetUpdatesRequestTypes(enabled_datatypes_);
+  }
+
   // Some ids to aid tests. Only the root one's value is specific. The rest
   // are named for test clarity.
   // TODO(chron): Get rid of these inbuilt IDs. They only make it
@@ -403,6 +418,8 @@ class SyncerTest : public testing::Test,
   base::TimeDelta last_long_poll_interval_received_;
   scoped_refptr<ModelSafeWorker> worker_;
 
+  std::bitset<syncable::MODEL_TYPE_COUNT> enabled_datatypes_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncerTest);
 };
 
@@ -713,6 +730,7 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNesting) {
       ASSERT_TRUE(parent.good());
       parent.Put(syncable::IS_UNSYNCED, true);
       parent.Put(syncable::IS_DIR, true);
+      parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
       parent.Put(syncable::IS_DEL, true);
       parent.Put(syncable::ID, ids_.FromNumber(103));
       parent.Put(syncable::BASE_VERSION, 1);
@@ -722,6 +740,7 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNesting) {
       ASSERT_TRUE(child.good());
       child.Put(syncable::IS_UNSYNCED, true);
       child.Put(syncable::IS_DIR, true);
+      child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
       child.Put(syncable::IS_DEL, true);
       child.Put(syncable::ID, ids_.FromNumber(104));
       child.Put(syncable::BASE_VERSION, 1);
@@ -733,6 +752,7 @@ TEST_F(SyncerTest, TestCommitListOrderingWithNesting) {
       grandchild.Put(syncable::ID, ids_.FromNumber(105));
       grandchild.Put(syncable::IS_DEL, true);
       grandchild.Put(syncable::IS_DIR, false);
+      grandchild.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
       grandchild.Put(syncable::BASE_VERSION, 1);
       grandchild.Put(syncable::MTIME, now_minus_2h);
     }
@@ -838,10 +858,13 @@ TEST_F(SyncerTest, TestCommitListOrderingCounterexample) {
     ASSERT_TRUE(child1.good());
     child1.Put(syncable::IS_UNSYNCED, true);
     child1.Put(syncable::ID, child_id_);
+    child1.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
     MutableEntry child2(&wtrans, syncable::CREATE, parent_id_, "2");
     ASSERT_TRUE(child2.good());
     child2.Put(syncable::IS_UNSYNCED, true);
+    child2.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
     child2.Put(syncable::ID, child2_id);
+
     parent.Put(syncable::BASE_VERSION, 1);
     child1.Put(syncable::BASE_VERSION, 1);
     child2.Put(syncable::BASE_VERSION, 1);
@@ -3366,6 +3389,7 @@ TEST_F(SyncerTest, UpdateFlipsTheFolderBit) {
     local_deleted.Put(IS_DEL, true);
     local_deleted.Put(IS_DIR, true);
     local_deleted.Put(IS_UNSYNCED, true);
+    local_deleted.Put(SPECIFICS, DefaultBookmarkSpecifics());
   }
 
   // Server update: entry-type object (not a container), revision 10.
@@ -4118,6 +4142,36 @@ TEST_F(SyncerTest, UniqueServerTagUpdates) {
   }
 }
 
+TEST_F(SyncerTest, GetUpdatesSetsRequestedTypes) {
+  // The expectations of this test happen in the MockConnectionManager's
+  // GetUpdates handler.  EnableDatatype sets the expectation value from our
+  // set of enabled/disabled datatypes.
+  EnableDatatype(syncable::BOOKMARKS);
+  syncer_->SyncShare(this);
+  EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
+
+  EnableDatatype(syncable::AUTOFILL);
+  syncer_->SyncShare(this);
+  EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
+
+  EnableDatatype(syncable::PREFERENCES);
+  syncer_->SyncShare(this);
+  EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
+
+  DisableDatatype(syncable::BOOKMARKS);
+  syncer_->SyncShare(this);
+  EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
+
+  DisableDatatype(syncable::AUTOFILL);
+  syncer_->SyncShare(this);
+  EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
+
+  DisableDatatype(syncable::PREFERENCES);
+  EnableDatatype(syncable::AUTOFILL);
+  syncer_->SyncShare(this);
+  EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
+}
+
 class SyncerPositionUpdateTest : public SyncerTest {
  public:
   SyncerPositionUpdateTest() : next_update_id_(1), next_revision_(1) {}
diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc
index d42305f..6ade37b 100755
--- a/chrome/browser/sync/engine/syncer_util.cc
+++ b/chrome/browser/sync/engine/syncer_util.cc
@@ -838,19 +838,4 @@ syncable::Id SyncerUtil::ComputePrevIdFromServerPosition(
   return closest_sibling;
 }
 
-syncable::ModelType SyncerUtil::GetModelType(
-    const SyncEntity& entry) {
-
-  const bool is_bookmark =
-      (entry.has_specifics() &&
-       entry.specifics().HasExtension(sync_pb::bookmark)) ||
-       entry.has_bookmarkdata();
-
-  if (is_bookmark) {
-    return syncable::BOOKMARKS;
-  }
-
-  return syncable::UNSPECIFIED;
-}
-
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/engine/syncer_util.h b/chrome/browser/sync/engine/syncer_util.h
index eaba33e..134f7b7 100755
--- a/chrome/browser/sync/engine/syncer_util.h
+++ b/chrome/browser/sync/engine/syncer_util.h
@@ -152,8 +152,6 @@ class SyncerUtil {
   // if they match.  For an up-to-date item, this should be the case.
   static bool ServerAndLocalOrdersMatch(syncable::Entry* entry);
 
-  static syncable::ModelType GetModelType(const SyncEntity& entry);
-
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(SyncerUtil);
 };
diff --git a/chrome/browser/sync/engine/syncproto.h b/chrome/browser/sync/engine/syncproto.h
index c34a709..b412bea 100644
--- a/chrome/browser/sync/engine/syncproto.h
+++ b/chrome/browser/sync/engine/syncproto.h
@@ -53,29 +53,8 @@ class SyncEntity : public IdWrapper<sync_pb::SyncEntity> {
             (has_bookmarkdata() && bookmarkdata().bookmark_folder()));
   }
 
-  // Note: keep this consistent with GetModelType in syncable.cc!
   syncable::ModelType GetModelType() const {
-    DCHECK(!id().IsRoot());  // Root shouldn't ever go over the wire.
-
-    if (deleted())
-      return syncable::UNSPECIFIED;
-
-    if (specifics().HasExtension(sync_pb::bookmark) || has_bookmarkdata())
-      return syncable::BOOKMARKS;
-
-    if (specifics().HasExtension(sync_pb::preference))
-      return syncable::PREFERENCES;
-
-    // Loose check for server-created top-level folders that aren't
-    // bound to a particular model type.
-    if (!server_defined_unique_tag().empty() && IsFolder())
-      return syncable::TOP_LEVEL_FOLDER;
-
-    // This is an item of a datatype we can't understand. Maybe it's
-    // from the future?  Either we mis-encoded the object, or the
-    // server sent us entries it shouldn't have.
-    NOTREACHED() << "Unknown datatype in sync proto.";
-    return syncable::UNSPECIFIED;
+    return syncable::GetModelType(*this);
   }
 };