[Sync] Remove ScopedServerStatusWatcher

Also deprecate a few error types that are not returned from server.

Without this class it is more clear how server_status gets from
HttpResponse to ServerConnectionManager's server_status.

R=zea@chromium.org,timonvo@chromium.org
BUG=567273

Review URL: https://codereview.chromium.org/1505953002

Cr-Commit-Position: refs/heads/master@{#363713}

4 files changed, 19 insertions, 80 deletions

diff --git a/sync/engine/net/server_connection_manager.cc b/sync/engine/net/server_connection_manager.cc
index 3101109..2531c97 100644
--- a/sync/engine/net/server_connection_manager.cc
+++ b/sync/engine/net/server_connection_manager.cc
@@ -163,17 +163,6 @@ int ServerConnectionManager::Connection::ReadResponse(string* out_buffer,
   return bytes_read;
 }
 
-ScopedServerStatusWatcher::ScopedServerStatusWatcher(
-    ServerConnectionManager* conn_mgr, HttpResponse* response)
-    : conn_mgr_(conn_mgr),
-      response_(response) {
-  response->server_status = conn_mgr->server_status_;
-}
-
-ScopedServerStatusWatcher::~ScopedServerStatusWatcher() {
-  conn_mgr_->SetServerStatus(response_->server_status);
-}
-
 ServerConnectionManager::ServerConnectionManager(
     const string& server,
     int port,
@@ -273,18 +262,19 @@ void ServerConnectionManager::NotifyStatusChanged() {
 }
 
 bool ServerConnectionManager::PostBufferWithCachedAuth(
-    PostBufferParams* params, ScopedServerStatusWatcher* watcher) {
+    PostBufferParams* params) {
   DCHECK(thread_checker_.CalledOnValidThread());
   string path =
       MakeSyncServerPath(proto_sync_path(), MakeSyncQueryString(client_id_));
-  return PostBufferToPath(params, path, auth_token(), watcher);
+  bool result = PostBufferToPath(params, path, auth_token());
+  SetServerStatus(params->response.server_status);
+  return result;
 }
 
 bool ServerConnectionManager::PostBufferToPath(PostBufferParams* params,
-    const string& path, const string& auth_token,
-    ScopedServerStatusWatcher* watcher) {
+                                               const string& path,
+                                               const string& auth_token) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(watcher != NULL);
 
   // TODO(pavely): crbug.com/273096. Check for "credentials_lost" is added as
   // workaround for M29 blocker to avoid sending RPC to sync with known invalid
@@ -364,8 +354,7 @@ void ServerConnectionManager::RemoveListener(
   listeners_.RemoveObserver(listener);
 }
 
-ServerConnectionManager::Connection* ServerConnectionManager::MakeConnection()
-{
+ServerConnectionManager::Connection* ServerConnectionManager::MakeConnection() {
   return NULL;  // For testing.
 }
 
diff --git a/sync/engine/net/server_connection_manager.h b/sync/engine/net/server_connection_manager.h
index 6514290..15be3b8 100644
--- a/sync/engine/net/server_connection_manager.h
+++ b/sync/engine/net/server_connection_manager.h
@@ -106,22 +106,6 @@ class SYNC_EXPORT_PRIVATE ServerConnectionEventListener {
   virtual ~ServerConnectionEventListener() {}
 };
 
-class ServerConnectionManager;
-// A helper class that automatically notifies when the status changes.
-// TODO(tim): This class shouldn't be exposed outside of the implementation,
-// bug 35060.
-class SYNC_EXPORT_PRIVATE ScopedServerStatusWatcher
-    : public base::NonThreadSafe {
- public:
-  ScopedServerStatusWatcher(ServerConnectionManager* conn_mgr,
-                            HttpResponse* response);
-  virtual ~ScopedServerStatusWatcher();
- private:
-  ServerConnectionManager* const conn_mgr_;
-  HttpResponse* const response_;
-  DISALLOW_COPY_AND_ASSIGN(ScopedServerStatusWatcher);
-};
-
 // Use this class to interact with the sync server.
 // The ServerConnectionManager currently supports POSTing protocol buffers.
 //
@@ -189,8 +173,7 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager : public CancelationObserver {
   // set auth token in our headers.
   //
   // Returns true if executed successfully.
-  virtual bool PostBufferWithCachedAuth(PostBufferParams* params,
-                                        ScopedServerStatusWatcher* watcher);
+  virtual bool PostBufferWithCachedAuth(PostBufferParams* params);
 
   void AddListener(ServerConnectionEventListener* listener);
   void RemoveListener(ServerConnectionEventListener* listener);
@@ -250,8 +233,7 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager : public CancelationObserver {
   // Internal PostBuffer base function.
   virtual bool PostBufferToPath(PostBufferParams*,
                                 const std::string& path,
-                                const std::string& auth_token,
-                                ScopedServerStatusWatcher* watcher);
+                                const std::string& auth_token);
 
   // An internal helper to clear our auth_token_ and cache the old version
   // in |previously_invalidated_token_| to shelter us from retrying with a
@@ -307,7 +289,6 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager : public CancelationObserver {
 
  private:
   friend class Connection;
-  friend class ScopedServerStatusWatcher;
 
   // A class to help deal with cleaning up active Connection objects when (for
   // ex) multiple early-exits are present in some scope. ScopedConnectionHelper
diff --git a/sync/engine/syncer_proto_util.cc b/sync/engine/syncer_proto_util.cc
index cd77e1e..306137b 100644
--- a/sync/engine/syncer_proto_util.cc
+++ b/sync/engine/syncer_proto_util.cc
@@ -4,6 +4,8 @@
 
 #include "sync/engine/syncer_proto_util.h"
 
+#include <map>
+
 #include "base/format_macros.h"
 #include "base/strings/stringprintf.h"
 #include "google_apis/google_api_keys.h"
@@ -130,10 +132,6 @@ SyncProtocolErrorType PBErrorTypeToSyncProtocolErrorType(
       return CLIENT_DATA_OBSOLETE;
     case sync_pb::SyncEnums::UNKNOWN:
       return UNKNOWN_ERROR;
-    case sync_pb::SyncEnums::USER_NOT_ACTIVATED:
-    case sync_pb::SyncEnums::AUTH_INVALID:
-    case sync_pb::SyncEnums::ACCESS_DENIED:
-      return INVALID_CREDENTIAL;
     default:
       NOTREACHED();
       return UNKNOWN_ERROR;
@@ -345,28 +343,13 @@ bool SyncerProtoUtil::PostAndProcessHeaders(ServerConnectionManager* scm,
             ClientToServerMessage::default_instance().protocol_version());
   msg.SerializeToString(&params.buffer_in);
 
-  ScopedServerStatusWatcher server_status_watcher(scm, &params.response);
   // Fills in params.buffer_out and params.response.
-  if (!scm->PostBufferWithCachedAuth(&params, &server_status_watcher)) {
+  if (!scm->PostBufferWithCachedAuth(&params)) {
     LOG(WARNING) << "Error posting from syncer:" << params.response;
     return false;
   }
 
-  if (response->ParseFromString(params.buffer_out)) {
-    // TODO(tim): This is an egregious layering violation (bug 35060).
-    switch (response->error_code()) {
-      case sync_pb::SyncEnums::ACCESS_DENIED:
-      case sync_pb::SyncEnums::AUTH_INVALID:
-      case sync_pb::SyncEnums::USER_NOT_ACTIVATED:
-        // Fires on ScopedServerStatusWatcher
-        params.response.server_status = HttpResponse::SYNC_AUTH_ERROR;
-        return false;
-      default:
-        return true;
-    }
-  }
-
-  return false;
+  return response->ParseFromString(params.buffer_out);
 }
 
 base::TimeDelta SyncerProtoUtil::GetThrottleDelay(
diff --git a/sync/engine/syncer_proto_util_unittest.cc b/sync/engine/syncer_proto_util_unittest.cc
index 0bc745f..ae55378 100644
--- a/sync/engine/syncer_proto_util_unittest.cc
+++ b/sync/engine/syncer_proto_util_unittest.cc
@@ -34,11 +34,11 @@ using sessions::SyncSessionContext;
 
 class MockDelegate : public sessions::SyncSession::Delegate {
  public:
-   MockDelegate() {}
-   ~MockDelegate() {}
+  MockDelegate() {}
+  ~MockDelegate() {}
 
   MOCK_METHOD1(OnReceivedShortPollIntervalUpdate, void(const base::TimeDelta&));
-  MOCK_METHOD1(OnReceivedLongPollIntervalUpdate ,void(const base::TimeDelta&));
+  MOCK_METHOD1(OnReceivedLongPollIntervalUpdate, void(const base::TimeDelta&));
   MOCK_METHOD1(OnReceivedSessionsCommitDelay, void(const base::TimeDelta&));
   MOCK_METHOD1(OnReceivedClientInvalidationHintBufferSize, void(int));
   MOCK_METHOD1(OnSyncProtocolError, void(const SyncProtocolError&));
@@ -225,22 +225,17 @@ TEST_F(SyncerProtoUtilTest, AddRequestBirthday) {
 
 class DummyConnectionManager : public ServerConnectionManager {
  public:
-  DummyConnectionManager(CancelationSignal* signal)
+  explicit DummyConnectionManager(CancelationSignal* signal)
       : ServerConnectionManager("unused", 0, false, signal),
-        send_error_(false),
-        access_denied_(false) {}
+        send_error_(false) {}
 
   ~DummyConnectionManager() override {}
-  bool PostBufferWithCachedAuth(PostBufferParams* params,
-                                ScopedServerStatusWatcher* watcher) override {
+  bool PostBufferWithCachedAuth(PostBufferParams* params) override {
     if (send_error_) {
       return false;
     }
 
     sync_pb::ClientToServerResponse response;
-    if (access_denied_) {
-      response.set_error_code(sync_pb::SyncEnums::ACCESS_DENIED);
-    }
     response.SerializeToString(&params->buffer_out);
 
     return true;
@@ -250,13 +245,8 @@ class DummyConnectionManager : public ServerConnectionManager {
     send_error_ = send;
   }
 
-  void set_access_denied(bool denied) {
-    access_denied_ = denied;
-  }
-
  private:
   bool send_error_;
-  bool access_denied_;
 };
 
 TEST_F(SyncerProtoUtilTest, PostAndProcessHeaders) {
@@ -275,10 +265,6 @@ TEST_F(SyncerProtoUtilTest, PostAndProcessHeaders) {
   dcm.set_send_error(false);
   EXPECT_TRUE(SyncerProtoUtil::PostAndProcessHeaders(&dcm, NULL,
       msg, &response));
-
-  dcm.set_access_denied(true);
-  EXPECT_FALSE(SyncerProtoUtil::PostAndProcessHeaders(&dcm, NULL,
-      msg, &response));
 }
 
 }  // namespace syncer