chromeos: Save display power state on config failure.

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 c28226be0c4e3cc3437d44714482b0252651f8f8..27fedcc40fa9e75adcb5e02035778066fd1acaec 100644
--- a/ui/display/chromeos/display_configurator_unittest.cc
+++ b/ui/display/chromeos/display_configurator_unittest.cc
@@ -1204,8 +1204,6 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
     outputs_[i].set_native_mode(modes[0]);
   }
 
-  configurator_.Init(false);
-
   // First test simply fails in MULTIPLE_DISPLAY_STATE_SINGLE mode. This is
   // probably unrealistic but we want to make sure any assumptions don't creep
   // in.
@@ -1273,4 +1271,41 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
       log_->GetActionsAndClear());
 }
 
+// Tests that power state requests are saved after failed configuration attempts
+// so they can be reused later: http://crosbug.com/p/31571
+TEST_F(DisplayConfiguratorTest, SaveDisplayPowerStateOnConfigFailure) {
+  // Start out with two displays in extended mode.
+  state_controller_.set_state(MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED);
+  configurator_.Init(false);
+  configurator_.ForceInitialConfigure(0);
+  log_->GetActionsAndClear();
+
+  // Turn off the internal display, simulating docked mode.
+  configurator_.SetDisplayPower(
+      chromeos::DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON,
+      DisplayConfigurator::kSetDisplayPowerNoFlags);
+  log_->GetActionsAndClear();
+
+  // 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,

[dnicoara, 2014/09/25 13:59:54]

It makes me feel a bit uncomfortable that this doesn't check that the transition
fails. If this ever succeeds the following state will always be
DISPLAY_POWER_ALL_ON.

[Daniel Erat, 2014/09/25 17:31:06]

i've added expectations for the return values to the two calls to
SetDisplayPower(). do you think that's good enough, or would you rather that the
test spelled out the actions passed to the delegate?

(is your specific concern that the stub delegate could regress such that it
reports success for this request?)

[dnicoara, 2014/09/25 17:50:35]

Thank you. I think this should be fine since we're ultimately testing the state
transitions.
The stub delegate was just part of my worry. I am more worried that
DisplayConfigurator could be modified to swallow the error incorrectly.

+                                DisplayConfigurator::kSetDisplayPowerNoFlags);
+  log_->GetActionsAndClear();
+
+  // Simulate the external display getting disconnected and check that the
+  // internal display is turned on (i.e. DISPLAY_POWER_ALL_ON is used) rather
+  // than the earlier DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON state.
+  native_display_delegate_->set_max_configurable_pixels(0);
+  UpdateOutputs(1, true);
+  EXPECT_EQ(
+      JoinActions(
+          kGrab,
+          GetFramebufferAction(small_mode_.size(), &outputs_[0], NULL).c_str(),
+          GetCrtcAction(outputs_[0], &small_mode_, gfx::Point(0, 0)).c_str(),
+          kUngrab,
+          NULL),
+      log_->GetActionsAndClear());
+}
+
 }  // namespace ui