Add ContentSuggestionsCategoryFactory; Store categories as ints

components/ntp_snippets/content_suggestions_service.h

Index: components/ntp_snippets/content_suggestions_service.h
diff --git a/components/ntp_snippets/content_suggestions_service.h b/components/ntp_snippets/content_suggestions_service.h
index 56666f23a559f6cc14c59d50fee7ace8c9fb75ea..747284ad032f1e9546fee9b53b251f5fae7a55a9 100644
--- a/components/ntp_snippets/content_suggestions_service.h
+++ b/components/ntp_snippets/content_suggestions_service.h
@@ -14,6 +14,7 @@
 #include "base/callback_forward.h"
 #include "base/observer_list.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/ntp_snippets/content_suggestions_category_factory.h"
 #include "components/ntp_snippets/content_suggestions_category_status.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
 
@@ -62,6 +63,13 @@ class ContentSuggestionsService : public KeyedService,
     DISABLED,
   };
 
+  // This is just an arbitrary ordering by ID, used by the maps in this class,
+  // because the ordering needs to be constant for maps.
+  struct CompareCategoriesByID {

[Marc Treib, 2016/07/28 11:41:46]

This should be private.

[Philipp Keck, 2016/07/28 13:50:55]

Done.

+    bool operator()(const ContentSuggestionsCategory& left,
+                    const ContentSuggestionsCategory& right) const;
+  };
+
   ContentSuggestionsService(State state);
   ~ContentSuggestionsService() override;
 
@@ -119,6 +127,10 @@ class ContentSuggestionsService : public KeyedService,
   // dismissed suggestions reappear (only for certain providers).
   void ClearDismissedSuggestionsForDebugging();
 
+  ContentSuggestionsCategoryFactory* category_factory() {

[Marc Treib, 2016/07/28 11:41:46]

Clients aren't supposed to register new categories with this, right? And also
they shouldn't keep the pointer around? In that case, you could return a const
ref.

[Philipp Keck, 2016/07/28 13:50:54]

Yes, the providers use this to register categories. And they do keep the pointer
around (none of the current providers does, but the server-provider will have to
do so, as it's the only way to access the factory).

[Marc Treib, 2016/07/28 14:31:17]

Ah - technically the providers just get a Factory ptr passed in, but the
ProviderFactory gets it from here. Alright, carry on then.

[Philipp Keck, 2016/07/28 14:55:47]

Acknowledged.

+    return &category_factory_;
+  }
+
  private:
   friend class ContentSuggestionsServiceTest;
 
@@ -139,22 +151,29 @@ class ContentSuggestionsService : public KeyedService,
   // Whether the content suggestions feature is enabled.
   State state_;
 
+  // Provides new categories and an order for them.

[Marc Treib, 2016/07/28 11:41:46]

Provides all categories really, no?

[Philipp Keck, 2016/07/28 13:50:55]

True. I put "new and existing" because "all" could confusingly also mean that
all categories are in there, even the inactive ones.

+  ContentSuggestionsCategoryFactory category_factory_;
+
   // All registered providers. A provider may be contained multiple times, if it
   // provides multiple categories. The keys of this map are exactly the entries
   // of |categories_|.
-  std::map<ContentSuggestionsCategory, ContentSuggestionsProvider*> providers_;
-
-  // All current suggestion categories, in an order determined by the service.
-  // Currently, this is simply the order in which the providers were registered.
-  // This vector contains exactly the same categories as |providers_|.
-  // TODO(pke): Implement a useful and consistent ordering for categories.
+  std::map<ContentSuggestionsCategory,
+           ContentSuggestionsProvider*,
+           CompareCategoriesByID>
+      providers_;
+
+  // All current suggestion categories, in an order determined by the
+  // |category_factory_|. This vector contains exactly the same categories as
+  // |providers_|.
   std::vector<ContentSuggestionsCategory> categories_;
 
   // All current suggestions grouped by category. This contains an entry for
   // every category in |categories_| whose status is an available status. It may
   // contain an empty vector if the category is available but empty (or still
   // loading).
-  std::map<ContentSuggestionsCategory, std::vector<ContentSuggestion>>
+  std::map<ContentSuggestionsCategory,
+           std::vector<ContentSuggestion>,
+           CompareCategoriesByID>
       suggestions_by_category_;
 
   // Map used to determine the category of a suggestion (of which only the ID