4 files changed, 244 insertions, 74 deletions

diff --git a/chrome/browser/sync/engine/syncer_proto_util.cc b/chrome/browser/sync/engine/syncer_proto_util.cc
index d6fec77..290fe17 100755
--- a/chrome/browser/sync/engine/syncer_proto_util.cc
+++ b/chrome/browser/sync/engine/syncer_proto_util.cc
@@ -34,25 +34,6 @@ namespace {
 // Time to backoff syncing after receiving a throttled response.
 static const int kSyncDelayAfterThrottled = 2 * 60 * 60;  // 2 hours
 
-// Verifies the store birthday, alerting/resetting as appropriate if there's a
-// mismatch.
-bool VerifyResponseBirthday(const ScopedDirLookup& dir,
-                            const ClientToServerResponse* response) {
-  // Process store birthday.
-  if (!response->has_store_birthday())
-    return true;
-  string birthday = dir->store_birthday();
-  if (response->store_birthday() == birthday)
-    return true;
-  LOG(INFO) << "New store birthday: " << response->store_birthday();
-  if (!birthday.empty()) {
-    LOG(ERROR) << "Birthday changed, showing syncer stuck";
-    return false;
-  }
-  dir->set_store_birthday(response->store_birthday());
-  return true;
-}
-
 void LogResponseProfilingData(const ClientToServerResponse& response) {
   if (response.has_profiling_data()) {
     stringstream response_trace;
@@ -93,79 +74,130 @@ void LogResponseProfilingData(const ClientToServerResponse& response) {
 
 }  // namespace
 
+
 // static
-bool SyncerProtoUtil::PostClientToServerMessage(ClientToServerMessage* msg,
-    ClientToServerResponse* response, SyncSession* session) {
-  bool rv = false;
-  string tx, rx;
-  CHECK(response);
+bool SyncerProtoUtil::VerifyResponseBirthday(syncable::Directory* dir,
+    const ClientToServerResponse* response) {
 
-  ScopedDirLookup dir(session->context()->directory_manager(),
-      session->context()->account_name());
-  if (!dir.good())
+  std::string local_birthday = dir->store_birthday();
+
+  if (local_birthday.empty()) {
+    if (!response->has_store_birthday()) {
+      LOG(WARNING) << "Expected a birthday on first sync.";
+      return false;
+    }
+
+    LOG(INFO) << "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()) {
+    LOG(WARNING) << "No birthday in server response?";
+    return true;
+  }
+
+  if (response->store_birthday() != local_birthday) {
+    LOG(WARNING) << "Birthday changed, showing syncer stuck";
     return false;
-  string birthday = dir->store_birthday();
-  if (!birthday.empty()) {
-    msg->set_store_birthday(birthday);
-  } else {
-    LOG(INFO) << "no birthday set";
   }
 
+  return true;
+}
+
+// static
+void SyncerProtoUtil::AddRequestBirthday(syncable::Directory* dir,
+                                         ClientToServerMessage* msg) {
+  if (!dir->store_birthday().empty()) {
+    msg->set_store_birthday(dir->store_birthday());
+  }
+}
+
+// static
+bool SyncerProtoUtil::PostAndProcessHeaders(ServerConnectionManager* scm,
+                                            ClientToServerMessage* msg,
+                                            ClientToServerResponse* response) {
+
+  std::string tx, rx;
   msg->SerializeToString(&tx);
+
   HttpResponse http_response;
   ServerConnectionManager::PostBufferParams params = {
     tx, &rx, &http_response
   };
 
-  ServerConnectionManager* scm = session->context()->connection_manager();
   ScopedServerStatusWatcher server_status_watcher(scm, &http_response);
   if (!scm->PostBufferWithCachedAuth(&params, &server_status_watcher)) {
     LOG(WARNING) << "Error posting from syncer:" << http_response;
+    return false;
   } else {
-    rv = response->ParseFromString(rx);
-  }
-  if (rv) {
-    if (!VerifyResponseBirthday(dir, response)) {
-      // TODO(ncarter): Add a unit test for the case where the syncer becomes
-      // stuck due to a bad birthday.
-      session->status_controller()->set_syncer_stuck(true);
-      return false;
+    if (response->ParseFromString(rx)) {
+      // TODO(tim): This is an egregious layering violation (bug 35060).
+      switch (response->error_code()) {
+        case ClientToServerResponse::ACCESS_DENIED:
+        case ClientToServerResponse::AUTH_INVALID:
+        case ClientToServerResponse::USER_NOT_ACTIVATED:
+          // Fires on ScopedServerStatusWatcher
+          http_response.server_status = HttpResponse::SYNC_AUTH_ERROR;
+          return false;
+        default:
+          return true;
+      }
     }
 
-    switch (response->error_code()) {
-      case ClientToServerResponse::SUCCESS:
-        if (!response->has_store_birthday() && birthday.empty()) {
-          LOG(ERROR) <<
-            "Server didn't provide birthday in proto buffer response.";
-          rv = false;
-        }
-        LogResponseProfilingData(*response);
-        break;
-      case ClientToServerResponse::USER_NOT_ACTIVATED:
-      case ClientToServerResponse::AUTH_INVALID:
-      case ClientToServerResponse::ACCESS_DENIED:
-        LOG(INFO) << "SyncerProtoUtil: Authentication expired.";
-        // TODO(tim): This is an egregious layering violation (bug 35060).
-        http_response.server_status = HttpResponse::SYNC_AUTH_ERROR;
-        rv = false;
-        break;
-      case ClientToServerResponse::NOT_MY_BIRTHDAY:
-        LOG(WARNING) << "Not my birthday return.";
-        rv = false;
-        break;
-      case ClientToServerResponse::THROTTLED:
-        LOG(WARNING) << "Client silenced by server.";
-        session->delegate()->OnSilencedUntil(base::TimeTicks::Now() +
-            base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled));
-        rv = false;
-        break;
-      default:
-        NOTREACHED();
-        break;
-    }
+    return false;
+  }
+}
+
+// static
+bool SyncerProtoUtil::PostClientToServerMessage(ClientToServerMessage* msg,
+    ClientToServerResponse* response, SyncSession* session) {
+
+  CHECK(response);
 
+  ScopedDirLookup dir(session->context()->directory_manager(),
+      session->context()->account_name());
+  if (!dir.good()) {
+    return false;
+  }
+
+  AddRequestBirthday(dir, msg);
+
+  if (!PostAndProcessHeaders(session->context()->connection_manager(),
+                             msg,
+                             response)) {
+    return false;
+  }
+
+  if (!VerifyResponseBirthday(dir, response)) {
+    // TODO(ncarter): Add a unit test for the case where the syncer becomes
+    // stuck due to a bad birthday.
+    session->status_controller()->set_syncer_stuck(true);
+    return false;
+  }
+
+  switch (response->error_code()) {
+    case ClientToServerResponse::SUCCESS:
+      LogResponseProfilingData(*response);
+      return true;
+    case ClientToServerResponse::NOT_MY_BIRTHDAY:
+      LOG(WARNING) << "Server thought we had wrong birthday.";
+      return false;
+    case ClientToServerResponse::THROTTLED:
+      LOG(WARNING) << "Client silenced by server.";
+      session->delegate()->OnSilencedUntil(base::TimeTicks::Now() +
+          base::TimeDelta::FromSeconds(kSyncDelayAfterThrottled));
+      return false;
+    case ClientToServerResponse::USER_NOT_ACTIVATED:
+    case ClientToServerResponse::AUTH_INVALID:
+    case ClientToServerResponse::ACCESS_DENIED:
+      // WARNING: PostAndProcessHeaders contains a hack for this case.
+      LOG(WARNING) << "SyncerProtoUtil: Authentication expired.";
+    default:
+      NOTREACHED();
+      return false;
   }
-  return rv;
 }
 
 // static
diff --git a/chrome/browser/sync/engine/syncer_proto_util.h b/chrome/browser/sync/engine/syncer_proto_util.h
index 4dbf0cf..af31251 100755
--- a/chrome/browser/sync/engine/syncer_proto_util.h
+++ b/chrome/browser/sync/engine/syncer_proto_util.h
@@ -9,8 +9,10 @@
 
 #include "chrome/browser/sync/syncable/blob.h"
 #include "chrome/browser/sync/util/sync_types.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"  // For FRIEND_TEST
 
 namespace syncable {
+class Directory;
 class Entry;
 class ScopedDirLookup;
 class SyncName;
@@ -27,6 +29,7 @@ class SyncSession;
 }
 
 class ClientToServerMessage;
+class ServerConnectionManager;
 class SyncEntity;
 class CommitResponse_EntryResponse;
 
@@ -69,6 +72,31 @@ class SyncerProtoUtil {
 
  private:
   SyncerProtoUtil() {}
+
+  // Helper functions for PostClientToServerMessage.
+
+  // 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);
+
+  // Pull the birthday from the dir and put it into the msg.
+  static void AddRequestBirthday(syncable::Directory* dir,
+                                 ClientToServerMessage* msg);
+
+  // Post the message using the scm, and do some processing on the returned
+  // headers. Decode the server response.
+  static bool PostAndProcessHeaders(browser_sync::ServerConnectionManager* scm,
+                                    ClientToServerMessage* msg,
+                                    sync_pb::ClientToServerResponse* response);
+
+  friend class SyncerProtoUtilTest;
+
+  FRIEND_TEST(SyncerProtoUtilTest, AddRequestBirthday);
+  FRIEND_TEST(SyncerProtoUtilTest, PostAndProcessHeaders);
+  FRIEND_TEST(SyncerProtoUtilTest, VerifyResponseBirthday);
+
+
   DISALLOW_COPY_AND_ASSIGN(SyncerProtoUtil);
 };
 
diff --git a/chrome/browser/sync/engine/syncer_proto_util_unittest.cc b/chrome/browser/sync/engine/syncer_proto_util_unittest.cc
index 7f7ba1f..a856e6b 100644..100755
--- a/chrome/browser/sync/engine/syncer_proto_util_unittest.cc
+++ b/chrome/browser/sync/engine/syncer_proto_util_unittest.cc
@@ -7,10 +7,15 @@
 #include "base/basictypes.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/syncable/blob.h"
+#include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/syncable.h"
+#include "chrome/test/sync/engine/mock_server_connection.h"
+#include "chrome/test/sync/engine/test_directory_setter_upper.h"
+
 #include "testing/gtest/include/gtest/gtest.h"
 
 using syncable::Blob;
+using syncable::ScopedDirLookup;
 using syncable::SyncName;
 
 namespace browser_sync {
@@ -135,4 +140,109 @@ TEST(SyncerProtoUtil, NameExtractionTwoNames) {
   EXPECT_TRUE(name_a == name_b);
 }
 
+class SyncerProtoUtilTest : public testing::Test {
+ public:
+  virtual void SetUp() {
+    setter_upper_.SetUp();
+  }
+
+  virtual void TearDown() {
+    setter_upper_.TearDown();
+  }
+
+ protected:
+  browser_sync::TestDirectorySetterUpper setter_upper_;
+};
+
+TEST_F(SyncerProtoUtilTest, VerifyResponseBirthday) {
+  ScopedDirLookup lookup(setter_upper_.manager(), setter_upper_.name());
+  ASSERT_TRUE(lookup.good());
+
+  // Both sides empty
+  EXPECT_TRUE(lookup->store_birthday().empty());
+  ClientToServerResponse response;
+  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(lookup, &response));
+
+  // Remote set, local empty
+  response.set_store_birthday("flan");
+  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(lookup, &response));
+  EXPECT_EQ(lookup->store_birthday(), "flan");
+
+  // Remote empty, local set.
+  response.clear_store_birthday();
+  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(lookup, &response));
+  EXPECT_EQ(lookup->store_birthday(), "flan");
+
+  // Doesn't match
+  response.set_store_birthday("meat");
+  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(lookup, &response));
+}
+
+TEST_F(SyncerProtoUtilTest, AddRequestBirthday) {
+  ScopedDirLookup lookup(setter_upper_.manager(), setter_upper_.name());
+  ASSERT_TRUE(lookup.good());
+
+  EXPECT_TRUE(lookup->store_birthday().empty());
+  ClientToServerMessage msg;
+  SyncerProtoUtil::AddRequestBirthday(lookup, &msg);
+  EXPECT_FALSE(msg.has_store_birthday());
+
+  lookup->set_store_birthday("meat");
+  SyncerProtoUtil::AddRequestBirthday(lookup, &msg);
+  EXPECT_EQ(msg.store_birthday(), "meat");
+}
+
+class DummyConnectionManager : public browser_sync::ServerConnectionManager {
+ public:
+  DummyConnectionManager()
+      : ServerConnectionManager("unused", 0, false, "version", "id"),
+        send_error_(false),
+        access_denied_(false) {}
+
+  virtual ~DummyConnectionManager() {}
+  virtual bool PostBufferWithCachedAuth(const PostBufferParams* params,
+                                        ScopedServerStatusWatcher* watcher) {
+    if (send_error_) {
+      return false;
+    }
+
+    ClientToServerResponse response;
+    if (access_denied_) {
+      response.set_error_code(ClientToServerResponse::ACCESS_DENIED);
+    }
+    response.SerializeToString(params->buffer_out);
+
+    return true;
+  }
+
+  void set_send_error(bool send) {
+    send_error_ = send;
+  }
+
+  void set_access_denied(bool denied) {
+    access_denied_ = denied;
+  }
+
+ private:
+  bool send_error_;
+  bool access_denied_;
+};
+
+TEST_F(SyncerProtoUtilTest, PostAndProcessHeaders) {
+  DummyConnectionManager dcm;
+  ClientToServerMessage msg;
+  msg.set_share("required");
+  msg.set_message_contents(ClientToServerMessage::GET_UPDATES);
+  ClientToServerResponse response;
+
+  dcm.set_send_error(true);
+  EXPECT_FALSE(SyncerProtoUtil::PostAndProcessHeaders(&dcm, &msg, &response));
+
+  dcm.set_send_error(false);
+  EXPECT_TRUE(SyncerProtoUtil::PostAndProcessHeaders(&dcm, &msg, &response));
+
+  dcm.set_access_denied(true);
+  EXPECT_FALSE(SyncerProtoUtil::PostAndProcessHeaders(&dcm, &msg, &response));
+}
+
 }  // namespace browser_sync
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 83bdf06..01c3a9a 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1454,7 +1454,7 @@
         'browser/sync/engine/auth_watcher_unittest.cc',
         'browser/sync/engine/net/gaia_authenticator_unittest.cc',
         'browser/sync/engine/process_commit_response_command_unittest.cc',
-	'browser/sync/engine/syncapi_unittest.cc',
+        'browser/sync/engine/syncapi_unittest.cc',
         'browser/sync/engine/syncer_proto_util_unittest.cc',
         'browser/sync/engine/syncer_thread_unittest.cc',
         'browser/sync/engine/syncer_unittest.cc',