[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

components/ntp_snippets/category_rankers/constant_category_ranker.cc

Index: components/ntp_snippets/category_rankers/constant_category_ranker.cc
diff --git a/components/ntp_snippets/category_rankers/constant_category_ranker.cc b/components/ntp_snippets/category_rankers/constant_category_ranker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3995bd00404c4b1b1ba06eb7b18c85e83230589a
--- /dev/null
+++ b/components/ntp_snippets/category_rankers/constant_category_ranker.cc
@@ -0,0 +1,61 @@
+// 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/category_rankers/constant_category_ranker.h"
+
+#include "base/stl_util.h"
+
+namespace ntp_snippets {
+
+ConstantCategoryRanker::ConstantCategoryRanker() {
+  // Add all local categories in a fixed order.
+  AppendKnownCategory(KnownCategories::DOWNLOADS);
+  AppendKnownCategory(KnownCategories::RECENT_TABS);
+  AppendKnownCategory(KnownCategories::FOREIGN_TABS);
+  AppendKnownCategory(KnownCategories::BOOKMARKS);
+  AppendKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES);
+
+  DCHECK_EQ(static_cast<size_t>(KnownCategories::LOCAL_CATEGORIES_COUNT),
+            ordered_categories_.size());
+}
+
+ConstantCategoryRanker::~ConstantCategoryRanker() = default;
+
+bool ConstantCategoryRanker::Compare(Category left, Category right) const {
+  for (Category category : ordered_categories_) {
+    if (category == left) {
+      return true;
+    }
+    if (category == right) {
+      return false;
+    }
+  }
+  return left.id() < right.id();

[tschumann, 2016/12/15 11:50:26]

that's a bit confusing. What are the cases where left.id() < right.id() would
return something different than the loop above?

Would be great to document the fallback.

Ideally, the ranker would know them all... That was actually a nice property
with the factory as that would see every instantiated category object and could
have a more complete view. But I also see that this is speculation for now.
Let's see how things go -- worst case we have to redo this exercise :-) 

[vitaliii, 2016/12/15 15:30:13]

When remote categories are sent in an order not sorted by id.
Marc has noted this, we agreed to require registering all categories explicitly
(I have added LOG(DFATAL)), so this fallback is rather to satisfy sort contract
(I added a comment explaining this).
 
While it was a nice property, the registration was implicit (so one could end up
with unpredictable order) and remote categories were registered in Fetcher,
which is not nice. Overall, currently we have only one provider who registers
remote categories, so this does not seem to be a problem. I also like the new
explicit order approach, because now if one cares about the order, they can just
grab the ranker and define the order, without worrying that it is handled
differently e.g. in Fetcher.

[tschumann, 2016/12/15 18:23:13]

I totally agree that it's good to get this consolidated. 

[vitaliii, 2016/12/16 08:15:43]

Acknowledged.

+}
+
+void ConstantCategoryRanker::ClearHistory(base::Time begin, base::Time end) {
+  // Ignored.
+}
+
+void ConstantCategoryRanker::AppendCategoryIfNecessary(Category category) {
+  if (!base::ContainsValue(ordered_categories_, category)) {
+    ordered_categories_.push_back(category);
+  }
+}
+
+void ConstantCategoryRanker::OnSuggestionOpened(Category category) {
+  // Ignored. The order is constant.
+}
+
+////////////////////////////////////////////////////////////////////////////////

[tschumann, 2016/12/15 11:50:26]

i'd drop this section comment. Ideally we don't get so large files that we need
visual separators.
Marc is this something common we should keep doing?

[Marc Treib, 2016/12/15 12:22:56]

No, it's not particularly common. I don't write them, but so far I haven't
complained when others did.
I guess I'd slightly prefer to leave them out (as you say, if they're needed,
then maybe the file is too big), but I don't feel strong enough to really argue.

[tschumann, 2016/12/15 13:05:20]

Then let's leave them out.

[vitaliii, 2016/12/15 15:30:13]

Done.

+// Private methods
+
+void ConstantCategoryRanker::AppendKnownCategory(
+    KnownCategories known_category) {
+  Category category = Category::FromKnownCategory(known_category);
+  DCHECK(!base::ContainsValue(ordered_categories_, category));
+  ordered_categories_.push_back(category);
+}
+
+}  // namespace ntp_snippets