Add ContentSuggestionsCategoryFactory; Store categories as ints

components/ntp_snippets/content_suggestions_category_factory.cc

+// 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.
+
+#include "components/ntp_snippets/content_suggestions_category_factory.h"
+
+#include <algorithm>
+
+#include "base/logging.h"
+
+namespace ntp_snippets {
+
+ContentSuggestionsCategoryFactory::ContentSuggestionsCategoryFactory() {
+  // Add all local categories in a fixed order.
+  AddKnownCategory(KnownSuggestionsCategories::OFFLINE_PAGES);
+}
+
+ContentSuggestionsCategoryFactory::~ContentSuggestionsCategoryFactory() {}
+
+ContentSuggestionsCategory ContentSuggestionsCategoryFactory::FromKnownCategory(
+    KnownSuggestionsCategories known_category) {
+  DCHECK_GE(static_cast<int>(known_category), 0);

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

What is this for? We're getting a KnownSuggestionsCategories, so we can assume
it has a valid value.

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

This is here because C++ allows you to do things like
"static_cast<KnownSuggestionsCategories>(999)" and it even delivers 999 if you
cast it back to int. A provider might read the value from a cache/DB or sth like
that and not even do the cast explicitly.

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

For some value of "allows" - it might work in practice, but I'm pretty sure
that's undefined behavior.
Anyway, the DCHECK below would catch that case, so I'd remove this one.

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

True. Done.

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

nit: I believe it's unspecified ;-) But yes, we'd be in troubles anyways if that
happens. Let's remove that check.

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

Acknowledged.

[Bernhard Bauer, 2016/07/28 15:39:26]

Not to add too much bikeshedding, but if you are actually concerned about a
corrupted DB or something, you should do something stronger than a DCHECK that
will be compiled out in an official build. 

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

That's also true.

+  if (known_category < KnownSuggestionsCategories::LOCAL_CATEGORIES_COUNT) {
+    // Local categories should have been added already.
+    DCHECK(categories_by_id_.find(static_cast<int>(known_category)) !=

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

IMO slightly clearer written as
DCHECK(categories_by_id_.count(...));

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

Done. And shorter. But when categories_by_id_ is gone, it's a long
std::find(_if).

+           categories_by_id_.end());
+  } else {
+    DCHECK_GT(known_category,

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

Hm, so this basically checks that the enum has no values between
LOCAL_CATEGORIES_COUNT and REMOTE_CATEGORIES_OFFSET. Since that's known at
compile time, it'd be nice to check it statically, but I guess that's not
possible...
Could we have a unit test for it? Is there a way to iterate over all values of
an enum class?

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

:D As far as I know, and I googled a lot, there is no way to enumerate an enum
in C++, even though that's what gave them their name..
If that was (or is?) possible, we could even make our design a bit nicer in some
places, and of course get rid of this DCHECK.

And additionally, this DCHECK protects against unlucky casts, just as the one in
line 22.

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

Hm, I guess the closest thing is something like this (from StackOverflow):
enum class Color {
    blue,
    red,
    green = 5,
    purple
};
const std::array<Color,4> all_colors = {blue,red,green,purple};
...and then have a unit test that verifies that indeed all values are included,
using a switch. But that might be more trouble than it's worth.

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

Acknowledged.

+              KnownSuggestionsCategories::REMOTE_CATEGORIES_OFFSET);
+  }
+  return InternalFromID(static_cast<int>(known_category));
+}
+
+ContentSuggestionsCategory
+ContentSuggestionsCategoryFactory::FromRemoteCategory(int remote_id) {
+  DCHECK_GT(remote_id, 0);
+  return InternalFromID(
+      static_cast<int>(KnownSuggestionsCategories::REMOTE_CATEGORIES_OFFSET) +
+      remote_id);
+}
+
+ContentSuggestionsCategory ContentSuggestionsCategoryFactory::FromIDValue(
+    int id) {
+  DCHECK_GE(id, 0);
+  DCHECK(id < static_cast<int>(
+                  KnownSuggestionsCategories::LOCAL_CATEGORIES_COUNT) ||
+         id > static_cast<int>(
+                  KnownSuggestionsCategories::REMOTE_CATEGORIES_OFFSET));
+  return InternalFromID(id);
+}
+
+bool ContentSuggestionsCategoryFactory::CompareCategories(
+    const ContentSuggestionsCategory& left,
+    const ContentSuggestionsCategory& right) const {
+  if (left == right)
+    return false;
+  return std::find(categories_.begin(), categories_.end(), left) <
+         std::find(categories_.begin(), categories_.end(), right);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Private methods
+
+void ContentSuggestionsCategoryFactory::AddKnownCategory(
+    KnownSuggestionsCategories known_category) {
+  InternalFromID(static_cast<int>(known_category));
+}
+
+ContentSuggestionsCategory ContentSuggestionsCategoryFactory::InternalFromID(
+    int id) {
+  auto entry = categories_by_id_.find(id);

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

nit: call this |it|? As-is, it sounds like it's the actual element rather than
an iterator.

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

Done.

+  if (entry != categories_by_id_.end()) {
+    return entry->second;
+  }
+
+  ContentSuggestionsCategory category(id);
+  categories_.push_back(category);
+  categories_by_id_.insert(std::make_pair(id, category));
+  return category;
+}
+
+}  // namespace ntp_snippets