chromeos: Save display power state on config failure.

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 <stdint.h>
 
 #include <cmath>
 #include <cstdarg>
@@ skipping 1186 matching lines @@
   modes.push_back(new DisplayMode(gfx::Size(1280, 720), false, 60.0));
   modes.push_back(new DisplayMode(gfx::Size(1920, 1080), false, 60.0));
   modes.push_back(new DisplayMode(gfx::Size(1920, 1080), false, 40.0));
 
   for (unsigned int i = 0; i < arraysize(outputs_); i++) {
     outputs_[i].set_modes(modes.get());
     outputs_[i].set_current_mode(modes[0]);
     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.
   native_display_delegate_->set_max_configurable_pixels(
       modes[2]->size().GetArea());
   state_controller_.set_state(MULTIPLE_DISPLAY_STATE_SINGLE);
   UpdateOutputs(1, true);
 
   EXPECT_EQ(
       JoinActions(
@@ skipping 47 matching lines @@
                         modes[3],
                         gfx::Point(0,
                                    modes[0]->size().height() +
                                        DisplayConfigurator::kVerticalGap))
               .c_str(),
           kUngrab,
           NULL),
       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