Simplify implementation of Profile Selection with Guest Profile

Since the approach is different than the System Profile, we will not be
able to perform the same experiment that allows us to change the default
value (for creation of the service or not) directly on all factories.

Therefore, removed `kGuestProfileSelectionDefaultNone` feature flag.

Default value for Guest Profile is now `ProfileSelection::kNone`.
All existing factories kept the same old behavior by using experimental
Profile Selection builders that have a `force_guest` value to force
the value to be the same as the one for the Regular Profile, which was the previous default behavior.
For existing factories that use explicit `ProfileSelections::Builder` but relied on the previous default value we do the following (`WithGuest() not set):
- if Regular Profile value is overwritten: explicitly set the same value for Guest Profile.
- if Regular Profile value is not set, explicitly set the Regular Profile default value `ProfileSelections::kRegularProfileDefault` for the Guest Profile.

Future factories should not use experimental builders and rely on
explicitly stating the behavior for the Guest profile if different than
`ProfileSelection::kNone`.

Transition to stop creating existing services for the Guest Profile can
start by setting the force_guest value to false on experimental builders.
Potentially a feature flag can be introduced for services where the
decision is not clear.


Bug: 1284664
Change-Id: I032244842a155208064e12a0e8261137a8fb5ddc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4235132
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Ryan Sultanem <rsult@google.com>
Cr-Commit-Position: refs/heads/main@{#1104984}

chrome/browser/ash/app_restore/full_restore_service_factory.cc

@@ -47,11 +47,13 @@ FullRestoreService* FullRestoreServiceFactory::GetForProfile(Profile* profile) {
 }
 
 FullRestoreServiceFactory::FullRestoreServiceFactory()
-    : ProfileKeyedServiceFactory("FullRestoreService",
-                                 ProfileSelections::Builder()
-                                     .WithSystem(ProfileSelection::kNone)
-                                     .WithAshInternals(ProfileSelection::kNone)
-                                     .Build()) {
+    : ProfileKeyedServiceFactory(
+          "FullRestoreService",
+          ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
+              .WithSystem(ProfileSelection::kNone)
+              .WithAshInternals(ProfileSelection::kNone)
+              .Build()) {
   DependsOn(NotificationDisplayServiceFactory::GetInstance());
   DependsOn(apps::AppServiceProxyFactory::GetInstance());
 }

chrome/browser/ash/bruschetta/bruschetta_service_factory.cc

@@ -27,12 +27,14 @@ BruschettaServiceFactory* BruschettaServiceFactory::GetInstance() {
 }
 
 BruschettaServiceFactory::BruschettaServiceFactory()
-    : ProfileKeyedServiceFactory("BruschettaService",
-                                 // Takes care of not creating the service for
-                                 // OTR and non user profiles.
-                                 ProfileSelections::Builder()
-                                     .WithAshInternals(ProfileSelection::kNone)
-                                     .Build()) {}
+    : ProfileKeyedServiceFactory(
+          "BruschettaService",
+          // Takes care of not creating the service for OTR and non user
+          // profiles.
+          ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
+              .WithAshInternals(ProfileSelection::kNone)
+              .Build()) {}
 
 BruschettaServiceFactory::~BruschettaServiceFactory() = default;
 

chrome/browser/ash/cert_provisioning/cert_provisioning_scheduler_user_service.cc

@@ -43,10 +43,12 @@ CertProvisioningSchedulerUserServiceFactory::GetInstance() {
 
 CertProvisioningSchedulerUserServiceFactory::
     CertProvisioningSchedulerUserServiceFactory()
-    : ProfileKeyedServiceFactory("CertProvisioningSchedulerUserService",
-                                 ProfileSelections::Builder()
-                                     .WithAshInternals(ProfileSelection::kNone)
-                                     .Build()) {
+    : ProfileKeyedServiceFactory(
+          "CertProvisioningSchedulerUserService",
+          ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
+              .WithAshInternals(ProfileSelection::kNone)
+              .Build()) {
   DependsOn(platform_keys::PlatformKeysServiceFactory::GetInstance());
   DependsOn(invalidation::ProfileInvalidationProviderFactory::GetInstance());
 }

chrome/browser/ash/ownership/owner_settings_service_ash_factory.cc

@@ -33,10 +33,12 @@ DeviceSettingsService* GetDeviceSettingsService() {
 }  // namespace
 
 OwnerSettingsServiceAshFactory::OwnerSettingsServiceAshFactory()
-    : ProfileKeyedServiceFactory("OwnerSettingsService",
-                                 ProfileSelections::Builder()
-                                     .WithAshInternals(ProfileSelection::kNone)
-                                     .Build()) {}
+    : ProfileKeyedServiceFactory(
+          "OwnerSettingsService",
+          ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
+              .WithAshInternals(ProfileSelection::kNone)
+              .Build()) {}
 
 OwnerSettingsServiceAshFactory::~OwnerSettingsServiceAshFactory() = default;
 

chrome/browser/ash/platform_keys/key_permissions/user_private_token_kpm_service_factory.cc

@@ -62,10 +62,12 @@ UserPrivateTokenKeyPermissionsManagerServiceFactory::GetInstance() {
 
 UserPrivateTokenKeyPermissionsManagerServiceFactory::
     UserPrivateTokenKeyPermissionsManagerServiceFactory()
-    : ProfileKeyedServiceFactory("UserPrivateTokenKeyPermissionsManagerService",
-                                 ProfileSelections::Builder()
-                                     .WithAshInternals(ProfileSelection::kNone)
-                                     .Build()) {
+    : ProfileKeyedServiceFactory(
+          "UserPrivateTokenKeyPermissionsManagerService",
+          ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
+              .WithAshInternals(ProfileSelection::kNone)
+              .Build()) {
   DependsOn(PlatformKeysServiceFactory::GetInstance());
 }
 

chrome/browser/ash/printing/history/print_job_history_service_factory.cc

@@ -32,14 +32,15 @@ PrintJobHistoryServiceFactory* PrintJobHistoryServiceFactory::GetInstance() {
 }
 
 PrintJobHistoryServiceFactory::PrintJobHistoryServiceFactory()
-    : ProfileKeyedServiceFactory("PrintJobHistoryService",
-                                 ProfileSelections::Builder()
-                                     // We do not want an instance of
-                                     // PrintJobHistory on the lock screen.  The
-                                     // result is multiple print job
-                                     // notifications. https://crbug.com/1011532
-                                     .WithAshInternals(ProfileSelection::kNone)
-                                     .Build()) {
+    : ProfileKeyedServiceFactory(
+          "PrintJobHistoryService",
+          ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
+              // We do not want an instance of PrintJobHistory on the lock
+              // screen.  The result is multiple print job notifications.
+              // https://crbug.com/1011532
+              .WithAshInternals(ProfileSelection::kNone)
+              .Build()) {
   DependsOn(CupsPrintJobManagerFactory::GetInstance());
   DependsOn(PrintJobReportingServiceFactory::GetInstance());
 }

chrome/browser/ash/printing/print_management/printing_manager_factory.cc

@@ -33,6 +33,8 @@ PrintingManagerFactory::PrintingManagerFactory()
           "PrintingManager",
           ProfileSelections::Builder()
               .WithRegular(ProfileSelection::kRedirectedToOriginal)
+              // Guest Profile follows Regular Profile selection mode.
+              .WithGuest(ProfileSelection::kRedirectedToOriginal)
               // We do not want an instance of PrintingManager on the lock
               // screen. The result is multiple print job notifications.
               // https://crbug.com/1011532

chrome/browser/ash/remote_apps/remote_apps_manager_factory.cc

@@ -30,11 +30,13 @@ RemoteAppsManagerFactory* RemoteAppsManagerFactory::GetInstance() {
 }
 
 RemoteAppsManagerFactory::RemoteAppsManagerFactory()
-    : ProfileKeyedServiceFactory("RemoteAppsManager",
-                                 ProfileSelections::Builder()
-                                     .WithSystem(ProfileSelection::kNone)
-                                     .WithAshInternals(ProfileSelection::kNone)
-                                     .Build()) {
+    : ProfileKeyedServiceFactory(
+          "RemoteAppsManager",
+          ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
+              .WithSystem(ProfileSelection::kNone)
+              .WithAshInternals(ProfileSelection::kNone)
+              .Build()) {
   DependsOn(app_list::AppListSyncableServiceFactory::GetInstance());
   DependsOn(apps::AppServiceProxyFactory::GetInstance());
   DependsOn(extensions::EventRouterFactory::GetInstance());

chrome/browser/ash/scanning/lorgnette_scanner_manager_factory.cc

@@ -29,6 +29,7 @@ LorgnetteScannerManagerFactory::LorgnetteScannerManagerFactory()
     : ProfileKeyedServiceFactory(
           "LorgnetteScannerManager",
           ProfileSelections::Builder()
+              .WithGuest(ProfileSelections::kRegularProfileDefault)
               .WithAshInternals(ProfileSelection::kNone)
               // Prevent an instance of LorgnetteScannerManager from being
               // created on the lock screen.

chrome/browser/ash/scanning/scan_service_factory.cc

@@ -50,6 +50,8 @@ ScanServiceFactory::ScanServiceFactory()
           "ScanService",
           ProfileSelections::Builder()
               .WithRegular(ProfileSelection::kRedirectedToOriginal)
+              // Guest Profile follows Regular Profile selection mode.
+              .WithGuest(ProfileSelection::kRedirectedToOriginal)
               // Prevent an instance of ScanService from being created on the
               // lock screen.
               .WithAshInternals(ProfileSelection::kNone)

chrome/browser/enterprise/connectors/reporting/realtime_reporting_client_factory.cc

@@ -30,6 +30,8 @@ RealtimeReportingClientFactory::RealtimeReportingClientFactory()
           "RealtimeReportingClient",
           ProfileSelections::Builder()
               .WithRegular(ProfileSelection::kOwnInstance)
+              // Guest Profile follows Regular Profile selection mode.
+              .WithGuest(ProfileSelection::kOwnInstance)
               .WithSystem(ProfileSelection::kNone)
               .Build()) {
   DependsOn(

chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_event_router_factory.cc

@@ -33,6 +33,8 @@ SafeBrowsingPrivateEventRouterFactory::SafeBrowsingPrivateEventRouterFactory()
           "SafeBrowsingPrivateEventRouter",
           ProfileSelections::Builder()
               .WithRegular(ProfileSelection::kOwnInstance)
+              // Guest Profile follows Regular Profile selection mode.
+              .WithGuest(ProfileSelection::kOwnInstance)
               .WithSystem(ProfileSelection::kNone)
               .Build()) {
   DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());

chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.cc

@@ -35,6 +35,8 @@ OptimizationGuideKeyedServiceFactory::OptimizationGuideKeyedServiceFactory()
           "OptimizationGuideKeyedService",
           ProfileSelections::Builder()
               .WithRegular(ProfileSelection::kOwnInstance)
+              // Guest Profile follows Regular Profile selection mode.
+              .WithGuest(ProfileSelection::kOwnInstance)
               // Do not build the OptimizationGuideKeyedService if it's a
               // sign-in or lockscreen profile since it basically is an
               // ephemeral profile anyway and we cannot provide hints or models

chrome/browser/policy/networking/user_network_configuration_updater_factory.cc

@@ -41,6 +41,8 @@ UserNetworkConfigurationUpdaterFactory::UserNetworkConfigurationUpdaterFactory()
           "UserNetworkConfigurationUpdater",
           ProfileSelections::Builder()
               .WithRegular(ProfileSelection::kRedirectedToOriginal)
+              // Guest Profile follows Regular Profile selection mode.
+              .WithGuest(ProfileSelection::kRedirectedToOriginal)
               // On the login/lock screen only device network policies apply.
               .WithAshInternals(ProfileSelection::kNone)
               .Build()) {

chrome/browser/profiles/profile_keyed_service_factory.md

@@ -127,36 +127,48 @@ recommended that any value that needs to be set (different that the default one)
 to be explicitly stated, and not creating a new static predefined builder for
 that.
 
-## Experiments to change default behavior for irregular Profiles
+## Experiments and default behaviors for irregular Profiles
 
 Given that a lot of existing keyed services are created for non regular profiles
 because of the previous default behavior, it is a complex and risky transition
 to do when trying to remove or add services for some profiles.
 
-The target of the experiments is  on the System Profile and on the Guest
-Profile. Analysis showed that most services should not be needed on the System
-Profile, whereas more services are needed for the Guest Profile. The following
-experiments were then created to affect the default `ProfileSelection` of those
-profile types (if no value is explicitly set):
+- System Profile:
 
-- `kSystemProfileSelectionDefaultNone`: When enabled, this feature flag changes
+The target of the experiment is on the System Profile. Analysis showed that most
+services should not be needed on the System Profile. The following experiment
+goal is to default the System Profile `ProfileSelection` to
+`ProfileSelection::kNone` so that no services will be created, unless explicitly
+stated.
+
+`kSystemProfileSelectionDefaultNone`: When enabled, this feature flag changes
 the default `ProfileSelection` for the System Profile to be `kNone`. When the
 feature flag is disabled, the previous behavior remains: which is following the
 same behavior as the Regular Profile. This approach considers that no keyed
 service is needed unless proven otherwise.
-- `kGuestProfileSelectionDefaultNone`: When enabled, this feature flag changes
-the default `ProfileSelection` for the Guest Profile to be `kNone`. When the
-feature flag is disabled, the previous behavior remains: which is following the
-same behavior as the Regular Profile. However, most values for the Guest
-profiles are expected to be forced (set explicitly) to be the same as the
-regular profile when the experiment is active, and the rollout will be done the
-other way around, removing the forced values when possible. This approach
-considers that all keyed services are needed unless proven otherwise (trying to
-remove the forced values).
-
-### Experiments status
+Force values on experimental builders can be used to easily bypass the
+experiment. If set to true, enforce the value set to the Regular Profile to also
+affect the System Profile so that the experiment condition is bypassed.
+
+- Guest Profile:
+
+For Guest Profile type, analysis showed that more services are actually needed,
+and we have no generic or easy way to determine which ones are needed.
+Therefore we cannot use the same approach with the same kind of exeperiment on
+affecting default value directly.
+
+However, work has been done to turn the default value to
+`ProfileSelection::kNone`, but keeping all existing factories have the same
+behavior, most of the time following the value set to the Regular Profile (which
+was the old default behavior), by using a specific set of "experimental"
+`ProfileSelection::Builder` with force values. This way future factories have to
+explicitly state the fact they have to construct services for the Guest Profile
+(preferably not using the experimental builders).
+By default all existing force values in experimental builders are set to true,
+setting them to false will stop creating the service for the Guest Profile.
+
+### Experiment status
 - `kSystemProfileSelectionDefaultNone`: Experiment started: [Bug](crbug/1376461).
-- `kGuestProfileSelectionDefaultNone`: Experiment not started yet.
 
 ### Future work
 It seems that the Ash internal profiles are very similar to the System Profile,

chrome/browser/profiles/profile_keyed_service_factory_unittest.cc

@@ -69,7 +69,7 @@ class RefcountedProfileKeyedServiceFactoryTest
 // `kSystemProfileSelectionDefaultNone` experiment.
 class ProfileKeyedServiceFactoryUnittest
     : public testing::Test,
-      public ::testing::WithParamInterface<std::tuple<bool, bool>> {
+      public ::testing::WithParamInterface<bool> {
  public:
   void SetUp() override {
     testing::Test::SetUp();
@@ -78,16 +78,12 @@ class ProfileKeyedServiceFactoryUnittest
     // TODO(rsult): move the below code to be in the
     // `ProfileSelectionsTestWithParams` constructor, once the System and Guest
     // Profiles can be created with the experiment activated.
-    bool activate_system_experiment = std::get<0>(GetParam());
-    bool activate_guest_experiment = std::get<1>(GetParam());
+    bool activate_system_experiment = GetParam();
     std::vector<base::test::FeatureRef> enabled_features;
     std::vector<base::test::FeatureRef> disabled_features;
     activate_system_experiment
         ? enabled_features.push_back(kSystemProfileSelectionDefaultNone)
         : disabled_features.push_back(kSystemProfileSelectionDefaultNone);
-    activate_guest_experiment
-        ? enabled_features.push_back(kGuestProfileSelectionDefaultNone)
-        : disabled_features.push_back(kGuestProfileSelectionDefaultNone);
     feature_list_.InitWithFeatures(enabled_features, disabled_features);
   }
 
@@ -104,10 +100,6 @@ class ProfileKeyedServiceFactoryUnittest
     return base::FeatureList::IsEnabled(kSystemProfileSelectionDefaultNone);
   }
 
-  bool IsGuestExperimentActive() const {
-    return base::FeatureList::IsEnabled(kGuestProfileSelectionDefaultNone);
-  }
-
   TestingProfile* regular_profile() {
     return profile_testing_helper_.regular_profile();
   }
@@ -147,9 +139,6 @@ TEST_P(ProfileKeyedServiceFactoryUnittest, DefaultFactoryTest) {
   TestProfileToUse(factory, regular_profile(), regular_profile());
   TestProfileToUse(factory, incognito_profile(), nullptr);
 
-  // Since force_guest = true by default and the value is not overridden by the
-  // ProfileSelections created, the profile selected will not depend on the
-  // `kGuestProfileSelectionDefaultNone` experiment.
   TestProfileToUse(factory, guest_profile(), guest_profile());
   TestProfileToUse(factory, guest_profile_otr(), nullptr);
 
@@ -165,7 +154,8 @@ TEST_P(ProfileKeyedServiceFactoryUnittest, DefaultFactoryTest) {
 class PredefinedProfileSelectionsFactoryTest
     : public ProfileKeyedServiceFactoryTest {
  public:
-  // Simulates the normal default value for Guest Profile.
+  // Simulates the normal default value for Guest Profile. Guest Profile will
+  // then follow the behavior of the Regular Profile.
   explicit PredefinedProfileSelectionsFactoryTest(bool force_guest = true)
       : ProfileKeyedServiceFactoryTest(
             "PredefinedProfileSelectionsFactoryTest",
@@ -178,9 +168,6 @@ TEST_P(ProfileKeyedServiceFactoryUnittest,
   TestProfileToUse(factory, regular_profile(), regular_profile());
   TestProfileToUse(factory, incognito_profile(), regular_profile());
 
-  // Since force_guest = true by default and the value is not overridden by the
-  // ProfileSelections created, the profile selected will not depend on the
-  // `kGuestProfileSelectionDefaultNone` experiment.
   TestProfileToUse(factory, guest_profile(), guest_profile());
   TestProfileToUse(factory, guest_profile_otr(), guest_profile());
 
@@ -199,11 +186,8 @@ TEST_P(ProfileKeyedServiceFactoryUnittest,
   TestProfileToUse(factory, regular_profile(), regular_profile());
   TestProfileToUse(factory, incognito_profile(), regular_profile());
 
-  bool guest_experiment = IsGuestExperimentActive();
-  TestProfileToUse(factory, guest_profile(),
-                   guest_experiment ? nullptr : guest_profile());
-  TestProfileToUse(factory, guest_profile_otr(),
-                   guest_experiment ? nullptr : guest_profile());
+  TestProfileToUse(factory, guest_profile(), nullptr);
+  TestProfileToUse(factory, guest_profile_otr(), nullptr);
 
 #if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_ANDROID)
   bool system_experiment = IsSystemExperimentActive();
@@ -258,9 +242,6 @@ TEST_P(ProfileKeyedServiceFactoryUnittest, DefaultRefcountedFactoryTest) {
   TestProfileToUse(factory, regular_profile(), regular_profile());
   TestProfileToUse(factory, incognito_profile(), nullptr);
 
-  // Since force_guest = true by default and the value is not overridden by the
-  // ProfileSelections created, the profile selected will not depend on the
-  // `kGuestProfileSelectionDefaultNone` experiment.
   TestProfileToUse(factory, guest_profile(), guest_profile());
   TestProfileToUse(factory, guest_profile_otr(), nullptr);
 
@@ -288,9 +269,6 @@ TEST_P(ProfileKeyedServiceFactoryUnittest,
   TestProfileToUse(factory, regular_profile(), regular_profile());
   TestProfileToUse(factory, incognito_profile(), incognito_profile());
 
-  // Since force_guest = true by default and the value is not overridden by the
-  // ProfileSelections created, the profile selected will not depend on the
-  // `kGuestProfileSelectionDefaultNone` experiment.
   TestProfileToUse(factory, guest_profile(), guest_profile());
   TestProfileToUse(factory, guest_profile_otr(), guest_profile_otr());
 
@@ -305,5 +283,4 @@ TEST_P(ProfileKeyedServiceFactoryUnittest,
 
 INSTANTIATE_TEST_SUITE_P(ExperimentalProfileKeyedServiceFactory,
                          ProfileKeyedServiceFactoryUnittest,
-                         ::testing::Combine(::testing::Bool(),
-                                            ::testing::Bool()));
+                         ::testing::Bool());