Use FakeServer-based invalidations for Sync tests

sync/test/fake_server/fake_server.h

Index: sync/test/fake_server/fake_server.h
diff --git a/sync/test/fake_server/fake_server.h b/sync/test/fake_server/fake_server.h
index a13cc980fc1e39b446c38f204adead715fa797c1..92d477d928e48bee6f1f55aedbeec44d46e68f97 100644
--- a/sync/test/fake_server/fake_server.h
+++ b/sync/test/fake_server/fake_server.h
@@ -7,28 +7,50 @@
 
 #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;
@@ -59,6 +81,7 @@ class FakeServer {
   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
@@ -75,14 +98,6 @@ 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_;
@@ -101,6 +116,9 @@ 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> observers_;
 };
 
 }  // namespace fake_server