Delay display configuration after waking up from suspend with multi displays

ui/display/chromeos/display_configurator_unittest.cc

Index: ui/display/chromeos/display_configurator_unittest.cc
diff --git a/ui/display/chromeos/display_configurator_unittest.cc b/ui/display/chromeos/display_configurator_unittest.cc
index 394533d7c2e26af543a467df62a6e98ffa8f952b..643faa6abb4d52ce3b2aa0905417c2d84ada3045 100644
--- a/ui/display/chromeos/display_configurator_unittest.cc
+++ b/ui/display/chromeos/display_configurator_unittest.cc
@@ -7,6 +7,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include "base/callback_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/scoped_vector.h"
@@ -33,6 +34,21 @@ std::unique_ptr<ui::DisplayMode> MakeDisplayMode(int width,
                                            is_interlaced, refresh_rate);
 }
 
+enum CallbackResult {

[Daniel Erat, 2016/10/25 16:05:33]

i don't understand what you're verifying by checking if the callback is run now.
the production code just passes DoNothing to it, and i _think_ it's always run
synchronously by tests. is it to check that SetDisplayPower is called
synchronously? if so, is there a simpler way you can check that, e.g. by just
checking the power state before calling Wait()?

[afakhry, 2016/10/25 20:14:11]

ResumeDisplays() can result in a delayed configuration in multiple tests for
which the callback will be invoked asynchronously. We're not just trying to
verify that SetDisplayPower() is called, but rather the whole operation
completed as expected.

So validate the correct delay AND the correct callback result (true/false).

[Daniel Erat, 2016/10/26 15:50:00]

thanks for the detailed explanation!

i understand why you're passing the callback to SetDisplayPower and verifying
that it's called with the correct value. the thing that confuses me is just why
the callback is also passed to ResumeDisplays. the real code in
PowerEventObserver passes DoNothing to ResumeDisplays, so it's not clear to me
what's being tested by passing a callback to ResumeDisplays in the tests.

sorry if i'm being dense. :-)

+  CALLBACK_FAILURE,
+  CALLBACK_SUCCESS,
+  CALLBACK_NOT_CALLED,
+};
+
+// The delay less than which we expect the configuration to be finished.
+constexpr base::TimeDelta kMaxShortDelayMs = base::TimeDelta::FromMilliseconds(
+    DisplayConfigurator::kConfigureDelayMs / 3);
+
+// The minimum configuration delay we expect when resuming while in 2+ display
+// mode.
+constexpr base::TimeDelta kMinLongDelayMs = base::TimeDelta::FromMilliseconds(
+    DisplayConfigurator::kResumeConfigureMultiDisplayDelayMs);
+
 class TestObserver : public DisplayConfigurator::Observer {
  public:
   explicit TestObserver(DisplayConfigurator* configurator)
@@ -129,23 +145,81 @@ class TestMirroringController
   DISALLOW_COPY_AND_ASSIGN(TestMirroringController);
 };
 
