Delay display configuration after waking up from suspend with multi displays

ui/display/chromeos/display_configurator_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/display/chromeos/display_configurator.h"
 
 #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"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/display/chromeos/test/action_logger_util.h"
 #include "ui/display/chromeos/test/test_display_snapshot.h"
 #include "ui/display/chromeos/test/test_native_display_delegate.h"
 #include "ui/display/util/display_util.h"
@@ skipping 159 matching lines @@
     o->set_modes(std::move(modes));
     o->set_type(DISPLAY_CONNECTION_TYPE_HDMI);
     o->set_is_aspect_preserving_scaling(true);
     o->set_display_id(789);
 
     UpdateOutputs(2, false);
   }
 
   void OnConfiguredCallback(bool status) {
     callback_result_ = (status ? CALLBACK_SUCCESS : CALLBACK_FAILURE);
+
+    if (!quit_closure_.is_null())
+      base::ResetAndReturn(&quit_closure_).Run();
+
+    configuration_done_ = true;
   }
 
   void OnDisplayControlUpdated(bool status) {
     display_control_result_ = (status ? CALLBACK_SUCCESS : CALLBACK_FAILURE);
   }
 
   void EnableContentProtectionCallback(bool status) {
     enable_content_protection_status_ = status;
     enable_content_protection_call_count_++;
   }
@@ skipping 44 matching lines @@
                                                NULL).c_str(),
                           GetCrtcAction(outputs_[0], &small_mode_,
                                         gfx::Point(0, 0)).c_str(),
                           kForceDPMS, kUngrab, NULL),
               log_->GetActionsAndClear());
   }
 
   CallbackResult PopCallbackResult() {
     CallbackResult result = callback_result_;
     callback_result_ = CALLBACK_NOT_CALLED;
+    configuration_done_ = false;
     return result;
   }
 
   CallbackResult PopDisplayControlResult() {
     CallbackResult result = display_control_result_;
     display_control_result_ = CALLBACK_NOT_CALLED;
+    configuration_done_ = false;
     return result;
   }
 
+  void WaitForConfiguration() {

[Daniel Erat, 2016/10/19 16:57:56]

how about making this take an expected_duration parameter and check that the
timer has the expected delay? it could e.g. return false if the delay is wrong,
and the callers could check that.

[afakhry, 2016/10/21 20:03:03]

I think the best approach is to encapsulate the waiting logic into its own
object. Please take a look at the ConfigurationWaiter.

+    if (!configuration_done_) {
+      base::RunLoop run_loop;
+      quit_closure_ = run_loop.QuitClosure();
+      run_loop.Run();
+    }
+    configuration_done_ = false;
+  }
+
   base::MessageLoop message_loop_;
+  base::Closure quit_closure_;
   TestStateController state_controller_;
   TestMirroringController mirroring_controller_;
   DisplayConfigurator configurator_;
   TestObserver observer_;
   std::unique_ptr<ActionLogger> log_;
   TestNativeDisplayDelegate* native_display_delegate_;  // not owned
   DisplayConfigurator::TestApi test_api_;
 
   bool enable_content_protection_status_;
   int enable_content_protection_call_count_;
   DisplayConfigurator::QueryProtectionResponse
       query_content_protection_response_;
   int query_content_protection_call_count_;
 
   TestDisplaySnapshot outputs_[3];
 
   CallbackResult callback_result_;
   CallbackResult display_control_result_;
 
+  bool configuration_done_ = false;

[Daniel Erat, 2016/10/18 22:17:41]

please add a comment describing when this is true and when it's false.

