[NTP::SectionOrder] Make dismissed category penalty a variation param.

components/ntp_snippets/category_rankers/click_based_category_ranker.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/category_rankers/click_based_category_ranker.h"
 
 #include <algorithm>
 
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "components/ntp_snippets/category_rankers/constant_category_ranker.h"
+#include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/variations/variations_associated_data.h"
 
 namespace ntp_snippets {
 
 namespace {
 
 // In order to increase stability and predictability of the order, an extra
 // level of "confidence" is required before moving a category upwards. In other
 // words, the category is moved not when it reaches the previous one, but rather
 // when it leads by some amount. We refer to this required extra "confidence" as
 // a passing margin. Each position has its own passing margin. The category is
@@ skipping 24 matching lines @@
 const base::TimeDelta kTimeBetweenDecays = base::TimeDelta::FromDays(1);
 
 // Decay factor as a fraction. The current value approximates the seventh root
 // of 0.5. This yields a 50% decay per seven decays. Seven weak decays are used
 // instead of one 50% decay in order to decrease difference of click weight in
 // time.
 const int kDecayFactorNumerator = 91;
 const int kDecayFactorDenominator = 100;  // pow(0.91, 7) = 0.517
 
 // Number of positions by which a dismissed category is downgraded.
-const int kDismissedCategoryPenalty = 1;
+const int kDefaultDismissedCategoryPenalty = 1;
+const char* kDismissedCategoryPenaltyParamName =
+    "click_based_category_ranker_dismissed_category_penalty";
 
 const char kCategoryIdKey[] = "category";
 const char kClicksKey[] = "clicks";
 
+int GetDismissedCategoryPenaltyVariationValue() {
+  return variations::GetVariationParamByFeatureAsInt(

[vitaliii, 2017/01/05 09:28:18]

Are you aware of any way to change the value in tests? I would like to test
penalty = 0.

[jkrcal, 2017/01/05 10:34:36]

Sure, see variations::testing::VariationParamsManager, used, e.g., in
https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/schedulin...

This may allow you to remove the function GetDismissedCategoryPenalty().

[vitaliii, 2017/01/05 12:42:28]

Done.

GetDismissedCategoryPenalty() allows to expose the default value.

[jkrcal, 2017/01/05 13:48:07]

I think you get better tests if you always override the penalty to 2 (except for
the test where you override it to 0). Penalty 2 shows more interesting behaviour
then (the default) 1. Then you do not need to ask what is the default value.

To keep the code simple, you may use the approach of
ntp_snippets_fetcher_unittest.cc with two test classes (one for value 2, the
other for value 0).

[vitaliii, 2017/01/16 16:00:16]

Partially done.

I override to 2 only where this is explicitly needed.

+      kCategoryRanker, kDismissedCategoryPenaltyParamName,
+      kDefaultDismissedCategoryPenalty);
+}
+
 }  // namespace
 
 ClickBasedCategoryRanker::ClickBasedCategoryRanker(
     PrefService* pref_service,
     std::unique_ptr<base::Clock> clock)
     : pref_service_(pref_service), clock_(std::move(clock)) {
   if (!ReadOrderFromPrefs(&ordered_categories_)) {
     // TODO(crbug.com/676273): Handle adding new hardcoded KnownCategories to
     // existing order from prefs. Currently such new category is completely
     // ignored and may be never shown.
@@ skipping 101 matching lines @@
   StoreOrderToPrefs(ordered_categories_);
 }
 
 void ClickBasedCategoryRanker::OnCategoryDismissed(Category category) {
   if (!ContainsCategory(category)) {
     LOG(DFATAL) << "The category with ID " << category.id()
                 << " has not been added using AppendCategoryIfNecessary.";
     return;
   }
 
+  const int penalty = GetDismissedCategoryPenaltyVariationValue();
+  if (penalty == 0) {

[vitaliii, 2017/01/05 09:28:18]

Is this ok in your view? Or should I consider penalty = -1 as "disabled" and
"penalty = 0" as "reduce dismissed category clicks to the next category clicks"?

[jkrcal, 2017/01/05 10:34:36]

This is okay, let's keep it simple.

[vitaliii, 2017/01/05 12:42:28]

Acknowledged.

+    // The dismissed category penalty is turned off, the call is ignored.
+    return;
+  }
+
   std::vector<RankedCategory>::iterator current = FindCategory(category);
-  for (int downgrade = 0; downgrade < kDismissedCategoryPenalty; ++downgrade) {
+  for (int downgrade = 0; downgrade < penalty; ++downgrade) {
     std::vector<RankedCategory>::iterator next = current + 1;
     if (next == ordered_categories_.end()) {
       break;
     }
     std::swap(*current, *next);
     current = next;
   }
 
   int next_clicks = 0;
   std::vector<RankedCategory>::iterator next = current + 1;
@@ skipping 22 matching lines @@
   return kPassingMargin;
 }
 
 // static
 int ClickBasedCategoryRanker::GetNumTopCategoriesWithExtraMargin() {
   return kNumTopCategoriesWithExtraMargin;
 }
 
 // static
 int ClickBasedCategoryRanker::GetDismissedCategoryPenalty() {
-  return kDismissedCategoryPenalty;
+  return GetDismissedCategoryPenaltyVariationValue();
 }
 
 ClickBasedCategoryRanker::RankedCategory::RankedCategory(Category category,
                                                          int clicks)
     : category(category), clicks(clicks) {}
 
 // Returns passing margin for a given position taking into account whether it is
 // a top category.
 int ClickBasedCategoryRanker::GetPositionPassingMargin(
     std::vector<RankedCategory>::const_iterator category_position) const {
@@ skipping 136 matching lines @@
                             num_pending_decays * kTimeBetweenDecays);
 
   if (executed_decays > 0) {
     StoreOrderToPrefs(ordered_categories_);
     return true;
   }
   return false;
 }
 
 }  // namespace ntp_snippets