Add return values to SyncerCommand

chrome/browser/sync/engine/resolve_conflicts_command.cc

Index: chrome/browser/sync/engine/resolve_conflicts_command.cc
diff --git a/chrome/browser/sync/engine/resolve_conflicts_command.cc b/chrome/browser/sync/engine/resolve_conflicts_command.cc
index e76b1c7f19259312697b15e331074dbf5521076f..253312e566608ff73b449d863d310b3069a6d429 100644
--- a/chrome/browser/sync/engine/resolve_conflicts_command.cc
+++ b/chrome/browser/sync/engine/resolve_conflicts_command.cc
@@ -19,26 +19,26 @@ std::set<ModelSafeGroup> ResolveConflictsCommand::GetGroupsToChange(
   return session.GetEnabledGroupsWithConflicts();
 }
 
-void ResolveConflictsCommand::ModelChangingExecuteImpl(
+SyncerError ResolveConflictsCommand::ModelChangingExecuteImpl(
     sessions::SyncSession* session) {
   ConflictResolver* resolver = session->context()->resolver();
-  DCHECK(resolver);
-  if (!resolver)
-    return;
+  CHECK(resolver);

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

This use of CHECK is not in accordance with Chromium general practices. Unless
I'm missing something and/or you are you trying to hunt something specific down.
The DCHECK was there to catch developer error when coding (i.e. I accidentally
refactor code away that was setting the resolver in the context), and NOT for
uber paranoid runtime errors / handling malformed input / security
vulnerability.
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

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

Good point.  

I had changed this because I didn't like the code that handles cases where the
resolver was NULL.  Normally I wouldn't bother to change this, but in this case
not removing the return statement would mean I need to return *something*.  I
didn't want to add a RESOVLER_LOOKUP_FAILED to the SyncerError enum, so I
changed the DCHECK and remove the "if (!resolver)" check.

According to the style guidelines, it's recommended to not handle DCHECK
failures, so I guess I'll leave the DCHECK as is and remove the "if
(!resolver)".

[tim (not reviewing), 2012/01/06 20:07:13]

Sounds good.

 
   syncable::ScopedDirLookup dir(session->context()->directory_manager(),
                                 session->context()->account_name());
   if (!dir.good())
-    return;
+    return DIRECTORY_LOOKUP_FAILED;
   sessions::StatusController* status = session->mutable_status_controller();
   const sessions::ConflictProgress* progress = status->conflict_progress();
   if (!progress)
-    return;  // Nothing to do.
+    return NO_ERROR;  // Nothing to do.
   syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
   const Cryptographer* cryptographer =
       session->context()->directory_manager()->GetCryptographer(&trans);
   status->update_conflicts_resolved(
       resolver->ResolveConflicts(&trans, cryptographer, *progress, status));
+
+  return NO_ERROR;
 }
 
 }  // namespace browser_sync