DisplayConfigurator: Support async initial power state

In order to move DisplayPrefs to src/ash (necessary for mustash)
we need DisplayConfigurator to support asynchronous setting of
the initial power state since prefs are loaded asynchronously
in ash.

Bug: 678949
Change-Id: I404c63ce7fdab27eee45d30e44b41cc1fcf0d357
Reviewed-on: https://chromium-review.googlesource.com/994282
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549346}

chrome/browser/chromeos/display/display_prefs.cc

@@ -556,7 +556,7 @@ void StoreDisplayPowerState(PrefService* local_state,
 void StoreCurrentDisplayPowerState(PrefService* local_state) {
   StoreDisplayPowerState(
       local_state,
-      ash::Shell::Get()->display_configurator()->requested_power_state());
+      ash::Shell::Get()->display_configurator()->GetRequestedPowerState());
 }
 
 void StoreDisplayRotationPrefs(PrefService* local_state,

chrome/browser/chromeos/display/display_prefs_unittest.cc

@@ -230,6 +230,10 @@ class DisplayPrefsTest : public ash::AshTestBase {
         .ToString();
   }
 
+  chromeos::DisplayPowerState GetRequestedPowerState() const {
+    return ash::Shell::Get()->display_configurator()->GetRequestedPowerState();
+  }
+
   PrefService* local_state() { return &local_state_; }
   DisplayPrefs* display_prefs() { return display_prefs_.get(); }
 
@@ -263,8 +267,7 @@ TEST_F(DisplayPrefsTest, ListedLayoutOverrides) {
 
   display_prefs()->LoadDisplayPreferences(true);
   // DisplayPowerState should be ignored at boot.
-  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON,
-            shell->display_configurator()->requested_power_state());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON, GetRequestedPowerState());
 
   shell->display_manager()->UpdateDisplays();
   // Check if the layout settings are notified to the system properly.
@@ -776,22 +779,21 @@ TEST_F(DisplayPrefsTest, DisplayPowerStateAfterRestart) {
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON);
   display_prefs()->LoadDisplayPreferences(false);
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            ash::Shell::Get()->display_configurator()->requested_power_state());
+            GetRequestedPowerState());
 }
 
 TEST_F(DisplayPrefsTest, DontSaveAndRestoreAllOff) {
-  ash::Shell* shell = ash::Shell::Get();
   display_prefs()->StoreDisplayPowerStateForTest(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON);
   display_prefs()->LoadDisplayPreferences(false);
   // DisplayPowerState should be ignored at boot.
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            shell->display_configurator()->requested_power_state());
+            GetRequestedPowerState());
 
   display_prefs()->StoreDisplayPowerStateForTest(
       chromeos::DISPLAY_POWER_ALL_OFF);
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            shell->display_configurator()->requested_power_state());
+            GetRequestedPowerState());
   EXPECT_EQ("internal_off_external_on",
             local_state()->GetString(prefs::kDisplayPowerState));
 
@@ -799,7 +801,7 @@ TEST_F(DisplayPrefsTest, DontSaveAndRestoreAllOff) {
   local_state()->SetString(prefs::kDisplayPowerState, "all_off");
   display_prefs()->LoadDisplayPreferences(false);
   EXPECT_EQ(chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
-            shell->display_configurator()->requested_power_state());
+            GetRequestedPowerState());
 }
 
 // Tests that display configuration changes caused by TabletModeController

chrome/browser/ui/ash/chrome_shell_delegate.cc

@@ -156,6 +156,8 @@ void ChromeShellDelegate::PreInit() {
       chromeos::switches::kFirstExecAfterBoot);
   display_prefs_ = std::make_unique<chromeos::DisplayPrefs>(
       g_browser_process->local_state());
+  // TODO(stevenjb): Move this to ash::Shell which will call
+  // LoadDisplayPreferences asynchronously when it receives local state.
   display_prefs_->LoadDisplayPreferences(first_run_after_boot);
   // Object owns itself, and deletes itself when Observer::OnShutdown is called:
   new policy::DisplayRotationDefaultHandler();

ui/display/manager/chromeos/display_configurator.cc

@@ -504,7 +504,6 @@ DisplayConfigurator::DisplayConfigurator()
       current_display_state_(MULTIPLE_DISPLAY_STATE_INVALID),
       current_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
       requested_display_state_(MULTIPLE_DISPLAY_STATE_INVALID),
-      requested_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
       pending_power_state_(chromeos::DISPLAY_POWER_ALL_ON),
       has_pending_power_state_(false),
       pending_power_flags_(kSetDisplayPowerNoFlags),
