[Tango->FCM] Remove topic from saved prefs before the request has finished.

* As of today, when subscription to the topic gets deleted,
the value in prefs is cleaned up only after the response for HTTP
unsubscription request arrives.
* It can happen, e.g. on the browser shutdown that the clean-up
code is never executed, hence prefs won't be cleaned up.
* This CL makes the prefs cleanup happen when the decision to unsubscribe
from the topic notifications is made.

BUG=913486

Change-Id: I3d58df8896ef87ecef4535fb588d9a73494c2de1
Reviewed-on: https://chromium-review.googlesource.com/c/1370164
Commit-Queue: Tatiana Gornak <melandory@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615852}

components/invalidation/impl/per_user_topic_registration_manager.cc

@@ -202,6 +202,10 @@ void PerUserTopicRegistrationManager::UpdateRegisteredTopics(
               base::Unretained(this)),
           PerUserTopicRegistrationRequest::UNSUBSCRIBE);
       it = topic_to_private_topic_.erase(it);
+      // The descision to unregister from the invalidations for the |topic| was
+      // made, the preferences should be cleaned up immediatelly.
+      DictionaryPrefUpdate update(local_state_, kTypeRegisteredForInvalidation);
+      update->RemoveKey(topic);
     } else {
       ++it;
     }
@@ -248,19 +252,12 @@ void PerUserTopicRegistrationManager::RegistrationFinishedForTopic(
   if (code.IsSuccess()) {
     auto it = registration_statuses_.find(topic);
     registration_statuses_.erase(it);
-    DictionaryPrefUpdate update(local_state_, kTypeRegisteredForInvalidation);
-    switch (type) {
-      case PerUserTopicRegistrationRequest::SUBSCRIBE: {
-        update->SetKey(topic, base::Value(private_topic_name));
-        topic_to_private_topic_[topic] = private_topic_name;
-        break;
-      }
-      case PerUserTopicRegistrationRequest::UNSUBSCRIBE: {
-        update->RemoveKey(topic);
-        break;
-      }
+    if (type == PerUserTopicRegistrationRequest::SUBSCRIBE) {
+      DictionaryPrefUpdate update(local_state_, kTypeRegisteredForInvalidation);
+      update->SetKey(topic, base::Value(private_topic_name));
+      topic_to_private_topic_[topic] = private_topic_name;
+      local_state_->CommitPendingWrite();
     }
-    local_state_->CommitPendingWrite();
   } else {
     if (code.IsAuthFailure()) {
       // Re-request access token and fire registrations again.

components/invalidation/impl/per_user_topic_registration_manager_unittest.cc

@@ -296,4 +296,45 @@ TEST_F(PerUserTopicRegistrationManagerTest,
   }
 }
 
+TEST_F(PerUserTopicRegistrationManagerTest,
+       ShouldDeletTopicsFromPrefsWhenRequestFails) {
+  TopicSet ids = GetSequenceOfTopics(kInvalidationObjectIdsCount);
+
+  AddCorrectSubscriptionResponce();
+
+  auto per_user_topic_registration_manager = BuildRegistrationManager();
+  EXPECT_TRUE(per_user_topic_registration_manager->GetRegisteredIds().empty());
+
+  per_user_topic_registration_manager->UpdateRegisteredTopics(
+      ids, kFakeInstanceIdToken);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(ids, per_user_topic_registration_manager->GetRegisteredIds());
+
+  // Disable some ids.
+  TopicSet disabled_ids = GetSequenceOfTopics(3);
+  TopicSet enabled_ids =
+      GetSequenceOfTopicsStartingAt(3, kInvalidationObjectIdsCount - 3);
+  // Without configuring the responce, the request will not happen.
+  per_user_topic_registration_manager->UpdateRegisteredTopics(
+      enabled_ids, kFakeInstanceIdToken);
+  base::RunLoop().RunUntilIdle();
+
+  // Ids should still be removed from prefs.
+  for (const auto& id : disabled_ids) {
+    const base::DictionaryValue* topics =
+        pref_service()->GetDictionary(kTypeRegisteredForInvalidation);
+    const base::Value* private_topic_value = topics->FindKey(id);
+    ASSERT_EQ(private_topic_value, nullptr);
+  }
+
+  // Check that enable ids are still in the prefs.
+  for (const auto& id : enabled_ids) {
+    const base::DictionaryValue* topics =
+        pref_service()->GetDictionary(kTypeRegisteredForInvalidation);
+    const base::Value* private_topic_value =
+        topics->FindKeyOfType(id, base::Value::Type::STRING);
+    ASSERT_NE(private_topic_value, nullptr);
+  }
+}
+
 }  // namespace syncer