chromeos: Update cached displays whenever fetching state.

This updates OutputConfigurator to update its cached outputs
whenever it fetches the current state via XRandR.

Previously, it only updated the cached outputs after
successfully configuring them. If an output was added and
configuration failed due to e.g. a request to enter an
invalid state, the new output wouldn't be present in the
cached list. This wasn't much of a problem before, since the
cached outputs were only used to ignore subsequent XRandR
notifications and to notify observers about changes (also,
we shouldn't be requesting invalid states), but having stale
outputs may be more of a problem if output-protection code
expects to be able to use the cached outputs as a mostly-up-
to-date snapshot of currently-connected outputs.

BUG=none

Review URL: https://chromiumcodereview.appspot.com/23447052

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@224890 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 103 insertions, 90 deletions

diff --git a/chromeos/display/output_configurator.cc b/chromeos/display/output_configurator.cc
index 4f0d3cd..4ff0aca 100644
--- a/chromeos/display/output_configurator.cc
+++ b/chromeos/display/output_configurator.cc
@@ -258,18 +258,18 @@ void OutputConfigurator::Start(uint32 background_color_argb) {
   delegate_->GrabServer();
   delegate_->InitXRandRExtension(&xrandr_event_base_);
 
-  std::vector<OutputSnapshot> outputs = GetOutputs();
-  if (outputs.size() > 1 && background_color_argb)
+  UpdateCachedOutputs();
+  if (cached_outputs_.size() > 1 && background_color_argb)
     delegate_->SetBackgroundColor(background_color_argb);
-  const OutputState new_state = GetOutputState(outputs, power_state_);
+  const OutputState new_state = ChooseOutputState(power_state_);
   const bool success = EnterStateOrFallBackToSoftwareMirroring(
-      new_state, power_state_, outputs);
+      new_state, power_state_);
 
   // Force the DPMS on chrome startup as the driver doesn't always detect
   // that all displays are on when signing out.
   delegate_->ForceDPMSOn();
   delegate_->UngrabServer();
-  delegate_->SendProjectingStateToPowerManager(IsProjecting(outputs));
+  delegate_->SendProjectingStateToPowerManager(IsProjecting(cached_outputs_));
   NotifyObservers(success, new_state);
 }
 
@@ -288,18 +288,18 @@ bool OutputConfigurator::SetDisplayPower(DisplayPowerState power_state,
     return true;
 
   delegate_->GrabServer();
-  std::vector<OutputSnapshot> outputs = GetOutputs();
+  UpdateCachedOutputs();
 
-  const OutputState new_state = GetOutputState(outputs, power_state);
+  const OutputState new_state = ChooseOutputState(power_state);
   bool attempted_change = false;
   bool success = false;
 
   bool only_if_single_internal_display =
       flags & kSetDisplayPowerOnlyIfSingleInternalDisplay;
-  bool single_internal_display = outputs.size() == 1 && outputs[0].is_internal;
+  bool single_internal_display =
+      cached_outputs_.size() == 1 && cached_outputs_[0].is_internal;
   if (single_internal_display || !only_if_single_internal_display) {
-    success = EnterStateOrFallBackToSoftwareMirroring(
-        new_state, power_state, outputs);
+    success = EnterStateOrFallBackToSoftwareMirroring(new_state, power_state);
     attempted_change = true;
 
     // Force the DPMS on since the driver doesn't always detect that it
@@ -329,9 +329,9 @@ bool OutputConfigurator::SetDisplayMode(OutputState new_state) {
   }
 
   delegate_->GrabServer();
-  std::vector<OutputSnapshot> outputs = GetOutputs();
+  UpdateCachedOutputs();
   const bool success = EnterStateOrFallBackToSoftwareMirroring(
-      new_state, power_state_, outputs);
+      new_state, power_state_);
   delegate_->UngrabServer();
 
   NotifyObservers(success, new_state);
@@ -455,13 +455,12 @@ void OutputConfigurator::ScheduleConfigureOutputs() {
   }
 }
 
