[Synced MVTs] Create local-state int Pref for Synced MVTs.

This change prevents foreign history from being included in segments data when the following conditions are met: 1. The New Tab Page (on iOS) has been displayed, at least, 5 times. 2. The app is being launched freshly. From this point on, foreign history will not be included in segments data. (cherry picked from commit e8aaed2) Change-Id: Ic54bb0bd58e1f9b56cbc54949eae6cfd29c66afd Bug: 1431954 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4324438 Commit-Queue: Benjamin Williams <[email protected]> Code-Coverage: Findit <[email protected]> Reviewed-by: Scott Violet <[email protected]> Cr-Original-Commit-Position: refs/heads/main@{#1135990} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4479997 Reviewed-by: Rohit Rao <[email protected]> Cr-Commit-Position: refs/branch-heads/5735@{#58} Cr-Branched-From: 2f562e4-refs/heads/main@{#1135570}
blueboxd · Apr 28, 2023 · 65328fc · 65328fc
1 parent b160292
commit 65328fc
Show file tree

Hide file tree

Showing 13 changed files with 95 additions and 3 deletions.
diff --git a/components/history/core/browser/features.cc b/components/history/core/browser/features.cc
@@ -79,6 +79,13 @@ BASE_FEATURE(kSyncSegmentsData,
              "SyncSegmentsData",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+// The maximum number of New Tab Page displays to show with synced segments
+// data.
+const base::FeatureParam<int> kMaxNumNewTabPageDisplays(
+    &kSyncSegmentsData,
+    "MaxNumNumNewTabPageDisplays",
+    5);
+
 bool IsSyncSegmentsDataEnabled() {
   return base::FeatureList::IsEnabled(syncer::kSyncEnableHistoryDataType) &&
          base::FeatureList::IsEnabled(kSyncSegmentsData);

diff --git a/components/history/core/browser/features.h b/components/history/core/browser/features.h
@@ -18,6 +18,7 @@ extern const base::FeatureParam<bool> kPrivilegeRepeatableQueries;
 extern const base::FeatureParam<bool> kRepeatableQueriesIgnoreDuplicateVisits;
 extern const base::FeatureParam<int> kRepeatableQueriesMaxAgeDays;
 extern const base::FeatureParam<int> kRepeatableQueriesMinVisitCount;
+extern const base::FeatureParam<int> kMaxNumNewTabPageDisplays;
 
 // Synced Segments Data
 // NOTE: Use `IsSyncSegmentsDataEnabled()` below to check if `kSyncSegmentsData`

diff --git a/components/history/core/browser/history_backend.cc b/components/history/core/browser/history_backend.cc
@@ -611,8 +611,9 @@ void HistoryBackend::UpdateSegmentForExistingForeignVisit(VisitRow& visit_row) {
   }
 
   SegmentID new_segment_id =
-      CanAddForeignVisitToSegments(
-          visit_row, local_device_originator_cache_guid_, sync_device_info_)
+      (can_add_foreign_visits_to_segments_ &&
+       CanAddForeignVisitToSegments(
+           visit_row, local_device_originator_cache_guid_, sync_device_info_))
           ? CalculateSegmentID(url_row.url(), visit_row.referring_visit,
                                visit_row.transition)
           : 0;
@@ -1701,7 +1702,8 @@ VisitID HistoryBackend::AddSyncedVisit(
 
   db_->SetMayContainForeignVisits(true);
 
-  if (CanAddForeignVisitToSegments(visit, local_device_originator_cache_guid_,
+  if (can_add_foreign_visits_to_segments_ &&
+      CanAddForeignVisitToSegments(visit, local_device_originator_cache_guid_,
                                    sync_device_info_)) {
     AssignSegmentForNewVisit(url, visit.referring_visit, visit_id,
                              visit.transition, visit.visit_time);
@@ -3473,6 +3475,10 @@ void HistoryBackend::SetLocalDeviceOriginatorCacheGuid(
       std::move(local_device_originator_cache_guid);
 }
 
+void HistoryBackend::SetCanAddForeignVisitsToSegments(bool add_foreign_visits) {
+  can_add_foreign_visits_to_segments_ = add_foreign_visits;
+}
+
 void HistoryBackend::ProcessDBTask(
     std::unique_ptr<HistoryDBTask> task,
     scoped_refptr<base::SequencedTaskRunner> origin_loop,

diff --git a/components/history/core/browser/history_backend.h b/components/history/core/browser/history_backend.h
@@ -587,6 +587,10 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   void SetLocalDeviceOriginatorCacheGuid(
       std::string local_device_originator_cache_guid);
 
+  // Notifies the history backend that it should consider adding foreign
+  // visits to local segments data.
+  void SetCanAddForeignVisitsToSegments(bool add_foreign_visits);
+
   void ProcessDBTask(
       std::unique_ptr<HistoryDBTask> task,
       scoped_refptr<base::SequencedTaskRunner> origin_loop,
@@ -1089,6 +1093,12 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   // Contains the local device Originator Cache GUID, a unique, sync-specific
   // identifier for the local device.
   std::string local_device_originator_cache_guid_;
+
+  // Whether segments data should include foreign history; Note that setting
+  // this to true doesn't guarantee segments data is synced, as feature
+  // `kSyncSegmentsData` may be enabled or disabled, or
+  // `kMaxNumNewTabPageDisplays` is reached.
+  bool can_add_foreign_visits_to_segments_ = false;
 };
 
 }  // namespace history

diff --git a/components/history/core/browser/history_backend_unittest.cc b/components/history/core/browser/history_backend_unittest.cc
@@ -4679,6 +4679,8 @@ TEST_F(HistoryBackendTest,
   feature_list.InitWithFeatures(
       {syncer::kSyncEnableHistoryDataType, history::kSyncSegmentsData}, {});
 
+  backend_->SetCanAddForeignVisitsToSegments(true);
+
   SyncDeviceInfoMap sync_device_info =
       MakeSyncDeviceInfo({"foreign"}, {}, "local");
 
@@ -4749,6 +4751,8 @@ TEST_F(HistoryBackendTest,
   feature_list.InitWithFeatures(
       {syncer::kSyncEnableHistoryDataType, history::kSyncSegmentsData}, {});
 
+  backend_->SetCanAddForeignVisitsToSegments(true);
+
   SyncDeviceInfoMap sync_device_info =
       MakeSyncDeviceInfo({"foreign"}, {}, "local");
 
@@ -4820,6 +4824,8 @@ TEST_F(HistoryBackendTest,
   feature_list.InitWithFeatures(
       {syncer::kSyncEnableHistoryDataType, history::kSyncSegmentsData}, {});
 
+  backend_->SetCanAddForeignVisitsToSegments(true);
+
   SyncDeviceInfoMap sync_device_info =
       MakeSyncDeviceInfo({"foreign"}, {}, "local");
 
@@ -4857,6 +4863,8 @@ TEST_F(
   feature_list.InitWithFeatures(
       {syncer::kSyncEnableHistoryDataType, history::kSyncSegmentsData}, {});
 
+  backend_->SetCanAddForeignVisitsToSegments(true);
+
   SyncDeviceInfoMap sync_device_info = MakeSyncDeviceInfo({}, {}, "local");
   sync_device_info["foreign-invalid"] =
       std::make_pair(syncer::DeviceInfo::OsType::kAndroid,
@@ -4897,6 +4905,8 @@ TEST_F(
   feature_list.InitWithFeatures(
       {syncer::kSyncEnableHistoryDataType, history::kSyncSegmentsData}, {});
 
+  backend_->SetCanAddForeignVisitsToSegments(true);
+
   SyncDeviceInfoMap sync_device_info = MakeSyncDeviceInfo({"foreign"}, {});
   sync_device_info["local-invalid"] =
       std::make_pair(syncer::DeviceInfo::OsType::kIOS,
@@ -4936,6 +4946,8 @@ TEST_F(HistoryBackendTest,
   feature_list.InitWithFeatures(
       {syncer::kSyncEnableHistoryDataType, history::kSyncSegmentsData}, {});
 
+  backend_->SetCanAddForeignVisitsToSegments(true);
+
   SyncDeviceInfoMap sync_device_info = MakeSyncDeviceInfo({}, {});
   sync_device_info["foreign-invalid"] =
       std::make_pair(syncer::DeviceInfo::OsType::kAndroid,

diff --git a/components/history/core/browser/history_service.cc b/components/history/core/browser/history_service.cc
@@ -431,6 +431,17 @@ void HistoryService::SetDeviceInfoServices(
   SendLocalDeviceOriginatorCacheGuidToBackend();
 }
 
+void HistoryService::SetCanAddForeignVisitsToSegmentsOnBackend(
+    bool add_foreign_visits) {
+  CHECK(history::IsSyncSegmentsDataEnabled());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&HistoryBackend::SetCanAddForeignVisitsToSegments,
+                     history_backend_, add_foreign_visits));
+}
+
 void HistoryService::OnDeviceInfoChange() {
   CHECK(history::IsSyncSegmentsDataEnabled());
   CHECK(device_info_tracker_ != nullptr);

diff --git a/components/history/core/browser/history_service.h b/components/history/core/browser/history_service.h
@@ -665,6 +665,10 @@ class HistoryService : public KeyedService,
       syncer::DeviceInfoTracker* device_info_tracker,
       syncer::LocalDeviceInfoProvider* local_device_info_provider);
 
+  // Tells the `HistoryBackend` whether or not foreign history should be
+  // added to segments data.
+  void SetCanAddForeignVisitsToSegmentsOnBackend(bool add_foreign_visits);
+
   // syncer::DeviceInfoTracker::Observer overrides.
   void OnDeviceInfoChange() override;
 

diff --git a/ios/chrome/browser/prefs/browser_prefs.mm b/ios/chrome/browser/prefs/browser_prefs.mm
@@ -236,6 +236,9 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
   // Preferences related to tab grid.
   // Default to 0 which is the unassigned value.
   registry->RegisterIntegerPref(prefs::kInactiveTabsTimeThreshold, 0);
+
+  registry->RegisterIntegerPref(prefs::kIosSyncSegmentsNewTabPageDisplayCount,
+                                0);
 }
 
 void RegisterBrowserStatePrefs(user_prefs::PrefRegistrySyncable* registry) {

diff --git a/ios/chrome/browser/prefs/pref_names.cc b/ios/chrome/browser/prefs/pref_names.cc
@@ -173,6 +173,11 @@ const char kIosShareChromeCount[] = "ios.share_chrome.count";
 // Preference to store the last time the user shared the chrome app.
 const char kIosShareChromeLastShare[] = "ios.share_chrome.last_share";
 
+// Preference to store the number of times the user opens the New Tab Page
+// with foreign history included in segments data (i.e. Most Visited Tiles).
+const char kIosSyncSegmentsNewTabPageDisplayCount[] =
+    "ios.sync_segments.ntp.display_count";
+
 // Preference that hold a boolean indicating if the user has already dismissed
 // the sign-in promo in the ntp feed top section.
 const char kIosNtpFeedTopPromoAlreadySeen[] =

diff --git a/ios/chrome/browser/prefs/pref_names.h b/ios/chrome/browser/prefs/pref_names.h
@@ -38,6 +38,7 @@ extern const char kIosBookmarkSigninPromoDisplayedCount[];
 extern const char kIosBringAndroidTabsPromptDisplayed[];
 extern const char kIosShareChromeCount[];
 extern const char kIosShareChromeLastShare[];
+extern const char kIosSyncSegmentsNewTabPageDisplayCount[];
 extern const char kIosDiscoverFeedLastRefreshTime[];
 extern const char kIosDiscoverFeedLastUnseenRefreshTime[];
 extern const char kIosPasswordBottomSheetDismissCount[];

diff --git a/ios/chrome/browser/sync/sync_service_factory.mm b/ios/chrome/browser/sync/sync_service_factory.mm
@@ -14,6 +14,7 @@
 #import "components/keyed_service/core/service_access_type.h"
 #import "components/keyed_service/ios/browser_state_dependency_manager.h"
 #import "components/network_time/network_time_tracker.h"
+#import "components/prefs/pref_service.h"
 #import "components/sync/base/command_line_switches.h"
 #import "components/sync/base/sync_util.h"
 #import "components/sync/driver/sync_service.h"
@@ -34,6 +35,7 @@
 #import "ios/chrome/browser/history/history_service_factory.h"
 #import "ios/chrome/browser/passwords/ios_chrome_account_password_store_factory.h"
 #import "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
+#import "ios/chrome/browser/prefs/pref_names.h"
 #import "ios/chrome/browser/reading_list/reading_list_model_factory.h"
 #import "ios/chrome/browser/search_engines/template_url_service_factory.h"
 #import "ios/chrome/browser/signin/about_signin_internals_factory.h"
@@ -173,6 +175,17 @@
         DeviceInfoSyncServiceFactory::GetForBrowserState(browser_state);
 
     if (history_service && device_info_sync_service) {
+      PrefService* local_state = GetApplicationContext()->GetLocalState();
+      CHECK(local_state);
+
+      int display_count = local_state->GetInteger(
+          prefs::kIosSyncSegmentsNewTabPageDisplayCount);
+
+      int display_limit = history::kMaxNumNewTabPageDisplays.Get();
+
+      history_service->SetCanAddForeignVisitsToSegmentsOnBackend(display_count <
+                                                                 display_limit);
+
       history_service->SetDeviceInfoServices(
           device_info_sync_service->GetDeviceInfoTracker(),
           device_info_sync_service->GetLocalDeviceInfoProvider());

diff --git a/ios/chrome/browser/ui/content_suggestions/BUILD.gn b/ios/chrome/browser/ui/content_suggestions/BUILD.gn
@@ -49,6 +49,7 @@ source_set("content_suggestions") {
     "//components/favicon/ios",
     "//components/feed/core/shared_prefs:feed_shared_prefs",
     "//components/feed/core/v2/public/ios:feed_ios_public",
+    "//components/history/core/browser",
     "//components/ntp_tiles",
     "//components/pref_registry",
     "//components/prefs",

diff --git a/ios/chrome/browser/ui/content_suggestions/content_suggestions_mediator.mm b/ios/chrome/browser/ui/content_suggestions/content_suggestions_mediator.mm
@@ -15,6 +15,7 @@
 #import "base/strings/sys_string_conversions.h"
 #import "components/favicon/ios/web_favicon_driver.h"
 #import "components/feed/core/v2/public/ios/pref_names.h"
+#import "components/history/core/browser/features.h"
 #import "components/ntp_tiles/features.h"
 #import "components/ntp_tiles/metrics.h"
 #import "components/ntp_tiles/most_visited_sites.h"
@@ -450,6 +451,7 @@ - (void)onMostVisitedURLsAvailable:
 
   if (mostVisited.size() && !self.recordedPageImpression) {
     self.recordedPageImpression = YES;
+    [self recordMostVisitedTilesDisplayed];
     [self.faviconMediator setMostVisitedDataForLogging:mostVisited];
     ntp_tiles::metrics::RecordPageImpression(mostVisited.size());
   }
@@ -471,6 +473,22 @@ - (void)onIconMadeAvailable:(const GURL&)siteURL {
 
 #pragma mark - Private
 
+// Updates `prefs::kIosSyncSegmentsNewTabPageDisplayCount` with the number of
+// remaining New Tab Page displays that include synced history in the Most
+// Visited Tiles.
+- (void)recordMostVisitedTilesDisplayed {
+  PrefService* local_state = GetApplicationContext()->GetLocalState();
+
+  CHECK(local_state != nullptr);
+
+  const int displayCount =
+      local_state->GetInteger(prefs::kIosSyncSegmentsNewTabPageDisplayCount) +
+      1;
+
+  local_state->SetInteger(prefs::kIosSyncSegmentsNewTabPageDisplayCount,
+                          displayCount);
+}
+
 // Replaces the Most Visited items currently displayed by the most recent ones.
 - (void)useFreshMostVisited {
   if ([self shouldHideMVTForTileAblation]) {