-class DisplayConfiguratorTest : public testing::Test {
+// Abstracts waiting for the display configuration to be completed and getting
+// the time it took to complete.
+class ConfigurationWaiter {
  public:
-  enum CallbackResult {
-    CALLBACK_FAILURE,
-    CALLBACK_SUCCESS,
-    CALLBACK_NOT_CALLED,
-  };
+  ConfigurationWaiter(DisplayConfigurator::TestApi* test_api)
+      : on_configured_callback_(base::Bind(&ConfigurationWaiter::OnConfigured,
+                                           base::Unretained(this))),
+        test_api_(test_api),
+        callback_result_(CALLBACK_NOT_CALLED),
+        pending_configuration_(true) {}
+
+  ~ConfigurationWaiter() = default;
+
+  void Reset() {
+    callback_result_ = CALLBACK_NOT_CALLED;
+    pending_configuration_ = true;
+  }
+
+  // Simulates Waiting for the configuration to complete by manually triggering

[Daniel Erat, 2016/10/25 16:05:32]

nit: s/Waiting/waiting/

[afakhry, 2016/10/25 20:14:11]

Done.

+  // the configuration timer and returns how long it was supposed to take for
+  // timer task to complete.
+  base::TimeDelta Wait() WARN_UNUSED_RESULT {
+    base::RunLoop().RunUntilIdle();
+    if (!pending_configuration_)
+      return base::TimeDelta();
+
+    const base::TimeDelta delay = test_api_->GetConfigureDelay();
+    if (!test_api_->TriggerConfigureTimeout())
+      return base::TimeDelta::Max();
+
+    return delay;
+  }
+
+  const DisplayConfigurator::ConfigurationCallback& on_configuration_callback()
+      const {
+    return on_configured_callback_;
+  }
+
+  CallbackResult callback_result() const { return callback_result_; }
 
+ private:
+  void OnConfigured(bool status) {
+    callback_result_ = status ? CALLBACK_SUCCESS : CALLBACK_FAILURE;
+
+    pending_configuration_ = false;
+  }
+
+  // The callback that should be used with the DisplayConfigurator calls to be
+  // invoked with the display configuration is done. It will always execute this
+  // waiter's OnConfigured().
+  const DisplayConfigurator::ConfigurationCallback on_configured_callback_;
+
+  DisplayConfigurator::TestApi* test_api_;  // Not owned.
+
+  // The status of the display configuration.
+  CallbackResult callback_result_;
+
+  // True if we are expecting a display configuration completion, false if the
+  // configuration has already been completed.
+  bool pending_configuration_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConfigurationWaiter);
+};
+
+class DisplayConfiguratorTest : public testing::Test {
+ public:
   DisplayConfiguratorTest()
       : small_mode_(gfx::Size(1366, 768), false, 60.0f),
         big_mode_(gfx::Size(2560, 1600), false, 60.0f),
         observer_(&configurator_),
         test_api_(&configurator_),
+        config_waiter_(&test_api_),
         enable_content_protection_status_(0),
         enable_content_protection_call_count_(0),
         query_content_protection_call_count_(0),
-        callback_result_(CALLBACK_NOT_CALLED),
         display_control_result_(CALLBACK_NOT_CALLED) {}
   ~DisplayConfiguratorTest() override {}
 
@@ -187,10 +261,6 @@ class DisplayConfiguratorTest : public testing::Test {
     UpdateOutputs(2, false);
   }
 
-  void OnConfiguredCallback(bool status) {
-    callback_result_ = (status ? CALLBACK_SUCCESS : CALLBACK_FAILURE);
-  }
-
   void OnDisplayControlUpdated(bool status) {
     display_control_result_ = (status ? CALLBACK_SUCCESS : CALLBACK_FAILURE);
   }
@@ -251,12 +321,6 @@ class DisplayConfiguratorTest : public testing::Test {
         log_->GetActionsAndClear());
   }
 
