Only launch the Contained Shell when its preference is enabled.

At the moment the Contained Shell launches any time its feature flag is
enabled. But since we want users to toggle the experience from settings,
we added a pref to control it. This change actually enforces this new
pref by aways checking it when we try to launch the experience.

Bug: 922687
Change-Id: Icf1ffa2422bbb68fda80e22f626d48bcbe8c8c18
Reviewed-on: https://chromium-review.googlesource.com/c/1477821
Commit-Queue: Lucas Tenório <ltenorio@chromium.org>
Reviewed-by: Mitsuru Oshima (OOO til 3/4) <oshima@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636447}

apps/test/app_window_waiter.cc

@@ -4,6 +4,7 @@
 
 #include "apps/test/app_window_waiter.h"
 
+#include "base/task/post_task.h"
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/native_app_window.h"
 
@@ -25,7 +26,7 @@ extensions::AppWindow* AppWindowWaiter::Wait() {
     return window_;
 
   wait_type_ = WAIT_FOR_ADDED;
-  run_loop_.reset(new base::RunLoop);
+  run_loop_ = std::make_unique<base::RunLoop>();
   run_loop_->Run();
 
   return window_;
@@ -37,7 +38,21 @@ extensions::AppWindow* AppWindowWaiter::WaitForShown() {
     return window_;
 
   wait_type_ = WAIT_FOR_SHOWN;
-  run_loop_.reset(new base::RunLoop);
+  run_loop_ = std::make_unique<base::RunLoop>();
+  run_loop_->Run();
+
+  return window_;
+}
+
+extensions::AppWindow* AppWindowWaiter::WaitForShownWithTimeout(
+    base::TimeDelta timeout) {
+  window_ = registry_->GetCurrentAppWindowForApp(app_id_);
+  if (window_ && !window_->is_hidden())
+    return window_;
+
+  wait_type_ = WAIT_FOR_SHOWN;
+  run_loop_ = std::make_unique<base::RunLoop>();
+  base::PostDelayedTask(FROM_HERE, run_loop_->QuitClosure(), timeout);
   run_loop_->Run();
 
   return window_;
@@ -49,7 +64,7 @@ extensions::AppWindow* AppWindowWaiter::WaitForActivated() {
     return window_;
 
   wait_type_ = WAIT_FOR_ACTIVATED;
-  run_loop_.reset(new base::RunLoop);
+  run_loop_ = std::make_unique<base::RunLoop>();
   run_loop_->Run();
 
   return window_;

apps/test/app_window_waiter.h

@@ -34,6 +34,10 @@ class AppWindowWaiter : public extensions::AppWindowRegistry::Observer {
   // Waits for an AppWindow of the app to be shown.
   extensions::AppWindow* WaitForShown();
 
+  // Waits for an AppWindow of the app to be shown or returns nullptr if the
+  // given timeout expires.
+  extensions::AppWindow* WaitForShownWithTimeout(base::TimeDelta timeout);
+
   // Waits for an AppWindow of the app to be activated.
   extensions::AppWindow* WaitForActivated();
 

ash/contained_shell/contained_shell_controller.cc

@@ -10,6 +10,7 @@
 #include "ash/shell.h"
 #include "components/account_id/account_id.h"
 #include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 
 #include <utility>
 
@@ -32,12 +33,22 @@ void ContainedShellController::BindRequest(
 }
 
 bool ContainedShellController::IsEnabled() {
-  // TODO(ltenorio): Also check the ash.contained_shell.enabled pref here when
-  // it's available.
-  return base::FeatureList::IsEnabled(features::kContainedShell);
+  if (!base::FeatureList::IsEnabled(features::kContainedShell))
+    return false;
+
+  PrefService* prefs =
+      Shell::Get()->session_controller()->GetPrimaryUserPrefService();
+
+  DCHECK(prefs) << "PrefService should not be null when reading Contained "
+                   "Shell pref. This usually happens when calling "
+                   "ContainedShellController::IsEnabled() before sign in.";
+  return prefs->GetBoolean(prefs::kContainedShellEnabled);
 }
 
-void ContainedShellController::LaunchContainedShell() {
+void ContainedShellController::LaunchContainedShellIfEnabled() {
+  if (!IsEnabled())
+    return;
+
   contained_shell_client_->LaunchContainedShell(Shell::Get()
                                                     ->session_controller()
                                                     ->GetPrimaryUserSession()

ash/contained_shell/contained_shell_controller.h

@@ -33,9 +33,13 @@ class ASH_EXPORT ContainedShellController
   // Returns if the Contained Shell is enabled for the current user.
   bool IsEnabled();
 
-  // Starts the ContainedShell feature by sending LaunchContainedShell
-  // request to ContainedShellClient.
-  void LaunchContainedShell();
+  // Tries to start the Contained Shell feature by sending a
+  // LaunchContainedShell command to the ContainedShellClient. We will only
+  // launch if |IsEnabled()| is true, so it's safe to call this every time a
+  // successful sign in happens.
+  // Warning: This method should not be called before sign in since the prefs
+  // would not be initialized.
+  void LaunchContainedShellIfEnabled();
 
   // mojom::ContainedShellController:
   void SetClient(mojom::ContainedShellClientPtr client) override;

ash/shell.cc

@@ -1431,10 +1431,8 @@ void Shell::OnSessionStateChanged(session_manager::SessionState state) {
   // the controller when the session is active. https://crbug.com/464118
   drag_drop_controller_->set_enabled(is_session_active);
 
-  if (base::FeatureList::IsEnabled(features::kContainedShell) &&
-      is_session_active) {
-    contained_shell_controller_->LaunchContainedShell();
-  }
+  if (is_session_active)
+    contained_shell_controller_->LaunchContainedShellIfEnabled();
 }
 
 void Shell::OnLoginStatusChanged(LoginStatus login_status) {

chrome/browser/chromeos/login/login_utils_browsertest.cc

@@ -11,6 +11,7 @@
 #include "base/run_loop.h"
 #include "base/scoped_observer.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/test_timeouts.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/screens/gaia_view.h"
@@ -41,6 +42,7 @@
 
 #if defined(GOOGLE_CHROME_BUILD)
 #include "apps/test/app_window_waiter.h"
+#include "ash/public/cpp/ash_pref_names.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -99,6 +101,24 @@ class LoginUtilsContainedShellTest : public LoginUtilsTest {
     LoginUtilsTest::SetUp();
   }
 
+  void LoginAndSetContainedShellPref(bool contained_shell_pref_value) {
+    extensions::ComponentLoader::EnableBackgroundExtensionsForTesting();
+
+    WaitForSigninScreen();
+
+    Login("username");
+
+    // Take some time to finish login and register user prefs.
+    base::RunLoop().RunUntilIdle();
+
+    // Update the now registered Contained Shell pref.
+    ProfileHelper::Get()
+        ->GetProfileByUser(user_manager::UserManager::Get()->GetActiveUser())
+        ->GetPrefs()
+        ->SetBoolean(ash::prefs::kContainedShellEnabled,
+                     contained_shell_pref_value);
+  }
+
  private:
   base::test::ScopedFeatureList feature_list_;
 };
@@ -138,8 +158,12 @@ class FullscreenWindowEnvObserver : public aura::EnvObserver,
   DISALLOW_COPY_AND_ASSIGN(FullscreenWindowEnvObserver);
 };
 
+IN_PROC_BROWSER_TEST_F(LoginUtilsContainedShellTest, PRE_ContainedShellLaunch) {
+  LoginAndSetContainedShellPref(true);
+}
+
 // Checks that the Contained Experience window is launched on sign-in when the
-// feature is enabled.
+// feature is enabled and its pref allows it.
 IN_PROC_BROWSER_TEST_F(LoginUtilsContainedShellTest, ContainedShellLaunch) {
   // Enable all component extensions.
   extensions::ComponentLoader::EnableBackgroundExtensionsForTesting();
@@ -150,17 +174,40 @@ IN_PROC_BROWSER_TEST_F(LoginUtilsContainedShellTest, ContainedShellLaunch) {
 
   Login("username");
 
-  base::RunLoop().RunUntilIdle();
-
   // Wait for the app to launch before verifying it is fullscreen.
   apps::AppWindowWaiter waiter(
       extensions::AppWindowRegistry::Get(ProfileHelper::Get()->GetProfileByUser(
           user_manager::UserManager::Get()->GetActiveUser())),
       extension_misc::kContainedHomeAppId);
-  waiter.WaitForShown();
-
+  EXPECT_NE(nullptr,
+            waiter.WaitForShownWithTimeout(TestTimeouts::action_timeout()));
   EXPECT_TRUE(fullscreen_observer.did_fullscreen_window_launch());
 }
+
+IN_PROC_BROWSER_TEST_F(LoginUtilsContainedShellTest,
+                       PRE_ContainedShellDoesntLaunchWhenPrefIsDisabled) {
+  LoginAndSetContainedShellPref(false);
+}
+
+// Checks that the Contained Experience window does not launch in sign-in when
+// its pref is disabled
+IN_PROC_BROWSER_TEST_F(LoginUtilsContainedShellTest,
+                       ContainedShellDoesntLaunchWhenPrefIsDisabled) {
+  // Enable all component extensions.
+  extensions::ComponentLoader::EnableBackgroundExtensionsForTesting();
+
+  WaitForSigninScreen();
+
+  Login("username");
+
+  // Wait for the app to launch before verifying that it didn't.
+  apps::AppWindowWaiter waiter(
+      extensions::AppWindowRegistry::Get(ProfileHelper::Get()->GetProfileByUser(
+          user_manager::UserManager::Get()->GetActiveUser())),
+      extension_misc::kContainedHomeAppId);
+  EXPECT_EQ(nullptr,
+            waiter.WaitForShownWithTimeout(TestTimeouts::action_timeout()));
+}
 #endif  // defined(GOOGLE_CHROME_BUILD)
 
 // Exercises login, like the desktopui_MashLogin Chrome OS autotest.