Add return values to SyncerCommand

chrome/browser/sync/engine/download_updates_command.cc

Index: chrome/browser/sync/engine/download_updates_command.cc
diff --git a/chrome/browser/sync/engine/download_updates_command.cc b/chrome/browser/sync/engine/download_updates_command.cc
index 86414a6e1e2ce26a4ca93c306a6f83af2bfd3fcb..ee29aac200fd61cf2e86f65b3266091165910677 100644
--- a/chrome/browser/sync/engine/download_updates_command.cc
+++ b/chrome/browser/sync/engine/download_updates_command.cc
@@ -30,7 +30,7 @@ using syncable::ModelTypeSetToString;
 DownloadUpdatesCommand::DownloadUpdatesCommand() {}
 DownloadUpdatesCommand::~DownloadUpdatesCommand() {}
 
-void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
+SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
   ClientToServerMessage client_to_server_message;
   ClientToServerResponse update_response;
 
@@ -48,7 +48,7 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
                       session->context()->account_name());
   if (!dir.good()) {
     LOG(ERROR) << "Scoped dir lookup failed!";
-    return;
+    return DIRECTORY_LOOKUP_FAILED;
   }
 
   // Request updates for all enabled types.
@@ -105,7 +105,7 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
     status->increment_num_consecutive_errors();
     status->mutable_updates_response()->Clear();
     LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates";
-    return;
+    return NO_ERROR; // TODO(rlarocque): Return an error here.

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

I'm assuming you aren't already because PostClientToServerMessage can fail for
several reasons? (If so, it'd be useful to at least add that to the comment
here).  Perhaps we should add a CLIENT_TO_SERVER_POST_FAILED for now so we are a
little more accurate, file a bug, and iterate on that (a sketch of a plan would
be by passing in the lookup to PostClientToServerMessage to avoid
DIRECTORY_OPEN_FAILED, examining the protocol_error in status to avoid having to
return proto errors, thus reducing the errors possibly returned from
PostClientToServerMessage to those of PostAndProcessHeaders.)  

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

This will be fixed in the next patch in this series.  Right now there's no harm
in returning NO_ERROR, since the return values are ignored.  The next patch will
add a return value to PostClientToServerMessage(), and will return it here.

The fix I'm planning doesn't look exactly like what you're suggesting, but
should be equally effective.

   }
 
   status->mutable_updates_response()->CopyFrom(update_response);
@@ -115,6 +115,7 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
            << " updates and indicated "
            << update_response.get_updates().changes_remaining()
            << " updates left on server.";
+  return NO_ERROR;
 }
 
 void DownloadUpdatesCommand::AppendClientDebugInfoIfNeeded(