[memories] Use base::FeatureParams and add persist params.

- Replace the param getters & char literals with `base::FeatureParam`s. - Add "MemoriesMaxDaysToCluster" and "MemoriesMaxDaysToCluster" params to be used in a followup CL persisting clusters. - Renames the "MemoriesStoreVisitsInHistoryDb" param to "MemoriesPersistContextAnnotationsInHistoryDb" to more precisely reflect what's being persisted and allow us to later add a similar param for content annotations. - Rename the helper `RemoteModelEndpointForDebugging()` to `RemoteModelEndpoint()` to be consistent with its param name & value. - Rename the helper `ExperimentNameForRemoteModelEndpoint()` to param `kRemoteModelEndpointExperimentName` to be consistent with its param value. - Remove the helper `DebugLoggingEnabled()` and always enable its effects, since it was effectively always enabled. Bug: 1184879, 1171352 Change-Id: I755c81e34a69b1f1becbd2964b15aabcc3673b51 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2869831 Commit-Queue: manuk hovanesian <manukh@chromium.org> Reviewed-by: Tommy Li <tommycli@chromium.org> Cr-Commit-Position: refs/heads/master@{#880543}
FairyWorld · May 7, 2021 · 4aa2526 · 4aa2526
1 parent a4754ce
commit 4aa2526
Show file tree

Hide file tree

