Add ContentSuggestionsCategoryFactory; Store categories as ints

components/ntp_snippets/content_suggestions_category_factory.h

+// Copyright 2016 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.
+
+#ifndef COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_FACTORY_H_
+#define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_FACTORY_H_
+
+#include <stddef.h>

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

Was this added automatically by Eclipse or something? Doesn't really matter,
just curious.

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

This indirectly comes from ntp_snippets_service.h, I now deleted it here and in
content_suggestions_service.h, where it isn't needed.

+
+#include <map>
+#include <vector>
+
+#include "base/macros.h"
+#include "components/ntp_snippets/content_suggestions_category.h"
+
+namespace ntp_snippets {
+
+// Creates and orders instances of ContentSuggestionsCategory.
+class ContentSuggestionsCategoryFactory {
+ public:
+  ContentSuggestionsCategoryFactory();
+  ~ContentSuggestionsCategoryFactory();
+
+  ContentSuggestionsCategory FromKnownCategory(
+      KnownSuggestionsCategories known_category);
+  ContentSuggestionsCategory FromRemoteCategory(int remote_id);

[tschumann, 2016/07/28 12:50:14]

i think passing in the index from the server response would be good, so that we
can keep the order we received from the server.

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

We can add that once we need it. Currently, the server-provider would just add
them in the order it received them. And the order cannot change (at least that's
what we agreed on).
Passing the index in here is only useful if you also pass in the provider, to
keep track of per-provider indices. And then you'd need some ordering among
providers. Currently, the ContentSuggestionsCategory and -Factory are nicely
decoupled from all the other stuff that's going on, but receiving the provider
instance here would lead to circular dependencies. We could use a provider_id
for that, but we don't have that yet. Neither do we need the indices yet, so
let's just worry about that once we need it. It would only be one provider that
needs to be adjusted then.

[tschumann, 2016/07/28 15:03:45]

The problem i'm seeing is: Keeping the order as we received it from the service
is a requirement and it only works due to undocumented implementation details.
If for some reason we change how the provider processes the server response,
we'd violate the order requirement.
I'd rather hard-code that we only support one remote-provider. If you pass the
second instance, we could have the following semantics:
-- the first time we see a remote_id, we use the given index as a secondary
sort-key (i.e. as a tie breaker when comparing remote categories.
-- if another remote_id already used that secondary sort-key, we'd assign a new
one (the next available secondary key) essentially sorting at the end. This is
needed in case we get updates in a different order.

another nit: should we rename 'remote_id' to 'remote_category'? (would be in
line with 'known_category'.

[Marc Treib, 2016/07/28 15:13:26]

Or, alternatively: Document that remote categories will be ordered by order of
creation. That way, we don't introduce any extra unused plumbing right now, and
longer-term, the ordering will change anyway and likely those secondary
sort-keys won't cover it. YAGNI
That makes sense!

[Philipp Keck, 2016/07/28 15:15:25]

We could document the way it works in this file. It's also in the design doc.
There currently is no sorting at all, categories are simply kept in the order in
which they were added. So it wouldn't be an implementation detail of the
provider, but it's a requirement that the provider has to fulfill when using
this interface: If the provider wants its categories to appear in the right
order, it needs to sort them the way it needs them before calling
FromRemoteCategory for each of them.

I don't see where and how we could hard-code the one remote-provider (or do we
just trust that only one provider ever uses this method?) and where we would
store that secondary sort-key. We would need a separate data structure for that,
or we would have to pack it into the Category itself. I could create a follow-up
CL which attempts that and we can discuss it there? But since the current
approach works, maybe documenting it is sufficient?

I'd call something a "category" if it unambiguously is a category. And something
is a "[category] id" if it's an int that somehow refers to a category. There is
a different between the "ids" we store in ContentSuggestionsCategory::id_ and
the 'remote_id' passed in here -- namely a numerical difference of 10000. So the
parameter is not the category itself, it's something that refers to it.
Maybe the method should be named "FromRemoteCategoryID" and the one below should
be named "FromCategoryID"?

[Philipp Keck, 2016/07/28 15:25:58]

Agree. I documented it here and on CompareCategories below.
I'm fine with renaming it, but I still think there is a difference between
categories and their (different kinds of) IDs.

[tschumann, 2016/07/28 15:36:10]

yes, hence the different type and factory function. if you call it a id, then it
gets confusing with FromIDValue() which in addition has the same type as
'remote_id' -- and we really want to keep those things separated.

[tschumann, 2016/07/28 15:36:10]

just read-up on the implementation (didn't have a careful look before) and tend
to agree that simply documenting that the insertion order will be reflected in
the sort order seems to be good enough for now. Especially as it naturally
handles the case of changing orders.

[Philipp Keck, 2016/07/28 16:59:58]

Acknowledged.

[Philipp Keck, 2016/07/28 16:59:58]

Ok, I renamed it.

+  ContentSuggestionsCategory FromIDValue(int id);

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

Please add comments for these. E.g. what must |remote_id| satisfy.

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

Done.

+
+  bool CompareCategories(const ContentSuggestionsCategory& left,

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

Please add a comment on what exactly this does - I guess it's a "less than"?

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

Done.

+                         const ContentSuggestionsCategory& right) const;
+
+ private:
+  void AddKnownCategory(KnownSuggestionsCategories known_category);
+  ContentSuggestionsCategory InternalFromID(int id);
+
+  std::vector<ContentSuggestionsCategory> categories_;

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

Add a comment saying that these are in order?

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

Done.

+  std::map<int, ContentSuggestionsCategory> categories_by_id_;

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

Hm. We could also leave this out and just do std::find on the vector, then we
don't need two data structures. WDYT?

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

That's possible. I was hoping that the map is faster because it sorts the keys
(although that doesn't really matter). And also, it would either need to be a
find_if with a lambda expression, or we have to create a new
ContentSuggestionsCategory instance and then find that.

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

Rule of thumb: For < 100 small elements, vector will be faster than anything
else, because cache coherency. Anyway, performance won't be a problem either
way, I was just thinking of simplicity. I'll leave it to you :)

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

I changed it and introduced a CategoryExists helper function.

+
+  DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsCategoryFactory);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_FACTORY_H_