Delay display configuration after waking up from suspend with multi displays

ui/display/chromeos/display_configurator.cc

Index: ui/display/chromeos/display_configurator.cc
diff --git a/ui/display/chromeos/display_configurator.cc b/ui/display/chromeos/display_configurator.cc
index 6c3062317944388f13615d2e7ae7d6db5a276a5a..4488c5599a906d6873cb29a8786d8e80e59095af 100644
--- a/ui/display/chromeos/display_configurator.cc
+++ b/ui/display/chromeos/display_configurator.cc
@@ -31,10 +31,6 @@ namespace {
 
 typedef std::vector<const DisplayMode*> DisplayModeList;
 
-// The delay to perform configuration after RRNotify. See the comment for
-// |configure_timer_|.
-const int kConfigureDelayMs = 500;
-
 // The EDID specification marks the top bit of the manufacturer id as reserved.
 const int16_t kReservedManufacturerID = static_cast<int16_t>(1 << 15);
 
@@ -68,6 +64,12 @@ bool DisplayConfigurator::TestApi::TriggerConfigureTimeout() {
   }
 }
 
+base::TimeDelta DisplayConfigurator::TestApi::GetConfigureDelay() const {
+  return configurator_->configure_timer_.IsRunning()
+             ? configurator_->configure_timer_.GetCurrentDelay()
+             : base::TimeDelta();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // DisplayConfigurator::DisplayLayoutManagerImpl implementation
 
@@ -862,6 +864,14 @@ void DisplayConfigurator::SetDisplayPowerInternal(
   pending_power_flags_ = flags;
   queued_configuration_callbacks_.push_back(callback);
 
+  if (configure_timer_.IsRunning()) {
+    // If there is a configuration task scheduled, avoid performing
+    // configuration immediately. Instead reset the timer to wait for things to
+    // settle.
+    configure_timer_.Reset();
+    return;
+  }
+
   RunPendingConfiguration();
 }
 
@@ -916,13 +926,11 @@ void DisplayConfigurator::OnConfigurationChanged() {
 
   // Configure displays with |kConfigureDelayMs| delay,
   // so that time-consuming ConfigureDisplays() won't be called multiple times.
-  if (configure_timer_.IsRunning()) {
-    // Note: when the timer is running it is possible that a different task
-    // (RestoreRequestedPowerStateAfterResume()) is scheduled. In these cases,
-    // prefer the already scheduled task to ConfigureDisplays() since
-    // ConfigureDisplays() performs only basic configuration while
-    // RestoreRequestedPowerStateAfterResume() will perform additional
-    // operations.
+  if (configure_timer_.IsRunning() &&
+      configure_timer_.GetCurrentDelay() <=
+          base::TimeDelta::FromMilliseconds(kConfigureDelayMs)) {
+    // Avoid resetting the timer to a huge delay if we are currently pending
+    // the delayed configuration after waking up from suspend.

[Daniel Erat, 2016/10/31 16:20:01]

i don't understand this condition or comment. the timer appears to only be
started within this class with kConfigureDelayMs (500) or
kResumeConfigureMultiDisplayDelayMs (2000). this condition starts the timer here
with its current delay (which i think would always be 500), and then the 'else'
block also starts the timer with 500. it seems like this if/else block can just
be replaced with a single call to Start() using 500.

or did you mean '>', so we'll preserve already-set longer delays?

[afakhry, 2016/10/31 17:37:19]

Your observation here is correct. We don't need this condition.
No, I didn't want to preserve the longer delays, rather, I didn't want to make
them even longer by resetting (e.g. when we have been waiting for 1.5s [out of
2s] and OnConfigurationChanged() event is received, I didn't want to reset the
timer back to 2s [to avoid a long delay of 3.5s]).

     configure_timer_.Reset();
   } else {
     configure_timer_.Start(
@@ -972,6 +980,19 @@ void DisplayConfigurator::ResumeDisplays() {
 
   displays_suspended_ = false;
 
+  if (current_display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_MIRROR ||
+      current_display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED ||
+      current_display_state_ == MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED) {
+    // When waking up from suspend while being in a multi display mode, we
+    // schedule a delayed forced configuration, which will make
+    // SetDisplayPowerInternal() avoid performing the configuration immediately.
+    // This gives a chance to wait for all displays to be added and detected
+    // before configuration is performed.

[Daniel Erat, 2016/10/31 16:20:02]

nit: also mention why this is done at the end of the comment: "... so we won't
immediately resize the desktops and the windows on it to fit on a single
display."

[afakhry, 2016/10/31 17:37:19]

Done.

+    configure_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
+                                          kResumeConfigureMultiDisplayDelayMs),
+                           this, &DisplayConfigurator::ConfigureDisplays);
+  }
+
   // 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,