[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}

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.
 }
 

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

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(

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

sync/internal_api/syncapi_server_connection_manager_unittest.cc

@@ -72,10 +72,8 @@ TEST(SyncAPIServerConnectionManagerTest, VeryEarlyAbortPost) {
       "server", 0, true, new BlockingHttpPostFactory(), &signal);
 
   ServerConnectionManager::PostBufferParams params;
-  ScopedServerStatusWatcher watcher(&server, &params.response);
 
-  bool result = server.PostBufferToPath(
-      &params, "/testpath", "testauth", &watcher);
+  bool result = server.PostBufferToPath(&params, "/testpath", "testauth");
 
   EXPECT_FALSE(result);
   EXPECT_EQ(HttpResponse::CONNECTION_UNAVAILABLE,
@@ -89,11 +87,9 @@ TEST(SyncAPIServerConnectionManagerTest, EarlyAbortPost) {
       "server", 0, true, new BlockingHttpPostFactory(), &signal);
 
   ServerConnectionManager::PostBufferParams params;
-  ScopedServerStatusWatcher watcher(&server, &params.response);
 
   signal.Signal();
-  bool result = server.PostBufferToPath(
-      &params, "/testpath", "testauth", &watcher);
+  bool result = server.PostBufferToPath(&params, "/testpath", "testauth");
 
   EXPECT_FALSE(result);
   EXPECT_EQ(HttpResponse::CONNECTION_UNAVAILABLE,
@@ -107,7 +103,6 @@ TEST(SyncAPIServerConnectionManagerTest, AbortPost) {
       "server", 0, true, new BlockingHttpPostFactory(), &signal);
 
   ServerConnectionManager::PostBufferParams params;
-  ScopedServerStatusWatcher watcher(&server, &params.response);
 
   base::Thread abort_thread("Test_AbortThread");
   ASSERT_TRUE(abort_thread.Start());
@@ -117,8 +112,7 @@ TEST(SyncAPIServerConnectionManagerTest, AbortPost) {
                  base::Unretained(&signal)),
       TestTimeouts::tiny_timeout());
 
-  bool result = server.PostBufferToPath(
-      &params, "/testpath", "testauth", &watcher);
+  bool result = server.PostBufferToPath(&params, "/testpath", "testauth");
 
   EXPECT_FALSE(result);
   EXPECT_EQ(HttpResponse::CONNECTION_UNAVAILABLE,

sync/protocol/proto_enum_conversions.cc

@@ -155,12 +155,12 @@ const char* GetErrorTypeString(sync_pb::SyncEnums::ErrorType error_type) {
   ASSERT_ENUM_BOUNDS(sync_pb::SyncEnums, ErrorType, SUCCESS, UNKNOWN);
   switch (error_type) {
     ENUM_CASE(sync_pb::SyncEnums, SUCCESS);
-    ENUM_CASE(sync_pb::SyncEnums, ACCESS_DENIED);
+    ENUM_CASE(sync_pb::SyncEnums, DEPRECATED_ACCESS_DENIED);
     ENUM_CASE(sync_pb::SyncEnums, NOT_MY_BIRTHDAY);
     ENUM_CASE(sync_pb::SyncEnums, THROTTLED);
-    ENUM_CASE(sync_pb::SyncEnums, AUTH_EXPIRED);
-    ENUM_CASE(sync_pb::SyncEnums, USER_NOT_ACTIVATED);
-    ENUM_CASE(sync_pb::SyncEnums, AUTH_INVALID);
+    ENUM_CASE(sync_pb::SyncEnums, DEPRECATED_AUTH_EXPIRED);
+    ENUM_CASE(sync_pb::SyncEnums, DEPRECATED_USER_NOT_ACTIVATED);
+    ENUM_CASE(sync_pb::SyncEnums, DEPRECATED_AUTH_INVALID);
     ENUM_CASE(sync_pb::SyncEnums, CLEAR_PENDING);
     ENUM_CASE(sync_pb::SyncEnums, TRANSIENT_ERROR);
     ENUM_CASE(sync_pb::SyncEnums, MIGRATION_DONE);
@@ -187,7 +187,6 @@ const char* GetActionString(sync_pb::SyncEnums::Action action) {
   }
   NOTREACHED();
   return "";
-
 }
 
 const char* GetLaunchTypeString(sync_pb::AppSpecifics::LaunchType launch_type) {

sync/protocol/sync_enums.proto

@@ -77,16 +77,14 @@ message SyncEnums {
 
   enum ErrorType {
     SUCCESS               = 0;
-    ACCESS_DENIED         = 1;  // Returned when the user doesn't have access to
-                                // store (instead of HTTP 401).
+    DEPRECATED_ACCESS_DENIED = 1;
     NOT_MY_BIRTHDAY       = 2;  // Returned when the server and client disagree
                                 // on the store birthday.
     THROTTLED             = 3;  // Returned when the store has exceeded the
                                 // allowed bandwidth utilization.
-    AUTH_EXPIRED          = 4;  // Auth token or cookie has expired.
-    USER_NOT_ACTIVATED    = 5;  // User doesn't have the Chrome bit set on that
-                                // Google Account.
-    AUTH_INVALID          = 6;  // Auth token or cookie is otherwise invalid.
+    DEPRECATED_AUTH_EXPIRED = 4;
+    DEPRECATED_USER_NOT_ACTIVATED = 5;
+    DEPRECATED_AUTH_INVALID = 6;
     CLEAR_PENDING         = 7;  // A clear of the user data is pending (e.g.
                                 // initiated by privacy request).  Client should
                                 // come back later.

sync/test/engine/mock_connection_manager.cc

@@ -48,7 +48,6 @@ MockConnectionManager::MockConnectionManager(syncable::Directory* directory,
       mid_commit_observer_(NULL),
       throttling_(false),
       partialThrottling_(false),
-      fail_with_auth_invalid_(false),
       fail_non_periodic_get_updates_(false),
       next_position_in_parent_(2),
       use_legacy_bookmarks_protocol_(false),
@@ -76,9 +75,8 @@ void MockConnectionManager::SetMidCommitObserver(
 }
 
 bool MockConnectionManager::PostBufferToPath(PostBufferParams* params,
-    const string& path,
-    const string& auth_token,
-    ScopedServerStatusWatcher* watcher) {
+                                             const string& path,
+                                             const string& auth_token) {
   ClientToServerMessage post;
   CHECK(post.ParseFromString(params->buffer_in));
   CHECK(post.has_protocol_version());
@@ -171,9 +169,6 @@ bool MockConnectionManager::PostBufferToPath(PostBufferParams* params,
       }
       partialThrottling_ = false;
     }
-
-    if (fail_with_auth_invalid_)
-      response.set_error_code(SyncEnums::AUTH_INVALID);
   }
 
   response.SerializeToString(&params->buffer_out);

sync/test/engine/mock_connection_manager.h

@@ -40,8 +40,7 @@ class MockConnectionManager : public ServerConnectionManager {
   // Overridden ServerConnectionManager functions.
   bool PostBufferToPath(PostBufferParams*,
                         const std::string& path,
-                        const std::string& auth_token,
-                        ScopedServerStatusWatcher* watcher) override;
+                        const std::string& auth_token) override;
 
   // Control of commit response.
   // NOTE: Commit callback is invoked only once then reset.
@@ -390,11 +389,6 @@ class MockConnectionManager : public ServerConnectionManager {
   // Protected by |response_code_override_lock_|.
   bool partialThrottling_;
 
-  // Whether we are failing all requests by returning
-  // ClientToServerResponse::AUTH_INVALID.
-  // Protected by |response_code_override_lock_|.
-  bool fail_with_auth_invalid_;
-
   base::Lock response_code_override_lock_;
 
   // True if we are only accepting GetUpdatesCallerInfo::PERIODIC requests.