lacros: Introduce IsLacrosEnabled to check feature, user, and channel

In most places in the code we check both for the base::Feature
LacrosSupport and browser_util::IsLacrosAllowed(). In a recent code
review I forgot one. Move the feature check into IsLacrosAllowed()
and rename the function to IsLacrosEnabled().

This requires a small change to BrowserManager which also fixes a
bug. If enterprise policy switches from allowing Lacros to not
allowing Lacros we now clean up the binary in the component updater.
Previously the binary would stay there forever.

Follow-up to code review comment in:
https://chromium-review.googlesource.com/c/chromium/src/+/2542678

Bug: none
Test: manual, login with enterprise device, toggle flag
Test: added to unit_tests

Change-Id: I3938e6fed4af7831f41f947e9a30f03000f75cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547782
Auto-Submit: James Cook <jamescook@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829028}

chrome/browser/apps/app_service/app_service_proxy.cc

@@ -37,7 +37,6 @@
 #include "chrome/browser/chromeos/crosapi/browser_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/supervised_user/grit/supervised_user_unscaled_resources.h"
-#include "chromeos/constants/chromeos_features.h"
 #include "components/user_manager/user.h"
 #include "extensions/common/constants.h"
 #endif
@@ -183,8 +182,7 @@ void AppServiceProxy::Initialize() {
     // Lacros does not support multi-signin, so only create for the primary
     // profile. This also avoids creating an instance for the lock screen app
     // profile and ensures there is only one instance of LacrosApps.
-    if (chromeos::features::IsLacrosSupportEnabled() &&
-        crosapi::browser_util::IsLacrosAllowed() &&
+    if (crosapi::browser_util::IsLacrosEnabled() &&
         chromeos::ProfileHelper::IsPrimaryProfile(profile_)) {
       lacros_apps_ = std::make_unique<LacrosApps>(app_service_);
     }

chrome/browser/apps/app_service/lacros_apps.cc

@@ -12,9 +12,9 @@
 #include "chrome/browser/apps/app_service/app_icon_factory.h"
 #include "chrome/browser/apps/app_service/menu_util.h"
 #include "chrome/browser/chromeos/crosapi/browser_manager.h"
+#include "chrome/browser/chromeos/crosapi/browser_util.h"
 #include "chrome/grit/chrome_unscaled_resources.h"
 #include "chrome/grit/generated_resources.h"
-#include "chromeos/constants/chromeos_features.h"
 #include "components/services/app_service/public/mojom/types.mojom.h"
 #include "extensions/common/constants.h"
 
@@ -22,7 +22,7 @@ namespace apps {
 
 LacrosApps::LacrosApps(
     const mojo::Remote<apps::mojom::AppService>& app_service) {
-  DCHECK(chromeos::features::IsLacrosSupportEnabled());
+  DCHECK(crosapi::browser_util::IsLacrosEnabled());
   PublisherBase::Initialize(app_service, apps::mojom::AppType::kLacros);
 }
 

chrome/browser/chromeos/crosapi/browser_loader.cc

@@ -52,7 +52,7 @@ BrowserLoader::BrowserLoader(
 BrowserLoader::~BrowserLoader() = default;
 
 void BrowserLoader::Load(LoadCompletionCallback callback) {
-  DCHECK(browser_util::IsLacrosAllowed());
+  DCHECK(browser_util::IsLacrosEnabled());
 
   // TODO(crbug.com/1078607): Remove non-error logging from this class.
   LOG(WARNING) << "Starting lacros component load.";
@@ -78,7 +78,7 @@ void BrowserLoader::Load(LoadCompletionCallback callback) {
 }
 
 void BrowserLoader::Unload() {
-  DCHECK(browser_util::IsLacrosAllowed());
+  // Can be called even if Lacros isn't enabled, to clean up the old install.
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {base::MayBlock()},
       base::BindOnce(&CheckInstalledAndMaybeRemoveUserDirectory,

chrome/browser/chromeos/crosapi/browser_manager.cc

@@ -37,7 +37,6 @@
 #include "chrome/browser/component_updater/cros_component_manager.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
-#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
 #include "components/session_manager/core/session_manager.h"
@@ -203,7 +202,7 @@ void BrowserManager::SetLoadCompleteCallback(LoadCompleteCallback callback) {
 }
 
 void BrowserManager::NewWindow() {
-  if (!browser_util::IsLacrosAllowed())
+  if (!browser_util::IsLacrosEnabled())
     return;
 
   if (!IsReady()) {
@@ -438,17 +437,15 @@ void BrowserManager::OnSessionStateChanged() {
   // Ensure this isn't run multiple times.
   session_manager::SessionManager::Get()->RemoveObserver(this);
 
-  // Must be checked after user session start because it depends on user type.
-  if (!browser_util::IsLacrosAllowed())
-    return;
-
   // May be null in tests.
   if (!component_manager_)
     return;
 
   DCHECK(!browser_loader_);
   browser_loader_ = std::make_unique<BrowserLoader>(component_manager_);
-  if (chromeos::features::IsLacrosSupportEnabled()) {
+
+  // Must be checked after user session start because it depends on user type.
+  if (browser_util::IsLacrosEnabled()) {
     state_ = State::LOADING;
     browser_loader_->Load(base::BindOnce(&BrowserManager::OnLoadComplete,
                                          weak_factory_.GetWeakPtr()));

chrome/browser/chromeos/crosapi/browser_util.cc

@@ -21,6 +21,7 @@
 #include "chrome/common/channel_info.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/crosapi/cpp/crosapi_constants.h"
 #include "chromeos/crosapi/mojom/crosapi.mojom.h"
 #include "components/exo/shell_surface_util.h"
@@ -107,11 +108,14 @@ base::FilePath GetUserDataDir() {
   return base_path.Append("lacros");
 }
 
-bool IsLacrosAllowed() {
-  return IsLacrosAllowed(chrome::GetChannel());
+bool IsLacrosEnabled() {
+  return IsLacrosEnabled(chrome::GetChannel());
 }
 
-bool IsLacrosAllowed(Channel channel) {
+bool IsLacrosEnabled(Channel channel) {
+  if (!base::FeatureList::IsEnabled(chromeos::features::kLacrosSupport))
+    return false;
+
   const User* user = user_manager::UserManager::Get()->GetPrimaryUser();
   if (!user)
     return false;

chrome/browser/chromeos/crosapi/browser_util.h

@@ -44,12 +44,12 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry);
 // Returns the user directory for lacros-chrome.
 base::FilePath GetUserDataDir();
 
-// Returns true if lacros is allowed for the current user type, chrome channel,
-// etc.
-bool IsLacrosAllowed();
+// Returns true if the Lacros feature is enabled and Lacros is allowed for the
+// current user type, chrome channel, and enterprise policy.
+bool IsLacrosEnabled();
 
 // As above, but takes a channel. Exposed for testing.
-bool IsLacrosAllowed(version_info::Channel channel);
+bool IsLacrosEnabled(version_info::Channel channel);
 
 // Returns true if |window| is an exo ShellSurface window representing a Lacros
 // browser.

chrome/browser/chromeos/crosapi/browser_util_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/crosapi/browser_util.h"
 
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -13,6 +14,7 @@
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/crosapi/mojom/crosapi.mojom.h"
 #include "components/account_id/account_id.h"
 #include "components/user_manager/scoped_user_manager.h"
@@ -56,41 +58,61 @@ class LacrosUtilTest : public testing::Test {
   ScopedTestingLocalState local_state_;
 };
 
+TEST_F(LacrosUtilTest, LacrosEnabledByFlag) {
+  AddRegularUser("user@test.com");
+
+  // Lacros is disabled because the feature isn't enabled by default.
+  EXPECT_FALSE(browser_util::IsLacrosEnabled());
+
+  // Enabling the flag enables Lacros.
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kLacrosSupport);
+  EXPECT_TRUE(browser_util::IsLacrosEnabled());
+}
+
 TEST_F(LacrosUtilTest, ChannelTest) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kLacrosSupport);
   AddRegularUser("user@test.com");
 
-  EXPECT_TRUE(browser_util::IsLacrosAllowed(Channel::UNKNOWN));
-  EXPECT_TRUE(browser_util::IsLacrosAllowed(Channel::CANARY));
-  EXPECT_TRUE(browser_util::IsLacrosAllowed(Channel::DEV));
-  EXPECT_TRUE(browser_util::IsLacrosAllowed(Channel::BETA));
-  EXPECT_FALSE(browser_util::IsLacrosAllowed(Channel::STABLE));
+  EXPECT_TRUE(browser_util::IsLacrosEnabled(Channel::UNKNOWN));
+  EXPECT_TRUE(browser_util::IsLacrosEnabled(Channel::CANARY));
+  EXPECT_TRUE(browser_util::IsLacrosEnabled(Channel::DEV));
+  EXPECT_TRUE(browser_util::IsLacrosEnabled(Channel::BETA));
+  EXPECT_FALSE(browser_util::IsLacrosEnabled(Channel::STABLE));
 }
 
-TEST_F(LacrosUtilTest, ManagedAccountLacrosAllowed) {
+TEST_F(LacrosUtilTest, ManagedAccountLacrosEnabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kLacrosSupport);
   AddRegularUser("user@managedchrome.com");
   testing_profile_.GetProfilePolicyConnector()->OverrideIsManagedForTesting(
       true);
   g_browser_process->local_state()->SetBoolean(prefs::kLacrosAllowed, true);
 
-  EXPECT_TRUE(browser_util::IsLacrosAllowed(Channel::CANARY));
+  EXPECT_TRUE(browser_util::IsLacrosEnabled(Channel::CANARY));
 }
 
-TEST_F(LacrosUtilTest, ManagedAccountLacrosNotAllowed) {
+TEST_F(LacrosUtilTest, ManagedAccountLacrosDisabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kLacrosSupport);
   AddRegularUser("user@managedchrome.com");
   testing_profile_.GetProfilePolicyConnector()->OverrideIsManagedForTesting(
       true);
   g_browser_process->local_state()->SetBoolean(prefs::kLacrosAllowed, false);
 
-  EXPECT_FALSE(browser_util::IsLacrosAllowed(Channel::CANARY));
+  EXPECT_FALSE(browser_util::IsLacrosEnabled(Channel::CANARY));
 }
 
 TEST_F(LacrosUtilTest, BlockedForChildUser) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kLacrosSupport);
   AccountId account_id = AccountId::FromUserEmail("user@test.com");
   const User* user = fake_user_manager_->AddChildUser(account_id);
   fake_user_manager_->UserLoggedIn(account_id, user->username_hash(),
                                    /*browser_restart=*/false,
                                    /*is_child=*/true);
-  EXPECT_FALSE(browser_util::IsLacrosAllowed(Channel::UNKNOWN));
+  EXPECT_FALSE(browser_util::IsLacrosEnabled(Channel::UNKNOWN));
 }
 
 TEST_F(LacrosUtilTest, GetInterfaceVersions) {

chrome/browser/feedback/system_logs/chrome_system_logs_fetcher.cc

@@ -27,7 +27,6 @@
 #include "chrome/browser/chromeos/system_logs/shill_log_source.h"
 #include "chrome/browser/chromeos/system_logs/touch_log_source.h"
 #include "chrome/browser/chromeos/system_logs/ui_hierarchy_log_source.h"
-#include "chromeos/constants/chromeos_features.h"
 #endif
 
 #if defined(OS_CHROMEOS) || BUILDFLAG(IS_LACROS)
@@ -82,8 +81,7 @@ SystemLogsFetcher* BuildChromeSystemLogsFetcher(bool scrub_data) {
 #endif
 
 #if defined(OS_CHROMEOS)
-  if (chromeos::features::IsLacrosSupportEnabled() &&
-      crosapi::browser_util::IsLacrosAllowed()) {
+  if (crosapi::browser_util::IsLacrosEnabled()) {
     fetcher->AddSource(std::make_unique<UserLogFilesLogSource>(
         base::FilePath(kDefaultLogPath), kLacrosUserLogKey));
   }

chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc

@@ -51,7 +51,6 @@
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/metrics/chromeos_metrics_provider.h"
 #include "chrome/browser/metrics/enrollment_status.h"
-#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/dbus/util/version_loader.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/system/statistics_provider.h"
@@ -257,8 +256,7 @@ std::string GetChromeVersionString() {
   // is indicated by |lacros_version| in BrowserManager being set to non-empty
   // string during lacros startup, attach its version in the chrome
   // version string.
-  if (chromeos::features::IsLacrosSupportEnabled() &&
-      crosapi::browser_util::IsLacrosAllowed() &&
+  if (crosapi::browser_util::IsLacrosEnabled() &&
       !crosapi::BrowserManager::Get()->lacros_version().empty()) {
     std::string lacros_version =
         crosapi::BrowserManager::Get()->lacros_version();

chrome/browser/ui/ash/chrome_launcher_prefs.cc

@@ -615,8 +615,7 @@ std::vector<ash::ShelfID> GetPinnedAppsFromSync(
   // If Lacros is enabled and allowed for this user type, ensure the Lacros icon
   // is pinned. Lacros doesn't support multi-signin, so only add the icon for
   // the primary user.
-  if (chromeos::features::IsLacrosSupportEnabled() &&
-      crosapi::browser_util::IsLacrosAllowed() &&
+  if (crosapi::browser_util::IsLacrosEnabled() &&
       chromeos::ProfileHelper::IsPrimaryProfile(helper->profile())) {
     syncer::StringOrdinal lacros_position =
         syncable_service->GetPinPosition(extension_misc::kLacrosAppId);

chromeos/constants/chromeos_features.cc

@@ -398,6 +398,10 @@ const base::Feature kKerberosSettingsSection{"KerberosSettingsSection",
 // Enables "Linux and Chrome OS" support. Allows a Linux version of Chrome
 // ("lacros-chrome") to run as a Wayland client with this instance of Chrome
 // ("ash-chrome") acting as the Wayland server and window manager.
+// NOTE: Use crosapi::browser_util::IsLacrosEnabled() instead of checking the
+// feature directly. Lacros is not allowed for certain user types and can be
+// disabled by policy. These restrictions will be lifted when Lacros development
+// is complete.
 const base::Feature kLacrosSupport{"LacrosSupport",
                                    base::FEATURE_DISABLED_BY_DEFAULT};
 
@@ -713,10 +717,6 @@ bool IsKerberosSettingsSectionEnabled() {
   return base::FeatureList::IsEnabled(kKerberosSettingsSection);
 }
 
-bool IsLacrosSupportEnabled() {
-  return base::FeatureList::IsEnabled(kLacrosSupport);
-}
-
 bool IsLoginDeviceManagementDisclosureEnabled() {
   return base::FeatureList::IsEnabled(kLoginDeviceManagementDisclosure);
 }

chromeos/constants/chromeos_features.h

@@ -318,7 +318,6 @@ COMPONENT_EXPORT(CHROMEOS_CONSTANTS) bool IsImeSandboxEnabled();
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
 bool IsInstantTetheringBackgroundAdvertisingSupported();
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS) bool IsKerberosSettingsSectionEnabled();
-COMPONENT_EXPORT(CHROMEOS_CONSTANTS) bool IsLacrosSupportEnabled();
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
 bool IsLoginDeviceManagementDisclosureEnabled();
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS) bool IsLoginDisplayPasswordButtonEnabled();