Wait for display configuration to finish before suspending

If the suspend doesn't wait, the display configuration query will race with the
suspend call. It may be possible to suspend before the configuration query
finishes. That would mean that we'd be reading display configuration at
resume.

BUG=477433

Review URL: https://codereview.chromium.org/1130823003

Cr-Commit-Position: refs/heads/master@{#328612}

ash/system/chromeos/power/power_event_observer.cc

@@ -37,6 +37,11 @@ void ResumeRenderingRequests() {
     window->GetHost()->compositor()->SetVisible(true);
 }
 
+void OnSuspendDisplaysCompleted(const base::Closure& suspend_callback,
+                                bool status) {
+  suspend_callback.Run();
+}
+
 }  // namespace
 
 PowerEventObserver::PowerEventObserver()
@@ -113,7 +118,10 @@ void PowerEventObserver::SuspendImminent() {
   }
 
   ui::UserActivityDetector::Get()->OnDisplayPowerChanging();
-  shell->display_configurator()->SuspendDisplays();
+  shell->display_configurator()->SuspendDisplays(base::Bind(
+      &OnSuspendDisplaysCompleted, chromeos::DBusThreadManager::Get()
+                                       ->GetPowerManagerClient()
+                                       ->GetSuspendReadinessCallback()));
 }
 
 void PowerEventObserver::SuspendDone(const base::TimeDelta& sleep_duration) {

ui/display/chromeos/display_configurator.cc

@@ -854,21 +854,23 @@ void DisplayConfigurator::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void DisplayConfigurator::SuspendDisplays() {
+void DisplayConfigurator::SuspendDisplays(
+    const ConfigurationCallback& callback) {
   // If the display is off due to user inactivity and there's only a single
   // internal display connected, switch to the all-on state before
   // suspending.  This shouldn't be very noticeable to the user since the
   // backlight is off at this point, and doing this lets us resume directly
   // into the "on" state, which greatly reduces resume times.
   if (requested_power_state_ == chromeos::DISPLAY_POWER_ALL_OFF) {
     SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
-                    kSetDisplayPowerOnlyIfSingleInternalDisplay,
-                    base::Bind(&DoNothing));
+                    kSetDisplayPowerOnlyIfSingleInternalDisplay, callback);
 
     // We need to make sure that the monitor configuration we just did actually
     // completes before we return, because otherwise the X message could be
     // racing with the HandleSuspendReadiness message.
     native_display_delegate_->SyncWithServer();
+  } else {
+    callback.Run(true);
   }
 
   displays_suspended_ = true;

ui/display/chromeos/display_configurator.h

@@ -224,8 +224,9 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
 
   // Sets all the displays into pre-suspend mode; usually this means
   // configure them for their resume state. This allows faster resume on
-  // machines where display configuration is slow.
-  void SuspendDisplays();
+  // machines where display configuration is slow. On completion of the display
+  // configuration |callback| is executed.
+  void SuspendDisplays(const ConfigurationCallback& callback);
 
   // Reprobes displays to handle changes made while the system was
   // suspended.

ui/display/chromeos/display_configurator_unittest.cc

@@ -659,7 +659,9 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
   // was connected while suspended.
   const gfx::Size framebuffer_size = configurator_.framebuffer_size();
   DCHECK(!framebuffer_size.IsEmpty());
-  configurator_.SuspendDisplays();
+  configurator_.SuspendDisplays(base::Bind(
+      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(framebuffer_size.ToString(),
             configurator_.framebuffer_size().ToString());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
@@ -692,7 +694,9 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
           NULL),
       log_->GetActionsAndClear());
 
-  configurator_.SuspendDisplays();
+  configurator_.SuspendDisplays(base::Bind(
+      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -747,7 +751,9 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
                   NULL),
       log_->GetActionsAndClear());
 
-  configurator_.SuspendDisplays();
+  configurator_.SuspendDisplays(base::Bind(
+      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(JoinActions(kGrab, kUngrab, kSync, NULL),
             log_->GetActionsAndClear());
 
@@ -1020,7 +1026,9 @@ TEST_F(DisplayConfiguratorTest, DoNotConfigureWithSuspendedDisplays) {
   // after the displays have been suspended.  This event should be ignored since
   // the DisplayConfigurator will force a probe and reconfiguration of displays
   // at resume time.
-  configurator_.SuspendDisplays();
+  configurator_.SuspendDisplays(base::Bind(
+      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
   // The configuration timer should not be started when the displays
@@ -1091,7 +1099,9 @@ TEST_F(DisplayConfiguratorTest, DoNotConfigureWithSuspendedDisplays) {
   // If a configuration task is pending when the displays are suspended, that
   // task should not run either and the timer should be stopped.
   configurator_.OnConfigurationChanged();
-  configurator_.SuspendDisplays();
+  configurator_.SuspendDisplays(base::Bind(
+      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
   EXPECT_FALSE(test_api_.TriggerConfigureTimeout());
@@ -1380,7 +1390,9 @@ TEST_F(DisplayConfiguratorTest, DontRestoreStalePowerStateAfterResume) {
 
   // Suspend and resume the system. Resuming should post a task to restore the
   // previous power state, additionally forcing a probe.
-  configurator_.SuspendDisplays();
+  configurator_.SuspendDisplays(base::Bind(
+      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
+  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   configurator_.ResumeDisplays();
 
   // Before the task runs, exit docked mode.