Fix the false-positive detection of commit errors

chrome/browser/sync/engine/syncer_proto_util.cc

Index: chrome/browser/sync/engine/syncer_proto_util.cc
diff --git a/chrome/browser/sync/engine/syncer_proto_util.cc b/chrome/browser/sync/engine/syncer_proto_util.cc
index 26eed05cb70a36c211394a58ce8269ff6ed074a5..10a30ca24f4a101589e5982cffe15b6601b4fc37 100644
--- a/chrome/browser/sync/engine/syncer_proto_util.cc
+++ b/chrome/browser/sync/engine/syncer_proto_util.cc
@@ -19,7 +19,7 @@
 #include "chrome/browser/sync/syncable/syncable.h"
 #include "chrome/browser/sync/util/time.h"
 
-using browser_sync::SyncProtocolErrorType;
+using browser_sync::SyncOperationResultType;
 using std::string;
 using std::stringstream;
 using syncable::BASE_VERSION;
@@ -38,6 +38,44 @@ using sessions::SyncSession;
 
 namespace {
 
+SyncOperationResult ServerConnectionErrorToSyncOperationResult(
+    const browser_sync::HttpResponse::ServerConnectionCode server_status)
+{
+  SyncOperationResult result;
+
+  switch (server_status) {
+    case browser_sync::HttpResponse::CONNECTION_UNAVAILABLE:
+      result.error_type =  NETWORK_CONNECTION_UNAVAILABLE;
+      break;
+    case browser_sync::HttpResponse::IO_ERROR:
+      result.error_type = NETWORK_IO_ERROR;
+      break;
+    case browser_sync::HttpResponse::SYNC_SERVER_ERROR:
+      // FIXME: how does this happen?  Can we map this to TRANSIENT_ERROR or
+      // NON_RETRIABLE_ERROR?
+      result.error_type = SYNC_SERVER_ERROR;
+      break;
+    case browser_sync::HttpResponse::SYNC_AUTH_ERROR:
+      // FIXME: how does this compare to invalid credential?
+      result.error_type = SYNC_AUTH_ERROR;
+      break;
+    case browser_sync::HttpResponse::RETRY:
+      result.error_type = TRANSIENT_ERROR;
+      break;
+
+    // If we succeeded in contacting the server, then the server will provide us
+    // with all the data we need to compute a SyncOperationResult.  We don't
+    // handle that here.
+    case browser_sync::HttpResponse::SERVER_CONNECTION_OK:
+    case browser_sync::HttpResponse::NONE:
+    default:
+      NOTREACHED();
+      result.error_type = INVALID;
+  }
+
+  return result;
+}
+
 // Time to backoff syncing after receiving a throttled response.
 static const int kSyncDelayAfterThrottled = 2 * 60 * 60;  // 2 hours
 void LogResponseProfilingData(const ClientToServerResponse& response) {
@@ -203,11 +241,11 @@ bool IsVeryFirstGetUpdates(const ClientToServerMessage& message) {
   return true;
 }
 
-SyncProtocolErrorType ConvertSyncProtocolErrorTypePBToLocalType(
+SyncOperationResultType ConvertSyncProtocolErrorTypePBToLocalType(
     const sync_pb::ClientToServerResponse::ErrorType& error_type) {
   switch (error_type) {
     case ClientToServerResponse::SUCCESS:
-      return browser_sync::SYNC_SUCCESS;
+      return browser_sync::OPERATION_SUCCESS;
     case ClientToServerResponse::NOT_MY_BIRTHDAY:
       return browser_sync::NOT_MY_BIRTHDAY;
     case ClientToServerResponse::THROTTLED:
@@ -251,9 +289,9 @@ browser_sync::ClientAction ConvertClientActionPBToLocalClientAction(
   }
 }
 
-browser_sync::SyncProtocolError ConvertErrorPBToLocalType(
+browser_sync::SyncOperationResult ConvertErrorPBToLocalType(
     const sync_pb::ClientToServerResponse::Error& error) {
-  browser_sync::SyncProtocolError sync_protocol_error;
+  browser_sync::SyncOperationResult sync_protocol_error;
   sync_protocol_error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(
       error.error_type());
   sync_protocol_error.error_description = error.error_description();
@@ -264,9 +302,9 @@ browser_sync::SyncProtocolError ConvertErrorPBToLocalType(
 }
 
 // TODO(lipalani) : Rename these function names as per the CR for issue 7740067.
-browser_sync::SyncProtocolError ConvertLegacyErrorCodeToNewError(
+browser_sync::SyncOperationResult ConvertLegacyErrorCodeToNewError(
     const sync_pb::ClientToServerResponse::ErrorType& error_type) {
-  browser_sync::SyncProtocolError error;
+  browser_sync::SyncOperationResult error;
   error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(error_type);
   if (error_type == ClientToServerResponse::CLEAR_PENDING ||
       error_type == ClientToServerResponse::NOT_MY_BIRTHDAY) {
@@ -278,7 +316,7 @@ browser_sync::SyncProtocolError ConvertLegacyErrorCodeToNewError(
 }  // namespace
 
 // static
-bool SyncerProtoUtil::PostClientToServerMessage(
+SyncOperationResult SyncerProtoUtil::PostClientToServerMessage(
     const ClientToServerMessage& msg,
     ClientToServerResponse* response,
     SyncSession* session) {
@@ -291,14 +329,26 @@ bool SyncerProtoUtil::PostClientToServerMessage(
 
   ScopedDirLookup dir(session->context()->directory_manager(),
       session->context()->account_name());
-  if (!dir.good())
-    return false;
+  if (!dir.good()) {
+    SyncOperationResult result;
+    result.error_type = DIRECTORY_LOOKUP_FAILED;
+    return result;
+  }
 
   if (!PostAndProcessHeaders(session->context()->connection_manager(), session,
-                             msg, response))
-    return false;
+                             msg, response)) {
+    // There was an error establishing communication with the server.
+    // We can not proceed beyond this point.
+    const browser_sync::HttpResponse::ServerConnectionCode server_status =
+        session->context()->connection_manager()->server_status();
+
+    DCHECK(server_status != browser_sync::HttpResponse::NONE);
+    DCHECK(server_status != browser_sync::HttpResponse::SERVER_CONNECTION_OK);
+
+    return ServerConnectionErrorToSyncOperationResult(server_status);
+  }
 
-  browser_sync::SyncProtocolError sync_protocol_error;
+  browser_sync::SyncOperationResult sync_protocol_error;
 
   // Birthday mismatch overrides any error that is sent by the server.
   if (!VerifyResponseBirthday(dir, response)) {
@@ -327,27 +377,25 @@ bool SyncerProtoUtil::PostClientToServerMessage(
     case browser_sync::UNKNOWN_ERROR:
       LOG(WARNING) << "Sync protocol out-of-date. The server is using a more "
                    << "recent version.";
-      return false;
-    case browser_sync::SYNC_SUCCESS:
+      break;
+    case browser_sync::OPERATION_SUCCESS:
       LogResponseProfilingData(*response);
-      return true;
+      break;
     case browser_sync::THROTTLED:
       LOG(WARNING) << "Client silenced by server.";
       session->delegate()->OnSilencedUntil(base::TimeTicks::Now() +
           GetThrottleDelay(*response));
-      return false;
-    case browser_sync::TRANSIENT_ERROR:
-      return false;
+      break;
     case browser_sync::MIGRATION_DONE:
       HandleMigrationDoneResponse(response, session);
-      return false;
-    case browser_sync::CLEAR_PENDING:
-    case browser_sync::NOT_MY_BIRTHDAY:
-      return false;
-    default:
+      break;
+    case INVALID:
       NOTREACHED();
-      return false;
+    default:
+      break;
   }
+
+  return sync_protocol_error;
 }
 
 // static