-  CallbackResult PopCallbackResult() {
-    CallbackResult result = callback_result_;
-    callback_result_ = CALLBACK_NOT_CALLED;
-    return result;
-  }
-
   CallbackResult PopDisplayControlResult() {
     CallbackResult result = display_control_result_;
     display_control_result_ = CALLBACK_NOT_CALLED;
@@ -271,7 +335,7 @@ class DisplayConfiguratorTest : public testing::Test {
   std::unique_ptr<ActionLogger> log_;
   TestNativeDisplayDelegate* native_display_delegate_;  // not owned
   DisplayConfigurator::TestApi test_api_;
-
+  ConfigurationWaiter config_waiter_;
   bool enable_content_protection_status_;
   int enable_content_protection_call_count_;
   DisplayConfigurator::QueryProtectionResponse
@@ -280,7 +344,6 @@ class DisplayConfiguratorTest : public testing::Test {
 
   std::unique_ptr<DisplaySnapshot> outputs_[3];
 
-  CallbackResult callback_result_;
   CallbackResult display_control_result_;
 
  private:
@@ -657,12 +720,13 @@ TEST_F(DisplayConfiguratorTest, SetDisplayPower) {
   // Turning off the internal display should switch the external display to
   // its native mode.
   observer_.Reset();
+  config_waiter_.Reset();
   configurator_.SetDisplayPower(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
       DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+      config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(big_mode_.size(), outputs_[0].get(),
@@ -678,12 +742,12 @@ TEST_F(DisplayConfiguratorTest, SetDisplayPower) {
   // When all displays are turned off, the framebuffer should switch back
   // to the mirrored size.
   observer_.Reset();
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(small_mode_.size(), outputs_[0].get(),
@@ -699,12 +763,12 @@ TEST_F(DisplayConfiguratorTest, SetDisplayPower) {
 
   // Turn all displays on and check that mirroring is still used.
   observer_.Reset();
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(small_mode_.size(), outputs_[0].get(),
@@ -754,12 +818,13 @@ TEST_F(DisplayConfiguratorTest, SetDisplayPower) {
   // Turning off the internal display should switch the external display to
   // its native mode.
   observer_.Reset();
+  config_waiter_.Reset();
   configurator_.SetDisplayPower(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
       DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+      config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(big_mode_.size(), outputs_[0].get(),
@@ -776,12 +841,12 @@ TEST_F(DisplayConfiguratorTest, SetDisplayPower) {
   // When all displays are turned off, the framebuffer should switch back
   // to the extended + software mirroring.
   observer_.Reset();
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -802,12 +867,12 @@ TEST_F(DisplayConfiguratorTest, SetDisplayPower) {
 
   // Turn all displays on and check that mirroring is still used.
   observer_.Reset();
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -835,9 +900,10 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
   // was connected while suspended.
   const gfx::Size framebuffer_size = configurator_.framebuffer_size();
   DCHECK(!framebuffer_size.IsEmpty());
-  configurator_.SuspendDisplays(base::Bind(
-      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(framebuffer_size.ToString(),
             configurator_.framebuffer_size().ToString());
   EXPECT_EQ(JoinActions(
@@ -847,7 +913,12 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
                 GetCrtcAction(*outputs_[0], nullptr, gfx::Point(0, 0)).c_str(),
                 kUngrab, kSync, nullptr),
             log_->GetActionsAndClear());
-  configurator_.ResumeDisplays();
+
+  // No resume delay in single display mode.
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -859,12 +930,12 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
 
   // Now turn the display off before suspending and check that the
   // configurator turns it back on and syncs with the server.
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(JoinActions(
                 kGrab, GetFramebufferAction(small_mode_.size(),
                                             outputs_[0].get(), nullptr)
@@ -873,19 +944,24 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
                 kUngrab, nullptr),
             log_->GetActionsAndClear());
 
-  configurator_.SuspendDisplays(base::Bind(
-      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(kSync, log_->GetActionsAndClear());
 
-  configurator_.ResumeDisplays();
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -907,12 +983,13 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
           kUngrab, nullptr),
       log_->GetActionsAndClear());
 
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_DUAL_MIRROR, configurator_.display_state());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(small_mode_.size(), outputs_[0].get(),
@@ -923,22 +1000,32 @@ TEST_F(DisplayConfiguratorTest, SuspendAndResume) {
           kUngrab, nullptr),
       log_->GetActionsAndClear());
 
-  configurator_.SuspendDisplays(base::Bind(
-      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  // No delay in suspend.
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_OFF,
+            configurator_.current_power_state());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_DUAL_MIRROR, configurator_.display_state());
   EXPECT_EQ(kSync, log_->GetActionsAndClear());
 
   // If a display is disconnected while suspended, the configurator should
-  // pick up the change and only turn on the internal display.
+  // pick up the change and only turn on the internal display. The should be
+  // a longer configuration delay when we set the displays back to on.
   UpdateOutputs(1, false);
-  configurator_.ResumeDisplays();
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_GE(config_waiter_.Wait(), kMinLongDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -960,19 +1047,19 @@ TEST_F(DisplayConfiguratorTest, Headless) {
 
   // Not much should happen when the display power state is changed while
   // no displays are connected.
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(JoinActions(kGrab, kUngrab, nullptr), log_->GetActionsAndClear());
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(JoinActions(kGrab, kForceDPMS, kUngrab, nullptr),
             log_->GetActionsAndClear());
 
@@ -1216,9 +1303,10 @@ 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(base::Bind(
-      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(JoinActions(
                 kGrab, GetFramebufferAction(small_mode_.size(),
                                             outputs_[0].get(), nullptr)
@@ -1235,19 +1323,19 @@ TEST_F(DisplayConfiguratorTest, DoNotConfigureWithSuspendedDisplays) {
 
   // Calls to SetDisplayPower should do nothing if the power state doesn't
   // change.
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -1271,17 +1359,21 @@ TEST_F(DisplayConfiguratorTest, DoNotConfigureWithSuspendedDisplays) {
 
   // The DisplayConfigurator should do nothing at resume time if there is no
   // state change.
+  config_waiter_.Reset();
   UpdateOutputs(1, false);
-  configurator_.ResumeDisplays();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
   // If a configuration task is pending when the displays are suspended, that
   // task should not run either and the timer should be stopped. The displays
   // should be turned off by suspend.
   configurator_.OnConfigurationChanged();
-  configurator_.SuspendDisplays(base::Bind(
-      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(JoinActions(
                 kGrab, GetFramebufferAction(small_mode_.size(),
                                             outputs_[0].get(), nullptr)
@@ -1289,11 +1381,13 @@ TEST_F(DisplayConfiguratorTest, DoNotConfigureWithSuspendedDisplays) {
                 GetCrtcAction(*outputs_[0], nullptr, gfx::Point(0, 0)).c_str(),
                 kUngrab, kSync, nullptr),
             log_->GetActionsAndClear());
-
   EXPECT_FALSE(test_api_.TriggerConfigureTimeout());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
-  configurator_.ResumeDisplays();
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab,
@@ -1527,9 +1621,9 @@ TEST_F(DisplayConfiguratorTest, SaveDisplayPowerStateOnConfigFailure) {
   configurator_.SetDisplayPower(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
       DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+      config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(1, observer_.num_changes());
   EXPECT_EQ(0, observer_.num_failures());
   log_->GetActionsAndClear();
@@ -1537,12 +1631,11 @@ TEST_F(DisplayConfiguratorTest, SaveDisplayPowerStateOnConfigFailure) {
   // Make all subsequent configuration requests fail and try to turn the
   // internal display back on.
   native_display_delegate_->set_max_configurable_pixels(1);
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_FAILURE, PopCallbackResult());
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_FAILURE, config_waiter_.callback_result());
   EXPECT_EQ(1, observer_.num_changes());
   EXPECT_EQ(1, observer_.num_failures());
   log_->GetActionsAndClear();
@@ -1577,9 +1670,9 @@ TEST_F(DisplayConfiguratorTest, DontRestoreStalePowerStateAfterResume) {
   configurator_.SetDisplayPower(
       chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
       DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+      config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(1, observer_.num_changes());
   EXPECT_EQ(0, observer_.num_failures());
   EXPECT_EQ(
@@ -1595,9 +1688,10 @@ TEST_F(DisplayConfiguratorTest, DontRestoreStalePowerStateAfterResume) {
   // Suspend and resume the system. Resuming should restore the previous power
   // state and force a probe. Suspend should turn off the displays since an
   // external monitor is connected.
-  configurator_.SuspendDisplays(base::Bind(
-      &DisplayConfiguratorTest::OnConfiguredCallback, base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(2, observer_.num_changes());
   EXPECT_EQ(
       JoinActions(
@@ -1610,12 +1704,12 @@ TEST_F(DisplayConfiguratorTest, DontRestoreStalePowerStateAfterResume) {
       log_->GetActionsAndClear());
 
   // Before the task runs, exit docked mode.
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(3, observer_.num_changes());
   EXPECT_EQ(0, observer_.num_failures());
   EXPECT_EQ(
@@ -1629,7 +1723,12 @@ TEST_F(DisplayConfiguratorTest, DontRestoreStalePowerStateAfterResume) {
       log_->GetActionsAndClear());
 
   // Check that the display states are not changed after resuming.
-  configurator_.ResumeDisplays();
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON,
+            configurator_.current_power_state());
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 }
 
@@ -1667,21 +1766,18 @@ TEST_F(DisplayConfiguratorTest,
 
   native_display_delegate_->set_run_async(true);
 
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
 
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
 
-  EXPECT_EQ(CALLBACK_NOT_CALLED, PopCallbackResult());
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  EXPECT_EQ(CALLBACK_NOT_CALLED, config_waiter_.callback_result());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(1, observer_.num_changes());
   EXPECT_EQ(0, observer_.num_failures());
 
@@ -1702,9 +1798,10 @@ TEST_F(DisplayConfiguratorTest,
           kUngrab, nullptr),
       log_->GetActionsAndClear());
 
+  config_waiter_.Reset();
   EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(2, observer_.num_changes());
   EXPECT_EQ(0, observer_.num_failures());
 
@@ -1735,13 +1832,12 @@ TEST_F(DisplayConfiguratorTest,
   // Fail display configuration.
   native_display_delegate_->set_max_configurable_pixels(-1);
 
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-
-  EXPECT_EQ(CALLBACK_FAILURE, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_FAILURE, config_waiter_.callback_result());
   EXPECT_EQ(0, observer_.num_changes());
   EXPECT_EQ(1, observer_.num_failures());
 
@@ -1765,11 +1861,11 @@ TEST_F(DisplayConfiguratorTest,
 
   // This configuration should trigger a display configuration since the
   // previous configuration failed.
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
 
   EXPECT_EQ(0, observer_.num_changes());
   EXPECT_EQ(2, observer_.num_failures());
@@ -1855,13 +1951,12 @@ TEST_F(DisplayConfiguratorTest, TestWithThreeDisplays) {
       log_->GetActionsAndClear());
 
   // Verify that turning the power off works.
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_OFF,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_OFF,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(
@@ -1882,13 +1977,12 @@ TEST_F(DisplayConfiguratorTest, TestWithThreeDisplays) {
           kUngrab, nullptr),
       log_->GetActionsAndClear());
 
-  configurator_.SetDisplayPower(
-      chromeos::DISPLAY_POWER_ALL_ON,
-      DisplayConfigurator::kSetDisplayPowerNoFlags,
-      base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
-                 base::Unretained(this)));
-
-  EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
+  config_waiter_.Reset();
+  configurator_.SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
+                                DisplayConfigurator::kSetDisplayPowerNoFlags,
+                                config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(
@@ -1928,5 +2022,89 @@ TEST_F(DisplayConfiguratorTest, TestWithThreeDisplays) {
       log_->GetActionsAndClear());
 }
 
+// Tests the suspend and resume behavior when in dual or multi display modes.
+TEST_F(DisplayConfiguratorTest, SuspendResumeWithMultipleDisplays) {
+  InitWithSingleOutput();
+  state_controller_.set_state(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED);
+  observer_.Reset();
+  UpdateOutputs(2, true);
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED,
+            configurator_.display_state());
+  EXPECT_FALSE(mirroring_controller_.SoftwareMirroringEnabled());
+  EXPECT_EQ(1, observer_.num_changes());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON,
+            configurator_.current_power_state());
+
+  // Suspending displays should result in an immediate configuration without
+  // delays, even in dual display mode.
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_OFF,
+            configurator_.current_power_state());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED,
+            configurator_.display_state());
+
+  // Resuming from suspend with dual displays, should be done after a delay, and
+  // after the configuration we should still expect to be in a dual display
+  // mode.
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_GE(test_api_.GetConfigureDelay(), kMinLongDelayMs);

[Daniel Erat, 2016/10/25 16:05:32]

you should use EXPECT_EQ rather than EXPECT_GE for all of these now, right? you
should get back the configured delay instead of some arbitrary delta, i think.

[afakhry, 2016/10/25 20:14:11]

Done.

+  EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON,
+            configurator_.current_power_state());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED,
+            configurator_.display_state());
+
+  // Suspend displays and disconnect one of them while in suspend.
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED,
+            configurator_.display_state());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_OFF,
+            configurator_.current_power_state());
+  UpdateOutputs(1, false);
+
+  // Now resume, and expect that we'll still have a long delay since we were in
+  // dual mode before suspend. The configurator should pick up the change and
+  // detect that we are in single display mode now.
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_GE(config_waiter_.Wait(), kMinLongDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON,
+            configurator_.current_power_state());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_SINGLE, configurator_.display_state());
+
+  // Verify that the above is the exact same behavior for 3+ displays.
+  UpdateOutputs(3, true);
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED,
+            configurator_.display_state());
+  // Suspend.
+  config_waiter_.Reset();
+  configurator_.SuspendDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_LT(config_waiter_.Wait(), kMaxShortDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED,
+            configurator_.display_state());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_OFF,
+            configurator_.current_power_state());
+
+  // Resume and expect the correct delay.
+  config_waiter_.Reset();
+  configurator_.ResumeDisplays(config_waiter_.on_configuration_callback());
+  EXPECT_GE(config_waiter_.Wait(), kMinLongDelayMs);
+  EXPECT_EQ(CALLBACK_SUCCESS, config_waiter_.callback_result());
+  EXPECT_EQ(chromeos::DISPLAY_POWER_ALL_ON,
+            configurator_.current_power_state());
+  EXPECT_EQ(MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED,
+            configurator_.display_state());
+}
+
 }  // namespace test
 }  // namespace ui