📰 Persist category dismissals

components/ntp_snippets/content_suggestions_provider.h

Index: components/ntp_snippets/content_suggestions_provider.h
diff --git a/components/ntp_snippets/content_suggestions_provider.h b/components/ntp_snippets/content_suggestions_provider.h
index f6741356d66e6cfc0a3773c83c8afe25a6d63d83..476385468b52a38dd219cea5ddcb891af09590fe 100644
--- a/components/ntp_snippets/content_suggestions_provider.h
+++ b/components/ntp_snippets/content_suggestions_provider.h
@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_PROVIDER_H_
 #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_PROVIDER_H_
 
+#include <map>
 #include <string>
 #include <vector>
 
@@ -35,19 +36,34 @@ class ContentSuggestionsProvider {
   // The observer of a provider is notified when new data is available.
   class Observer {
    public:
+    typedef std::
+        map<Category, std::vector<ContentSuggestion>, Category::CompareByID>
+            SuggestionBatch;
+
     // Called when the available content changed.
-    // If a provider provides suggestions for multiple categories, this callback
-    // is called once per category. The |suggestions| parameter always contains
-    // the full list of currently available suggestions for that category, i.e.,
-    // an empty list will remove all suggestions from the given category. Note
-    // that to clear them from the UI immediately, the provider needs to change
-    // the status of the respective category. If the given |category| is not
-    // known yet, the calling |provider| will be registered as its provider.
+    // If a provider provides suggestions for multiple categories, it should
+    // use OnNewSuggestionBatch() instead. The |suggestions| parameter always
+    // contains the full list of currently available suggestions for that
+    // category, i.e., an empty list will remove all suggestions from the given
+    // category. Note that to clear them from the UI immediately, the provider
+    // needs to change the status of the respective category. If the given
+    // |category| is not known yet, the calling |provider| will be registered as
+    // its provider.
     virtual void OnNewSuggestions(
         ContentSuggestionsProvider* provider,
         Category category,
         std::vector<ContentSuggestion> suggestions) = 0;
 
+    // Called when the available content for multiple categories changed.
+    // The |suggestions| parameter always contains the full list of currently
+    // available suggestions for that category, i.e., an empty list will remove
+    // all suggestions from the given category. Note that to clear them from the
+    // UI immediately, the provider needs to change the status of the respective
+    // category. If the given |category| is not known yet, the calling
+    // |provider| will be registered as its provider.
+    virtual void OnNewSuggestionBatch(ContentSuggestionsProvider* provider,

[tschumann, 2016/10/10 16:50:54]

Do we really need this batching functionality? Why not just have n calls to
OnNewSuggestions?

[dgn, 2016/10/10 18:27:59]

This is to have the full list of remote categories to clean up the dismissed
one. Since the remote provider notifies for all the known categories, we
consider the server deprecated a category when we don't get anything back about
that one. We then can remove the data saved about it.

I suppose I could have a separate UpdateDismissals(vector<Category>) instead

[dgn, 2016/10/10 19:54:53]

Actually, I still need the suggestions, or at least whether they are empty or
not, so that won't be enough.

Maybe I can rename the method and make it more obviously targeted at the
SnippetService/RemoteSuggestionsProvider. Would that be enough?

[tschumann, 2016/10/10 20:40:23]

How important is that cleanup? Does the UI need to worry about this?
It feels like this change breaks a lot of abstractions... IMO it should not
matter if suggestions from different catgories come from the same provider or
not. What if a provider just stops delivering results (think of the PhysicalWeb
integration). Would that be handled differently?

[dgn, 2016/10/10 21:10:18]

The UI does not need to worry about it. The only place where this is a concern
is between the provider for remote sections and the suggestions service.

If we have orphaned dismissed categories it will stay in the preferences and
never be removed. If we start syncing them they will be carried over, etc.

I totally agree that where the categories come from should not matter. There is
nothing in the CL to cleanup dismissed suggestions from local sources like
PhysicalWeb, but since they can't be added dynamically if feels less of an
issue.

Another option could be to have the server report deprecated categories via a
different request maybe? Or Finch? And we could purge them separately on launch.
Then sure, I can remove all the category-absence based cleanup, and land that
one later, when we would start needing it.

+                                      SuggestionBatch suggestion_batch) = 0;
+
     // Called when the status of a category changed, including when it is added.
     // If |new_status| is NOT_PROVIDED, the calling provider must be the one
     // that currently provides the |category|, and the category is unregistered