Use FakeServer-based invalidations for Sync tests

sync/test/fake_server/fake_server.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SYNC_TEST_FAKE_SERVER_FAKE_SERVER_H_
 #define SYNC_TEST_FAKE_SERVER_FAKE_SERVER_H_
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/synchronization/lock.h"
+#include "base/observer_list.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/test/fake_server/fake_server_entity.h"
 
+namespace base {
+class WaitableEvent;
+}
+
 namespace fake_server {
 
 // A fake version of the Sync server used for testing.
 class FakeServer {
  public:
+  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();
 
   // 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);
+  void HandleCommand(const std::string& request,
+                     const base::Closure& callback,

[rlarocque, 2014/04/22 18:22:22]

Rather than asking the caller to provide space for the returned values, I think
it would be more idiomatic to have the callback take arguments.  Could you make
it a base::Callback<void(int, int, const std::string&)>?

[pval...(no longer on Chromium), 2014/04/24 01:08:53]

Done.

+                     int* error_code,
+                     int* response_code,
+                     std::string* response);
+
+  // Adds |observer| to FakeServer's observer list. This should be called
+  // before the Profile associated with |observer| is connected to the server.
+  // |observer| will remain an Observer for the rest of FakeServer's lifetime.
+  void AddObserver(Observer* observer);

[rlarocque, 2014/04/22 18:22:22]

This is a bit confusing.  Normally the observed object outlives the observer,
and has a RemoveObserver() method that must be called before its observers are
deleted.  

As it is, I think this will only work if the FakeServer is destroyed before its
observers.

I'd prefer the explicit unregister pattern.  It's a bit easier to DCHECK() when
it's used improperly.  See also the ScopedObserver class if you'd like some help
making sure the unregister method is called.

[pval...(no longer on Chromium), 2014/04/24 01:08:53]

I've added a RemoveAllObservers method to FakeServer and modified
FakeServer.observers_'s definition to check that it's empty on destruction.

Do you think this is sufficient? There's no point right now where we would
remove observers separately so I think one method makes sense.

[rlarocque, 2014/04/25 17:41:46]

I'd prefer to remove them each individually.  Having a "remove all" might hide
bugs, and is likely to cause problems later on.

Come to think of it, don't you need to unregister an individual observer in
those tests where we disable one client?  I know most of them just disable one
type at a time, but I thought there might be one or two that fully shut down the
sync backend.

[pval...(no longer on Chromium), 2014/04/29 00:06:07]

I've added TODOs in SyncTest.{Enable,Disable}Network() as we will need to adjust
the observer lifetime in that case.

I believe the full disable case is handled in SyncBackendHost, so nothing needs
to be done on my end. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

Are there any other cases you think we should be concerned about?

[rlarocque, 2014/04/29 00:13:02]

That helps with some concerns, but I still worry about hiding bugs. 
RemoveAllObservers() is too easy to abuse.

 
  private:
   typedef std::map<std::string, FakeServerEntity*> EntityMap;
 
   // Processes a GetUpdates call.
   bool HandleGetUpdatesRequest(const sync_pb::GetUpdatesMessage& get_updates,
                                sync_pb::GetUpdatesResponse* response);
 
   // Processes a Commit call.
   bool HandleCommitRequest(const sync_pb::CommitMessage& commit,
@@ skipping 10 matching lines @@
   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| 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,
+                    syncer::ModelType* model_type,
                     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|.
   void BuildEntryResponseForSuccessfulCommit(
       sync_pb::CommitResponse_EntryResponse* entry_response,
       FakeServerEntity* entity);
 
   // Determines whether the SyncEntity with id_string |id| is a child of an
   // entity with id_string |potential_parent_id|.
   bool IsChild(const std::string& id, const std::string& potential_parent_id);
 
   // Creates and saves tombstones for all children of the entity with the given
   // |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_;
 
   // The current birthday value.
   std::string birthday_;
 
   // All SyncEntity objects saved by the server. The key value is the entity's
   // id string.
   EntityMap entities_;
 
   // All Keystore keys known to the server.
   std::vector<std::string> keystore_keys_;
 
   // All ModelTypes for which permanent entities have been created. These types
   // 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> observers_;
 };
 
 }  // namespace fake_server
 
 #endif  // SYNC_TEST_FAKE_SERVER_FAKE_SERVER_H_