Revert 267422 because it broke compile on linux-clang buildbots.

The build failure:

sync/test/fake_server/fake_sync_server_http_handler.cc:51:61: error: too many arguments to function call, expected 2, have 3
                                                            &response);
                                                            ^~~~~~~~~
sync/test/fake_server/fake_server.h:44:3: note: 'HandleCommand' declared here
  void HandleCommand(const std::string& request,
  ^

> Use FakeServer-based invalidations for Sync tests
> 
> This CL creates a new InvalidationService implementation,
> FakeServerInvalidationService, to remove FakeServer-based tests'
> dependency on the Python XMPP server.
> 
> Another major change is that FakeServer's HandleCommand is now executed
> on the UI thread and locking has been removed from FakeServer itself.
> 
> BUG=323265
> 
> Review URL: https://codereview.chromium.org/234113002

TBR=pvalenzuela@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267432 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 62 insertions, 146 deletions

diff --git a/sync/test/fake_server/fake_server.cc b/sync/test/fake_server/fake_server.cc
index 798797e..d758e5d 100644
--- a/sync/test/fake_server/fake_server.cc
+++ b/sync/test/fake_server/fake_server.cc
@@ -4,7 +4,6 @@
 
 #include "sync/test/fake_server/fake_server.h"
 
-#include <algorithm>
 #include <limits>
 #include <string>
 #include <vector>
@@ -30,6 +29,7 @@
 using std::string;
 using std::vector;
 
+using base::AutoLock;
 using syncer::GetModelType;
 using syncer::ModelType;
 using syncer::ModelTypeSet;
@@ -238,8 +238,11 @@ void FakeServer::SaveEntity(FakeServerEntity* entity) {
   entities_[entity->GetId()] = entity;
 }
 
-void FakeServer::HandleCommand(const string& request,
-                               const HandleCommandCallback& callback) {
+int FakeServer::HandleCommand(const string& request,
+                              int* response_code,
+                              string* response) {
+  AutoLock lock(lock_);
+
   sync_pb::ClientToServerMessage message;
   bool parsed = message.ParseFromString(request);
   DCHECK(parsed);
@@ -256,20 +259,20 @@ void FakeServer::HandleCommand(const string& request,
                                     response_proto.mutable_commit());
       break;
     default:
-      callback.Run(net::ERR_NOT_IMPLEMENTED, 0, string());;
-      return;
+      return net::ERR_NOT_IMPLEMENTED;
   }
 
   if (!success) {
     // TODO(pvalenzuela): Add logging here so that tests have more info about
     // the failure.
-    callback.Run(net::ERR_FAILED, 0, string());
-    return;
+    return net::HTTP_BAD_REQUEST;
   }
 
   response_proto.set_error_code(sync_pb::SyncEnums::SUCCESS);
   response_proto.set_store_birthday(birthday_);
-  callback.Run(0, net::HTTP_OK, response_proto.SerializeAsString());
+  *response_code = net::HTTP_OK;
+  *response = response_proto.SerializeAsString();
+  return 0;
 }
 
 bool FakeServer::HandleGetUpdatesRequest(
@@ -321,13 +324,13 @@ bool FakeServer::HandleGetUpdatesRequest(
   return true;
 }
 
-string FakeServer::CommitEntity(
+bool FakeServer::CommitEntity(
     const sync_pb::SyncEntity& client_entity,
     sync_pb::CommitResponse_EntryResponse* entry_response,
     string client_guid,
-    string parent_id) {
+    std::map<string, string>* client_to_server_ids) {
   if (client_entity.version() == 0 && client_entity.deleted()) {
-    return string();
+    return false;
   }
 
   FakeServerEntity* entity;
@@ -336,7 +339,7 @@ string FakeServer::CommitEntity(
     // TODO(pvalenzuela): Change the behavior of DeleteChilden so that it does
     // not modify server data if it fails.
     if (!DeleteChildren(client_entity.id_string())) {
-      return string();
+      return false;
     }
   } else if (GetModelType(client_entity) == syncer::NIGORI) {
     // NIGORI is the only permanent item type that should be updated by the
@@ -353,6 +356,11 @@ string FakeServer::CommitEntity(
       entity = UniqueClientEntity::CreateNew(client_entity);
     }
   } else {
+    string parent_id = client_entity.parent_id_string();
+    if (client_to_server_ids->find(parent_id) !=
+        client_to_server_ids->end()) {
+      parent_id = (*client_to_server_ids)[parent_id];
+    }
     // TODO(pvalenzuela): Validate entity's parent ID.
     if (entities_.find(client_entity.id_string()) != entities_.end()) {
       entity = BookmarkEntity::CreateUpdatedVersion(
@@ -367,12 +375,17 @@ string FakeServer::CommitEntity(
   if (entity == NULL) {
     // TODO(pvalenzuela): Add logging so that it is easier to determine why
     // creation failed.
-    return string();
+    return false;
+  }
+
+  // Record the ID if it was renamed.
+  if (client_entity.id_string() != entity->GetId()) {
+    (*client_to_server_ids)[client_entity.id_string()] = entity->GetId();
   }
 
   SaveEntity(entity);
   BuildEntryResponseForSuccessfulCommit(entry_response, entity);
-  return entity->GetId();
+  return true;
 }
 
 void FakeServer::BuildEntryResponseForSuccessfulCommit(
@@ -429,7 +442,6 @@ bool FakeServer::HandleCommitRequest(
     sync_pb::CommitResponse* response) {
   std::map<string, string> client_to_server_ids;
   string guid = commit.cache_guid();
-  ModelTypeSet committed_model_types;
 
   // TODO(pvalenzuela): Add validation of CommitMessage.entries.
   ::google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>::const_iterator it;
@@ -437,30 +449,11 @@ bool FakeServer::HandleCommitRequest(
     sync_pb::CommitResponse_EntryResponse* entry_response =
         response->add_entryresponse();
 
-    sync_pb::SyncEntity client_entity = *it;
-    string parent_id = client_entity.parent_id_string();
-    if (client_to_server_ids.find(parent_id) !=
-        client_to_server_ids.end()) {
-      parent_id = client_to_server_ids[parent_id];
-    }
-
-    string entity_id = CommitEntity(client_entity,
-                                    entry_response,
-                                    guid,
-                                    parent_id);
-    if (entity_id.empty()) {
+    if (!CommitEntity(*it, entry_response, guid, &client_to_server_ids)) {
       return false;
     }
-
-    // Record the ID if it was renamed.
-    if (entity_id != client_entity.id_string()) {
-      client_to_server_ids[client_entity.id_string()] = entity_id;
-    }
-    FakeServerEntity* entity = entities_[entity_id];
-    committed_model_types.Put(entity->GetModelType());
   }
 
-  FOR_EACH_OBSERVER(Observer, observers_, OnCommit(committed_model_types));
   return true;
 }
 
@@ -496,12 +489,4 @@ scoped_ptr<base::DictionaryValue> FakeServer::GetEntitiesAsDictionaryValue() {
   return dictionary.Pass();
 }
 
-void FakeServer::AddObserver(Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void FakeServer::RemoveObserver(Observer* observer) {
-  observers_.RemoveObserver(observer);
-}
-
 }  // namespace fake_server
diff --git a/sync/test/fake_server/fake_server.h b/sync/test/fake_server/fake_server.h
index 6af77ab..0621bbc 100644
--- a/sync/test/fake_server/fake_server.h
+++ b/sync/test/fake_server/fake_server.h
@@ -7,12 +7,10 @@
 
 #include <map>
 #include <string>
-#include <vector>
 
 #include "base/basictypes.h"
-#include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/observer_list.h"
+#include "base/synchronization/lock.h"
 #include "base/values.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/protocol/sync.pb.h"
@@ -23,26 +21,15 @@ namespace fake_server {
 // A fake version of the Sync server used for testing.
 class FakeServer {
  public:
-  typedef base::Callback<void(int, int, const std::string&)>
-      HandleCommandCallback;
-
-  class Observer {
-   public:
-    virtual ~Observer() {}
-
-    // Called after FakeServer has processed a successful commit. The types
-    // updated as part of the commit are passed in |committed_model_types|.
-    virtual void OnCommit(syncer::ModelTypeSet committed_model_types) = 0;
-  };
-
   FakeServer();
   virtual ~FakeServer();
 
-  // Asynchronously handles a /command POST to the server. If the error_code is
-  // passed to |callback| is 0 (success), the POST's response code and content
-  // will also be passed.
-  void HandleCommand(const std::string& request,
-                     const HandleCommandCallback& callback);
+  // Handles a /command POST to the server. If the return value is 0 (success),
+  // |response_code| and |response| will be set. Otherwise, the return value
+  // will be a network error code.
+  int HandleCommand(const std::string& request,
+                    int* response_code,
+                    std::string* response);
 
   // Creates a DicionaryValue representation of all entities present in the
   // server. The dictionary keys are the strings generated by ModelTypeToString
@@ -50,14 +37,6 @@ class FakeServer {
   // names.
   scoped_ptr<base::DictionaryValue> GetEntitiesAsDictionaryValue();
 
-  // Adds |observer| to FakeServer's observer list. This should be called
-  // before the Profile associated with |observer| is connected to the server.
-  void AddObserver(Observer* observer);
-
-  // Removes |observer| from the FakeServer's observer list. This method
-  // must be called if AddObserver was ever called with |observer|.
-  void RemoveObserver(Observer* observer);
-
  private:
   typedef std::map<std::string, FakeServerEntity*> EntityMap;
 
@@ -80,15 +59,14 @@ class FakeServer {
   void SaveEntity(FakeServerEntity* entity);
 
   // Commits a client-side SyncEntity to the server as a FakeServerEntity.
-  // The client that sent the commit is identified via |client_guid|. The
-  // parent ID string present in |client_entity| should be ignored in favor
-  // of |parent_id|. If the commit is successful, the entity's server ID string
-  // is returned and a new FakeServerEntity is saved in |entities_|.
-  std::string CommitEntity(
-      const sync_pb::SyncEntity& client_entity,
-      sync_pb::CommitResponse_EntryResponse* entry_response,
-      std::string client_guid,
-      std::string parent_id);
+  // The client that sent the commit is identified via |client_guid| and all
+  // entity ID renaming is tracked with |client_to_server_ids|. If the commit
+  // is successful, true is returned and the server's version of the SyncEntity
+  // is stored in |server_entity|.
+  bool CommitEntity(const sync_pb::SyncEntity& client_entity,
+                    sync_pb::CommitResponse_EntryResponse* entry_response,
+                    std::string client_guid,
+                    std::map<std::string, std::string>* client_to_server_ids);
 
   // Populates |entry_response| based on |entity|. It is assumed that
   // SaveEntity has already been called on |entity|.
@@ -104,6 +82,14 @@ class FakeServer {
   // |id|. A tombstone is not created for the entity itself.
   bool DeleteChildren(const std::string& id);
 
+  // The lock used to ensure that only one client is communicating with server
+  // at any given time.
+  //
+  // It is probably preferable to have FakeServer operate on its own thread and
+  // communicate with it via PostTask, but clients would still need to wait for
+  // requests to finish before proceeding.
+  base::Lock lock_;
+
   // This is the last version number assigned to an entity. The next entity will
   // have a version number of version_ + 1.
   int64 version_;
@@ -122,9 +108,6 @@ class FakeServer {
   // are kept track of so that permanent entities are not recreated for new
   // clients.
   syncer::ModelTypeSet created_permanent_entity_types_;
-
-  // FakeServer's observers.
-  ObserverList<Observer, true> observers_;
 };
 
 }  // namespace fake_server
diff --git a/sync/test/fake_server/fake_server_http_post_provider.cc b/sync/test/fake_server/fake_server_http_post_provider.cc
index 5dd45c2..e0b3436 100644
--- a/sync/test/fake_server/fake_server_http_post_provider.cc
+++ b/sync/test/fake_server/fake_server_http_post_provider.cc
@@ -6,11 +6,6 @@
 
 #include <string>
 
-#include "base/bind.h"
-#include "base/location.h"
-#include "base/memory/ref_counted.h"
-#include "base/sequenced_task_runner.h"
-#include "base/synchronization/waitable_event.h"
 #include "sync/test/fake_server/fake_server.h"
 
 using syncer::HttpPostProviderInterface;
@@ -18,9 +13,7 @@ using syncer::HttpPostProviderInterface;
 namespace fake_server {
 
 FakeServerHttpPostProviderFactory::FakeServerHttpPostProviderFactory(
-    FakeServer* fake_server,
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-        : fake_server_(fake_server), task_runner_(task_runner) { }
+    FakeServer* fake_server) : fake_server_(fake_server) { }
 
 FakeServerHttpPostProviderFactory::~FakeServerHttpPostProviderFactory() { }
 
@@ -28,7 +21,7 @@ void FakeServerHttpPostProviderFactory::Init(const std::string& user_agent) { }
 
 HttpPostProviderInterface* FakeServerHttpPostProviderFactory::Create() {
   FakeServerHttpPostProvider* http =
-      new FakeServerHttpPostProvider(fake_server_, task_runner_);
+      new FakeServerHttpPostProvider(fake_server_);
   http->AddRef();
   return http;
 }
@@ -39,11 +32,7 @@ void FakeServerHttpPostProviderFactory::Destroy(
 }
 
 FakeServerHttpPostProvider::FakeServerHttpPostProvider(
-    FakeServer* fake_server,
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-        : fake_server_(fake_server),
-          task_runner_(task_runner),
-          post_complete_(false, false) { }
+    FakeServer* fake_server) : fake_server_(fake_server) { }
 
 FakeServerHttpPostProvider::~FakeServerHttpPostProvider() { }
 
@@ -65,35 +54,13 @@ void FakeServerHttpPostProvider::SetPostPayload(const char* content_type,
   request_content_.assign(content, content_length);
 }
 
-void FakeServerHttpPostProvider::OnPostComplete(int error_code,
-                                                int response_code,
-                                                const std::string& response) {
-  post_error_code_ = error_code;
-  post_response_code_ = response_code;
-  response_ = response;
-  post_complete_.Signal();
-}
-
 bool FakeServerHttpPostProvider::MakeSynchronousPost(int* error_code,
                                                      int* response_code) {
-  // It is assumed that a POST is being made to /command.
-  FakeServer::HandleCommandCallback callback = base::Bind(
-      &FakeServerHttpPostProvider::OnPostComplete, base::Unretained(this));
-  task_runner_->PostNonNestableTask(FROM_HERE,
-                                    base::Bind(&FakeServer::HandleCommand,
-                                               base::Unretained(fake_server_),
-                                               base::ConstRef(request_content_),
-                                               base::ConstRef(callback)));
-  const int kTimeoutSecs = 5;
-  bool signaled = post_complete_.TimedWait(
-      base::TimeDelta::FromSeconds(kTimeoutSecs));
-  if (!signaled || *error_code != 0) {
-    return false;
-  }
-
-  *error_code = post_error_code_;
-  *response_code = post_response_code_;
-  return true;
+  // This assumes that a POST is being made to /command.
+  *error_code = fake_server_->HandleCommand(request_content_,
+                                            response_code,
+                                            &response_);
+  return (*error_code == 0);
 }
 
 int FakeServerHttpPostProvider::GetResponseContentLength() const {
diff --git a/sync/test/fake_server/fake_server_http_post_provider.h b/sync/test/fake_server/fake_server_http_post_provider.h
index 9e2aa0e..63f1eda 100644
--- a/sync/test/fake_server/fake_server_http_post_provider.h
+++ b/sync/test/fake_server/fake_server_http_post_provider.h
@@ -8,8 +8,6 @@
 #include <string>
 
 #include "base/memory/ref_counted.h"
-#include "base/sequenced_task_runner.h"
-#include "base/synchronization/waitable_event.h"
 #include "sync/internal_api/public/http_post_provider_factory.h"
 #include "sync/internal_api/public/http_post_provider_interface.h"
 
@@ -21,9 +19,7 @@ class FakeServerHttpPostProvider
     : public syncer::HttpPostProviderInterface,
       public base::RefCountedThreadSafe<FakeServerHttpPostProvider> {
  public:
-  explicit FakeServerHttpPostProvider(
-      FakeServer* fake_server,
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
+  explicit FakeServerHttpPostProvider(FakeServer* fake_server);
 
   // HttpPostProviderInterface implementation.
   virtual void SetExtraRequestHeaders(const char* headers) OVERRIDE;
@@ -43,22 +39,13 @@ class FakeServerHttpPostProvider
   virtual ~FakeServerHttpPostProvider();
 
  private:
-  void OnPostComplete(int error_code,
-                      int response_code,
-                      const std::string& response);
-
   FakeServer* const fake_server_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
-
   std::string response_;
   std::string request_url_;
   int request_port_;
   std::string request_content_;
   std::string request_content_type_;
   std::string extra_request_headers_;
-  int post_error_code_;
-  int post_response_code_;
-  base::WaitableEvent post_complete_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeServerHttpPostProvider);
 };
@@ -66,9 +53,7 @@ class FakeServerHttpPostProvider
 class FakeServerHttpPostProviderFactory
     : public syncer::HttpPostProviderFactory {
  public:
-  explicit FakeServerHttpPostProviderFactory(
-      FakeServer* fake_server,
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
+  explicit FakeServerHttpPostProviderFactory(FakeServer* fake_server);
   virtual ~FakeServerHttpPostProviderFactory();
 
   // HttpPostProviderFactory:
@@ -78,7 +63,6 @@ class FakeServerHttpPostProviderFactory
 
  private:
   FakeServer* const fake_server_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeServerHttpPostProviderFactory);
 };
diff --git a/sync/test/fake_server/fake_server_network_resources.cc b/sync/test/fake_server/fake_server_network_resources.cc
index c3bedb0..56d5ea2 100644
--- a/sync/test/fake_server/fake_server_network_resources.cc
+++ b/sync/test/fake_server/fake_server_network_resources.cc
@@ -5,7 +5,6 @@
 #include "sync/test/fake_server/fake_server_network_resources.h"
 
 #include "base/memory/scoped_ptr.h"
-#include "base/message_loop/message_loop.h"
 #include "sync/internal_api/public/base/cancelation_signal.h"
 #include "sync/internal_api/public/http_post_provider_factory.h"
 #include "sync/internal_api/public/network_time_update_callback.h"
@@ -29,9 +28,7 @@ scoped_ptr<syncer::HttpPostProviderFactory>
         const NetworkTimeUpdateCallback& network_time_update_callback,
         CancelationSignal* cancelation_signal) {
   return make_scoped_ptr<syncer::HttpPostProviderFactory>(
-      new FakeServerHttpPostProviderFactory(
-          fake_server_,
-          base::MessageLoop::current()->message_loop_proxy()));
+      new FakeServerHttpPostProviderFactory(fake_server_));
 }
 
 }  // namespace fake_server