Add return values to SyncerCommand

chrome/browser/sync/engine/syncer_command.h

-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.

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

FYI, you'll have to change these again :)

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

Done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_SYNC_ENGINE_SYNCER_COMMAND_H_
 #define CHROME_BROWSER_SYNC_ENGINE_SYNCER_COMMAND_H_
 #pragma once
 
 #include "base/basictypes.h"
 
+#include "chrome/browser/sync/engine/syncer_error.h"
+
 namespace browser_sync {
 
 namespace sessions {
 class SyncSession;
 }
 
 // Implementation of a simple command pattern intended to be driven by the
 // Syncer. SyncerCommand is abstract and all subclasses must implement
 // ExecuteImpl(). This is done so that chunks of syncer operation can be unit
 // tested.
 //
 // Example Usage:
 //
 //   SyncSession session = ...;
 //   SyncerCommand *cmd = SomeCommandFactory.createCommand(...);
 //   cmd->Execute(session);
 //   delete cmd;
 
 class SyncerCommand {
  public:
   SyncerCommand();
   virtual ~SyncerCommand();
 
   // Execute dispatches to a derived class's ExecuteImpl.
-  void Execute(sessions::SyncSession* session);
+  SyncerError Execute(sessions::SyncSession* session);

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

I think we should slap WARN_UNUSED_RESULT on this.  You'll have to edit
syncer.cc, but for now something like if (cmd.Execute() == UNINITIALIZED)
NOTREACHED() should be sufficient if there's nothing more intelligent to do.

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

I don't see a point in adding WARN_UNUSED_RESULT if we're going to hack around
it.  Maybe we should wait until the error detection and handling is more fully
baked?

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

I admit, that's true.  But I do think long term it makes sense, unless things
change considerably (as you may be alluding to with the fully-baked comment). 
If you can't come up with something useful to do now with the result values, I
guess this is fine.  Consider a TODO.

[rlarocque, 2012/01/06 21:25:34]

I think the best way to track this is with crbug.com/109422.  It might be worth
an OKR, too.  

 
   // ExecuteImpl is where derived classes actually do work.
-  virtual void ExecuteImpl(sessions::SyncSession* session) = 0;
+  virtual SyncerError ExecuteImpl(sessions::SyncSession* session) = 0;
  private:
   void SendNotifications(sessions::SyncSession* session);
   DISALLOW_COPY_AND_ASSIGN(SyncerCommand);
 };
 
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_ENGINE_SYNCER_COMMAND_H_