Pass initial display prefs from Chrome to ash::Shell

The solution to this was kind of subtle. Here are some notes for posterity:

* ash::Shell::local_state_ (the device PrefService used by Ash) is
  loaded asynchronously, after Shell::Init.
* Display preferences need to be initialized synchronously; there are
  currently too many dependencies to make asynchronous initialization
  robust.
* Thus for 'classic' mode, we need to provide initial display prefs from
  the synchronously loaded browser local state prefs to ash::Shell::Init.
* Registering prefs has to occur early (before feature flags are loaded),
  so using different registration for classic/mus/mash would be
  complicated. Instead we always register display prefs in chrome and
  treat them as foreign prefs in ash (except for tests).
* Because of the complications of pref registration, accessing a second,
  synchronously loaded, PrefService instance in Ash would be difficult.
  Instead we extract a base::Value dictionary of display prefs and pass
  that in ShellInitParams.

This fixes 'classic' mode, but does not solve the problem for mus/mash
(which has never worked). Potential long term solutions:
(a) Store display prefs in an Ash specific value store that is
    synchronously loaded (along with other similar prefs).
(b) Initialize Shell::local_state_ with the initial state before the Mash
    process forks (solving any registration issues this entails).
(c) Make initial display configuration work asynchronously.

Bug: 834775
Change-Id: I75e561394c817e7c93d8c724bdadcaa37ef868af
Reviewed-on: https://chromium-review.googlesource.com/1028479
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555348}

ash/display/display_prefs.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 #include <array>
+#include <memory>
 
 #include "ash/ash_export.h"
 #include "ash/shell_observer.h"
@@ -15,9 +16,14 @@
 #include "ui/display/display.h"
 #include "ui/display/display_layout.h"
 
+class PrefRegistry;
 class PrefRegistrySimple;
 class PrefService;
 
+namespace base {
+class Value;
+}
+
 namespace gfx {
 class Point;
 }