Showing 7 changed files with 68 additions and 57 deletions.
diff --git a/chrome/browser/ui/webui/memories/memories_handler.cc b/chrome/browser/ui/webui/memories/memories_handler.cc
@@ -91,7 +91,7 @@ void MemoriesHandler::QueryMemories(
   auto result_callback =
       base::BindOnce(&MemoriesHandler::OnMemoriesQueryResult,
                      weak_ptr_factory_.GetWeakPtr(), std::move(result_mojom));
-  if (history_clusters::RemoteModelEndpointForDebugging().is_valid()) {
+  if (history_clusters::RemoteModelEndpoint().is_valid()) {
     // Cancel pending queries, if any.
     query_task_tracker_.TryCancelAll();
     auto* memory_service =

diff --git a/components/history_clusters/core/memories_features.cc b/components/history_clusters/core/memories_features.cc
@@ -8,41 +8,36 @@
 
 namespace history_clusters {
 
-GURL RemoteModelEndpointForDebugging() {
-  return GURL(base::GetFieldTrialParamValueByFeature(
-      kRemoteModelForDebugging, "MemoriesRemoteModelEndpoint"));
-}
+namespace {
 
-std::string ExperimentNameForRemoteModelEndpoint() {
-  return base::GetFieldTrialParamValueByFeature(
-      kRemoteModelForDebugging, "MemoriesRemoteModelEndpointExperimentName");
-}
+const base::FeatureParam<std::string> kRemoteModelEndpoint{
+    &kRemoteModelForDebugging, "MemoriesRemoteModelEndpoint", ""};
 
-bool DebugLoggingEnabled() {
-  // Do debug logging if the kDebug feature is enabled. Additionally, remote
-  // endpoint for debugging users are implied to be in debug mode.
-  return base::FeatureList::IsEnabled(kDebug) ||
-         RemoteModelEndpointForDebugging().is_valid();
-}
+}  // namespace
 
-bool StoreVisitsInHistoryDb() {
-  return base::GetFieldTrialParamByFeatureAsBool(
-      kMemories, "MemoriesStoreVisitsInHistoryDb", false);
+GURL RemoteModelEndpoint() {
+  return GURL(kRemoteModelEndpoint.Get());
 }
 
-int MaxVisitsToCluster() {
-  return base::GetFieldTrialParamByFeatureAsInt(
-      kMemories, "MemoriesMaxVisitsToCluster", 10);
-}
+const base::FeatureParam<std::string> kRemoteModelEndpointExperimentName{
+    &kRemoteModelForDebugging, "MemoriesRemoteModelEndpointExperimentName", ""};
+
+const base::FeatureParam<bool> kPersistContextAnnotationsInHistoryDb{
+    &kMemories, "MemoriesPersistContextAnnotationsInHistoryDb", false};
+
+const base::FeatureParam<int> kMaxVisitsToCluster{
+    &kMemories, "MemoriesMaxVisitsToCluster", 10};
+
+const base::FeatureParam<int> kMaxDaysToCluster{&kMemories,
+                                                "MemoriesMaxDaysToCluster", 9};
+
+const base::FeatureParam<bool> kPersistClustersInHistoryDb{
+    &kMemories, "MemoriesPersistClustersInHistoryDb", false};
 
-// Enables the Chrome Memories history clustering feature.
 const base::Feature kMemories{"Memories", base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Enables debug info; e.g. shows visit metadata on chrome://history entries.
 const base::Feature kDebug{"MemoriesDebug", base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Enables using a remote model endpoint for Memories clustering for debugging
-// purposes. This should not be ever enabled in production.
 const base::Feature kRemoteModelForDebugging{"MemoriesRemoteModelForDebugging",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
 

diff --git a/components/history_clusters/core/memories_features.h b/components/history_clusters/core/memories_features.h
@@ -6,34 +6,57 @@
 #define COMPONENTS_HISTORY_CLUSTERS_CORE_MEMORIES_FEATURES_H_
 
 #include "base/feature_list.h"
+#include "base/metrics/field_trial_params.h"
 #include "url/gurl.h"
 
 namespace history_clusters {
 
+// Params & helpers functions
+
 // Returns the remote model debug endpoint used to cluster visits into memories.
 // Returns an empty GURL() when the remote model debug endpoint is disabled.
-GURL RemoteModelEndpointForDebugging();
+GURL RemoteModelEndpoint();
 
 // Returns the experiment name to pass through to the remote model debug
 // endpoint to control how the visits get clustered. Returns an empty string if
 // this client should just use be returned the default clustering or if the
 // remote model debug endpoint is disabled.
-std::string ExperimentNameForRemoteModelEndpoint();
-
-// Returns true if debug logs should be generated and shown in WebUI inspector.
-bool DebugLoggingEnabled();
+extern const base::FeatureParam<std::string> kRemoteModelEndpointExperimentName;
 
-// If enabled, completed visits are persisted to the history DB and read back
-// when clustering. If disabled, completed visits are kept in-memory and used
-// these in-memory visits are used when clustering.
-bool StoreVisitsInHistoryDb();
+// If enabled, completed visits context annotations are persisted to the history
+// DB and read back when clustering. If disabled, completed visit context
+// annotations are kept in-memory and used these in-memory visits are used when
+// clustering.
+extern const base::FeatureParam<bool> kPersistContextAnnotationsInHistoryDb;
 
 // The max number of visits to use for each clustering iteration. When using the
-// remote model, this limits the number of visits sent.
-int MaxVisitsToCluster();
+// remote model, this limits the number of visits sent.  Only applies when using
+// persisted visit context annotations; i.e.
+// `kPersistContextAnnotationsInHistoryDb` is true.
+extern const base::FeatureParam<int> kMaxVisitsToCluster;
+
+// The recency threshold controlling which visits will be clustered. This isn't
+// the only factor; i.e. visits older than `MaxDaysToCluster()` may still be
+// clustered. Only applies when using persisted visit context annotations; i.e.
+// `kPersistContextAnnotationsInHistoryDb` is true.
+extern const base::FeatureParam<int> kMaxDaysToCluster;
+
+// If enabled, updating clusters will persist the results to the history DB and
+// accessing clusters will retrieve them from the history DB. If disabled,
+// updating clusters is a no-op and accessing clusters will generate and return
+// new clusters without persisting them.
+extern const base::FeatureParam<bool> kPersistClustersInHistoryDb;
 
+// Features
+
+// Enables the Chrome Memories history clustering feature.
 extern const base::Feature kMemories;
+
+// Enables debug info; e.g. shows visit metadata on chrome://history entries.
 extern const base::Feature kDebug;
+
+// Enables using a remote model endpoint for Memories clustering for debugging
+// purposes. This should not be ever enabled in production.
 extern const base::Feature kRemoteModelForDebugging;
 
 }  // namespace history_clusters

diff --git a/components/history_clusters/core/memories_remote_model_helper.cc b/components/history_clusters/core/memories_remote_model_helper.cc
@@ -28,7 +28,7 @@ proto::GetClustersRequest CreateRequestProto(
     const std::vector<history::AnnotatedVisit>& visits,
     base::Optional<DebugLoggerCallback> debug_logger) {
   proto::GetClustersRequest request;
-  request.set_experiment_name(ExperimentNameForRemoteModelEndpoint());
+  request.set_experiment_name(kRemoteModelEndpointExperimentName.Get());
 
   base::ListValue debug_visits_list;
   for (auto& visit : visits) {
@@ -143,7 +143,7 @@ MemoriesRemoteModelHelper::~MemoriesRemoteModelHelper() = default;
 void MemoriesRemoteModelHelper::GetMemories(
     MemoriesCallback callback,
     const std::vector<history::AnnotatedVisit>& visits) {
-  const GURL endpoint(RemoteModelEndpointForDebugging());
+  const GURL endpoint(RemoteModelEndpoint());
   if (!endpoint.is_valid() || visits.empty()) {
     std::move(callback).Run({});
     return;

diff --git a/components/history_clusters/core/memories_service.cc b/components/history_clusters/core/memories_service.cc
@@ -114,14 +114,10 @@ MemoriesService::MemoriesService(
     history::HistoryService* history_service,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
     : history_service_(history_service) {
-  base::Optional<DebugLoggerCallback> debug_logger;
-  if (DebugLoggingEnabled()) {
-    debug_logger = base::BindRepeating(&MemoriesService::NotifyDebugMessage,
-                                       weak_ptr_factory_.GetWeakPtr());
-  }
-
   remote_model_helper_ = std::make_unique<MemoriesRemoteModelHelper>(
-      url_loader_factory, debug_logger);
+      url_loader_factory,
+      base::BindRepeating(&MemoriesService::NotifyDebugMessage,
+                          weak_ptr_factory_.GetWeakPtr()));
   remote_model_helper_weak_factory_ =
       std::make_unique<base::WeakPtrFactory<MemoriesRemoteModelHelper>>(
           remote_model_helper_.get());
@@ -140,8 +136,6 @@ void MemoriesService::RemoveObserver(Observer* obs) {
 }
 
 void MemoriesService::NotifyDebugMessage(const std::string& message) const {
-  DCHECK(DebugLoggingEnabled()) << "Callers must ensure logging is enabled.";
-
   for (Observer& obs : observers_) {
     obs.OnMemoriesDebugMessage(message);
   }
@@ -175,7 +169,7 @@ void MemoriesService::CompleteVisitContextAnnotationsIfReady(int64_t nav_id) {
       (visit_context_annotations.status.ukm_page_end_signals ||
        !visit_context_annotations.status.expect_ukm_page_end_signals)) {
     if (base::FeatureList::IsEnabled(kMemories)) {
-      if (StoreVisitsInHistoryDb())
+      if (kPersistContextAnnotationsInHistoryDb.Get())
         history_service_->AddAnnotatedVisit(
             {visit_context_annotations.visit_row.visit_id,
              visit_context_annotations.context_annotations,
@@ -211,9 +205,9 @@ void MemoriesService::QueryMemories(
                                               std::move(query_params)))
                          .Then(std::move(callback)));
 
-  if (StoreVisitsInHistoryDb()) {
+  if (kPersistContextAnnotationsInHistoryDb.Get()) {
     history_service_->GetAnnotatedVisits(
-        MaxVisitsToCluster(),
+        kMaxVisitsToCluster.Get(),
         base::BindOnce(
             // This echo callback is necessary to copy the |AnnotatedVisit|
             // refs.

diff --git a/components/history_clusters/core/memories_service.h b/components/history_clusters/core/memories_service.h
@@ -56,8 +56,7 @@ class MemoriesService : public KeyedService {
   void AddObserver(Observer* obs);
   void RemoveObserver(Observer* obs);
 
-  // Notifies the observers of a debug message being available. Caller is
-  // responsible for checking that logging is enabled before calling this.
+  // Notifies the observers of a debug message being available.
   void NotifyDebugMessage(const std::string& message) const;
 
   // TODO(manukh) |MemoriesService| should be responsible for constructing the
@@ -97,8 +96,8 @@ class MemoriesService : public KeyedService {
   friend class MemoriesServiceTestApi;
 
   // If the Memories flag is enabled, this contains all the visits in-memory
-  // during the Profile lifetime. If the "MemoriesStoreVisitsInHistoryDb" param
-  // is true, this will be empty.
+  // during the Profile lifetime. If the `kPersistContextAnnotationsInHistoryDb`
+  // param is true, this will be empty.
   // TODO(tommycli): Hide this better behind a new debug flag.
   std::vector<history::AnnotatedVisit> visits_;
 

diff --git a/components/history_clusters/core/memories_service_unittest.cc b/components/history_clusters/core/memories_service_unittest.cc
@@ -548,7 +548,7 @@ TEST_F(MemoriesServiceTest, QueryMemoriesWithHistoryDb) {
       {
           {
               kMemories,
-              {{"MemoriesStoreVisitsInHistoryDb", "true"}},
+              {{"MemoriesPersistContextAnnotationsInHistoryDb", "true"}},
           },
           {
               kRemoteModelForDebugging,
@@ -617,7 +617,7 @@ TEST_F(MemoriesServiceTest, QueryMemoriesWithHistoryDbWithPendingRequest) {
       {
           {
               kMemories,
-              {{"MemoriesStoreVisitsInHistoryDb", "true"}},
+              {{"MemoriesPersistContextAnnotationsInHistoryDb", "true"}},
           },
           {
               kRemoteModelForDebugging,