Add return values to SyncerCommand

chrome/browser/sync/engine/model_changing_syncer_command.cc

Index: chrome/browser/sync/engine/model_changing_syncer_command.cc
diff --git a/chrome/browser/sync/engine/model_changing_syncer_command.cc b/chrome/browser/sync/engine/model_changing_syncer_command.cc
index 1c392cbd20877ecfc27fa47abddc076dd2627361..da59a5117b361c0537de28a4922fbaf3528c8f61 100644
--- a/chrome/browser/sync/engine/model_changing_syncer_command.cc
+++ b/chrome/browser/sync/engine/model_changing_syncer_command.cc
@@ -9,14 +9,16 @@
 #include "base/bind_helpers.h"
 #include "chrome/browser/sync/sessions/status_controller.h"
 #include "chrome/browser/sync/sessions/sync_session.h"
-#include "chrome/browser/sync/util/unrecoverable_error_info.h"
 
 namespace browser_sync {
 
-void ModelChangingSyncerCommand::ExecuteImpl(sessions::SyncSession* session) {
+SyncerError ModelChangingSyncerCommand::ExecuteImpl(
+    sessions::SyncSession* session) {
   work_session_ = session;
-  if (!ModelNeutralExecuteImpl(work_session_)) {
-    return;
+  SyncerError result = ModelNeutralExecuteImpl(work_session_);
+
+  if (result != NO_ERROR) {
+    return result;
   }
 
   const std::set<ModelSafeGroup>& groups_to_change =
@@ -40,15 +42,19 @@ void ModelChangingSyncerCommand::ExecuteImpl(sessions::SyncSession* session) {
         // unretained.
         base::Unretained(this));
 
-    // TODO(lipalani): Check the return value for an unrecoverable error.
-    ignore_result(worker->DoWorkAndWaitUntilDone(c));
-
+    SyncerError this_worker_result = worker->DoWorkAndWaitUntilDone(c);
+    // TODO(rlarocque): Figure out a better way to deal with errors from
+    // multiple models at once.

[lipalani1, 2012/01/05 22:59:41]

I think you should return from here on the first instance of the error.

Any error that has surfaced here indicates no forward progress could not be made
in the sync cycle. If indeed forward progress could be made(like the error is
something benign) we should not return them because then in all places  down the
call stack(and up in the call stack as well) is this a benign error or not.

We should probably add the comment to the place where the error is defined as
well.

[rlarocque, 2012/01/06 00:24:31]

I agree this is probably the right thing to do.  I don't like the idea of having
one model affect another but, in this case, that's what makes the most sense. 
However, I don't want to do this yet.  

We don't currently kick out of the state machine if we detect an unrecoverable
error.  Any error handling we implement here would affect only this
SyncerCommand.  Other SyncerCommands would proceed as normal (unless they, too,
triggered UnrecoverableErrors).

Adding error handling in syncer.cc is beyond the scope of this change.  This
change is big enough as is, and I like the fact that it has no effect on syncer
behaviour.

+    if (this_worker_result != NO_ERROR)
+      result = this_worker_result;
   }
+
+  return result;
 }
 
-bool ModelChangingSyncerCommand::ModelNeutralExecuteImpl(
+SyncerError ModelChangingSyncerCommand::ModelNeutralExecuteImpl(
     sessions::SyncSession* session) {
-  return true;
+  return NO_ERROR;
 }
 
 }  // namespace browser_sync