[M-115][journeys] Fix regression where WebUI toggle disappears if pre…

…f false There's a regression in M115 where if the user turns the pref false from WebUI, then closes the tab, and re-opens History, the toggle is gone. This regression happened here: https://bugs.chromium.org/p/chromium/issues/detail?id=1450346 This CL is a minimal fix meant to be easily mergeable to M115. I tested it manually. A more extensive cleanup can be done after the client-side locale check can be removed from the code. (cherry picked from commit f50b325) Bug: 1450346 Change-Id: I860103c365b276735627a3c15cf39031330492b5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4577921 Reviewed-by: Roman Arora <romanarora@chromium.org> Auto-Submit: Tommy Li <tommycli@chromium.org> Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Reviewed-by: Sophie Chang <sophiechang@chromium.org> Commit-Queue: Roman Arora <romanarora@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1151536} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4591110 Reviewed-by: manuk hovanesian <manukh@chromium.org> Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org> Commit-Queue: Sophie Chang <sophiechang@chromium.org> Cr-Commit-Position: refs/branch-heads/5790@{#374} Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
chromium · Jun 5, 2023 · 0dce5c9 · 0dce5c9
1 parent 3dc34a7
commit 0dce5c9
Show file tree

Hide file tree

Showing 10 changed files with 35 additions and 27 deletions.
diff --git a/chrome/browser/chrome_browser_interface_binders.cc b/chrome/browser/chrome_browser_interface_binders.cc
@@ -1011,7 +1011,7 @@ void PopulateChromeWebUIFrameBinders(
       HistoryClustersServiceFactory::GetForBrowserContext(
           render_frame_host->GetProcess()->GetBrowserContext());
   if (history_clusters_service &&
-      history_clusters_service->IsJourneysEnabled()) {
+      history_clusters_service->is_journeys_feature_flag_enabled()) {
     if (base::FeatureList::IsEnabled(history_clusters::kSidePanelJourneys)) {
       RegisterWebUIControllerInterfaceBinder<
           history_clusters::mojom::PageHandler, HistoryUI,
@@ -1023,7 +1023,7 @@ void PopulateChromeWebUIFrameBinders(
   }
 
   if ((history_clusters_service &&
-       history_clusters_service->IsJourneysEnabled() &&
+       history_clusters_service->is_journeys_feature_flag_enabled() &&
        history_clusters_service->IsJourneysImagesEnabled()) ||
       base::FeatureList::IsEnabled(ntp_features::kNtpHistoryClustersModule) ||
       base::FeatureList::IsEnabled(

diff --git a/chrome/browser/new_tab_page/modules/history_clusters/history_clusters_module_service.cc b/chrome/browser/new_tab_page/modules/history_clusters/history_clusters_module_service.cc
@@ -143,7 +143,7 @@ HistoryClustersModuleService::HistoryClustersModuleService(
 HistoryClustersModuleService::~HistoryClustersModuleService() = default;
 
 void HistoryClustersModuleService::GetClusters(GetClustersCallback callback) {
-  if (!history_clusters_service_->IsJourneysEnabled()) {
+  if (!history_clusters_service_->IsJourneysEnabledAndVisible()) {
     std::move(callback).Run({}, {});
     return;
   }

diff --git a/...e/browser/ui/views/side_panel/history_clusters/history_clusters_side_panel_coordinator.cc b/...e/browser/ui/views/side_panel/history_clusters/history_clusters_side_panel_coordinator.cc
@@ -49,7 +49,7 @@ bool HistoryClustersSidePanelCoordinator::IsSupported(Profile* profile) {
       HistoryClustersServiceFactory::GetForBrowserContext(profile);
   return base::FeatureList::IsEnabled(history_clusters::kSidePanelJourneys) &&
          history_clusters_service &&
-         history_clusters_service->IsJourneysEnabled() &&
+         history_clusters_service->IsJourneysEnabledAndVisible() &&
          !profile->IsIncognitoProfile() && !profile->IsGuestSession();
 }
 

diff --git a/chrome/browser/ui/webui/cr_components/history_clusters/history_clusters_util.cc b/chrome/browser/ui/webui/cr_components/history_clusters/history_clusters_util.cc
@@ -29,9 +29,10 @@ void HistoryClustersUtil::PopulateSource(content::WebUIDataSource* source,
   source->AddBoolean("inSidePanel", in_side_panel);
   auto* history_clusters_service =
       HistoryClustersServiceFactory::GetForBrowserContext(profile);
-  source->AddBoolean("isHistoryClustersEnabled",
-                     history_clusters_service &&
-                         history_clusters_service->IsJourneysEnabled());
+  source->AddBoolean(
+      "isHistoryClustersEnabled",
+      history_clusters_service &&
+          history_clusters_service->is_journeys_feature_flag_enabled());
   source->AddBoolean(kIsHistoryClustersVisibleKey,
                      prefs->GetBoolean(history_clusters::prefs::kVisible));
   source->AddBoolean(

diff --git a/components/history_clusters/core/history_clusters_service.cc b/components/history_clusters/core/history_clusters_service.cc
@@ -121,19 +121,19 @@ HistoryClustersService::HistoryClustersService(
     optimization_guide::NewOptimizationGuideDecider* optimization_guide_decider,
     PrefService* prefs)
     : persist_caches_to_prefs_(GetConfig().persist_caches_to_prefs),
-      is_journeys_enabled_(
+      is_journeys_feature_flag_enabled_(
           GetConfig().is_journeys_enabled_no_locale_check &&
           IsApplicationLocaleSupportedByJourneys(application_locale)),
       history_service_(history_service),
       pref_service_(prefs) {
-  if (prefs && is_journeys_enabled_) {
+  if (prefs && is_journeys_feature_flag_enabled_) {
     // Log whether the user has Journeys enabled if they are eligible for it.
     base::UmaHistogramBoolean(
         "History.Clusters.JourneysEligibleAndEnabledAtSessionStart",
         prefs->GetBoolean(prefs::kVisible));
   }
 
-  if (!is_journeys_enabled_) {
+  if (!is_journeys_feature_flag_enabled_) {
     return;
   }
 
@@ -168,8 +168,9 @@ base::WeakPtr<HistoryClustersService> HistoryClustersService::GetWeakPtr() {
 
 void HistoryClustersService::Shutdown() {}
 
-bool HistoryClustersService::IsJourneysEnabled() const {
-  return is_journeys_enabled_ && pref_service_->GetBoolean(prefs::kVisible);
+bool HistoryClustersService::IsJourneysEnabledAndVisible() const {
+  return is_journeys_feature_flag_enabled_ &&
+         pref_service_->GetBoolean(prefs::kVisible);
 }
 
 // static
@@ -228,7 +229,7 @@ void HistoryClustersService::CompleteVisitContextAnnotationsIfReady(
        !visit_context_annotations.status.expect_ukm_page_end_signals)) {
     // If the main Journeys feature is enabled, we want to persist visits.
     // And if the persist-only switch is enabled, we also want to persist them.
-    if (IsJourneysEnabled() ||
+    if (IsJourneysEnabledAndVisible() ||
         GetConfig().persist_context_annotations_in_history_db) {
       history_service_->SetOnCloseContextAnnotationsForVisit(
           visit_context_annotations.visit_row.visit_id,
@@ -246,7 +247,7 @@ HistoryClustersService::QueryClusters(
     QueryClustersContinuationParams continuation_params,
     bool recluster,
     QueryClustersCallback callback) {
-  if (!IsJourneysEnabled()) {
+  if (!IsJourneysEnabledAndVisible()) {
     // TODO(crbug/1441974): Make this into a CHECK after verifying all callers.
     std::move(callback).Run({}, QueryClustersContinuationParams::DoneParams());
     return nullptr;
@@ -334,8 +335,9 @@ void HistoryClustersService::UpdateClusters() {
 
 absl::optional<history::ClusterKeywordData>
 HistoryClustersService::DoesQueryMatchAnyCluster(const std::string& query) {
-  if (!IsJourneysEnabled())
+  if (!IsJourneysEnabledAndVisible()) {
     return absl::nullopt;
+  }
 
   // We don't want any omnibox jank for low-end devices.
   if (base::SysInfo::IsLowEndDevice())

diff --git a/components/history_clusters/core/history_clusters_service.h b/components/history_clusters/core/history_clusters_service.h
@@ -83,12 +83,15 @@ class HistoryClustersService : public base::SupportsUserData,
   // KeyedService:
   void Shutdown() override;
 
-  // Returns true if the Journeys feature is enabled for the current application
-  // locale. This is a cached wrapper of `IsJourneysEnabled()` within features.h
-  // that's already evaluated against the g_browser_process application locale.
-  // This now also includes checking the pref for whether Journeys is visible to
-  // the user. Virtual for testing.
-  virtual bool IsJourneysEnabled() const;
+  // Returns true if the Journeys feature is enabled both by feature flag AND
+  // by the user pref / policy value. Virtual for testing.
+  virtual bool IsJourneysEnabledAndVisible() const;
+
+  // Returns true if the Journeys feature is enabled by feature flag, but
+  // ignores the pref / policy value.
+  bool is_journeys_feature_flag_enabled() const {
+    return is_journeys_feature_flag_enabled_;
+  }
 
   // Returns true if the Journeys use of Images is enabled.
   static bool IsJourneysImagesEnabled();
@@ -212,8 +215,10 @@ class HistoryClustersService : public base::SupportsUserData,
   // Whether keyword caches should persisted via the pref service.
   const bool persist_caches_to_prefs_;
 
-  // True if Journeys is enabled based on field trial and locale checks.
-  const bool is_journeys_enabled_;
+  // True if Journeys is enabled based on feature flag and locale checks.
+  // But critically, this does NOT check the pref or policy value to see if
+  // either the user or Enterprise has disabled Journeys.
+  const bool is_journeys_feature_flag_enabled_;
 
   // Non-owning pointer, but never nullptr.
   history::HistoryService* const history_service_;

diff --git a/components/history_clusters/core/test_history_clusters_service.cc b/components/history_clusters/core/test_history_clusters_service.cc
@@ -19,7 +19,7 @@ TestHistoryClustersService::TestHistoryClustersService()
                              /*pref_service=*/nullptr) {}
 TestHistoryClustersService::~TestHistoryClustersService() = default;
 
-bool TestHistoryClustersService::IsJourneysEnabled() const {
+bool TestHistoryClustersService::IsJourneysEnabledAndVisible() const {
   return is_journeys_enabled_;
 }
 

diff --git a/components/history_clusters/core/test_history_clusters_service.h b/components/history_clusters/core/test_history_clusters_service.h
@@ -20,7 +20,7 @@ class TestHistoryClustersService : public HistoryClustersService {
   ~TestHistoryClustersService() override;
 
   // HistoryClustersService:
-  bool IsJourneysEnabled() const override;
+  bool IsJourneysEnabledAndVisible() const override;
   std::unique_ptr<HistoryClustersServiceTask> QueryClusters(
       ClusteringRequestSource clustering_request_source,
       QueryClustersFilterParams filter_params,

diff --git a/components/omnibox/browser/actions/history_clusters_action.cc b/components/omnibox/browser/actions/history_clusters_action.cc
@@ -194,7 +194,7 @@ void AttachHistoryClustersActions(
   return;
 #else
 
-  if (!service || !service->IsJourneysEnabled()) {
+  if (!service || !service->IsJourneysEnabledAndVisible()) {
     return;
   }
 

diff --git a/components/omnibox/browser/history_cluster_provider.cc b/components/omnibox/browser/history_cluster_provider.cc
@@ -66,7 +66,7 @@ void HistoryClusterProvider::Start(const AutocompleteInput& input,
     return;
 
   if (!client_->GetHistoryClustersService() ||
-      !client_->GetHistoryClustersService()->IsJourneysEnabled()) {
+      !client_->GetHistoryClustersService()->IsJourneysEnabledAndVisible()) {
     return;
   }