[Sync] Add tests for invalid specifics field number handling

This is a follow up to r172232.

Change NOTREACHED() to DLOG(WARNING), since the server sending down
unknown/invalid field numbers is a valid event.

Add tests for the code that uses GetModelTypeFromSpecificsFieldNumber().

Clean up some code in sync/engine/ a bit.

BUG=165171


Review URL: https://chromiumcodereview.appspot.com/11485019

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

sync/engine/DEPS

@@ -1,5 +1,6 @@
 include_rules = [
   "+googleurl",
+  "+sync/base",
   "+sync/internal_api/public/base",
   "+sync/internal_api/public/engine",
   "+sync/internal_api/public/sessions",

sync/engine/store_timestamps_command.cc

@@ -4,39 +4,44 @@
 
 #include "sync/engine/store_timestamps_command.h"
 
-#include "sync/internal_api/public/base/model_type.h"
+#include "base/logging.h"
 #include "sync/sessions/status_controller.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/directory.h"
 
 namespace syncer {
 
+ModelTypeSet ProcessNewProgressMarkers(
+    const sync_pb::GetUpdatesResponse& response,
+    syncable::Directory* dir) {
+  ModelTypeSet forward_progress_types;
+  // If a marker was omitted for any one type, that indicates no
+  // change from the previous state.
+  for (int i = 0; i < response.new_progress_marker_size(); ++i) {
+    int field_number = response.new_progress_marker(i).data_type_id();
+    ModelType model_type = GetModelTypeFromSpecificsFieldNumber(field_number);
+    if (!IsRealDataType(model_type)) {
+      DLOG(WARNING) << "Unknown field number " << field_number;
+      continue;
+    }
+    forward_progress_types.Put(model_type);
+    dir->SetDownloadProgress(model_type, response.new_progress_marker(i));
+  }
+  return forward_progress_types;
+}
+
 StoreTimestampsCommand::StoreTimestampsCommand() {}
 StoreTimestampsCommand::~StoreTimestampsCommand() {}
 
 SyncerError StoreTimestampsCommand::ExecuteImpl(
     sessions::SyncSession* session) {
-  syncable::Directory* dir = session->context()->directory();
-
   const sync_pb::GetUpdatesResponse& updates =
       session->status_controller().updates_response().get_updates();
 
   sessions::StatusController* status = session->mutable_status_controller();
 
-  // Update the progress marker tokens from the server result.  If a marker
-  // was omitted for any one type, that indicates no change from the previous
-  // state.
-  ModelTypeSet forward_progress_types;
-  for (int i = 0; i < updates.new_progress_marker_size(); ++i) {
-    int field_number = updates.new_progress_marker(i).data_type_id();
-    ModelType model_type = GetModelTypeFromSpecificsFieldNumber(field_number);
-    if (!IsRealDataType(model_type)) {
-      NOTREACHED() << "Unknown field number " << field_number;
-      continue;
-    }
-    forward_progress_types.Put(model_type);
-    dir->SetDownloadProgress(model_type, updates.new_progress_marker(i));
-  }
+  ModelTypeSet forward_progress_types =
+      ProcessNewProgressMarkers(updates, session->context()->directory());
   DCHECK(!forward_progress_types.Empty() ||
          updates.changes_remaining() == 0);
   if (VLOG_IS_ON(1)) {

sync/engine/store_timestamps_command.h

@@ -6,11 +6,27 @@
 #define SYNC_ENGINE_STORE_TIMESTAMPS_COMMAND_H_
 
 #include "base/compiler_specific.h"
+#include "sync/base/sync_export.h"
 #include "sync/engine/syncer_command.h"
 #include "sync/engine/syncer_types.h"
+#include "sync/internal_api/public/base/model_type.h"
+
+namespace sync_pb {
+class GetUpdatesResponse;
+}  // namespace sync_pb
 
 namespace syncer {
 
+namespace syncable {
+class Directory;
+}  // namespace syncable
+
+// Sets |dir|'s progress markers from the data in |response|.  Returns
+// the set of model types with new progress markers.
+SYNC_EXPORT_PRIVATE ModelTypeSet ProcessNewProgressMarkers(
+    const sync_pb::GetUpdatesResponse& response,
+    syncable::Directory* dir);
+
 // A syncer command that extracts the changelog timestamp information from
 // a GetUpdatesResponse (fetched in DownloadUpdatesCommand) and stores
 // it in the directory.  This is meant to run immediately after

sync/engine/store_timestamps_command_unittest.cc

@@ -0,0 +1,83 @@
+// Copyright (c) 2012 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 "base/basictypes.h"
+#include "sync/engine/store_timestamps_command.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/protocol/sync.pb.h"
+#include "sync/test/engine/syncer_command_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+
+namespace {
+
+// Adds a progress marker to |response| for the given field number and
+// token.
+void AddProgressMarkerForFieldNumber(
+    sync_pb::GetUpdatesResponse* response,
+    int field_number, const std::string& token) {
+  sync_pb::DataTypeProgressMarker* marker =
+      response->add_new_progress_marker();
+  marker->set_data_type_id(field_number);
+  marker->set_token(token);
+}
+
+// Adds a progress marker to |response| for the given model type and
+// token.
+void AddProgressMarkerForModelType(
+    sync_pb::GetUpdatesResponse* response,
+    ModelType model_type, const std::string& token) {
+  AddProgressMarkerForFieldNumber(
+      response, GetSpecificsFieldNumberFromModelType(model_type), token);
+}
+
+class StoreTimestampsCommandTest : public SyncerCommandTest {
+ protected:
+  // Gets the directory's progress marker's token for the given model
+  // type.
+  std::string GetProgessMarkerToken(ModelType model_type) {
+    sync_pb::DataTypeProgressMarker progress_marker;
+    session()->context()->directory()->GetDownloadProgress(
+        model_type, &progress_marker);
+    EXPECT_EQ(
+        GetSpecificsFieldNumberFromModelType(model_type),
+        progress_marker.data_type_id());
+    return progress_marker.token();
+  }
+};
+
+// Builds a GetUpdatesResponse with some progress markers, including
+// invalid ones.  ProcessNewProgressMarkers() should return the model
+// types for the valid progress markers and fill in the progress
+// markers in the directory.
+TEST_F(StoreTimestampsCommandTest, ProcessNewProgressMarkers) {
+  sync_pb::GetUpdatesResponse response;
+  AddProgressMarkerForModelType(&response, BOOKMARKS, "token1");
+  AddProgressMarkerForModelType(&response,
+                                HISTORY_DELETE_DIRECTIVES, "token2");
+  AddProgressMarkerForFieldNumber(&response, -1, "bad token");
+
+  ModelTypeSet forward_progress_types =
+      ProcessNewProgressMarkers(
+          response, session()->context()->directory());
+
+  EXPECT_TRUE(
+      forward_progress_types.Equals(
+          ModelTypeSet(BOOKMARKS, HISTORY_DELETE_DIRECTIVES)));
+
+  EXPECT_EQ("token1", GetProgessMarkerToken(BOOKMARKS));
+  EXPECT_EQ("token2", GetProgessMarkerToken(HISTORY_DELETE_DIRECTIVES));
+
+  ModelTypeSet non_forward_progress_types =
+      Difference(ModelTypeSet::All(), forward_progress_types);
+  for (ModelTypeSet::Iterator it = non_forward_progress_types.First();
+       it.Good(); it.Inc()) {
+    EXPECT_TRUE(GetProgessMarkerToken(it.Get()).empty());
+  }
+}
+
+}  // namespace
+
+}  // namespace syncer

sync/engine/syncer_proto_util.cc

@@ -13,7 +13,6 @@
 #include "sync/engine/throttled_data_type_tracker.h"
 #include "sync/engine/traffic_logger.h"
 #include "sync/internal_api/public/base/model_type.h"
-#include "sync/protocol/sync.pb.h"
 #include "sync/protocol/sync_enums.pb.h"
 #include "sync/protocol/sync_protocol_error.h"
 #include "sync/sessions/sync_session.h"
@@ -107,53 +106,122 @@ SyncerError ServerConnectionErrorAsSyncerError(
   }
 }
 
+SyncProtocolErrorType ConvertSyncProtocolErrorTypePBToLocalType(
+    const sync_pb::SyncEnums::ErrorType& error_type) {
+  switch (error_type) {
+    case sync_pb::SyncEnums::SUCCESS:
+      return SYNC_SUCCESS;
+    case sync_pb::SyncEnums::NOT_MY_BIRTHDAY:
+      return NOT_MY_BIRTHDAY;
+    case sync_pb::SyncEnums::THROTTLED:
+      return THROTTLED;
+    case sync_pb::SyncEnums::CLEAR_PENDING:
+      return CLEAR_PENDING;
+    case sync_pb::SyncEnums::TRANSIENT_ERROR:
+      return TRANSIENT_ERROR;
+    case sync_pb::SyncEnums::MIGRATION_DONE:
+      return MIGRATION_DONE;
+    case sync_pb::SyncEnums::UNKNOWN:
+      return UNKNOWN_ERROR;
+    case sync_pb::SyncEnums::USER_NOT_ACTIVATED:
+    case sync_pb::SyncEnums::AUTH_INVALID:
+    case sync_pb::SyncEnums::ACCESS_DENIED:
+      return INVALID_CREDENTIAL;
+    default:
+      NOTREACHED();
+      return UNKNOWN_ERROR;
+  }
+}
+
+ClientAction ConvertClientActionPBToLocalClientAction(
+    const sync_pb::SyncEnums::Action& action) {
+  switch (action) {
+    case sync_pb::SyncEnums::UPGRADE_CLIENT:
+      return UPGRADE_CLIENT;
+    case sync_pb::SyncEnums::CLEAR_USER_DATA_AND_RESYNC:
+      return CLEAR_USER_DATA_AND_RESYNC;
+    case sync_pb::SyncEnums::ENABLE_SYNC_ON_ACCOUNT:
+      return ENABLE_SYNC_ON_ACCOUNT;
+    case sync_pb::SyncEnums::STOP_AND_RESTART_SYNC:
+      return STOP_AND_RESTART_SYNC;
+    case sync_pb::SyncEnums::DISABLE_SYNC_ON_CLIENT:
+      return DISABLE_SYNC_ON_CLIENT;
+    case sync_pb::SyncEnums::UNKNOWN_ACTION:
+      return UNKNOWN_ACTION;
+    default:
+      NOTREACHED();
+      return UNKNOWN_ACTION;
+  }
+}
+
 }  // namespace
 
-// static
-void SyncerProtoUtil::HandleMigrationDoneResponse(
-  const ClientToServerResponse* response,
-  sessions::SyncSession* session) {
-  LOG_IF(ERROR, 0 >= response->migrated_data_type_id_size())
-      << "MIGRATION_DONE but no types specified.";
+ModelTypeSet GetTypesToMigrate(const ClientToServerResponse& response) {
   ModelTypeSet to_migrate;
-  for (int i = 0; i < response->migrated_data_type_id_size(); i++) {
-    int field_number = response->migrated_data_type_id(i);
+  for (int i = 0; i < response.migrated_data_type_id_size(); i++) {
+    int field_number = response.migrated_data_type_id(i);
     ModelType model_type = GetModelTypeFromSpecificsFieldNumber(field_number);
     if (!IsRealDataType(model_type)) {
-      NOTREACHED() << "Unknown field number " << field_number;
+      DLOG(WARNING) << "Unknown field number " << field_number;
       continue;
     }
     to_migrate.Put(model_type);
   }
-  // TODO(akalin): This should be a set union.
-  session->mutable_status_controller()->
-      set_types_needing_local_migration(to_migrate);
+  return to_migrate;
 }
 
-// static
-bool SyncerProtoUtil::VerifyResponseBirthday(syncable::Directory* dir,
-    const ClientToServerResponse* response) {
+SyncProtocolError ConvertErrorPBToLocalType(
+    const sync_pb::ClientToServerResponse_Error& error) {
+  SyncProtocolError sync_protocol_error;
+  sync_protocol_error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(
+      error.error_type());
+  sync_protocol_error.error_description = error.error_description();
+  sync_protocol_error.url = error.url();
+  sync_protocol_error.action = ConvertClientActionPBToLocalClientAction(
+      error.action());
+
+  if (error.error_data_type_ids_size() > 0) {
+    // THROTTLED is currently the only error code that uses |error_data_types|.
+    DCHECK_EQ(error.error_type(), sync_pb::SyncEnums::THROTTLED);
+    for (int i = 0; i < error.error_data_type_ids_size(); ++i) {
+      int field_number = error.error_data_type_ids(i);
+      ModelType model_type =
+          GetModelTypeFromSpecificsFieldNumber(field_number);
+      if (!IsRealDataType(model_type)) {
+        DLOG(WARNING) << "Unknown field number " << field_number;
+        continue;
+      }
+      sync_protocol_error.error_data_types.Put(model_type);
+    }
+  }
+
+  return sync_protocol_error;
+}
+
+bool SyncerProtoUtil::VerifyResponseBirthday(
+    const ClientToServerResponse& response,
+    syncable::Directory* dir) {
 
   std::string local_birthday = dir->store_birthday();
 
   if (local_birthday.empty()) {
-    if (!response->has_store_birthday()) {
+    if (!response.has_store_birthday()) {
       LOG(WARNING) << "Expected a birthday on first sync.";
       return false;
     }
 
-    DVLOG(1) << "New store birthday: " << response->store_birthday();
-    dir->set_store_birthday(response->store_birthday());
+    DVLOG(1) << "New store birthday: " << response.store_birthday();
+    dir->set_store_birthday(response.store_birthday());
     return true;
   }
 
   // Error situation, but we're not stuck.
-  if (!response->has_store_birthday()) {
+  if (!response.has_store_birthday()) {
     LOG(WARNING) << "No birthday in server response?";
     return true;
   }
 
-  if (response->store_birthday() != local_birthday) {
+  if (response.store_birthday() != local_birthday) {
     LOG(WARNING) << "Birthday changed, showing syncer stuck";
     return false;
   }
@@ -265,82 +333,6 @@ bool IsVeryFirstGetUpdates(const ClientToServerMessage& message) {
   return true;
 }
 
-SyncProtocolErrorType ConvertSyncProtocolErrorTypePBToLocalType(
-    const sync_pb::SyncEnums::ErrorType& error_type) {
-  switch (error_type) {
-    case sync_pb::SyncEnums::SUCCESS:
-      return SYNC_SUCCESS;
-    case sync_pb::SyncEnums::NOT_MY_BIRTHDAY:
-      return NOT_MY_BIRTHDAY;
-    case sync_pb::SyncEnums::THROTTLED:
-      return THROTTLED;
-    case sync_pb::SyncEnums::CLEAR_PENDING:
-      return CLEAR_PENDING;
-    case sync_pb::SyncEnums::TRANSIENT_ERROR:
-      return TRANSIENT_ERROR;
-    case sync_pb::SyncEnums::MIGRATION_DONE:
-      return MIGRATION_DONE;
-    case sync_pb::SyncEnums::UNKNOWN:
-      return UNKNOWN_ERROR;
-    case sync_pb::SyncEnums::USER_NOT_ACTIVATED:
-    case sync_pb::SyncEnums::AUTH_INVALID:
-    case sync_pb::SyncEnums::ACCESS_DENIED:
-      return INVALID_CREDENTIAL;
-    default:
-      NOTREACHED();
-      return UNKNOWN_ERROR;
-  }
-}
-
-ClientAction ConvertClientActionPBToLocalClientAction(
-    const sync_pb::SyncEnums::Action& action) {
-  switch (action) {
-    case sync_pb::SyncEnums::UPGRADE_CLIENT:
-      return UPGRADE_CLIENT;
-    case sync_pb::SyncEnums::CLEAR_USER_DATA_AND_RESYNC:
-      return CLEAR_USER_DATA_AND_RESYNC;
-    case sync_pb::SyncEnums::ENABLE_SYNC_ON_ACCOUNT:
-      return ENABLE_SYNC_ON_ACCOUNT;
-    case sync_pb::SyncEnums::STOP_AND_RESTART_SYNC:
-      return STOP_AND_RESTART_SYNC;
-    case sync_pb::SyncEnums::DISABLE_SYNC_ON_CLIENT:
-      return DISABLE_SYNC_ON_CLIENT;
-    case sync_pb::SyncEnums::UNKNOWN_ACTION:
-      return UNKNOWN_ACTION;
-    default:
-      NOTREACHED();
-      return UNKNOWN_ACTION;
-  }
-}
-
-SyncProtocolError ConvertErrorPBToLocalType(
-    const ClientToServerResponse::Error& error) {
-  SyncProtocolError sync_protocol_error;
-  sync_protocol_error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(
-      error.error_type());
-  sync_protocol_error.error_description = error.error_description();
-  sync_protocol_error.url = error.url();
-  sync_protocol_error.action = ConvertClientActionPBToLocalClientAction(
-      error.action());
-
-  if (error.error_data_type_ids_size() > 0) {
-    // THROTTLED is currently the only error code that uses |error_data_types|.
-    DCHECK_EQ(error.error_type(), sync_pb::SyncEnums::THROTTLED);
-    for (int i = 0; i < error.error_data_type_ids_size(); ++i) {
-      int field_number = error.error_data_type_ids(i);
-      ModelType model_type =
-          GetModelTypeFromSpecificsFieldNumber(field_number);
-      if (!IsRealDataType(model_type)) {
-        NOTREACHED() << "Unknown field number " << field_number;
-        continue;
-      }
-      sync_protocol_error.error_data_types.Put(model_type);
-    }
-  }
-
-  return sync_protocol_error;
-}
-
 // TODO(lipalani) : Rename these function names as per the CR for issue 7740067.
 SyncProtocolError ConvertLegacyErrorCodeToNewError(
     const sync_pb::SyncEnums::ErrorType& error_type) {
@@ -360,7 +352,6 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
     ClientToServerMessage* msg,
     ClientToServerResponse* response,
     SyncSession* session) {
-
   CHECK(response);
   DCHECK(!msg->get_updates().has_from_timestamp());  // Deprecated.
   DCHECK(!msg->get_updates().has_requested_types());  // Deprecated.
@@ -400,7 +391,7 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
   SyncProtocolError sync_protocol_error;
 
   // Birthday mismatch overrides any error that is sent by the server.
-  if (!VerifyResponseBirthday(dir, response)) {
+  if (!VerifyResponseBirthday(*response, dir)) {
     sync_protocol_error.error_type = NOT_MY_BIRTHDAY;
      sync_protocol_error.action =
          DISABLE_SYNC_ON_CLIENT;
@@ -465,7 +456,11 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
     case TRANSIENT_ERROR:
       return SERVER_RETURN_TRANSIENT_ERROR;
     case MIGRATION_DONE:
-      HandleMigrationDoneResponse(response, session);
+      LOG_IF(ERROR, 0 >= response->migrated_data_type_id_size())
+          << "MIGRATION_DONE but no types specified.";
+      // TODO(akalin): This should be a set union.
+      session->mutable_status_controller()->
+          set_types_needing_local_migration(GetTypesToMigrate(*response));
       return SERVER_RETURN_MIGRATION_DONE;
     case CLEAR_PENDING:
       return SERVER_RETURN_CLEAR_PENDING;

sync/engine/syncer_proto_util.h

@@ -9,6 +9,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/time.h"
+#include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/util/syncer_error.h"
 #include "sync/sessions/sync_session.h"
@@ -17,6 +18,7 @@
 namespace sync_pb {
 class ClientToServerMessage;
 class ClientToServerResponse;
+class ClientToServerResponse_Error;
 class CommitResponse_EntryResponse;
 class EntitySpecifics;
 class SyncEntity;
@@ -37,6 +39,14 @@ class Directory;
 class Entry;
 }
 
+// Returns the types to migrate from the data in |response|.
+SYNC_EXPORT_PRIVATE ModelTypeSet GetTypesToMigrate(
+    const sync_pb::ClientToServerResponse& response);
+
+// Builds a SyncProtocolError from the data in |error|.
+SYNC_EXPORT_PRIVATE SyncProtocolError ConvertErrorPBToLocalType(
+    const sync_pb::ClientToServerResponse_Error& error);
+
 class SyncerProtoUtil {
  public:
   // Posts the given message and fills the buffer with the returned value.
@@ -114,14 +124,9 @@ class SyncerProtoUtil {
 
   // Verifies the store birthday, alerting/resetting as appropriate if there's a
   // mismatch. Return false if the syncer should be stuck.
-  static bool VerifyResponseBirthday(syncable::Directory* dir,
-      const sync_pb::ClientToServerResponse* response);
-
-  // Builds and sends a SyncEngineEvent to begin migration for types (specified
-  // in notification).
-  static void HandleMigrationDoneResponse(
-      const sync_pb::ClientToServerResponse* response,
-      sessions::SyncSession* session);
+  static bool VerifyResponseBirthday(
+      const sync_pb::ClientToServerResponse& response,
+      syncable::Directory* dir);
 
   // Post the message using the scm, and do some processing on the returned
   // headers. Decode the server response.

sync/engine/syncer_proto_util_unittest.cc

@@ -48,6 +48,38 @@ class MockDelegate : public sessions::SyncSession::Delegate {
   MOCK_METHOD1(OnSilencedUntil, void(const base::TimeTicks&));
 };
 
+// Builds a ClientToServerResponse with some data type ids, including
+// invalid ones.  GetTypesToMigrate() should return only the valid
+// model types.
+TEST(SyncerProtoUtil, GetTypesToMigrate) {
+  sync_pb::ClientToServerResponse response;
+  response.add_migrated_data_type_id(
+      GetSpecificsFieldNumberFromModelType(BOOKMARKS));
+  response.add_migrated_data_type_id(
+      GetSpecificsFieldNumberFromModelType(HISTORY_DELETE_DIRECTIVES));
+  response.add_migrated_data_type_id(-1);
+  EXPECT_TRUE(
+      GetTypesToMigrate(response).Equals(
+          ModelTypeSet(BOOKMARKS, HISTORY_DELETE_DIRECTIVES)));
+}
+
+// Builds a ClientToServerResponse_Error with some error data type
+// ids, including invalid ones.  ConvertErrorPBToLocalType() should
+// return a SyncProtocolError with only the valid model types.
+TEST(SyncerProtoUtil, ConvertErrorPBToLocalType) {
+  sync_pb::ClientToServerResponse_Error error_pb;
+  error_pb.set_error_type(sync_pb::SyncEnums::THROTTLED);
+  error_pb.add_error_data_type_ids(
+      GetSpecificsFieldNumberFromModelType(BOOKMARKS));
+  error_pb.add_error_data_type_ids(
+      GetSpecificsFieldNumberFromModelType(HISTORY_DELETE_DIRECTIVES));
+  error_pb.add_error_data_type_ids(-1);
+  SyncProtocolError error = ConvertErrorPBToLocalType(error_pb);
+  EXPECT_TRUE(
+      error.error_data_types.Equals(
+          ModelTypeSet(BOOKMARKS, HISTORY_DELETE_DIRECTIVES)));
+}
+
 TEST(SyncerProtoUtil, TestBlobToProtocolBufferBytesUtilityFunctions) {
   unsigned char test_data1[] = {1, 2, 3, 4, 5, 6, 7, 8, 0, 1, 4, 2, 9};
   unsigned char test_data2[] = {1, 99, 3, 4, 5, 6, 7, 8, 0, 1, 4, 2, 9};
@@ -175,24 +207,24 @@ TEST_F(SyncerProtoUtilTest, VerifyResponseBirthday) {
   // Both sides empty
   EXPECT_TRUE(directory()->store_birthday().empty());
   sync_pb::ClientToServerResponse response;
-  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(directory(), &response));
+  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
 
   // Remote set, local empty
   response.set_store_birthday("flan");
-  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(directory(), &response));
+  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
   EXPECT_EQ(directory()->store_birthday(), "flan");
 
   // Remote empty, local set.
   response.clear_store_birthday();
-  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(directory(), &response));
+  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
   EXPECT_EQ(directory()->store_birthday(), "flan");
 
   // Doesn't match
   response.set_store_birthday("meat");
-  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(directory(), &response));
+  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
 
   response.set_error_code(sync_pb::SyncEnums::CLEAR_PENDING);
-  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(directory(), &response));
+  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
 }
 
 TEST_F(SyncerProtoUtilTest, AddRequestBirthday) {
@@ -291,4 +323,5 @@ TEST_F(SyncerProtoUtilTest, HandleThrottlingNoDatatypes) {
   SyncerProtoUtil::HandleThrottleError(error, ticks, &tracker, &delegate);
   EXPECT_TRUE(tracker.GetThrottledTypes().Empty());
 }
+
 }  // namespace syncer

sync/internal_api/public/base/model_type.h

@@ -174,7 +174,7 @@ SYNC_EXPORT bool IsControlType(ModelType model_type);
 //     ModelType model_type =
 //         GetModelTypeFromSpecificsFieldNumber(field_number);
 //     if (!IsRealDataType(model_type)) {
-//       NOTREACHED() << "Unknown field number " << field_number;
+//       DLOG(WARNING) << "Unknown field number " << field_number;
 //       continue;
 //     }
 //     model_types.Put(model_type);

sync/sync.gyp

@@ -665,6 +665,7 @@
           'engine/model_changing_syncer_command_unittest.cc',
           'engine/process_commit_response_command_unittest.cc',
           'engine/process_updates_command_unittest.cc',
+          'engine/store_timestamps_command_unittest.cc',
           'engine/sync_session_job_unittest.cc',
           'engine/sync_scheduler_unittest.cc',
           'engine/sync_scheduler_whitebox_unittest.cc',