[Sync] Rework unit tests for ChromeInvalidationClient

sync/notifier/chrome_invalidation_client.h

 // Copyright (c) 2012 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.
 //
 // A simple wrapper around invalidation::InvalidationClient that
 // handles all the startup/shutdown details and hookups.
 
 #ifndef SYNC_NOTIFIER_CHROME_INVALIDATION_CLIENT_H_
 #define SYNC_NOTIFIER_CHROME_INVALIDATION_CLIENT_H_
 
 #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"
 #include "base/threading/non_thread_safe.h"
 #include "google/cacheinvalidation/include/invalidation-listener.h"
 #include "jingle/notifier/listener/push_client_observer.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/notifier/chrome_system_resources.h"
 #include "sync/notifier/invalidation_state_tracker.h"
 #include "sync/notifier/notifications_disabled_reason.h"
@@ skipping 13 matching lines @@
 class RegistrationManager;
 
 // ChromeInvalidationClient is not thread-safe and lives on the sync
 // thread.
 class ChromeInvalidationClient
     : public invalidation::InvalidationListener,
       public StateWriter,
       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();
 
     virtual void OnInvalidate(const ObjectIdPayloadMap& id_payloads) = 0;
 
     virtual void OnNotificationsEnabled() = 0;
 
     virtual void OnNotificationsDisabled(
         NotificationsDisabledReason reason) = 0;
   };
 
   explicit ChromeInvalidationClient(
       scoped_ptr<notifier::PushClient> push_client);
 
   // Calls Stop().
   virtual ~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,
       const WeakHandle<InvalidationStateTracker>& invalidation_state_tracker,
       Listener* listener);
 
   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(
       invalidation::InvalidationClient* client) OVERRIDE;
   virtual void Invalidate(
       invalidation::InvalidationClient* client,
       const invalidation::Invalidation& invalidation,
       const invalidation::AckHandle& ack_handle) OVERRIDE;
   virtual void InvalidateUnknownVersion(
       invalidation::InvalidationClient* client,
@@ skipping 22 matching lines @@
   // StateWriter implementation.
   virtual void WriteState(const std::string& state) OVERRIDE;
 
   // notifier::PushClientObserver implementation.
   virtual void OnNotificationsEnabled() OVERRIDE;
   virtual void OnNotificationsDisabled(
       notifier::NotificationsDisabledReason reason) OVERRIDE;
   virtual void OnIncomingNotification(
       const notifier::Notification& notification) OVERRIDE;
 
+  void StopForTest();
+
  private:
-  friend class ChromeInvalidationClientTest;
-
   void Stop();
 
   NotificationsDisabledReason GetState() const;
 
   void EmitStateChange();
 
   void EmitInvalidation(const ObjectIdPayloadMap& id_payloads);
 
   // Owned by |chrome_system_resources_|.
   notifier::PushClient* const push_client_;
@@ skipping 10 matching lines @@
   // NO_NOTIFICATION_ERROR meaning notifications are enabled).
   NotificationsDisabledReason ticl_state_;
   NotificationsDisabledReason push_client_state_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeInvalidationClient);
 };
 
 }  // namespace syncer
 
 #endif  // SYNC_NOTIFIER_CHROME_INVALIDATION_CLIENT_H_