+
  private:
   DISALLOW_COPY_AND_ASSIGN(DisplayConfiguratorTest);
 };
 
 }  // namespace
 
 TEST_F(DisplayConfiguratorTest, FindDisplayModeMatchingSize) {
   std::vector<const DisplayMode*> modes;
 
   // Fields are width, height, interlaced, refresh rate.
@@ skipping 419 matching lines @@
   EXPECT_FALSE(mirroring_controller_.SoftwareMirroringEnabled());
   EXPECT_EQ(1, observer_.num_changes());
 
   // 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)));
+  WaitForConfiguration();
   EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(
       JoinActions(
           kGrab,
           GetFramebufferAction(small_mode_.size(), &outputs_[0], &outputs_[1])
               .c_str(),
           GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
           GetCrtcAction(outputs_[1], &small_mode_, gfx::Point(0, 0)).c_str(),
           kForceDPMS,
           kUngrab,
@@ skipping 90 matching lines @@
   EXPECT_TRUE(mirroring_controller_.SoftwareMirroringEnabled());
   EXPECT_EQ(1, observer_.num_changes());
 
   // 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)));
+  WaitForConfiguration();
   EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(
       JoinActions(
           kGrab,
           GetFramebufferAction(gfx::Size(big_mode_.size().width(), kDualHeight),
                                &outputs_[0],
                                &outputs_[1]).c_str(),
           GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
           GetCrtcAction(outputs_[1],
                         &big_mode_,
@@ skipping 123 matching lines @@
   // pick up the change and only turn on the internal display.
   UpdateOutputs(1, false);
   configurator_.ResumeDisplays();
   EXPECT_EQ(kNoActions, log_->GetActionsAndClear());
 
   configurator_.SetDisplayPower(
       chromeos::DISPLAY_POWER_ALL_ON,
       DisplayConfigurator::kSetDisplayPowerNoFlags,
       base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
                  base::Unretained(this)));
+  WaitForConfiguration();
   EXPECT_EQ(
       JoinActions(
           kGrab,
           GetFramebufferAction(small_mode_.size(), &outputs_[0], NULL).c_str(),
           GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
           kForceDPMS,
           kUngrab,
           NULL),
       log_->GetActionsAndClear());
 }
@@ skipping 649 matching lines @@
           kSync,
           NULL),
       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)));
+  WaitForConfiguration();
   EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(3, observer_.num_changes());
   EXPECT_EQ(0, observer_.num_failures());
   EXPECT_EQ(
       JoinActions(
           kGrab,
           GetFramebufferAction(small_mode_.size(), &outputs_[0], &outputs_[1])
               .c_str(),
           GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
           GetCrtcAction(outputs_[1], &small_mode_, gfx::Point(0, 0)).c_str(),
@@ skipping 256 matching lines @@
                                 2 * DisplayConfigurator::kVerticalGap))
               .c_str(),
           kUngrab, NULL),
       log_->GetActionsAndClear());
 
   configurator_.SetDisplayPower(
       chromeos::DISPLAY_POWER_ALL_ON,
       DisplayConfigurator::kSetDisplayPowerNoFlags,
       base::Bind(&DisplayConfiguratorTest::OnConfiguredCallback,
                  base::Unretained(this)));
+  WaitForConfiguration();
 
   EXPECT_EQ(CALLBACK_SUCCESS, PopCallbackResult());
   EXPECT_EQ(
       JoinActions(
           kGrab, GetFramebufferAction(
                      gfx::Size(big_mode_.size().width(), kTripleHeight),
                      &outputs_[0], &outputs_[1])
                      .c_str(),
           GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
           GetCrtcAction(outputs_[1], &big_mode_,
@@ skipping 20 matching lines @@
                                &outputs_[0], &outputs_[1])
               .c_str(),
           GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
           GetCrtcAction(outputs_[1], &big_mode_,
                         gfx::Point(0, small_mode_.size().height() +
                                           DisplayConfigurator::kVerticalGap))
               .c_str(),
           kUngrab, NULL),
       log_->GetActionsAndClear());
 }
 

[Daniel Erat, 2016/10/18 22:17:41]

please add a new test that verifies that the code you added is getting called.

[afakhry, 2016/10/21 20:03:03]

Done.

 }  // namespace test
 }  // namespace ui