@@ -35,10 +41,15 @@ class DisplayPrefsTest;
 // for the session.
 class ASH_EXPORT DisplayPrefs : public ShellObserver {
  public:
+  // Returns a dictionary of display pref values stored in PrefService.
+  // See chrome/browser/ui/ash/ash_shell_init.cc for details.
+  static std::unique_ptr<base::Value> GetInitialDisplayPrefsFromPrefService(
+      PrefService* pref_service);
   // Registers the prefs associated with display settings.
   static void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
+  static void RegisterForeignPrefs(PrefRegistry* registry);
 
-  DisplayPrefs();
+  explicit DisplayPrefs(std::unique_ptr<base::Value> initial_prefs);
   ~DisplayPrefs() override;
 
   // ShellObserver
@@ -69,24 +80,21 @@ class ASH_EXPORT DisplayPrefs : public ShellObserver {
   void StoreDisplayMixedMirrorModeParamsForTest(
       const base::Optional<display::MixedMirrorModeParams>& mixed_params);
 
-  void set_local_state_for_test(PrefService* local_state) {
-    local_state_ = local_state;
-  }
+  // Class wrapping a PrefService and a dictionary of initial pref values to use
+  // until OnLocalStatePrefServiceInitialized is called.
+  class LocalState;
 
  protected:
   friend class DisplayPrefsTest;
 
-  // Loads display preferences from |local_state| and sets |local_state_|.
-  // |first_run_after_boot| is used to determine whether power state preferences
-  // should be applied.
-  void LoadDisplayPreferences(bool first_run_after_boot,
-                              PrefService* local_state);
+  // Loads display preferences from |local_state_|.
+  void LoadDisplayPreferences();
+
+  // Constructs a LocalState instance with |pref_service| for testing.
+  void SetPrefServiceForTest(PrefService* pref_service);
 
  private:
-  // Set in LoadDisplayPreferences once the local pref store is available.
-  // While null, StoreDisplayPrefs just sets |store_requested_| and prefs are
-  // stored when |local_state_| is set.
-  PrefService* local_state_ = nullptr;
+  std::unique_ptr<LocalState> local_state_;
   bool store_requested_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(DisplayPrefs);

ash/display/display_prefs_unittest.cc

@@ -28,6 +28,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
+#include "chromeos/chromeos_switches.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/user_manager/user_type.h"
@@ -123,7 +124,7 @@ class DisplayPrefsTest : public AshTestBase {
     disable_provide_local_state();
     AshTestBase::SetUp();
     DisplayPrefs::RegisterLocalStatePrefs(local_state_.registry());
-    display_prefs()->set_local_state_for_test(&local_state_);
+    display_prefs()->SetPrefServiceForTest(&local_state_);
     observer_ = std::make_unique<DisplayConfigurationObserver>();
     observer_->OnDisplaysInitialized();
   }
@@ -137,10 +138,7 @@ class DisplayPrefsTest : public AshTestBase {
 
   void LoggedInAsGuest() { SimulateGuestLogin(); }
 
-  void LoadDisplayPreferences(bool first_run_after_boot) {
-    display_prefs()->LoadDisplayPreferences(first_run_after_boot,
-                                            local_state());
-  }
+  void LoadDisplayPreferences() { display_prefs()->LoadDisplayPreferences(); }
 
   // Do not use the implementation of display_prefs.cc directly to avoid
   // notifying the update to the system.
@@ -273,19 +271,21 @@ TEST_F(DisplayPrefsTest, ListedLayoutOverrides) {
   display_prefs()->StoreDisplayPowerStateForTest(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON);
 
-  ash::Shell* shell = ash::Shell::Get();
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      chromeos::switches::kFirstExecAfterBoot);
+  LoadDisplayPreferences();
 
-  LoadDisplayPreferences(true);
   // DisplayPowerState should be ignored at boot.
   EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON, GetRequestedPowerState());
 
-  shell->display_manager()->UpdateDisplays();
+  Shell::Get()->display_manager()->UpdateDisplays();
   // Check if the layout settings are notified to the system properly.
   // The new layout overrides old layout.
   // Inverted one of for specified pair (id1, id2).  Not used for the list
   // (id1, dummy_id) since dummy_id is not connected right now.
   EXPECT_EQ("id=2200000001, parent=2200000000, top, 20",
-            shell->display_manager()
+            Shell::Get()
+                ->display_manager()
                 ->GetCurrentDisplayLayout()
                 .placement_list[0]
                 .ToString());
@@ -802,15 +802,15 @@ TEST_F(DisplayPrefsTest, StorePowerStateNormalUser) {
 TEST_F(DisplayPrefsTest, DisplayPowerStateAfterRestart) {
   display_prefs()->StoreDisplayPowerStateForTest(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
             GetRequestedPowerState());
 }
 
 TEST_F(DisplayPrefsTest, DontSaveAndRestoreAllOff) {
   display_prefs()->StoreDisplayPowerStateForTest(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
   // DisplayPowerState should be ignored at boot.
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
             GetRequestedPowerState());
@@ -824,7 +824,7 @@ TEST_F(DisplayPrefsTest, DontSaveAndRestoreAllOff) {
 
   // Don't try to load
   local_state()->SetString(prefs::kDisplayPowerState, "all_off");
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
             GetRequestedPowerState());
 }
@@ -978,7 +978,7 @@ TEST_F(DisplayPrefsTest, LoadRotationNoLogin) {
 
   display_prefs()->StoreDisplayRotationPrefsForTest(display::Display::ROTATE_90,
                                                     true);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
 
   bool display_rotation_lock =
       display_manager()->registered_internal_display_rotation_lock();
@@ -1105,7 +1105,7 @@ TEST_F(DisplayPrefsTest, RestoreUnifiedMode) {
   StoreDisplayPropertyForList(
       list, "primary-id",
       std::make_unique<base::Value>(base::Int64ToString(first_display_id)));
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
 
   // Should not restore to unified unless unified desktop is enabled.
   display_info_list.emplace_back(second_display_info);
@@ -1115,7 +1115,7 @@ TEST_F(DisplayPrefsTest, RestoreUnifiedMode) {
   // Restored to unified.
   display_manager()->SetUnifiedDesktopEnabled(true);
   StoreDisplayBoolPropertyForList(list, "default_unified", true);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
   display_manager()->OnNativeDisplaysChanged(display_info_list);
   EXPECT_TRUE(display_manager()->IsInUnifiedMode());
 
@@ -1130,7 +1130,7 @@ TEST_F(DisplayPrefsTest, RestoreUnifiedMode) {
       display::GetDisplayIdWithoutOutputIndex(second_display_id));
   StoreExternalDisplayMirrorInfo(external_display_mirror_info);
   StoreDisplayBoolPropertyForList(list, "default_unified", true);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
   display_info_list.emplace_back(second_display_info);
   display_manager()->OnNativeDisplaysChanged(display_info_list);
   EXPECT_TRUE(display_manager()->IsInMirrorMode());
@@ -1147,7 +1147,7 @@ TEST_F(DisplayPrefsTest, RestoreUnifiedMode) {
   external_display_mirror_info.clear();
   StoreExternalDisplayMirrorInfo(external_display_mirror_info);
   StoreDisplayBoolPropertyForList(list, "default_unified", false);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
   display_info_list.emplace_back(second_display_info);
   display_manager()->OnNativeDisplaysChanged(display_info_list);
   EXPECT_FALSE(display_manager()->IsInMirrorMode());
@@ -1187,7 +1187,7 @@ TEST_F(DisplayPrefsTest, RestoreThreeDisplays) {
   builder.AddDisplayPlacement(list[2], list[1],
                               display::DisplayPlacement::BOTTOM, 100);
   display_prefs()->StoreDisplayLayoutPrefForTest(list, *builder.Build());
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
 
   UpdateDisplay("200x200,200x200,300x300");
   display::DisplayIdList new_list =
@@ -1268,6 +1268,9 @@ TEST_F(DisplayPrefsTest, LegacyTouchCalibrationDataSupport) {
 TEST_F(DisplayPrefsTest, ExternalDisplayMirrorInfo) {
   LoggedInAsUser();
 
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      chromeos::switches::kFirstExecAfterBoot);
+
   const int64_t internal_display_id =
       display::test::DisplayManagerTestApi(display_manager())
           .SetFirstDisplayAsInternalDisplay();
@@ -1292,7 +1295,7 @@ TEST_F(DisplayPrefsTest, ExternalDisplayMirrorInfo) {
   std::set<int64_t> external_display_mirror_info;
   external_display_mirror_info.emplace(first_display_masked_id);
   StoreExternalDisplayMirrorInfo(external_display_mirror_info);
-  LoadDisplayPreferences(true);
+  LoadDisplayPreferences();
   const base::ListValue* pref_external_display_mirror_info =
       local_state()->GetList(prefs::kExternalDisplayMirrorInfo);
   EXPECT_EQ(1U, pref_external_display_mirror_info->GetSize());
@@ -1332,7 +1335,7 @@ TEST_F(DisplayPrefsTest, ExternalDisplayMirrorInfo) {
   external_display_mirror_info.clear();
   external_display_mirror_info.emplace(second_display_masked_id);
   StoreExternalDisplayMirrorInfo(external_display_mirror_info);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
   pref_external_display_mirror_info =
       local_state()->GetList(prefs::kExternalDisplayMirrorInfo);
   EXPECT_EQ(1U, pref_external_display_mirror_info->GetSize());
@@ -1383,7 +1386,7 @@ TEST_F(DisplayPrefsTest, DisplayMixedMirrorMode) {
   base::Optional<display::MixedMirrorModeParams> mixed_params(
       base::in_place, internal_display_id, dst_ids);
   display_prefs()->StoreDisplayMixedMirrorModeParamsForTest(mixed_params);
-  LoadDisplayPreferences(false);
+  LoadDisplayPreferences();
 
   // Connect both first and second external display. Mixed mirror mode is
   // restored.

ash/shell.cc

@@ -269,7 +269,8 @@ Shell* Shell::CreateInstance(ShellInitParams init_params) {
   instance_ = new Shell(std::move(init_params.delegate),
                         std::move(init_params.shell_port));
   instance_->Init(init_params.context_factory,
-                  init_params.context_factory_private);
+                  init_params.context_factory_private,
+                  std::move(init_params.initial_display_prefs));
   return instance_;
 }
 
@@ -403,12 +404,18 @@ bool Shell::ShouldUseIMEService() {
 }
 
 // static
-void Shell::RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
-  DisplayPrefs::RegisterLocalStatePrefs(registry);
+void Shell::RegisterLocalStatePrefs(PrefRegistrySimple* registry,
+                                    bool for_test) {
   PaletteTray::RegisterLocalStatePrefs(registry);
   WallpaperController::RegisterLocalStatePrefs(registry);
   BluetoothPowerController::RegisterLocalStatePrefs(registry);
   DetachableBaseHandler::RegisterPrefs(registry);
+  // Note: DisplayPrefs are registered in chrome in AshShellInit::RegisterPrefs
+  // (see comment there for details).
+  if (for_test)
+    DisplayPrefs::RegisterLocalStatePrefs(registry);
+  else
+    DisplayPrefs::RegisterForeignPrefs(registry);
 }
 
 // static
@@ -943,7 +950,8 @@ Shell::~Shell() {
 }
 
 void Shell::Init(ui::ContextFactory* context_factory,
-                 ui::ContextFactoryPrivate* context_factory_private) {
+                 ui::ContextFactoryPrivate* context_factory_private,
+                 std::unique_ptr<base::Value> initial_display_prefs) {
   const Config config = shell_port_->GetAshConfig();
 
   // This creates the MessageCenter object which is used by some other objects
@@ -974,7 +982,7 @@ void Shell::Init(ui::ContextFactory* context_factory,
     // connecting to the profile pref service. The login screen has a temporary
     // user profile that is not associated with a real user.
     auto pref_registry = base::MakeRefCounted<PrefRegistrySimple>();
-    RegisterLocalStatePrefs(pref_registry.get());
+    RegisterLocalStatePrefs(pref_registry.get(), false);
     prefs::ConnectToPrefService(
         shell_delegate_->GetShellConnector(), std::move(pref_registry),
         base::Bind(&Shell::OnLocalStatePrefServiceInitialized,
@@ -1015,6 +1023,13 @@ void Shell::Init(ui::ContextFactory* context_factory,
 
   shell_delegate_->PreInit();
 
+  // In CLASSIC mode, |initial_display_prefs| contains the synchronously
+  // loaded display pref values. Otherwise |initial_display_prefs| is null and
+  // the pref values will be loaded once |local_state_| is available. (Any store
+  // requests in the meanwhile will be queued).
+  display_prefs_ =
+      std::make_unique<DisplayPrefs>(std::move(initial_display_prefs));
+
   InitializeDisplayManager();
 
   if (config == Config::CLASSIC) {
@@ -1284,11 +1299,6 @@ void Shell::Init(ui::ContextFactory* context_factory,
 }
 
 void Shell::InitializeDisplayManager() {
-  // Construct DisplayPrefs here so that display_prefs()->StoreDisplayPrefs()
-  // can safely be called. DisplayPrefs will be loaded once |local_state_|
-  // is available and store requests will be queued in the meanwhile.
-  display_prefs_ = std::make_unique<DisplayPrefs>();
-
   const Config config = shell_port_->GetAshConfig();
   bool display_initialized = display_manager_->InitFromCommandLine();
 

ash/shell.h

@@ -277,7 +277,8 @@ class ASH_EXPORT Shell : public SessionObserver,
   static bool ShouldUseIMEService();
 
   // Registers all ash related local state prefs to the given |registry|.
-  static void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
+  static void RegisterLocalStatePrefs(PrefRegistrySimple* registry,
+                                      bool for_test);
 
   // Registers all ash related signin/user profile prefs to the given
   // |registry|. Can be called before Shell is initialized. When |for_test| is
@@ -646,7 +647,8 @@ class ASH_EXPORT Shell : public SessionObserver,
   ~Shell() override;
 
   void Init(ui::ContextFactory* context_factory,
-            ui::ContextFactoryPrivate* context_factory_private);
+            ui::ContextFactoryPrivate* context_factory_private,
+            std::unique_ptr<base::Value> initial_display_prefs);
 
   // Initializes the display manager and related components.
   void InitializeDisplayManager();

ash/shell_init_params.h

@@ -9,6 +9,10 @@
 
 #include "ash/ash_export.h"
 
+namespace base {
+class Value;
+}
+
 namespace ui {
 class ContextFactory;
 class ContextFactoryPrivate;
@@ -28,6 +32,9 @@ struct ASH_EXPORT ShellInitParams {
   std::unique_ptr<ShellDelegate> delegate;
   ui::ContextFactory* context_factory = nullptr;                 // Non-owning.
   ui::ContextFactoryPrivate* context_factory_private = nullptr;  // Non-owning.
+  // Dictionary of pref values used by DisplayPrefs before
+  // ShellObserver::OnLocalStatePrefServiceInitialized is called.
+  std::unique_ptr<base::Value> initial_display_prefs;
 };
 
 }  // namespace ash

ash/shell_unittest.cc

@@ -563,7 +563,7 @@ TEST_F(ShellLocalStateTest, LocalState) {
   // Prefs service wrapper code creates a PrefService.
   std::unique_ptr<TestingPrefServiceSimple> local_state =
       std::make_unique<TestingPrefServiceSimple>();
-  Shell::RegisterLocalStatePrefs(local_state->registry());
+  Shell::RegisterLocalStatePrefs(local_state->registry(), true);
   TestingPrefServiceSimple* local_state_ptr = local_state.get();
   ShellTestApi().OnLocalStatePrefServiceInitialized(std::move(local_state));
   EXPECT_EQ(local_state_ptr, observer.last_local_state_);

ash/test/ash_test_helper.cc

@@ -183,7 +183,7 @@ void AshTestHelper::SetUp(bool start_session, bool provide_local_state) {
 
   if (provide_local_state) {
     auto pref_service = std::make_unique<TestingPrefServiceSimple>();
-    Shell::RegisterLocalStatePrefs(pref_service->registry());
+    Shell::RegisterLocalStatePrefs(pref_service->registry(), true);
     Shell::Get()->OnLocalStatePrefServiceInitialized(std::move(pref_service));
   }
 

ash/wallpaper/wallpaper_controller_unittest.cc

@@ -2270,7 +2270,7 @@ TEST_F(WallpaperControllerDisableLocalStateTest, IgnoreShowUserWallpaper) {
   // the wallpaper, and |OnReadyToSetWallpaper| is invoked.
   std::unique_ptr<TestingPrefServiceSimple> local_state =
       std::make_unique<TestingPrefServiceSimple>();
-  Shell::RegisterLocalStatePrefs(local_state->registry());
+  Shell::RegisterLocalStatePrefs(local_state->registry(), true);
   ShellTestApi().OnLocalStatePrefServiceInitialized(std::move(local_state));
 
   controller_->ShowUserWallpaper(InitializeUser(account_id_1));