-std::vector<OutputConfigurator::OutputSnapshot>
-OutputConfigurator::GetOutputs() {
-  std::vector<OutputSnapshot> outputs = delegate_->GetOutputs();
+void OutputConfigurator::UpdateCachedOutputs() {
+  cached_outputs_ = delegate_->GetOutputs();
 
   // Set |selected_mode| fields.
-  for (size_t i = 0; i < outputs.size(); ++i) {
-    OutputSnapshot* output = &outputs[i];
+  for (size_t i = 0; i < cached_outputs_.size(); ++i) {
+    OutputSnapshot* output = &cached_outputs_[i];
     if (output->has_display_id) {
       int width = 0, height = 0;
       if (state_controller_ &&
@@ -477,9 +476,9 @@ OutputConfigurator::GetOutputs() {
   }
 
   // Set |mirror_mode| fields.
-  if (outputs.size() == 2) {
-    bool one_is_internal = outputs[0].is_internal;
-    bool two_is_internal = outputs[1].is_internal;
+  if (cached_outputs_.size() == 2) {
+    bool one_is_internal = cached_outputs_[0].is_internal;
+    bool two_is_internal = cached_outputs_[1].is_internal;
     int internal_outputs = (one_is_internal ? 1 : 0) +
         (two_is_internal ? 1 : 0);
     DCHECK_LT(internal_outputs, 2);
@@ -494,30 +493,28 @@ OutputConfigurator::GetOutputs() {
 
       if (internal_outputs == 1) {
         if (one_is_internal) {
-          can_mirror = FindMirrorMode(&outputs[0], &outputs[1],
+          can_mirror = FindMirrorMode(&cached_outputs_[0], &cached_outputs_[1],
               is_panel_fitting_enabled_, preserve_aspect);
         } else {
           DCHECK(two_is_internal);
-          can_mirror = FindMirrorMode(&outputs[1], &outputs[0],
+          can_mirror = FindMirrorMode(&cached_outputs_[1], &cached_outputs_[0],
               is_panel_fitting_enabled_, preserve_aspect);
         }
       } else {  // if (internal_outputs == 0)
         // No panel fitting for external outputs, so fall back to exact match.
-        can_mirror = FindMirrorMode(&outputs[0], &outputs[1], false,
-                                    preserve_aspect);
+        can_mirror = FindMirrorMode(&cached_outputs_[0], &cached_outputs_[1],
+                                    false, preserve_aspect);
         if (!can_mirror && preserve_aspect) {
           // FindMirrorMode() will try to preserve aspect ratio of what it
           // thinks is external display, so if it didn't succeed with one, maybe
           // it will succeed with the other.  This way we will have the correct
           // aspect ratio on at least one of them.
-          can_mirror = FindMirrorMode(&outputs[1], &outputs[0], false,
-                                      preserve_aspect);
+          can_mirror = FindMirrorMode(&cached_outputs_[1], &cached_outputs_[0],
+                                      false, preserve_aspect);
         }
       }
     }
   }
-
-  return outputs;
 }
 
 bool OutputConfigurator::FindMirrorMode(OutputSnapshot* internal_output,
@@ -585,14 +582,14 @@ void OutputConfigurator::ConfigureOutputs() {
   configure_timer_.reset();
 
   delegate_->GrabServer();
-  std::vector<OutputSnapshot> outputs = GetOutputs();
-  const OutputState new_state = GetOutputState(outputs, power_state_);
+  UpdateCachedOutputs();
+  const OutputState new_state = ChooseOutputState(power_state_);
   const bool success = EnterStateOrFallBackToSoftwareMirroring(
-      new_state, power_state_, outputs);
+      new_state, power_state_);
   delegate_->UngrabServer();
 
   NotifyObservers(success, new_state);
-  delegate_->SendProjectingStateToPowerManager(IsProjecting(outputs));
+  delegate_->SendProjectingStateToPowerManager(IsProjecting(cached_outputs_));
 }
 
 void OutputConfigurator::NotifyObservers(bool success,
@@ -608,14 +605,13 @@ void OutputConfigurator::NotifyObservers(bool success,
 
 bool OutputConfigurator::EnterStateOrFallBackToSoftwareMirroring(
     OutputState output_state,
-    DisplayPowerState power_state,
-    const std::vector<OutputSnapshot>& outputs) {
-  bool success = EnterState(output_state, power_state, outputs);
+    DisplayPowerState power_state) {
+  bool success = EnterState(output_state, power_state);
   if (mirroring_controller_) {
     bool enable_software_mirroring = false;
     if (!success && output_state == STATE_DUAL_MIRROR) {
       if (output_state_ != STATE_DUAL_EXTENDED || power_state_ != power_state)
-        EnterState(STATE_DUAL_EXTENDED, power_state, outputs);
+        EnterState(STATE_DUAL_EXTENDED, power_state);
       enable_software_mirroring = success =
           output_state_ == STATE_DUAL_EXTENDED;
     }
@@ -626,34 +622,34 @@ bool OutputConfigurator::EnterStateOrFallBackToSoftwareMirroring(
 
 bool OutputConfigurator::EnterState(
     OutputState output_state,
-    DisplayPowerState power_state,
-    const std::vector<OutputSnapshot>& outputs) {
+    DisplayPowerState power_state) {
   std::vector<bool> output_power;
-  int num_on_outputs = GetOutputPower(outputs, power_state, &output_power);
+  int num_on_outputs = GetOutputPower(
+      cached_outputs_, power_state, &output_power);
   VLOG(1) << "EnterState: output=" << OutputStateToString(output_state)
           << " power=" << DisplayPowerStateToString(power_state);
 
   // Framebuffer dimensions.
   int width = 0, height = 0;
-  std::vector<OutputSnapshot> updated_outputs = outputs;
+  std::vector<OutputSnapshot> updated_outputs = cached_outputs_;
 
   switch (output_state) {
     case STATE_INVALID:
       NOTREACHED() << "Ignoring request to enter invalid state with "
-                   << outputs.size() << " connected output(s)";
+                   << updated_outputs.size() << " connected output(s)";
       return false;
     case STATE_HEADLESS:
-      if (outputs.size() != 0) {
+      if (updated_outputs.size() != 0) {
         LOG(WARNING) << "Ignoring request to enter headless mode with "
-                     << outputs.size() << " connected output(s)";
+                     << updated_outputs.size() << " connected output(s)";
         return false;
       }
       break;
     case STATE_SINGLE: {
       // If there are multiple outputs connected, only one should be turned on.
-      if (outputs.size() != 1 && num_on_outputs != 1) {
+      if (updated_outputs.size() != 1 && num_on_outputs != 1) {
         LOG(WARNING) << "Ignoring request to enter single mode with "
-                     << outputs.size() << " connected outputs and "
+                     << updated_outputs.size() << " connected outputs and "
                      << num_on_outputs << " turned on";
         return false;
       }
@@ -664,7 +660,7 @@ bool OutputConfigurator::EnterState(
         output->y = 0;
         output->current_mode = output_power[i] ? output->selected_mode : None;
 
-        if (output_power[i] || outputs.size() == 1) {
+        if (output_power[i] || updated_outputs.size() == 1) {
           const ModeInfo* mode_info =
               GetModeInfo(*output, output->selected_mode);
           if (!mode_info)
@@ -676,23 +672,24 @@ bool OutputConfigurator::EnterState(
       break;
     }
     case STATE_DUAL_MIRROR: {
-      if (outputs.size() != 2 || (num_on_outputs != 0 && num_on_outputs != 2)) {
+      if (updated_outputs.size() != 2 ||
+          (num_on_outputs != 0 && num_on_outputs != 2)) {
         LOG(WARNING) << "Ignoring request to enter mirrored mode with "
-                     << outputs.size() << " connected output(s) and "
+                     << updated_outputs.size() << " connected output(s) and "
                      << num_on_outputs << " turned on";
         return false;
       }
 
-      if (!outputs[0].mirror_mode)
+      if (!updated_outputs[0].mirror_mode)
         return false;
       const ModeInfo* mode_info =
-          GetModeInfo(outputs[0], outputs[0].mirror_mode);
+          GetModeInfo(updated_outputs[0], updated_outputs[0].mirror_mode);
       if (!mode_info)
         return false;
       width = mode_info->width;
       height = mode_info->height;
 
-      for (size_t i = 0; i < outputs.size(); ++i) {
+      for (size_t i = 0; i < updated_outputs.size(); ++i) {
         OutputSnapshot* output = &updated_outputs[i];
         output->x = 0;
         output->y = 0;
@@ -711,14 +708,15 @@ bool OutputConfigurator::EnterState(
       break;
     }
     case STATE_DUAL_EXTENDED: {
-      if (outputs.size() != 2 || (num_on_outputs != 0 && num_on_outputs != 2)) {
+      if (updated_outputs.size() != 2 ||
+          (num_on_outputs != 0 && num_on_outputs != 2)) {
         LOG(WARNING) << "Ignoring request to enter extended mode with "
-                     << outputs.size() << " connected output(s) and "
+                     << updated_outputs.size() << " connected output(s) and "
                      << num_on_outputs << " turned on";
         return false;
       }
 
-      for (size_t i = 0; i < outputs.size(); ++i) {
+      for (size_t i = 0; i < updated_outputs.size(); ++i) {
         OutputSnapshot* output = &updated_outputs[i];
         output->x = 0;
         output->y = height ? height + kVerticalGap : 0;
@@ -728,14 +726,14 @@ bool OutputConfigurator::EnterState(
         // same desktop configuration can be restored when the outputs are
         // turned back on.
         const ModeInfo* mode_info =
-            GetModeInfo(outputs[i], outputs[i].selected_mode);
+            GetModeInfo(updated_outputs[i], updated_outputs[i].selected_mode);
         if (!mode_info)
           return false;
         width = std::max<int>(width, mode_info->width);
         height += (height ? kVerticalGap : 0) + mode_info->height;
       }
 
-      for (size_t i = 0; i < outputs.size(); ++i) {
+      for (size_t i = 0; i < updated_outputs.size(); ++i) {
         OutputSnapshot* output = &updated_outputs[i];
         if (output->touch_device_id) {
           const ModeInfo* mode_info =
@@ -753,37 +751,35 @@ bool OutputConfigurator::EnterState(
   }
 
   // Finally, apply the desired changes.
-  DCHECK_EQ(outputs.size(), updated_outputs.size());
-  if (!outputs.empty()) {
+  DCHECK_EQ(cached_outputs_.size(), updated_outputs.size());
+  if (!updated_outputs.empty()) {
     delegate_->CreateFrameBuffer(width, height, updated_outputs);
-    for (size_t i = 0; i < outputs.size(); ++i) {
+    for (size_t i = 0; i < updated_outputs.size(); ++i) {
       const OutputSnapshot& output = updated_outputs[i];
       if (delegate_->ConfigureCrtc(output.crtc, output.current_mode,
                                    output.output, output.x, output.y)) {
         if (output.touch_device_id)
           delegate_->ConfigureCTM(output.touch_device_id, output.transform);
+        cached_outputs_[i] = updated_outputs[i];
       } else {
         LOG(WARNING) << "Unable to configure CRTC " << output.crtc << ":"
                      << " mode=" << output.current_mode
                      << " output=" << output.output
                      << " x=" << output.x
                      << " y=" << output.y;
-        updated_outputs[i] = outputs[i];
       }
     }
   }
 
   output_state_ = output_state;
   power_state_ = power_state;
-  cached_outputs_ = updated_outputs;
   return true;
 }
 
-OutputState OutputConfigurator::GetOutputState(
-    const std::vector<OutputSnapshot>& outputs,
+OutputState OutputConfigurator::ChooseOutputState(
     DisplayPowerState power_state) const {
-  int num_on_outputs = GetOutputPower(outputs, power_state, NULL);
-  switch (outputs.size()) {
+  int num_on_outputs = GetOutputPower(cached_outputs_, power_state, NULL);
+  switch (cached_outputs_.size()) {
     case 0:
       return STATE_HEADLESS;
     case 1:
@@ -797,11 +793,11 @@ OutputState OutputConfigurator::GetOutputState(
         // With either both outputs on or both outputs off, use one of the
         // dual modes.
         std::vector<int64> display_ids;
-        for (size_t i = 0; i < outputs.size(); ++i) {
+        for (size_t i = 0; i < cached_outputs_.size(); ++i) {
           // If display id isn't available, switch to extended mode.
-          if (!outputs[i].has_display_id)
+          if (!cached_outputs_[i].has_display_id)
             return STATE_DUAL_EXTENDED;
-          display_ids.push_back(outputs[i].display_id);
+          display_ids.push_back(cached_outputs_[i].display_id);
         }
         return state_controller_->GetStateForDisplayIds(display_ids);
       }
diff --git a/chromeos/display/output_configurator.h b/chromeos/display/output_configurator.h
index 2ff4484..ec45fc9 100644
--- a/chromeos/display/output_configurator.h
+++ b/chromeos/display/output_configurator.h
@@ -190,7 +190,8 @@ class CHROMEOS_EXPORT OutputConfigurator
 
     // Returns information about the current outputs. This method may block for
     // 60 milliseconds or more. The returned outputs are not fully initialized;
-    // the rest of the work happens in OutputConfigurator::GetOutputs().
+    // the rest of the work happens in
+    // OutputConfigurator::UpdateCachedOutputs().
     virtual std::vector<OutputSnapshot> GetOutputs() = 0;
 
     // Adds |mode| to |output|.
@@ -232,6 +233,10 @@ class CHROMEOS_EXPORT OutputConfigurator
           xrandr_event_base_(xrandr_event_base) {}
     ~TestApi() {}
 
+    const std::vector<OutputSnapshot>& cached_outputs() const {
+      return configurator_->cached_outputs_;
+    }
+
     // Dispatches an RRScreenChangeNotify event to |configurator_|.
     void SendScreenChangeEvent();
 
@@ -335,8 +340,7 @@ class CHROMEOS_EXPORT OutputConfigurator
   // Overridden from base::MessagePumpObserver:
   virtual base::EventStatus WillProcessEvent(
       const base::NativeEvent& event) OVERRIDE;
-  virtual void DidProcessEvent(
-      const base::NativeEvent& event) OVERRIDE;
+  virtual void DidProcessEvent(const base::NativeEvent& event) OVERRIDE;
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -359,15 +363,15 @@ class CHROMEOS_EXPORT OutputConfigurator
   void ScheduleConfigureOutputs();
 
  private:
-  // Returns currently-connected outputs. This method is a wrapper around
-  // |delegate_->GetOutputs()| that does additional work, like finding the
-  // mirror mode and setting user-preferred modes. Note that the server must
-  // be grabbed via |delegate_->GrabServer()| first.
-  std::vector<OutputSnapshot> GetOutputs();
-
-  // Helper method for GetOutputs() that initializes the passed-in outputs'
-  // |mirror_mode| fields by looking for a mode in |internal_output| and
-  // |external_output| having the same resolution. Returns false if a shared
+  // Updates |cached_outputs_| to contain currently-connected outputs. Calls
+  // |delegate_->GetOutputs()| and then does additional work, like finding the
+  // mirror mode and setting user-preferred modes. Note that the server must be
+  // grabbed via |delegate_->GrabServer()| first.
+  void UpdateCachedOutputs();
+
+  // Helper method for UpdateCachedOutputs() that initializes the passed-in
+  // outputs' |mirror_mode| fields by looking for a mode in |internal_output|
+  // and |external_output| having the same resolution. Returns false if a shared
   // mode wasn't found or created.
   //
   // |try_panel_fitting| allows creating a panel-fitting mode for
@@ -395,20 +399,16 @@ class CHROMEOS_EXPORT OutputConfigurator
   // and returns true.
   bool EnterStateOrFallBackToSoftwareMirroring(
       OutputState output_state,
-      DisplayPowerState power_state,
-      const std::vector<OutputSnapshot>& outputs);
+      DisplayPowerState power_state);
 
   // Switches to the state specified in |output_state| and |power_state|.
   // On success, updates |output_state_|, |power_state_|, and
   // |cached_outputs_| and returns true.
-  bool EnterState(OutputState output_state,
-                  DisplayPowerState power_state,
-                  const std::vector<OutputSnapshot>& outputs);
-
-  // Returns the output state that should be used with |outputs| connected
-  // while in |power_state|.
-  OutputState GetOutputState(const std::vector<OutputSnapshot>& outputs,
-                             DisplayPowerState power_state) const;
+  bool EnterState(OutputState output_state, DisplayPowerState power_state);
+
+  // Returns the output state that should be used with |cached_outputs_| while
+  // in |power_state|.
+  OutputState ChooseOutputState(DisplayPowerState power_state) const;
 
   // Computes the relevant transformation for mirror mode.
   // |output| is the output on which mirror mode is being applied.
diff --git a/chromeos/display/output_configurator_unittest.cc b/chromeos/display/output_configurator_unittest.cc
index ad3211e..d7f844d 100644
--- a/chromeos/display/output_configurator_unittest.cc
+++ b/chromeos/display/output_configurator_unittest.cc
@@ -1014,6 +1014,23 @@ TEST_F(OutputConfiguratorTest, AvoidUnnecessaryProbes) {
             delegate_->GetActionsAndClear());
 }
 
+TEST_F(OutputConfiguratorTest, UpdateCachedOutputsEvenAfterFailure) {
+  InitWithSingleOutput();
+  const std::vector<OutputConfigurator::OutputSnapshot>* cached =
+      &test_api_.cached_outputs();
+  ASSERT_EQ(static_cast<size_t>(1), cached->size());
+  EXPECT_EQ(outputs_[0].current_mode, (*cached)[0].current_mode);
+
+  // After connecting a second output, check that it shows up in
+  // |cached_outputs_| even if an invalid state is requested.
+  state_controller_.set_state(STATE_SINGLE);
+  UpdateOutputs(2, true);
+  cached = &test_api_.cached_outputs();
+  ASSERT_EQ(static_cast<size_t>(2), cached->size());
+  EXPECT_EQ(outputs_[0].current_mode, (*cached)[0].current_mode);
+  EXPECT_EQ(outputs_[1].current_mode, (*cached)[1].current_mode);
+}
+
 TEST_F(OutputConfiguratorTest, PanelFitting) {
   // Configure the internal display to support only the big mode and the
   // external display to support only the small mode.