Remove DisplayState from the public interface for DisplayConfigurator

ui/display/chromeos/display_configurator.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 "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/sys_info.h"
@@ skipping 14 matching lines @@
 // The delay to perform configuration after RRNotify. See the comment for
 // |configure_timer_|.
 const int kConfigureDelayMs = 500;
 
 // The delay spent before reading the display configuration after coming out of
 // suspend. While coming out of suspend the display state may be updating. This
 // is used to wait until the hardware had a chance to update the display state
 // such that we read an up to date state.
 const int kResumeDelayMs = 500;
 
+struct DisplayState {
+  DisplayState()
+      : display(nullptr), selected_mode(nullptr), mirror_mode(nullptr) {}
+
+  DisplaySnapshot* display;  // Not owned.
+
+  // User-selected mode for the display.
+  const DisplayMode* selected_mode;
+
+  // Mode used when displaying the same desktop on multiple displays.
+  const DisplayMode* mirror_mode;
+};
+
 void DoNothing(bool status) {
 }
 
 }  // namespace
 
-
 const int DisplayConfigurator::kSetDisplayPowerNoFlags = 0;
 const int DisplayConfigurator::kSetDisplayPowerForceProbe = 1 << 0;
 const int
 DisplayConfigurator::kSetDisplayPowerOnlyIfSingleInternalDisplay = 1 << 1;
 
