Update TODOs in components/invalidation

- Some obsolete TODOs are removed.
- Valid TODOs are linked to a bug instead of a username.
- Some valid TODOs are also made more specific etc.

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

components/invalidation/impl/fcm_invalidation_listener.cc

@@ -64,14 +64,11 @@ void FCMInvalidationListener::Invalidate(const std::string& payload,
                                          const std::string& private_topic,
                                          const std::string& public_topic,
                                          const std::string& version) {
-  // TODO(melandory): use |private_topic| in addition to
-  // |registered_topics_| to verify that topic is registered.
   int64_t v;
   if (!base::StringToInt64(version, &v)) {
-    // Version must always be in the message and
-    // in addition version must be number.
-    // TODO(melandory): Report error and consider not to process with the
-    // invalidation.
+    // The version must always be in the message, and be a number.
+    // TODO(crbug.com/1023813): Record UMA, and either use InitUnknownVersion()
+    // instead of Init() below, or don't process the invalidation at all.
   }
   // Note: |public_topic| is empty for some invalidations (e.g. Drive). Prefer
   // using |*expected_public_topic| over |public_topic|.

components/invalidation/impl/fcm_invalidation_listener.h

@@ -122,7 +122,12 @@ class FCMInvalidationListener : public InvalidationListener,
   UnackedInvalidationsMap unacked_invalidations_map_;
   Delegate* delegate_;
 
-  // Stored to pass to |per_user_topic_registration_manager_| on start.
+  // Stores all topics which are registered (or subscribed?).
+  // TODO(crbug.com/1029698,crbug.com/1020117): We should get this information
+  // from |per_user_topic_registration_manager_| instead of keeping another copy
+  // here. Also, figure out whether this refers to registered topics (i.e. have
+  // a handler in Chrome) or subscribed topics (i.e. we're subscribed on the
+  // server).
   Topics registered_topics_;
 
   // The states of the HTTP and FCM channel.

components/invalidation/impl/fcm_invalidation_service_base.cc

@@ -219,7 +219,6 @@ void FCMInvalidationServiceBase::StartInvalidator() {
 void FCMInvalidationServiceBase::StopInvalidator() {
   DCHECK(invalidation_listener_);
   diagnostic_info_.service_was_stopped = base::Time::Now();
-  // TODO(melandory): reset the network.
   invalidation_listener_.reset();
 }
 

components/invalidation/impl/fcm_network_handler.cc

@@ -249,9 +249,9 @@ void FCMNetworkHandler::OnMessage(const std::string& app_id,
 }
 
 void FCMNetworkHandler::OnMessagesDeleted(const std::string& app_id) {
-  // TODO(melandory): consider notifyint the client that messages were
-  // deleted. So the client can act on it, e.g. in case of sync request
-  // GetUpdates from the server.
+  // TODO(crbug.com/1023813): Record UMA. Then, if this actually happens in
+  // practice, consider notifying the client that messages were deleted, so it
+  // can act on it, e.g. in case of sync, trigger a GetUpdates.
 }
 
 void FCMNetworkHandler::OnSendError(

components/invalidation/impl/invalidator_registrar_with_memory.cc

@@ -138,14 +138,14 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
     }
   }
 
-  // TODO(treib): This seems inconsistent - if there's a duplicate, we don't
-  // update |registered_handler_to_topics_map_| but we *do* still update
-  // |handler_name_to_subscribed_topics_map_| and the prefs?!
+  // TODO(crbug.com/1020117): This seems inconsistent - if there's a duplicate,
+  // we don't update |registered_handler_to_topics_map_| but we *do* still
+  // update |handler_name_to_subscribed_topics_map_| and the prefs?!
 
   DictionaryPrefUpdate update(prefs_, kTopicsToHandler);
   base::Value* pref_data = update->FindDictKey(sender_id_);
-  // TODO(treib): This does *not* remove subscribed topics which were not
-  // registered. Bug or feature?
+  // TODO(crbug.com/1020117): This does currently *not* remove subscribed topics
+  // which are not registered, but it almost certainly should.
   auto to_unregister = FindRemovedTopics(old_topics, topics);
   for (const auto& topic : to_unregister) {
     pref_data->RemoveKey(topic);

components/invalidation/impl/per_user_topic_registration_manager.cc

@@ -390,7 +390,7 @@ void PerUserTopicRegistrationManager::ScheduleRequestForRepetition(
   registration_statuses_[topic]->completion_callback = base::BindOnce(
       &PerUserTopicRegistrationManager::RegistrationFinishedForTopic,
       base::Unretained(this));
-  // TODO(treib): We already called InformOfRequest(false) before in
+  // TODO(crbug.com/1020117): We already called InformOfRequest(false) before in
   // RegistrationFinishedForTopic(), should probably not call it again here?
   registration_statuses_[topic]->request_backoff_.InformOfRequest(false);
   registration_statuses_[topic]->request_retry_timer_.Start(
@@ -448,7 +448,7 @@ void PerUserTopicRegistrationManager::RemoveObserver(Observer* observer) {
 }
 
 void PerUserTopicRegistrationManager::RequestAccessToken() {
-  // TODO(melandory): Implement traffic optimisation.
+  // TODO(crbug.com/1020117): Implement traffic optimisation.
   // * Before sending request to server ask for access token from identity
   //   provider (don't invalidate previous token).
   //   Identity provider will take care of retrieving/caching.
@@ -524,7 +524,7 @@ PerUserTopicRegistrationManager::DropAllSavedRegistrationsOnTokenChange() {
   topic_to_private_topic_.clear();
   private_topic_to_topic_.clear();
   return TokenStateOnRegistrationRequest::kTokenChanged;
-  // TODO(melandory): Figure out if the unsubscribe request should be
+  // TODO(crbug.com/1020117): Figure out if the unsubscribe request should be
   // sent with the old token.
 }
 

components/invalidation/impl/per_user_topic_registration_manager.h

@@ -38,8 +38,8 @@ namespace syncer {
 // Manages the details of registering types for invalidation. For example,
 // Chrome Sync uses the ModelTypes (bookmarks, passwords, autofill data) as
 // topics, which will be registered for the invalidations.
-// TODO(melandory): Methods in this class have names which are similar to names
-// in RegistrationManager. As part of clean-up work for removing old
+// TODO(crbug.com/1029698): Methods in this class have names which are similar
+// to names in RegistrationManager. As part of clean-up work for removing old
 // RegistrationManger and cachinvalidation library it's worth to revisit methods
 // names in this class.
 class INVALIDATION_EXPORT PerUserTopicRegistrationManager {

components/invalidation/impl/per_user_topic_registration_request.cc

@@ -360,8 +360,8 @@ PerUserTopicRegistrationRequest::Builder::BuildURLFetcher(
   }
   request->url = url;
   request->headers = headers;
-  // TODO(treib): Should we set request->credentials_mode to kOmit, to match
-  // "cookies_allowed: NO" above?
+  // TODO(crbug.com/1020117): Should we set request->credentials_mode to kOmit,
+  // to match "cookies_allowed: NO" above?
 
   std::unique_ptr<network::SimpleURLLoader> url_loader =
       network::SimpleURLLoader::Create(std::move(request), traffic_annotation);