Add return values to SyncerCommand

chrome/browser/sync/engine/process_commit_response_command.cc

 // Copyright (c) 2011 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/process_commit_response_command.h"
 
 #include <cstddef>
 #include <set>
 #include <string>
 #include <vector>
@@ skipping 63 matching lines @@
   const StatusController& status = session.status_controller();
   for (size_t i = 0; i < status.commit_ids().size(); ++i) {
     groups_with_commits.insert(
         GetGroupForModelType(status.GetUnrestrictedCommitModelTypeAt(i),
                              session.routing_info()));
   }
 
   return groups_with_commits;
 }
 
-bool ProcessCommitResponseCommand::ModelNeutralExecuteImpl(
+SyncerError ProcessCommitResponseCommand::ModelNeutralExecuteImpl(
     sessions::SyncSession* session) {
   ScopedDirLookup dir(session->context()->directory_manager(),
                       session->context()->account_name());
   if (!dir.good()) {
     LOG(ERROR) << "Scoped dir lookup failed!";
-    return false;
+    return DIRECTORY_LOOKUP_FAILED;
   }
 
   const StatusController& status = session->status_controller();
   const ClientToServerResponse& response(status.commit_response());
   const vector<syncable::Id>& commit_ids(status.commit_ids());
 
   if (!response.has_commit()) {
     // TODO(sync): What if we didn't try to commit anything?

[tim (not reviewing), 2012/01/06 16:33:42]

Is this still a legit comment?

[rlarocque, 2012/01/06 19:44:21]

I doubt it.  We try not to post a commit request unless we have items to commit.
 But I'm not 100% sure that it's safe to remove.   

     LOG(WARNING) << "Commit response has no commit body!";
     IncrementErrorCounters(session->mutable_status_controller());
-    return false;
+    return SERVER_RESPONSE_VALIDATION_FAILED;
   }
 
   const CommitResponse& cr = response.commit();
   int commit_count = commit_ids.size();
   if (cr.entryresponse_size() != commit_count) {
     LOG(ERROR) << "Commit response has wrong number of entries! Expected:" <<
                commit_count << " Got:" << cr.entryresponse_size();
     for (int i = 0 ; i < cr.entryresponse_size() ; i++) {
       LOG(ERROR) << "Response #" << i << " Value: " <<
                  cr.entryresponse(i).response_type();
       if (cr.entryresponse(i).has_error_message())
         LOG(ERROR) << "  " << cr.entryresponse(i).error_message();
     }
     IncrementErrorCounters(session->mutable_status_controller());
-    return false;
+    return SERVER_RESPONSE_VALIDATION_FAILED;
   }
-  return true;
+  return NO_ERROR;
 }
 
-void ProcessCommitResponseCommand::ModelChangingExecuteImpl(
+SyncerError ProcessCommitResponseCommand::ModelChangingExecuteImpl(
     SyncSession* session) {
-  ProcessCommitResponse(session);
+  SyncerError result = ProcessCommitResponse(session);
   ExtensionsActivityMonitor* monitor = session->context()->extensions_monitor();
   if (session->status_controller().HasBookmarkCommitActivity() &&
       session->status_controller().syncer_status()
           .num_successful_bookmark_commits == 0) {
     monitor->PutRecords(session->extensions_activity());
     session->mutable_extensions_activity()->clear();
   }
+  return result;
 }
 
-void ProcessCommitResponseCommand::ProcessCommitResponse(
+SyncerError ProcessCommitResponseCommand::ProcessCommitResponse(
     SyncSession* session) {
-  // TODO(sync): This function returns if it sees problems. We probably want
-  // to flag the need for an update or similar.
   ScopedDirLookup dir(session->context()->directory_manager(),
                       session->context()->account_name());
   if (!dir.good()) {
     LOG(ERROR) << "Scoped dir lookup failed!";
-    return;
+    return DIRECTORY_LOOKUP_FAILED;
   }
 
   StatusController* status = session->mutable_status_controller();
   const ClientToServerResponse& response(status->commit_response());
   const CommitResponse& cr = response.commit();
   const sync_pb::CommitMessage& commit_message =
       status->commit_message().commit();
 
   // If we try to commit a parent and child together and the parent conflicts
   // the child will have a bad parent causing an error. As this is not a
@@ skipping 30 matching lines @@
           // TODO(sync): worry about sync_rate_ rate calc?
           ++successes;
           if (status->GetCommitModelTypeAt(proj[i]) == syncable::BOOKMARKS)
             status->increment_num_successful_bookmark_commits();
           status->increment_num_successful_commits();
           break;
         case CommitResponse::OVER_QUOTA:
           // We handle over quota like a retry, which is same as transient.
         case CommitResponse::RETRY:
         case CommitResponse::TRANSIENT_ERROR:
-          // TODO(tim): Now that we have SyncSession::Delegate, we
-          // should plumb this directly for exponential backoff purposes rather
-          // than trying to infer from HasMoreToSync(). See
-          // SyncerThread::CalculatePollingWaitTime.
           ++transient_error_commits;
           break;
         default:
           LOG(FATAL) << "Bad return from ProcessSingleCommitResponse";
       }
     }
   }
 
   // TODO(sync): move status reporting elsewhere.
   status->increment_num_conflicting_commits_by(conflicting_commits);
   if (0 == successes) {
     status->increment_num_consecutive_transient_error_commits_by(
         transient_error_commits);
     status->increment_num_consecutive_errors_by(transient_error_commits);
   } else {
     status->set_num_consecutive_transient_error_commits(0);
     status->set_num_consecutive_errors(0);
   }
-  int commit_count = static_cast<int>(proj.size());
   if (commit_count != (conflicting_commits + error_commits +
                        transient_error_commits)) {
     ResetErrorCounters(status);
   }
   SyncerUtil::MarkDeletedChildrenSynced(dir, &deleted_folders);
 
-  return;
+  int commit_count = static_cast<int>(proj.size());
+  if (commit_count == (successes + conflicting_commits)) {
+    // We consider conflicting commits as a success because things will work out
+    // on their own when we receive them.  Flags will be set so that
+    // HasMoreToSync() will cause SyncScheduler to enter another sync cycle to
+    // handle this condition.
+    return NO_ERROR;
+  } else if (error_commits > 0) {
+    return SERVER_RETURN_UNKNOWN_ERROR;
+  } else if (transient_error_commits > 0) {
+    return SERVER_RETURN_TRANSIENT_ERROR;
+  } else {
+    LOG(FATAL) << "Inconsistent counts when processing commit response";
+    return NO_ERROR;
+  }
 }
 
 void LogServerError(const CommitResponse_EntryResponse& res) {
   if (res.has_error_message())
     LOG(WARNING) << "  " << res.error_message();
   else
     LOG(WARNING) << "  No detailed error message returned from server";
 }
 
 CommitResponse::ResponseType
@@ skipping 275 matching lines @@
   // been recursively deleted.
   // TODO(nick): Here, commit_message.deleted() would be more correct than
   // local_entry->Get(IS_DEL).  For example, an item could be renamed, and then
   // deleted during the commit of the rename.  Unit test & fix.
   if (local_entry->Get(IS_DIR) && local_entry->Get(IS_DEL)) {
     deleted_folders->insert(local_entry->Get(ID));
   }
 }
 
 }  // namespace browser_sync