-DisplayConfigurator::DisplayState::DisplayState()
-    : display(NULL),
-      selected_mode(NULL),
-      mirror_mode(NULL) {}
-
 bool DisplayConfigurator::TestApi::TriggerConfigureTimeout() {
   if (configurator_->configure_timer_.IsRunning()) {
     configurator_->configure_timer_.user_task().Run();
     configurator_->configure_timer_.Stop();
     return true;
   } else {
     return false;
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DisplayConfigurator::DisplayLayoutManagerImpl implementation
 
 class DisplayConfigurator::DisplayLayoutManagerImpl
     : public DisplayLayoutManager {
  public:
   DisplayLayoutManagerImpl(DisplayConfigurator* configurator);
   ~DisplayLayoutManagerImpl() override;
 
   // DisplayConfigurator::DisplayLayoutManager:
   SoftwareMirroringController* GetSoftwareMirroringController() const override;
   StateController* GetStateController() const override;
   MultipleDisplayState GetDisplayState() const override;
   chromeos::DisplayPowerState GetPowerState() const override;
-  std::vector<DisplayState> ParseDisplays(
-      const std::vector<DisplaySnapshot*>& displays) const override;
-  bool GetDisplayLayout(const std::vector<DisplayState>& displays,
+  bool GetDisplayLayout(const std::vector<DisplaySnapshot*>& displays,
                         MultipleDisplayState new_display_state,
                         chromeos::DisplayPowerState new_power_state,
                         std::vector<DisplayConfigureRequest>* requests,
                         gfx::Size* framebuffer_size) const override;
 
  private:
+  // Parses the |displays| into a list of DisplayStates. This effectively adds
+  // |mirror_mode| and |selected_mode| to the returned results.
+  // TODO(dnicoara): Break this into GetSelectedMode() and GetMirrorMode() and
+  // remove DisplayState.
+  std::vector<DisplayState> ParseDisplays(
+      const std::vector<DisplaySnapshot*>& displays) const;
+
   // Helper method for ParseDisplays() that initializes the passed-in
   // displays' |mirror_mode| fields by looking for a mode in |internal_display|
   // and |external_display| 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
   // |internal_display| instead of only searching for a matching mode (note that
   // it may lead to a crash if |internal_display| is not capable of panel
   // fitting).
   //
@@ skipping 31 matching lines @@
 MultipleDisplayState
 DisplayConfigurator::DisplayLayoutManagerImpl::GetDisplayState() const {
   return configurator_->current_display_state_;
 }
 
 chromeos::DisplayPowerState
 DisplayConfigurator::DisplayLayoutManagerImpl::GetPowerState() const {
   return configurator_->current_power_state_;
 }
 
-std::vector<DisplayConfigurator::DisplayState>
+std::vector<DisplayState>
 DisplayConfigurator::DisplayLayoutManagerImpl::ParseDisplays(
     const std::vector<DisplaySnapshot*>& snapshots) const {
   std::vector<DisplayState> cached_displays;
   for (auto snapshot : snapshots) {
     DisplayState display_state;
     display_state.display = snapshot;
     cached_displays.push_back(display_state);
   }
 
   // Set |selected_mode| fields.
@@ skipping 48 matching lines @@
                                       false, preserve_aspect);
         }
       }
     }
   }
 
   return cached_displays;
 }
 
 bool DisplayConfigurator::DisplayLayoutManagerImpl::GetDisplayLayout(
-    const std::vector<DisplayState>& displays,
+    const std::vector<DisplaySnapshot*>& displays,
     MultipleDisplayState new_display_state,
     chromeos::DisplayPowerState new_power_state,
     std::vector<DisplayConfigureRequest>* requests,
     gfx::Size* framebuffer_size) const {
+  std::vector<DisplayState> states = ParseDisplays(displays);
   std::vector<bool> display_power;
   int num_on_displays =
       GetDisplayPower(displays, new_power_state, &display_power);
   VLOG(1) << "EnterState: display="
           << MultipleDisplayStateToString(new_display_state)
           << " power=" << DisplayPowerStateToString(new_power_state);
 
   // Framebuffer dimensions.
   gfx::Size size;
 
   for (size_t i = 0; i < displays.size(); ++i) {
     requests->push_back(DisplayConfigureRequest(
-        displays[i].display, displays[i].display->current_mode(),
-        gfx::Point()));
+        displays[i], displays[i]->current_mode(), gfx::Point()));
   }
 
   switch (new_display_state) {
     case MULTIPLE_DISPLAY_STATE_INVALID:
       NOTREACHED() << "Ignoring request to enter invalid state with "
                    << displays.size() << " connected display(s)";
       return false;
     case MULTIPLE_DISPLAY_STATE_HEADLESS:
       if (displays.size() != 0) {
         LOG(WARNING) << "Ignoring request to enter headless mode with "
                      << displays.size() << " connected display(s)";
         return false;
       }
       break;
     case MULTIPLE_DISPLAY_STATE_SINGLE: {
       // If there are multiple displays connected, only one should be turned on.
       if (displays.size() != 1 && num_on_displays != 1) {
         LOG(WARNING) << "Ignoring request to enter single mode with "
                      << displays.size() << " connected displays and "
                      << num_on_displays << " turned on";
         return false;
       }
 
-      for (size_t i = 0; i < displays.size(); ++i) {
-        const DisplayConfigurator::DisplayState* state = &displays[i];
+      for (size_t i = 0; i < states.size(); ++i) {
+        const DisplayState* state = &states[i];
         (*requests)[i].mode = display_power[i] ? state->selected_mode : NULL;
 
-        if (display_power[i] || displays.size() == 1) {
+        if (display_power[i] || states.size() == 1) {
           const DisplayMode* mode_info = state->selected_mode;
           if (!mode_info) {
             LOG(WARNING) << "No selected mode when configuring display: "
                          << state->display->ToString();
             return false;
           }
           if (mode_info->size() == gfx::Size(1024, 768)) {
             VLOG(1) << "Potentially misdetecting display(1024x768):"
-                    << " displays size=" << displays.size()
+                    << " displays size=" << states.size()
                     << ", num_on_displays=" << num_on_displays
                     << ", current size:" << size.width() << "x" << size.height()
                     << ", i=" << i << ", display=" << state->display->ToString()
                     << ", display_mode=" << mode_info->ToString();
           }
           size = mode_info->size();
         }
       }
       break;
     }
     case MULTIPLE_DISPLAY_STATE_DUAL_MIRROR: {
-      if (displays.size() != 2 ||
+      if (states.size() != 2 ||
           (num_on_displays != 0 && num_on_displays != 2)) {
         LOG(WARNING) << "Ignoring request to enter mirrored mode with "
-                     << displays.size() << " connected display(s) and "
+                     << states.size() << " connected display(s) and "
                      << num_on_displays << " turned on";
         return false;
       }
 
-      const DisplayMode* mode_info = displays[0].mirror_mode;
+      const DisplayMode* mode_info = states[0].mirror_mode;
       if (!mode_info) {
         LOG(WARNING) << "No mirror mode when configuring display: "
-                     << displays[0].display->ToString();
+                     << states[0].display->ToString();
         return false;
       }
       size = mode_info->size();
 
-      for (size_t i = 0; i < displays.size(); ++i) {
-        const DisplayConfigurator::DisplayState* state = &displays[i];
+      for (size_t i = 0; i < states.size(); ++i) {
+        const DisplayState* state = &states[i];
         (*requests)[i].mode = display_power[i] ? state->mirror_mode : NULL;
       }
       break;
     }
     case MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED:
     case MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED: {
       if ((new_display_state == MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED &&
-           displays.size() != 2) ||
+           states.size() != 2) ||
           (new_display_state == MULTIPLE_DISPLAY_STATE_MULTI_EXTENDED &&
-           displays.size() <= 2) ||
+           states.size() <= 2) ||
           (num_on_displays != 0 &&
            num_on_displays != static_cast<int>(displays.size()))) {
         LOG(WARNING) << "Ignoring request to enter extended mode with "
-                     << displays.size() << " connected display(s) and "
+                     << states.size() << " connected display(s) and "
                      << num_on_displays << " turned on";
         return false;
       }
 
-      for (size_t i = 0; i < displays.size(); ++i) {
-        const DisplayConfigurator::DisplayState* state = &displays[i];
+      for (size_t i = 0; i < states.size(); ++i) {
+        const DisplayState* state = &states[i];
         (*requests)[i].origin.set_y(size.height() ? size.height() + kVerticalGap
                                                   : 0);
         (*requests)[i].mode = display_power[i] ? state->selected_mode : NULL;
 
         // Retain the full screen size even if all displays are off so the
         // same desktop configuration can be restored when the displays are
         // turned back on.
-        const DisplayMode* mode_info = displays[i].selected_mode;
+        const DisplayMode* mode_info = states[i].selected_mode;
         if (!mode_info) {
           LOG(WARNING) << "No selected mode when configuring display: "
                        << state->display->ToString();
           return false;
         }
 
         size.set_width(std::max<int>(size.width(), mode_info->size().width()));
         size.set_height(size.height() + (size.height() ? kVerticalGap : 0) +
                         mode_info->size().height());
       }
@@ skipping 212 matching lines @@
   configuration_task_->Run();
 }
 
 bool DisplayConfigurator::IsMirroring() const {
   return current_display_state_ == MULTIPLE_DISPLAY_STATE_DUAL_MIRROR ||
          (mirroring_controller_ &&
           mirroring_controller_->SoftwareMirroringEnabled());
 }
 
 bool DisplayConfigurator::ApplyProtections(const ContentProtections& requests) {
   for (DisplayStateList::const_iterator it = cached_displays_.begin();

[dnicoara, 2015/03/18 20:01:49]

I was thinking of converting these for-loops to c++11 style for better clarity.
I was thinking of doing it as a followup to keep this CL small. What do you
think?

[oshima, 2015/03/18 23:02:45]

sg

        it != cached_displays_.end();
        ++it) {
     uint32_t all_desired = 0;
 
     // In mirror mode, protection request of all displays need to be fulfilled.
     // In non-mirror mode, only request of client's display needs to be
     // fulfilled.
     ContentProtections::const_iterator request_it;
     if (IsMirroring()) {
       for (request_it = requests.begin();
            request_it != requests.end();
            ++request_it)
         all_desired |= request_it->second;
     } else {
-      request_it = requests.find(it->display->display_id());
+      request_it = requests.find((*it)->display_id());
       if (request_it != requests.end())
         all_desired = request_it->second;
     }
 
-    switch (it->display->type()) {
+    switch ((*it)->type()) {
       case DISPLAY_CONNECTION_TYPE_UNKNOWN:
         return false;
       // DisplayPort, DVI, and HDMI all support HDCP.
       case DISPLAY_CONNECTION_TYPE_DISPLAYPORT:
       case DISPLAY_CONNECTION_TYPE_DVI:
       case DISPLAY_CONNECTION_TYPE_HDMI: {
         HDCPState current_state;
         // Need to poll the driver for updates since other applications may
         // have updated the state.
-        if (!native_display_delegate_->GetHDCPState(*it->display,
-                                                    &current_state))
+        if (!native_display_delegate_->GetHDCPState(**it, &current_state))
           return false;
         bool current_desired = (current_state != HDCP_STATE_UNDESIRED);
         bool new_desired = (all_desired & CONTENT_PROTECTION_METHOD_HDCP);
         // Don't enable again if HDCP is already active. Some buggy drivers
         // may disable and enable if setting "desired" in active state.
         if (current_desired != new_desired) {
           HDCPState new_state =
               new_desired ? HDCP_STATE_DESIRED : HDCP_STATE_UNDESIRED;
-          if (!native_display_delegate_->SetHDCPState(*it->display, new_state))
+          if (!native_display_delegate_->SetHDCPState(**it, new_state))
             return false;
         }
         break;
       }
       case DISPLAY_CONNECTION_TYPE_INTERNAL:
       case DISPLAY_CONNECTION_TYPE_VGA:
       case DISPLAY_CONNECTION_TYPE_NETWORK:
         // No protections for these types. Do nothing.
         break;
       case DISPLAY_CONNECTION_TYPE_NONE:
@@ skipping 40 matching lines @@
   if (!configure_display_ || display_externally_controlled_)
     return false;
 
   uint32_t enabled = 0;
   uint32_t unfulfilled = 0;
   *link_mask = 0;
   for (DisplayStateList::const_iterator it = cached_displays_.begin();
        it != cached_displays_.end();
        ++it) {
     // Query display if it is in mirror mode or client on the same display.
-    if (!IsMirroring() && it->display->display_id() != display_id)
+    if (!IsMirroring() && (*it)->display_id() != display_id)
       continue;
 
-    *link_mask |= it->display->type();
-    switch (it->display->type()) {
+    *link_mask |= (*it)->type();
+    switch ((*it)->type()) {
       case DISPLAY_CONNECTION_TYPE_UNKNOWN:
         return false;
       // DisplayPort, DVI, and HDMI all support HDCP.
       case DISPLAY_CONNECTION_TYPE_DISPLAYPORT:
       case DISPLAY_CONNECTION_TYPE_DVI:
       case DISPLAY_CONNECTION_TYPE_HDMI: {
         HDCPState state;
-        if (!native_display_delegate_->GetHDCPState(*it->display, &state))
+        if (!native_display_delegate_->GetHDCPState(**it, &state))
           return false;
         if (state == HDCP_STATE_ENABLED)
           enabled |= CONTENT_PROTECTION_METHOD_HDCP;
         else
           unfulfilled |= CONTENT_PROTECTION_METHOD_HDCP;
         break;
       }
       case DISPLAY_CONNECTION_TYPE_INTERNAL:
       case DISPLAY_CONNECTION_TYPE_VGA:
       case DISPLAY_CONNECTION_TYPE_NETWORK:
@@ skipping 55 matching lines @@
   }
 
   return true;
 }
 
 std::vector<ui::ColorCalibrationProfile>
 DisplayConfigurator::GetAvailableColorCalibrationProfiles(int64_t display_id) {
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableDisplayColorCalibration)) {
     for (size_t i = 0; i < cached_displays_.size(); ++i) {
-      if (cached_displays_[i].display &&
-          cached_displays_[i].display->display_id() == display_id) {
+      if (cached_displays_[i]->display_id() == display_id) {
         return native_display_delegate_->GetAvailableColorCalibrationProfiles(
-            *cached_displays_[i].display);
+            *cached_displays_[i]);
       }
     }
   }
 
   return std::vector<ui::ColorCalibrationProfile>();
 }
 
 bool DisplayConfigurator::SetColorCalibrationProfile(
     int64_t display_id,
     ui::ColorCalibrationProfile new_profile) {
   for (size_t i = 0; i < cached_displays_.size(); ++i) {
-    if (cached_displays_[i].display &&
-        cached_displays_[i].display->display_id() == display_id) {
+    if (cached_displays_[i]->display_id() == display_id) {
       return native_display_delegate_->SetColorCalibrationProfile(
-          *cached_displays_[i].display, new_profile);
+          *cached_displays_[i], new_profile);
     }
   }
 
   return false;
 }
 
 void DisplayConfigurator::PrepareForExit() {
   configure_display_ = false;
 }
 
@@ skipping 151 matching lines @@
   requested_display_state_ = MULTIPLE_DISPLAY_STATE_INVALID;
 
   DCHECK(in_progress_configuration_callbacks_.empty());
   in_progress_configuration_callbacks_.swap(queued_configuration_callbacks_);
 
   configuration_task_->Run();
 }
 
 void DisplayConfigurator::OnConfigured(
     bool success,
-    const std::vector<DisplayState>& displays,
+    const std::vector<DisplaySnapshot*>& displays,
     const gfx::Size& framebuffer_size,
     MultipleDisplayState new_display_state,
     chromeos::DisplayPowerState new_power_state) {
   VLOG(1) << "OnConfigured: success=" << success << " new_display_state="
           << MultipleDisplayStateToString(new_display_state)
           << " new_power_state=" << DisplayPowerStateToString(new_power_state);
 
   cached_displays_ = displays;
   if (success) {
     current_display_state_ = new_display_state;
@@ skipping 68 matching lines @@
     FOR_EACH_OBSERVER(
         Observer, observers_, OnDisplayModeChanged(cached_displays_));
   } else {
     FOR_EACH_OBSERVER(
         Observer, observers_, OnDisplayModeChangeFailed(cached_displays_,
                                                         attempted_state));
   }
 }
 
 }  // namespace ui