[Sync] Rework unit tests for ChromeInvalidationClient

sync/notifier/chrome_invalidation_client.h

Index: sync/notifier/chrome_invalidation_client.h
diff --git a/sync/notifier/chrome_invalidation_client.h b/sync/notifier/chrome_invalidation_client.h
index 2328f99af7705b2b555741df34e6c9b230ea5af8..e05808cbd11ddebcce3393c7c21ac2a01b20dcf6 100644
--- a/sync/notifier/chrome_invalidation_client.h
+++ b/sync/notifier/chrome_invalidation_client.h
@@ -11,6 +11,7 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -44,6 +45,13 @@ class ChromeInvalidationClient
       public notifier::PushClientObserver,
       public base::NonThreadSafe {
  public:
+  typedef base::Callback<invalidation::InvalidationClient*(
+      invalidation::SystemResources*,
+      int,
+      const invalidation::string&,
+      const invalidation::string&,
+      invalidation::InvalidationListener*)> CreateInvalidationClientCallback;

[rlarocque, 2012/08/07 18:21:48]

I'm not sure that adding a function parameter is the best way to do this.  Why
not extend ChromeInvalidationClient to override the Start() (or a related helper
function), rather than exposing this to the caller?

[akalin, 2012/08/08 22:46:35]

I think we want to move away from the mark-function-as-virtual-and-override
pattern, as it's less clear (IMO) than dep-injecting some sort of delegate.  In
this case, the only delegate method needed is a state-less constructor function,
so it can be replaced with a base::Callback.

I think it's the second-best way; the best way would be to dep-inject
InvalidationListener* directly, but that's difficult since you'd have to keep
around its SystemResources* object.

[rlarocque, 2012/08/08 23:22:56]

I agree that dependency injection is generally better.  However, in this case we
end up with an invalidation::InvalidationListener* parameter which is used to
pass a pointer to the listener back to tests.  It would be nice to avoid that.

It sounds like you've put some thought into this.  I won't hold up the review
over this point.  

+
   class Listener {
    public:
     virtual ~Listener();
@@ -65,6 +73,8 @@ class ChromeInvalidationClient
   // Does not take ownership of |listener| or |state_writer|.
   // |invalidation_state_tracker| must be initialized.
   void Start(
+      const CreateInvalidationClientCallback&
+          create_invalidation_client_callback,
       const std::string& client_id, const std::string& client_info,
       const std::string& state,
       const InvalidationVersionMap& initial_max_invalidation_versions,
@@ -73,9 +83,9 @@ class ChromeInvalidationClient
 
   void UpdateCredentials(const std::string& email, const std::string& token);
 
-  // Register the object IDs that we're interested in getting
+  // Update the set of object IDs that we're interested in getting
   // notifications for.  May be called at any time.
-  void RegisterIds(const ObjectIdSet& ids);
+  void UpdateRegisteredIds(const ObjectIdSet& ids);
 
   // invalidation::InvalidationListener implementation.
   virtual void Ready(
@@ -118,9 +128,9 @@ class ChromeInvalidationClient
   virtual void OnIncomingNotification(
       const notifier::Notification& notification) OVERRIDE;
 
- private:
-  friend class ChromeInvalidationClientTest;
+  void StopForTest();
 
+ private:
   void Stop();
 
   NotificationsDisabledReason GetState() const;