Various small invalidations cleanups

Making a few things private that should be private, renaming "ids" to
"topics", updating comments etc. No functional changes.

Bug: 1029698
Change-Id: If95081e1b68277bc0beb1e17b23dde0b4791a010
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1948438
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721504}

components/invalidation/impl/fcm_invalidation_listener.cc

@@ -21,7 +21,7 @@ FCMInvalidationListener::Delegate::~Delegate() {}
 
 FCMInvalidationListener::FCMInvalidationListener(
     std::unique_ptr<FCMSyncNetworkChannel> network_channel)
-    : network_channel_(std::move(network_channel)), delegate_(nullptr) {
+    : network_channel_(std::move(network_channel)) {
   network_channel_->AddObserver(this);
 }
 
@@ -55,7 +55,7 @@ void FCMInvalidationListener::Start(
 }
 
 void FCMInvalidationListener::UpdateRegisteredTopics(const Topics& topics) {
-  ids_update_requested_ = true;
+  topics_update_requested_ = true;
   registered_topics_ = topics;
   DoRegistrationUpdate();
 }
@@ -85,7 +85,8 @@ void FCMInvalidationListener::Invalidate(const std::string& payload,
   TopicInvalidationMap invalidations;
   Invalidation inv =
       Invalidation::Init(ConvertTopicToId(*expected_public_topic), v, payload);
-  inv.SetAckHandler(AsWeakPtr(), base::ThreadTaskRunnerHandle::Get());
+  inv.SetAckHandler(weak_factory_.GetWeakPtr(),
+                    base::ThreadTaskRunnerHandle::Get());
   DVLOG(1) << "Received invalidation with version " << inv.version() << " for "
            << *expected_public_topic;
 
@@ -106,14 +107,14 @@ void FCMInvalidationListener::DispatchInvalidations(
 void FCMInvalidationListener::SaveInvalidations(
     const TopicInvalidationMap& to_save) {
   ObjectIdSet objects_to_save = ConvertTopicsToIds(to_save.GetTopics());
-  for (auto it = objects_to_save.begin(); it != objects_to_save.end(); ++it) {
-    auto lookup = unacked_invalidations_map_.find(*it);
+  for (const invalidation::ObjectId& id : objects_to_save) {
+    auto lookup = unacked_invalidations_map_.find(id);
     if (lookup == unacked_invalidations_map_.end()) {
-      lookup = unacked_invalidations_map_
-                   .insert(std::make_pair(*it, UnackedInvalidationSet(*it)))
-                   .first;
+      lookup =
+          unacked_invalidations_map_.emplace(id, UnackedInvalidationSet(id))
+              .first;
     }
-    lookup->second.AddSet(to_save.ForTopic((*it).name()));
+    lookup->second.AddSet(to_save.ForTopic(id.name()));
   }
 }
 
@@ -123,7 +124,7 @@ void FCMInvalidationListener::EmitSavedInvalidations(
 }
 
 void FCMInvalidationListener::InformTokenReceived(const std::string& token) {
-  token_ = token;
+  instance_id_token_ = token;
   DoRegistrationUpdate();
 }
 
@@ -148,12 +149,12 @@ void FCMInvalidationListener::Drop(const invalidation::ObjectId& id,
 }
 
 void FCMInvalidationListener::DoRegistrationUpdate() {
-  if (!per_user_topic_registration_manager_ || token_.empty() ||
-      !ids_update_requested_) {
+  if (!per_user_topic_registration_manager_ || instance_id_token_.empty() ||
+      !topics_update_requested_) {
     return;
   }
   per_user_topic_registration_manager_->UpdateRegisteredTopics(
-      registered_topics_, token_);
+      registered_topics_, instance_id_token_);
 
   // TODO(melandory): remove unacked invalidations for unregistered objects.
   ObjectIdInvalidationMap object_id_invalidation_map;
@@ -163,7 +164,7 @@ void FCMInvalidationListener::DoRegistrationUpdate() {
       continue;
     }
 
-    unacked.second.ExportInvalidations(AsWeakPtr(),
+    unacked.second.ExportInvalidations(weak_factory_.GetWeakPtr(),
                                        base::ThreadTaskRunnerHandle::Get(),
                                        &object_id_invalidation_map);
   }
@@ -199,14 +200,10 @@ void FCMInvalidationListener::EmitSavedInvalidationsForTest(
   EmitSavedInvalidations(to_emit);
 }
 
-Topics FCMInvalidationListener::GetRegisteredIdsForTest() const {
+Topics FCMInvalidationListener::GetRegisteredTopicsForTest() const {
   return registered_topics_;
 }
 
-base::WeakPtr<FCMInvalidationListener> FCMInvalidationListener::AsWeakPtr() {
-  return weak_factory_.GetWeakPtr();
-}
-
 void FCMInvalidationListener::Stop() {
   delegate_ = nullptr;
 

components/invalidation/impl/fcm_invalidation_listener.h

@@ -51,7 +51,7 @@ class FCMInvalidationListener : public InvalidationListener,
       std::unique_ptr<PerUserTopicRegistrationManager>
           per_user_topic_registration_manager);
 
-  // Update the set of object IDs that we're interested in getting
+  // Update the set of topics that we're interested in getting
   // notifications for. May be called at any time.
   void UpdateRegisteredTopics(const Topics& topics);
 
@@ -75,8 +75,6 @@ class FCMInvalidationListener : public InvalidationListener,
   void OnSubscriptionChannelStateChanged(
       SubscriptionChannelState state) override;
 
-  void DoRegistrationUpdate();
-
   virtual void RequestDetailedStatus(
       const base::RepeatingCallback<void(const base::DictionaryValue&)>&
           callback) const;
@@ -86,11 +84,11 @@ class FCMInvalidationListener : public InvalidationListener,
   void EmitStateChangeForTest(InvalidatorState state);
   void EmitSavedInvalidationsForTest(const TopicInvalidationMap& to_emit);
 
-  Topics GetRegisteredIdsForTest() const;
-
-  base::WeakPtr<FCMInvalidationListener> AsWeakPtr();
+  Topics GetRegisteredTopicsForTest() const;
 
  private:
+  void DoRegistrationUpdate();
+
   void Stop();
 
   InvalidatorState GetState() const;
@@ -120,7 +118,7 @@ class FCMInvalidationListener : public InvalidationListener,
 
   std::unique_ptr<FCMSyncNetworkChannel> network_channel_;
   UnackedInvalidationsMap unacked_invalidations_map_;
-  Delegate* delegate_;
+  Delegate* delegate_ = nullptr;
 
   // Stores all topics which are registered (or subscribed?).
   // TODO(crbug.com/1029698,crbug.com/1020117): We should get this information
@@ -137,12 +135,12 @@ class FCMInvalidationListener : public InvalidationListener,
 
   std::unique_ptr<PerUserTopicRegistrationManager>
       per_user_topic_registration_manager_;
-  std::string token_;
+  std::string instance_id_token_;
   // Prevents call to DoRegistrationUpdate in cases when
   // UpdateRegisteredTopics wasn't called. For example, InformTokenReceived
   // can trigger DoRegistrationUpdate before any invalidation handler has
   // requested registration for topics.
-  bool ids_update_requested_ = false;
+  bool topics_update_requested_ = false;
 
   base::WeakPtrFactory<FCMInvalidationListener> weak_factory_{this};
 

components/invalidation/impl/fcm_invalidation_listener_unittest.cc

@@ -41,6 +41,12 @@ class TestFCMSyncNetworkChannel : public FCMSyncNetworkChannel {
  public:
   void StartListening() override {}
   void StopListening() override {}
+
+  void RequestDetailedStatus(
+      const base::RepeatingCallback<void(const base::DictionaryValue&)>&
+          callback) override {}
+
+  using FCMSyncNetworkChannel::NotifyChannelStateChange;
 };
 
 // Fake delegate that keeps track of invalidation counts, payloads,
@@ -249,7 +255,7 @@ class FCMInvalidationListenerTest : public testing::Test {
   }
 
   Topics GetRegisteredTopics() const {
-    return listener_.GetRegisteredIdsForTest();
+    return listener_.GetRegisteredTopicsForTest();
   }
 
   void RegisterAndFireInvalidate(const Topic& topic,
@@ -284,7 +290,7 @@ class FCMInvalidationListenerTest : public testing::Test {
  private:
   base::test::SingleThreadTaskEnvironment task_environment_;
   data_decoder::test::InProcessDataDecoder in_process_data_decoder_;
-  FCMSyncNetworkChannel* fcm_sync_network_channel_;
+  TestFCMSyncNetworkChannel* fcm_sync_network_channel_;
   MockRegistrationManager* registration_manager_;
 
  protected:

components/invalidation/impl/fcm_invalidation_service_unittest.cc

@@ -49,6 +49,10 @@ class TestFCMSyncNetworkChannel : public syncer::FCMSyncNetworkChannel {
  public:
   void StartListening() override {}
   void StopListening() override {}
+
+  void RequestDetailedStatus(
+      const base::RepeatingCallback<void(const base::DictionaryValue&)>&
+          callback) override {}
 };
 
 // TODO: Make FCMInvalidationListener class abstract and explicitly make all the

components/invalidation/impl/fcm_network_handler.cc

@@ -275,13 +275,15 @@ void FCMNetworkHandler::SetTokenValidationTimerForTesting(
 }
 
 void FCMNetworkHandler::RequestDetailedStatus(
-    base::Callback<void(const base::DictionaryValue&)> callback) {
+    const base::RepeatingCallback<void(const base::DictionaryValue&)>&
+        callback) {
   callback.Run(diagnostic_info_.CollectDebugData());
 }
 
-FCMNetworkHandlerDiagnostic::FCMNetworkHandlerDiagnostic() {}
+FCMNetworkHandler::FCMNetworkHandlerDiagnostic::FCMNetworkHandlerDiagnostic() {}
 
-base::DictionaryValue FCMNetworkHandlerDiagnostic::CollectDebugData() const {
+base::DictionaryValue
+FCMNetworkHandler::FCMNetworkHandlerDiagnostic::CollectDebugData() const {
   base::DictionaryValue status;
   status.SetString("NetworkHandler.Registration-result-code",
                    RegistrationResultToString(registration_result));
@@ -307,7 +309,8 @@ base::DictionaryValue FCMNetworkHandlerDiagnostic::CollectDebugData() const {
   return status;
 }
 
-std::string FCMNetworkHandlerDiagnostic::RegistrationResultToString(
+std::string
+FCMNetworkHandler::FCMNetworkHandlerDiagnostic::RegistrationResultToString(
     const instance_id::InstanceID::Result result) const {
   switch (registration_result) {
     case instance_id::InstanceID::SUCCESS:

components/invalidation/impl/fcm_network_handler.h

@@ -25,29 +25,6 @@ class InstanceIDDriver;
 
 namespace syncer {
 
-struct FCMNetworkHandlerDiagnostic {
-  FCMNetworkHandlerDiagnostic();
-
-  // Collect all the internal variables in a single readable dictionary.
-  base::DictionaryValue CollectDebugData() const;
-
-  std::string RegistrationResultToString(
-      const instance_id::InstanceID::Result result) const;
-
-  std::string token;
-  instance_id::InstanceID::Result registration_result =
-      instance_id::InstanceID::UNKNOWN_ERROR;
-  instance_id::InstanceID::Result token_verification_result =
-      instance_id::InstanceID::UNKNOWN_ERROR;
-  bool token_changed = false;
-  base::Time instance_id_token_requested;
-  base::Time instance_id_token_was_received;
-  base::Time instance_id_token_verification_requested;
-  base::Time instance_id_token_verified;
-
-  int token_validation_requested_num = 0;
-};
-
 /*
  * The class responsible for communication via GCM channel:
  *  - It retrieves the token required for the subscription
@@ -95,10 +72,33 @@ class FCMNetworkHandler : public gcm::GCMAppHandler,
       std::unique_ptr<base::OneShotTimer> token_validation_timer);
 
   void RequestDetailedStatus(
-      base::RepeatingCallback<void(const base::DictionaryValue&)> callback)
-      override;
+      const base::RepeatingCallback<void(const base::DictionaryValue&)>&
+          callback) override;
 
  private:
+  struct FCMNetworkHandlerDiagnostic {
+    FCMNetworkHandlerDiagnostic();
+
+    // Collect all the internal variables in a single readable dictionary.
+    base::DictionaryValue CollectDebugData() const;
+
+    std::string RegistrationResultToString(
+        const instance_id::InstanceID::Result result) const;
+
+    std::string token;
+    instance_id::InstanceID::Result registration_result =
+        instance_id::InstanceID::UNKNOWN_ERROR;
+    instance_id::InstanceID::Result token_verification_result =
+        instance_id::InstanceID::UNKNOWN_ERROR;
+    bool token_changed = false;
+    base::Time instance_id_token_requested;
+    base::Time instance_id_token_was_received;
+    base::Time instance_id_token_verification_requested;
+    base::Time instance_id_token_verified;
+
+    int token_validation_requested_num = 0;
+  };
+
   // Called when a subscription token is obtained from the GCM server.
   void DidRetrieveToken(const std::string& subscription_token,
                         instance_id::InstanceID::Result result);

components/invalidation/impl/fcm_sync_network_channel.cc

@@ -58,11 +58,4 @@ bool FCMSyncNetworkChannel::DeliverToken(const std::string& token) {
   return true;
 }
 
-int FCMSyncNetworkChannel::GetReceivedMessagesCount() const {
-  return received_messages_count_;
-}
-
-void FCMSyncNetworkChannel::RequestDetailedStatus(
-    base::Callback<void(const base::DictionaryValue&)> callback) {}
-
 }  // namespace syncer

components/invalidation/impl/fcm_sync_network_channel.h

 // Copyright 2018 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.
-//
-// Simple system resources class that uses the current thread for scheduling.
-// Assumes the current thread is already running tasks.
 
 #ifndef COMPONENTS_INVALIDATION_IMPL_FCM_SYNC_NETWORK_CHANNEL_H_
 #define COMPONENTS_INVALIDATION_IMPL_FCM_SYNC_NETWORK_CHANNEL_H_
@@ -42,9 +39,13 @@ class FCMSyncNetworkChannel : public NetworkChannel {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  // Get the count of how many valid received messages were received.
-  int GetReceivedMessagesCount() const;
+  // Subclass should implement RequestDetailedStatus to provide debugging
+  // information.
+  virtual void RequestDetailedStatus(
+      const base::RepeatingCallback<void(const base::DictionaryValue&)>&
+          callback) = 0;
 
+ protected:
   // Subclass should call NotifyNetworkStatusChange to notify about network
   // changes. This triggers cacheinvalidation to try resending failed message
   // ahead of schedule when client comes online or IP address changes.
@@ -67,11 +68,6 @@ class FCMSyncNetworkChannel : public NetworkChannel {
   // manager.
   bool DeliverToken(const std::string& token);
 
-  // Subclass should implement RequestDetailedStatus to provide debugging
-  // information.
-  virtual void RequestDetailedStatus(
-      base::Callback<void(const base::DictionaryValue&)> callback);
-
  private:
   // Callbacks into invalidation library
   MessageCallback incoming_receiver_;

components/invalidation/impl/unacked_invalidation_set.h

@@ -41,7 +41,7 @@ class INVALIDATION_EXPORT UnackedInvalidationSet {
  public:
   static const size_t kMaxBufferedInvalidations;
 
-  UnackedInvalidationSet(invalidation::ObjectId id);
+  explicit UnackedInvalidationSet(invalidation::ObjectId id);
   UnackedInvalidationSet(const UnackedInvalidationSet& other);
   ~UnackedInvalidationSet();