Refactoring display configuration state to allow generic state objects

chromeos/display/output_configurator.h

Index: chromeos/display/output_configurator.h
diff --git a/chromeos/display/output_configurator.h b/chromeos/display/output_configurator.h
index b063caa11571f3f1d3e90fd3d3cf25bb9b2ffb38..fa78655ce7db53c9d182432304e059a4bae74ebe 100644
--- a/chromeos/display/output_configurator.h
+++ b/chromeos/display/output_configurator.h
@@ -15,40 +15,31 @@
 #include "base/observer_list.h"
 #include "base/timer/timer.h"
 #include "chromeos/chromeos_export.h"
-#include "chromeos/display/native_display_observer.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
+#include "ui/display/chromeos/native_display_observer.h"
 #include "ui/display/display_constants.h"
 
-// Forward declarations for Xlib and Xrandr.
-// This is so unused X definitions don't pollute the namespace.
-typedef unsigned long XID;
-typedef XID RROutput;
-typedef XID RRCrtc;
-typedef XID RRMode;
-
-namespace chromeos {
+namespace gfx {
+class Point;
+class Size;
+}
 
+namespace ui {
+class DisplayMode;
+class DisplaySnapshot;
 class NativeDisplayDelegate;
+}
+
+namespace chromeos {
 
 // This class interacts directly with the underlying Xrandr API to manipulate
 // CTRCs and Outputs.
-class CHROMEOS_EXPORT OutputConfigurator
-    : public NativeDisplayObserver {
+class CHROMEOS_EXPORT OutputConfigurator : public ui::NativeDisplayObserver {
  public:
   typedef uint64_t OutputProtectionClientId;
   static const OutputProtectionClientId kInvalidClientId = 0;
 
-  struct ModeInfo {
-    ModeInfo();
-    ModeInfo(int width, int height, bool interlaced, float refresh_rate);
-
-    int width;
-    int height;
-    bool interlaced;
-    float refresh_rate;
-  };
-
-  typedef std::map<RRMode, ModeInfo> ModeInfoMap;
+  typedef std::vector<const ui::DisplayMode*> DisplayModeList;

[Daniel Erat, 2014/03/05 01:56:12]

doesn't look like you're using this anywhere in the header -- did you mean for
it to be an std::vector<InternalDisplayState> instead?

[dnicoara, 2014/03/05 17:29:25]

Done.

I was using this in some of the ash/ files that depend on this. But on further
analysis it makes more sense to typedef as you've mentioned since it covers more
of the ash/ changes.

 
   struct CoordinateTransformation {
     // Initialized to the identity transformation.
@@ -60,58 +51,21 @@ class CHROMEOS_EXPORT OutputConfigurator
     float y_offset;
   };
 
-  // Information about an output's current state.
-  struct OutputSnapshot {
-    OutputSnapshot();
-    ~OutputSnapshot();
-
-    RROutput output;
-
-    // CRTC that should be used for this output. Not necessarily the CRTC
-    // that XRandR reports is currently being used.
-    RRCrtc crtc;
-
-    // Mode currently being used by the output.
-    RRMode current_mode;
-
-    // "Best" mode supported by the output.
-    RRMode native_mode;
-
-    // Mode used when displaying the same desktop on multiple outputs.
-    RRMode mirror_mode;
-
-    // User-selected mode for the output.
-    RRMode selected_mode;
-
-    // Output's origin on the framebuffer.
-    int x;
-    int y;
-
-    // Output's physical dimensions.
-    uint64 width_mm;
-    uint64 height_mm;
-
-    bool is_aspect_preserving_scaling;
+  struct InternalDisplayState {

[Daniel Erat, 2014/03/05 01:56:12]

i think that the word "internal" here may be confusing, as it could imply that
this only applies to internal displays. "AdditionalDisplayState"?
"ExtraDisplayState"? or maybe just "DisplayState" is good enough?

[dnicoara, 2014/03/05 17:29:25]

Done.

+    InternalDisplayState();
 
-    // The type of output.
-    ui::OutputType type;
-
-    // Map from mode IDs to details about the corresponding modes.
-    ModeInfoMap mode_infos;
+    ui::DisplaySnapshot* display;  // Not owned.
 
     // XInput device ID or 0 if this output isn't a touchscreen.
     int touch_device_id;
 
     CoordinateTransformation transform;
 
-    // Display id for this output.
-    int64 display_id;
-
-    bool has_display_id;
+    // User-selected mode for the output.
+    const ui::DisplayMode* selected_mode;

[dnicoara, 2014/03/05 00:06:34]

Since this (and mirror_mode) is OutputConfigurator specific code I thought it
would be good to keep it here rather than in  DisplaySnapshot. I'm also not
convinced that we need to keep track of these, though I didn't want to add too
much complexity to this CL.

 
-    // This output's index in the array returned by XRandR. Stable even as
-    // outputs are connected or disconnected.
-    int index;
+    // Mode used when displaying the same desktop on multiple outputs.
+    const ui::DisplayMode* mirror_mode;
   };
 
   class Observer {
@@ -123,7 +77,7 @@ class CHROMEOS_EXPORT OutputConfigurator
     // when this method is called, so the actual configuration could've changed
     // already.
     virtual void OnDisplayModeChanged(
-        const std::vector<OutputSnapshot>& outputs) {}
+        const std::vector<InternalDisplayState>& outputs) {}
 
     // Called after a display mode change attempt failed. |failed_new_state| is
     // the new state which the system failed to enter.
@@ -143,8 +97,7 @@ class CHROMEOS_EXPORT OutputConfigurator
     // Queries the resolution (|width|x|height|) in pixels

[Daniel Erat, 2014/03/05 01:56:12]

nit: update this comment

[dnicoara, 2014/03/05 17:29:25]

Done.

     // to select output mode for the given display id.
     virtual bool GetResolutionForDisplayId(int64 display_id,
-                                           int* width,
-                                           int* height) const = 0;
+                                           gfx::Size* size) const = 0;
   };
 
   // Interface for classes that implement software based mirroring.
@@ -166,7 +119,7 @@ class CHROMEOS_EXPORT OutputConfigurator
     // If a touchscreen with same resolution as an output's native mode
     // is detected, its id will be stored in this output.
     virtual void AssociateTouchscreens(
-        std::vector<OutputSnapshot>* outputs) = 0;
+        std::vector<InternalDisplayState>* outputs) = 0;
 
     // Configures XInput's Coordinate Transformation Matrix property.
     // |touch_device_id| the ID of the touchscreen device to configure.
@@ -211,23 +164,18 @@ class CHROMEOS_EXPORT OutputConfigurator
   // See crbug.com/130188 for initial discussion.
   static const int kVerticalGap = 60;
 
-  // Returns a pointer to the ModeInfo struct in |output| corresponding to
-  // |mode|, or NULL if the struct isn't present.
-  static const ModeInfo* GetModeInfo(const OutputSnapshot& output,
-                                     RRMode mode);
-
   // Returns the mode within |output| that matches the given size with highest
   // refresh rate. Returns None if no matching output was found.
-  static RRMode FindOutputModeMatchingSize(const OutputSnapshot& output,
-                                           int width,
-                                           int height);
+  static const ui::DisplayMode* FindDisplayModeMatchingSize(
+      const ui::DisplaySnapshot& output,
+      const gfx::Size& size);
 
   OutputConfigurator();
   virtual ~OutputConfigurator();
 
   ui::OutputState output_state() const { return output_state_; }
   DisplayPowerState power_state() const { return power_state_; }
-  const std::vector<OutputSnapshot>& cached_outputs() const {
+  const std::vector<InternalDisplayState>& cached_outputs() const {
     return cached_outputs_;
   }
 
@@ -241,7 +189,7 @@ class CHROMEOS_EXPORT OutputConfigurator
   // Replaces |native_display_delegate_| with |delegate| and sets
   // |configure_display_| to true.  Should be called before Init().
   void SetNativeDisplayDelegateForTesting(
-      scoped_ptr<NativeDisplayDelegate> delegate);
+      scoped_ptr<ui::NativeDisplayDelegate> delegate);
 
   void SetTouchscreenDelegateForTesting(
       scoped_ptr<TouchscreenDelegate> delegate);
@@ -343,8 +291,8 @@ class CHROMEOS_EXPORT OutputConfigurator
   //
   // |preserve_aspect| limits the search/creation only to the modes having the
   // native aspect ratio of |external_output|.
-  bool FindMirrorMode(OutputSnapshot* internal_output,
-                      OutputSnapshot* external_output,
+  bool FindMirrorMode(InternalDisplayState* internal_output,
+                      InternalDisplayState* external_output,
                       bool try_panel_fitting,
                       bool preserve_aspect);
 
@@ -375,29 +323,27 @@ class CHROMEOS_EXPORT OutputConfigurator
   // Computes the relevant transformation for mirror mode.
   // |output| is the output on which mirror mode is being applied.
   // Returns the transformation or identity if computations fail.
-  CoordinateTransformation GetMirrorModeCTM(
-      const OutputConfigurator::OutputSnapshot& output);
+  CoordinateTransformation GetMirrorModeCTM(const InternalDisplayState& output);
 
   // Computes the relevant transformation for extended mode.
   // |output| is the output on which extended mode is being applied.
   // |width| and |height| are the width and height of the combined framebuffer.

[Daniel Erat, 2014/03/05 01:56:12]

nit: update this comment

[dnicoara, 2014/03/05 17:29:25]

Done.

   // Returns the transformation or identity if computations fail.
   CoordinateTransformation GetExtendedModeCTM(
-      const OutputConfigurator::OutputSnapshot& output,
-      int framebuffer_width,
-      int frame_buffer_height);
+      const InternalDisplayState& output,
+      const gfx::Point& new_origin,
+      const gfx::Size& framebuffer_size);
 
   // Returns the ratio between mirrored mode area and native mode area:
   // (mirror_mode_width * mirrow_mode_height) / (native_width * native_height)
-  float GetMirroredDisplayAreaRatio(
-      const OutputConfigurator::OutputSnapshot& output);
+  float GetMirroredDisplayAreaRatio(const InternalDisplayState& output);
 
   // Applies output protections according to requests.
   bool ApplyProtections(const DisplayProtections& requests);
 
   StateController* state_controller_;
   SoftwareMirroringController* mirroring_controller_;
-  scoped_ptr<NativeDisplayDelegate> native_display_delegate_;
+  scoped_ptr<ui::NativeDisplayDelegate> native_display_delegate_;
   scoped_ptr<TouchscreenDelegate> touchscreen_delegate_;
 
   // Used to enable modes which rely on panel fitting.
@@ -426,7 +372,7 @@ class CHROMEOS_EXPORT OutputConfigurator
 
   // Most-recently-used output configuration. Note that the actual
   // configuration changes asynchronously.
-  std::vector<OutputSnapshot> cached_outputs_;
+  std::vector<InternalDisplayState> cached_outputs_;
 
   ObserverList<Observer> observers_;
 
@@ -443,8 +389,6 @@ class CHROMEOS_EXPORT OutputConfigurator
   DISALLOW_COPY_AND_ASSIGN(OutputConfigurator);
 };
 
-typedef std::vector<OutputConfigurator::OutputSnapshot> OutputSnapshotList;
-
 }  // namespace chromeos
 
 #endif  // CHROMEOS_DISPLAY_OUTPUT_CONFIGURATOR_H_