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 b0bd0a08835208a066dc4f491a94676667bf8581..f00c4c28e94fd04b1f39e418e9c0d1cf3cc86d68 100644
--- a/ui/display/chromeos/display_configurator.cc
+++ b/ui/display/chromeos/display_configurator.cc
@@ -35,6 +35,11 @@ typedef std::vector<const DisplayMode*> DisplayModeList;
 // |configure_timer_|.
 const int kConfigureDelayMs = 500;
 
+// The delay to perform configuration after waking up from suspend when in dual
+// display mode. Should be bigger than |kConfigureDelayMs|. Generally big enough
+// for external displays to be detected and added.

[oshima, 2016/10/18 22:00:05]

please add reference to the bug

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

Done.

+const int kConfigureDualDisplayAfterSuspendDelayMs = 2000;
+
 // The EDID specification marks the top bit of the manufacturer id as reserved.
 const int16_t kReservedManufacturerID = static_cast<int16_t>(1 << 15);
 
@@ -862,7 +867,19 @@ void DisplayConfigurator::SetDisplayPowerInternal(
   pending_power_flags_ = flags;
   queued_configuration_callbacks_.push_back(callback);
 
-  RunPendingConfiguration();
+  if (power_state == chromeos::DISPLAY_POWER_ALL_ON &&

[dnicoara, 2016/10/19 14:42:41]

I don't think this is the correct place to start the timer since
SetDisplayPowerInternal() is called in other circumstances as well. You want to
set the timer in ResumeDisplays() to limit it to the resume case.

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

good catch; this seems wrong to me too. when powerd has turned the displays off
due to the user being inactive but not yet suspended, and the user then hits a
key, powerd makes a d-bus call that results in SetDisplayPower being called. we
don't want to delay turning them on there.

mind trying to add a test that would've caught this regression?

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

Done.

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

Thanks for catching that. Done. I modified the unit tests to validate that the
delay is kept at a minimum.

+      (current_display_state_ == MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED ||
+       current_display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_MIRROR)) {
+    // When waking up from suspend while being in a dual display mode, we
+    // perform a forced configuration after a delay to wait for all displays to
+    // be added and detected.
+    configure_timer_.Start(FROM_HERE,
+                           base::TimeDelta::FromMilliseconds(
+                               kConfigureDualDisplayAfterSuspendDelayMs),
+                           this, &DisplayConfigurator::ConfigureDisplays);
+  } else {
+    RunPendingConfiguration();
+  }
 }
 
 void DisplayConfigurator::SetDisplayPower(
@@ -916,13 +933,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.

[afakhry, 2016/10/17 22:35:28]

Outdated comment? Do we still need the below Reset()?

[oshima, 2016/10/18 22:00:05]

+derat

+  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.
     configure_timer_.Reset();
   } else {
     configure_timer_.Start(