@@ -547,9 +546,25 @@ void DisplayConfigurator::SetDelegateForTesting(
 
 void DisplayConfigurator::SetInitialDisplayPower(
     chromeos::DisplayPowerState power_state) {
-  DCHECK_EQ(current_display_state_, MULTIPLE_DISPLAY_STATE_INVALID);
-  requested_power_state_ = current_power_state_ = power_state;
-  NotifyPowerStateObservers();
+  if (requested_power_state_) {
+    // A new power state has alreday been requested so ignore the initial state.
+    return;
+  }
+
+  // Set the initial requested power state.
+  requested_power_state_ = power_state;
+
+  if (current_display_state_ == MULTIPLE_DISPLAY_STATE_INVALID) {
+    // DisplayConfigurator::OnConfigured has not been called yet so just set
+    // the current state and notify observers.
+    current_power_state_ = power_state;
+    NotifyPowerStateObservers();
+    return;
+  }
+
+  // DisplayConfigurator::OnConfigured has been called so update the current
+  // and pending states.
+  UpdatePowerState(power_state);
 }
 
 void DisplayConfigurator::Init(
@@ -591,9 +606,11 @@ void DisplayConfigurator::OnDisplayControlTaken(DisplayControlCallback callback,
   if (success) {
     // Force a configuration since the display configuration may have changed.
     force_configure_ = true;
-    // Restore the last power state used before releasing control.
-    SetDisplayPower(requested_power_state_, kSetDisplayPowerNoFlags,
-                    base::DoNothing());
+    if (requested_power_state_) {
+      // Restore the requested power state before releasing control.
+      SetDisplayPower(*requested_power_state_, kSetDisplayPowerNoFlags,
+                      base::DoNothing());
+    }
   }
 
   std::move(callback).Run(success);
@@ -668,8 +685,8 @@ void DisplayConfigurator::ForceInitialConfigure() {
 
   configuration_task_.reset(new UpdateDisplayConfigurationTask(
       native_display_delegate_.get(), layout_manager_.get(),
-      requested_display_state_, requested_power_state_,
-      kSetDisplayPowerForceProbe, true,
+      requested_display_state_, GetRequestedPowerState(),
+      kSetDisplayPowerForceProbe, /*force_configure=*/true,
       base::Bind(&DisplayConfigurator::OnConfigured,
                  weak_ptr_factory_.GetWeakPtr())));
   configuration_task_->Run();
@@ -867,6 +884,11 @@ bool DisplayConfigurator::SetColorCorrection(
   return false;
 }
 
+chromeos::DisplayPowerState DisplayConfigurator::GetRequestedPowerState()
+    const {
+  return requested_power_state_.value_or(chromeos::DISPLAY_POWER_ALL_ON);
+}
+
 void DisplayConfigurator::PrepareForExit() {
   configure_display_ = false;
 }
@@ -918,7 +940,7 @@ void DisplayConfigurator::SetDisplayPower(
           << (configure_timer_.IsRunning() ? "Running" : "Stopped");
 
   requested_power_state_ = power_state;
-  SetDisplayPowerInternal(requested_power_state_, flags, callback);
+  SetDisplayPowerInternal(*requested_power_state_, flags, callback);
 }
 
 void DisplayConfigurator::SetDisplayMode(MultipleDisplayState new_state) {
@@ -1016,8 +1038,10 @@ void DisplayConfigurator::ResumeDisplays() {
 
   // If requested_power_state_ is ALL_OFF due to idle suspend, powerd will turn
   // the display power on when it enables the backlight.
-  SetDisplayPower(requested_power_state_, kSetDisplayPowerNoFlags,
-                  base::DoNothing());
+  if (requested_power_state_) {
+    SetDisplayPower(*requested_power_state_, kSetDisplayPowerNoFlags,
+                    base::DoNothing());
+  }
 }
 
 void DisplayConfigurator::ConfigureDisplays() {
@@ -1072,19 +1096,8 @@ void DisplayConfigurator::OnConfigured(
 
   cached_displays_ = displays;
   if (success) {
-    chromeos::DisplayPowerState old_power_state = current_power_state_;
     current_display_state_ = new_display_state;
-    current_power_state_ = new_power_state;
-
-    // If the pending power state hasn't changed then make sure that value
-    // gets updated as well since the last requested value may have been
-    // dependent on certain conditions (ie: if only the internal monitor was
-    // present).
-    if (!has_pending_power_state_)
-      pending_power_state_ = new_power_state;
-
-    if (old_power_state != current_power_state_)
-      NotifyPowerStateObservers();
+    UpdatePowerState(new_power_state);
   }
 
   configuration_task_.reset();
@@ -1104,6 +1117,19 @@ void DisplayConfigurator::OnConfigured(
   }
 }
 
+void DisplayConfigurator::UpdatePowerState(
+    chromeos::DisplayPowerState new_power_state) {
+  chromeos::DisplayPowerState old_power_state = current_power_state_;
+  current_power_state_ = new_power_state;
+  // If the pending power state hasn't changed then make sure that value gets
+  // updated as well since the last requested value may have been dependent on
+  // certain conditions (ie: if only the internal monitor was present).
+  if (!has_pending_power_state_)
+    pending_power_state_ = new_power_state;
+  if (old_power_state != current_power_state_)
+    NotifyPowerStateObservers();
+}
+
 bool DisplayConfigurator::ShouldRunConfigurationTask() const {
   if (force_configure_)
     return true;

ui/display/manager/chromeos/display_configurator.h

@@ -16,6 +16,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "base/timer/timer.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "ui/display/manager/chromeos/query_content_protection_task.h"
@@ -179,9 +180,6 @@ class DISPLAY_MANAGER_EXPORT DisplayConfigurator
   ~DisplayConfigurator() override;
 
   MultipleDisplayState display_state() const { return current_display_state_; }
-  chromeos::DisplayPowerState requested_power_state() const {
-    return requested_power_state_;
-  }
   const std::vector<DisplaySnapshot*>& cached_displays() const {
     return cached_displays_;
   }
@@ -211,7 +209,11 @@ class DISPLAY_MANAGER_EXPORT DisplayConfigurator
   void SetDelegateForTesting(
       std::unique_ptr<NativeDisplayDelegate> display_delegate);
 
-  // Sets the initial value of |power_state_|.  Must be called before Start().
+  // Called asynchronously with the initial |power_state| loaded from prefs.
+  // This may be called after ForceInitialConfigure triggers a call to
+  // OnConfigured(), in which case UpdatePowerState() will be called with the
+  // correct initial value. Does nothing if |requested_power_state_| is set,
+  // e.g. via SetDisplayPower().
   void SetInitialDisplayPower(chromeos::DisplayPowerState power_state);
 
   // Initialization, must be called right after constructor.
@@ -289,6 +291,9 @@ class DISPLAY_MANAGER_EXPORT DisplayConfigurator
                           const std::vector<GammaRampRGBEntry>& gamma_lut,
                           const std::vector<float>& correction_matrix);
 
+  // Returns the requested power state if set or the default power state.
+  chromeos::DisplayPowerState GetRequestedPowerState() const;
+
   void set_is_multi_mirroring_enabled_for_test(bool enabled) {
     is_multi_mirroring_enabled_ = enabled;
   }
@@ -334,6 +339,9 @@ class DISPLAY_MANAGER_EXPORT DisplayConfigurator
                     MultipleDisplayState new_display_state,
                     chromeos::DisplayPowerState new_power_state);
 
+  // Updates the current and pending power state and notifies observers.
+  void UpdatePowerState(chromeos::DisplayPowerState new_power_state);
+
   // Helps in identifying if a configuration task needs to be scheduled.
   // Return true if any of the |requested_*| parameters have been updated. False
   // otherwise.
@@ -395,7 +403,7 @@ class DISPLAY_MANAGER_EXPORT DisplayConfigurator
   MultipleDisplayState requested_display_state_;
 
   // Stores the requested power state.
-  chromeos::DisplayPowerState requested_power_state_;
+  base::Optional<chromeos::DisplayPowerState> requested_power_state_;
 
   // The power state used by RunPendingConfiguration(). May be
   // |requested_power_state_| or DISPLAY_POWER_ALL_OFF for suspend.

ui/display/manager/chromeos/display_configurator_unittest.cc

@@ -686,6 +686,12 @@ TEST_F(DisplayConfiguratorTest, SetDisplayPower) {
 TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
   InitWithSingleOutput();
 
+  // Set the initial power state to on.
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+
   // No preparation is needed before suspending when the display is already
   // on.  The configurator should still reprobe on resume in case a display
   // was connected while suspended.
@@ -1406,6 +1412,14 @@ TEST_F(DisplayConfiguratorTest, DontRestoreStalePowerStateAfterResume) {
 TEST_F(DisplayConfiguratorTest, ExternalControl) {
   InitWithSingleOutput();
   state_controller_.set_state(MULTIPLE_DISPLAY_STATE_SINGLE);
+
+  // Set the initial power state and verify that it is restored when control is
+  // taken.
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+
   configurator_.RelinquishControl(
       base::BindOnce(&DisplayConfiguratorTest::OnDisplayControlUpdated,
                      base::Unretained(this)));
@@ -1652,6 +1666,13 @@ TEST_F(DisplayConfiguratorTest, TestWithThreeDisplays) {
 // Tests the suspend and resume behavior when in dual or multi display modes.
 TEST_F(DisplayConfiguratorTest, SuspendResumeWithMultipleDisplays) {
   InitWithSingleOutput();
+
+  // Set the initial power state and verify that it is restored on resume.
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+
   state_controller_.set_state(MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED);
   observer_.Reset();
   UpdateOutputs(2, true);