Create feed/core/common, feed/core/v2

This CL begins organizing code for Feed v2 by pulling some enums
into a new common directory.

Putting shared code in a different place makes it easier to understand
implications of modifying code, and will make it easier to understand
what pieces of code effect the 'v2' effort.

I'm starting with a few enums because they are very likely to remain
identical. More code will be moved here soon.

Bug: 1044139
Change-Id: I51f83f2fdc212aba9f72a1e9afd2ef8e79437e0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2012903
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734195}

chrome/browser/ui/webui/feed_internals/feed_internals_page_handler.cc

@@ -34,15 +34,15 @@ feed_internals::mojom::TimePtr ToMojoTime(base::Time time) {
                         : feed_internals::mojom::Time::New(time.ToJsTime());
 }
 
-std::string TriggerTypeToString(feed::FeedSchedulerHost::TriggerType* trigger) {
+std::string TriggerTypeToString(feed::TriggerType* trigger) {
   if (trigger == nullptr)
     return "Not set";
   switch (*trigger) {
-    case feed::FeedSchedulerHost::TriggerType::kNtpShown:
+    case feed::TriggerType::kNtpShown:
       return "NTP Shown";
-    case feed::FeedSchedulerHost::TriggerType::kForegrounded:
+    case feed::TriggerType::kForegrounded:
       return "Foregrounded";
-    case feed::FeedSchedulerHost::TriggerType::kFixedTimer:
+    case feed::TriggerType::kFixedTimer:
       return "Fixed Timer";
   }
 }

components/feed/core/BUILD.gn

@@ -48,6 +48,7 @@ source_set("feed_core") {
   ]
 
   deps = [
+    "//components/feed/core/common:feed_core_common",
     "//components/prefs",
     "//components/signin/public/identity_manager",
     "//components/variations",

components/feed/core/common/BUILD.gn

+# Copyright 2020 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.
+
+if (is_android) {
+  import("//build/config/android/rules.gni")
+}
+
+source_set("feed_core_common") {
+  sources = [ "enums.h" ]
+}

components/feed/core/common/README.md

+This directory contains code common to feed v2 and v1.

components/feed/core/common/enums.h

+// Copyright 2020 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_FEED_CORE_COMMON_ENUMS_H_
+#define COMPONENTS_FEED_CORE_COMMON_ENUMS_H_
+
+// This file contains enumerations common to Feed v1 and v2.
+
+namespace feed {
+
+// The TriggerType enum specifies values for the events that can trigger
+// refreshing articles. When adding values, be certain to also update the
+// corresponding definition in enums.xml.
+enum class TriggerType {
+  kNtpShown = 0,
+  kForegrounded = 1,
+  kFixedTimer = 2,
+  kMaxValue = kFixedTimer
+};
+
+// Different groupings of usage. A user will belong to exactly one of these at
+// any given point in time. Can change at runtime.
+enum class UserClass {
+  kRareSuggestionsViewer,      // Almost never opens surfaces that show
+                               // suggestions, like the NTP.
+  kActiveSuggestionsViewer,    // Frequently shown suggestions, but does not
+                               // usually open them.
+  kActiveSuggestionsConsumer,  // Frequently opens news articles.
+};
+
+// Enum for the status of the refresh, reported through UMA.
+// If any new values are added, update FeedSchedulerRefreshStatus in
+// enums.xml.
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class ShouldRefreshResult {
+  kShouldRefresh = 0,
+  kDontRefreshOutstandingRequest = 1,
+  kDontRefreshTriggerDisabled = 2,
+  kDontRefreshNetworkOffline = 3,
+  kDontRefreshEulaNotAccepted = 4,
+  kDontRefreshArticlesHidden = 5,
+  kDontRefreshRefreshSuppressed = 6,
+  kDontRefreshNotStale = 7,
+  kDontRefreshRefreshThrottled = 8,
+  kMaxValue = kDontRefreshRefreshThrottled,
+};
+
+}  // namespace feed
+
+#endif  // COMPONENTS_FEED_CORE_COMMON_ENUMS_H_

components/feed/core/feed_scheduler_host.h

@@ -13,6 +13,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "components/feed/core/common/enums.h"
 #include "components/feed/core/refresh_throttler.h"
 #include "components/feed/core/user_classifier.h"
 #include "components/web_resource/eula_accepted_notifier.h"
@@ -49,33 +50,6 @@ enum NativeRequestBehavior {
 // content.
 class FeedSchedulerHost : web_resource::EulaAcceptedNotifier::Observer {
  public:
-  // The TriggerType enum specifies values for the events that can trigger
-  // refreshing articles. When adding values, be certain to also update the
-  // corresponding definition in enums.xml.
-  enum class TriggerType {
-    kNtpShown = 0,
-    kForegrounded = 1,
-    kFixedTimer = 2,
-    kMaxValue = kFixedTimer
-  };
-
-  // Enum for the status of the refresh, reported through UMA.
-  // If any new values are added, update the corresponding definition in
-  // enums.xml.
-  // These values are persisted to logs. Entries should not be renumbered and
-  // numeric values should never be reused.
-  enum ShouldRefreshResult {
-    kShouldRefresh = 0,
-    kDontRefreshOutstandingRequest = 1,
-    kDontRefreshTriggerDisabled = 2,
-    kDontRefreshNetworkOffline = 3,
-    kDontRefreshEulaNotAccepted = 4,
-    kDontRefreshArticlesHidden = 5,
-    kDontRefreshRefreshSuppressed = 6,
-    kDontRefreshNotStale = 7,
-    kDontRefreshRefreshThrottled = 8,
-    kMaxValue = kDontRefreshRefreshThrottled,
-  };
 
   FeedSchedulerHost(PrefService* profile_prefs,
                     PrefService* local_state,
@@ -224,8 +198,7 @@ class FeedSchedulerHost : web_resource::EulaAcceptedNotifier::Observer {
 
   // In the case the user transitions between user classes, hold onto a
   // throttler for any situation.
-  base::flat_map<UserClassifier::UserClass, std::unique_ptr<RefreshThrottler>>
-      throttlers_;
+  base::flat_map<UserClass, std::unique_ptr<RefreshThrottler>> throttlers_;
 
   // Status of the last fetch for debugging.
   int last_fetch_status_ = 0;

components/feed/core/feed_scheduler_host_unittest.cc

@@ -92,7 +92,7 @@ class FeedSchedulerHostTest : public ::testing::Test {
     // into kRareNtpUser classification.
     test_clock()->Advance(TimeDelta::FromDays(7));
 
-    ASSERT_EQ(UserClassifier::UserClass::kRareSuggestionsViewer,
+    ASSERT_EQ(UserClass::kRareSuggestionsViewer,
               scheduler()->GetUserClassifierForDebugging()->GetUserClass());
   }
 
@@ -107,7 +107,7 @@ class FeedSchedulerHostTest : public ::testing::Test {
     // Depending on which events occurred over which period of time in the test
     // before this function was called, it may not necessarily be sufficient to
     // push the user into the active consumer class.
-    ASSERT_EQ(UserClassifier::UserClass::kActiveSuggestionsConsumer,
+    ASSERT_EQ(UserClass::kActiveSuggestionsConsumer,
               scheduler()->GetUserClassifierForDebugging()->GetUserClass());
   }
 
@@ -177,23 +177,22 @@ class FeedSchedulerHostTest : public ::testing::Test {
 TEST_F(FeedSchedulerHostTest, GetTriggerThreshold) {
   // Make sure that there is no missing configuration in the Cartesian product
   // of states between TriggerType and UserClass.
-  std::vector<FeedSchedulerHost::TriggerType> triggers = {
-      FeedSchedulerHost::TriggerType::kNtpShown,
-      FeedSchedulerHost::TriggerType::kForegrounded,
-      FeedSchedulerHost::TriggerType::kFixedTimer};
+  std::vector<TriggerType> triggers = {TriggerType::kNtpShown,
+                                       TriggerType::kForegrounded,
+                                       TriggerType::kFixedTimer};
 
   // Classification starts out as an active NTP user.
-  for (FeedSchedulerHost::TriggerType trigger : triggers) {
+  for (TriggerType trigger : triggers) {
     EXPECT_FALSE(scheduler()->GetTriggerThreshold(trigger).is_zero());
   }
 
   ClassifyAsActiveSuggestionsConsumer();
-  for (FeedSchedulerHost::TriggerType trigger : triggers) {
+  for (TriggerType trigger : triggers) {
     EXPECT_FALSE(scheduler()->GetTriggerThreshold(trigger).is_zero());
   }
 
   ClassifyAsRareNtpUser();
-  for (FeedSchedulerHost::TriggerType trigger : triggers) {
+  for (TriggerType trigger : triggers) {
     EXPECT_FALSE(scheduler()->GetTriggerThreshold(trigger).is_zero());
   }
 }
@@ -1034,14 +1033,14 @@ TEST_F(FeedSchedulerHostTest, RefreshThrottler) {
 TEST_F(FeedSchedulerHostTest, GetUserClassifierForDebuggingRareUser) {
   ClassifyAsRareNtpUser();
 
-  EXPECT_EQ(UserClassifier::UserClass::kRareSuggestionsViewer,
+  EXPECT_EQ(UserClass::kRareSuggestionsViewer,
             scheduler()->GetUserClassifierForDebugging()->GetUserClass());
 }
 
 TEST_F(FeedSchedulerHostTest, GetUserClassifierForDebuggingActiveConsumer) {
   ClassifyAsActiveSuggestionsConsumer();
 
-  EXPECT_EQ(UserClassifier::UserClass::kActiveSuggestionsConsumer,
+  EXPECT_EQ(UserClass::kActiveSuggestionsConsumer,
             scheduler()->GetUserClassifierForDebugging()->GetUserClass());
 }
 
@@ -1069,19 +1068,19 @@ TEST_F(FeedSchedulerHostTest, GetLastFetchStatusForDebugging) {
 TEST_F(FeedSchedulerHostTest, GetLastFetchTriggerTypeForDebugging) {
   scheduler()->OnForegrounded();
 
-  EXPECT_EQ(FeedSchedulerHost::TriggerType::kForegrounded,
+  EXPECT_EQ(TriggerType::kForegrounded,
             *scheduler()->GetLastFetchTriggerTypeForDebugging());
 
   scheduler()->OnArticlesCleared(/*suppress_refreshes*/ false);
 
-  EXPECT_EQ(FeedSchedulerHost::TriggerType::kNtpShown,
+  EXPECT_EQ(TriggerType::kNtpShown,
             *scheduler()->GetLastFetchTriggerTypeForDebugging());
 
   ClassifyAsActiveSuggestionsConsumer();  // Fixed timer at 48 hours.
   test_clock()->Advance(TimeDelta::FromHours(49));
   scheduler()->OnFixedTimer(base::OnceClosure());
 
-  EXPECT_EQ(FeedSchedulerHost::TriggerType::kFixedTimer,
+  EXPECT_EQ(TriggerType::kFixedTimer,
             *scheduler()->GetLastFetchTriggerTypeForDebugging());
 }
 

components/feed/core/refresh_throttler.cc

@@ -34,21 +34,20 @@ enum class RequestStatus {
 // When adding a new type here, extend also the "RequestThrottlerTypes"
 // <histogram_suffixes> in histograms.xml with the |name| string. First value in
 // the pair is the name, second is the default requests per day.
-std::pair<std::string, int> GetThrottlerParams(
-    UserClassifier::UserClass user_class) {
+std::pair<std::string, int> GetThrottlerParams(UserClass user_class) {
   switch (user_class) {
-    case UserClassifier::UserClass::kRareSuggestionsViewer:
+    case UserClass::kRareSuggestionsViewer:
       return {"SuggestionFetcherRareNTPUser", 5};
-    case UserClassifier::UserClass::kActiveSuggestionsViewer:
+    case UserClass::kActiveSuggestionsViewer:
       return {"SuggestionFetcherActiveNTPUser", 20};
-    case UserClassifier::UserClass::kActiveSuggestionsConsumer:
+    case UserClass::kActiveSuggestionsConsumer:
       return {"SuggestionFetcherActiveSuggestionsConsumer", 20};
   }
 }
 
 }  // namespace
 
-RefreshThrottler::RefreshThrottler(UserClassifier::UserClass user_class,
+RefreshThrottler::RefreshThrottler(UserClass user_class,
                                    PrefService* pref_service,
                                    base::Clock* clock)
     : pref_service_(pref_service), clock_(clock) {

components/feed/core/refresh_throttler.h

@@ -28,7 +28,7 @@ namespace feed {
 //  - "NewTabPage.RequestThrottler.PerDay_|name|" - the daily count of requests.
 class RefreshThrottler {
  public:
-  RefreshThrottler(UserClassifier::UserClass user_class,
+  RefreshThrottler(UserClass user_class,
                    PrefService* pref_service,
                    base::Clock* clock);
 

components/feed/core/refresh_throttler_unittest.cc

@@ -38,8 +38,7 @@ class RefreshThrottlerTest : public testing::Test {
         {{"quota_SuggestionFetcherActiveNTPUser", "2"}});
 
     throttler_ = std::make_unique<RefreshThrottler>(
-        UserClassifier::UserClass::kActiveSuggestionsViewer, &test_prefs_,
-        &test_clock_);
+        UserClass::kActiveSuggestionsViewer, &test_prefs_, &test_clock_);
   }
 
  protected:

components/feed/core/user_classifier.cc

@@ -253,7 +253,7 @@ double UserClassifier::GetEstimatedAvgTime(Event event) const {
                                        min_hours_, max_hours_);
 }
 
-UserClassifier::UserClass UserClassifier::GetUserClass() const {
+UserClass UserClassifier::GetUserClass() const {
   // The pref_service_ can be null in tests.
   if (!pref_service_) {
     return UserClass::kActiveSuggestionsViewer;

components/feed/core/user_classifier.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "components/feed/core/common/enums.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -24,16 +25,6 @@ namespace feed {
 // Based on these long-term user rates, it classifies the user in a UserClass.
 class UserClassifier {
  public:
-  // Different groupings of usage. A user will belong to exactly one of these at
-  // any given point in time. Can change at runtime.
-  enum class UserClass {
-    kRareSuggestionsViewer,      // Almost never opens surfaces that show
-                                 // suggestions, like the NTP.
-    kActiveSuggestionsViewer,    // Frequently shown suggestions, but does not
-                                 // usually open them.
-    kActiveSuggestionsConsumer,  // Frequently opens news articles.
-  };
-
   // For estimating the average length of the intervals between two successive
   // events, we keep a simple frequency model, a single value that we call
   // "rate" below.

components/feed/core/user_classifier_unittest.cc

@@ -59,7 +59,7 @@ class FeedUserClassifierTest : public testing::Test {
 TEST_F(FeedUserClassifierTest, ShouldBeActiveSuggestionsViewerInitially) {
   UserClassifier* user_classifier = CreateUserClassifier();
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kActiveSuggestionsViewer));
+              Eq(UserClass::kActiveSuggestionsViewer));
 }
 
 TEST_F(FeedUserClassifierTest,
@@ -69,7 +69,7 @@ TEST_F(FeedUserClassifierTest,
   // After one click still only an active user.
   user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kActiveSuggestionsViewer));
+              Eq(UserClass::kActiveSuggestionsViewer));
 
   // After a few more clicks, become an active consumer.
   for (int i = 0; i < 5; i++) {
@@ -77,7 +77,7 @@ TEST_F(FeedUserClassifierTest,
     user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   }
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kActiveSuggestionsConsumer));
+              Eq(UserClass::kActiveSuggestionsConsumer));
 }
 
 TEST_F(FeedUserClassifierTest,
@@ -95,13 +95,13 @@ TEST_F(FeedUserClassifierTest,
   test_clock()->Advance(base::TimeDelta::FromHours(1));
   user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kActiveSuggestionsViewer));
+              Eq(UserClass::kActiveSuggestionsViewer));
 
   // One more click to become an active consumer.
   test_clock()->Advance(base::TimeDelta::FromHours(1));
   user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kActiveSuggestionsConsumer));
+              Eq(UserClass::kActiveSuggestionsConsumer));
 }
 
 TEST_F(FeedUserClassifierTest,
@@ -111,12 +111,12 @@ TEST_F(FeedUserClassifierTest,
   // After two days of waiting still an active user.
   test_clock()->Advance(base::TimeDelta::FromDays(2));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kActiveSuggestionsViewer));
+              Eq(UserClass::kActiveSuggestionsViewer));
 
   // Two more days to become a rare user.
   test_clock()->Advance(base::TimeDelta::FromDays(2));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kRareSuggestionsViewer));
+              Eq(UserClass::kRareSuggestionsViewer));
 }
 
 TEST_F(FeedUserClassifierTest,
@@ -131,12 +131,12 @@ TEST_F(FeedUserClassifierTest,
   // After one days of waiting still an active user.
   test_clock()->Advance(base::TimeDelta::FromDays(1));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kActiveSuggestionsViewer));
+              Eq(UserClass::kActiveSuggestionsViewer));
 
   // One more day to become a rare user.
   test_clock()->Advance(base::TimeDelta::FromDays(1));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::kRareSuggestionsViewer));
+              Eq(UserClass::kRareSuggestionsViewer));
 }
 
 class FeedUserClassifierEventTest

components/feed/core/v2/README.md

+The next iteration of the feed component, in development.