Delay display configuration after waking up from suspend with multi displays

ui/display/chromeos/display_configurator.h

 // 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.
 
 #ifndef UI_DISPLAY_CHROMEOS_DISPLAY_CONFIGURATOR_H_
 #define UI_DISPLAY_CHROMEOS_DISPLAY_CONFIGURATOR_H_
 
 #include <stdint.h>
 
 #include <map>
@@ skipping 24 matching lines @@
 
 namespace ui {
 struct DisplayConfigureRequest;
 struct GammaRampRGBEntry;
 class DisplayLayoutManager;
 class DisplayMode;
 class DisplaySnapshot;
 class NativeDisplayDelegate;
 class UpdateDisplayConfigurationTask;
 
+namespace test {
+class DisplayConfiguratorTest;
+}  // namespace
+
 // This class interacts directly with the system display configurator.
 class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
  public:
   typedef uint64_t ContentProtectionClientId;
   static const ContentProtectionClientId kInvalidClientId = 0;
 
   typedef base::Callback<void(bool /* success */)> ConfigurationCallback;
 
   typedef base::Callback<void(bool /* success */)> EnableProtectionCallback;
 
@@ skipping 105 matching lines @@
 
   // Returns the mode within |display| that matches the given size with highest
   // refresh rate. Returns None if no matching display was found.
   static const DisplayMode* FindDisplayModeMatchingSize(
       const DisplaySnapshot& display,
       const gfx::Size& size);
 
   DisplayConfigurator();
   ~DisplayConfigurator() override;
 
-  MultipleDisplayState display_state() const { return current_display_state_; }
-  chromeos::DisplayPowerState requested_power_state() const {
-    return requested_power_state_;
-  }
-  const gfx::Size framebuffer_size() const { return framebuffer_size_; }
-  const std::vector<DisplaySnapshot*>& cached_displays() const {
-    return cached_displays_;
-  }
-
   // Called when an external process no longer needs to control the display
   // and Chrome can take control.
   void TakeControl(const DisplayControlCallback& callback);
 
   // Called when an external process needs to control the display and thus
   // Chrome should relinquish it.
   void RelinquishControl(const DisplayControlCallback& callback);
 
-  void set_state_controller(StateController* controller) {
-    state_controller_ = controller;
-  }
-  void set_mirroring_controller(SoftwareMirroringController* controller) {
-    mirroring_controller_ = controller;
-  }
-
   // Replaces |native_display_delegate_| with the delegate passed in and sets
   // |configure_display_| to true. Should be called before Init().
   void SetDelegateForTesting(
       std::unique_ptr<NativeDisplayDelegate> display_delegate);
 
   // Sets the initial value of |power_state_|.  Must be called before Start().
   void SetInitialDisplayPower(chromeos::DisplayPowerState power_state);
 
   // Initialization, must be called right after constructor.
   // |is_panel_fitting_enabled| indicates hardware panel fitting support.
@@ skipping 71 matching lines @@
   bool SetColorCalibrationProfile(int64_t display_id,
                                   ui::ColorCalibrationProfile new_profile);
 
   // Enables/disables virtual display.
   int64_t AddVirtualDisplay(const gfx::Size& display_size);
   bool RemoveVirtualDisplay(int64_t display_id);
 
   // Returns true if there is at least one display on.
   bool IsDisplayOn() const;
 
-  void set_configure_display(bool configure_display) {
-    configure_display_ = configure_display;
-  }
-
   // Sets the gamma, degamma and correction matrix for |display_id| to the
   // values in |degamma_lut|, |gamma_lut| and |correction_matrix|.
   bool SetColorCorrection(int64_t display_id,
                           const std::vector<GammaRampRGBEntry>& degamma_lut,
                           const std::vector<GammaRampRGBEntry>& gamma_lut,
                           const std::vector<float>& correction_matrix);
 
+  MultipleDisplayState display_state() const { return current_display_state_; }

[Daniel Erat, 2016/10/21 21:09:45]

please move all of these back below the d'tor -- chrome style is usually to put
inlined getters and setters above other methods.

[afakhry, 2016/10/22 00:28:48]

Really? I always thought it's the other way around! Apparently in our codebase
we have a mix of both styles (for example:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_process....
and
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/file_manager/vol...)

I moved them above as you suggested.

+  chromeos::DisplayPowerState requested_power_state() const {
+    return requested_power_state_;
+  }
+  const gfx::Size framebuffer_size() const { return framebuffer_size_; }
+  const std::vector<DisplaySnapshot*>& cached_displays() const {
+    return cached_displays_;
+  }
+  void set_state_controller(StateController* controller) {
+    state_controller_ = controller;
+  }
+  void set_mirroring_controller(SoftwareMirroringController* controller) {
+    mirroring_controller_ = controller;
+  }
+  void set_configure_display(bool configure_display) {
+    configure_display_ = configure_display;
+  }
+  chromeos::DisplayPowerState current_power_state() const {
+    return current_power_state_;
+  }
+
  private:
   class DisplayLayoutManagerImpl;
+  friend class test::DisplayConfiguratorTest;

[Daniel Erat, 2016/10/21 21:09:45]

please avoid listing test classes as friends; it often results in brittle tests
that need to be updated whenever the implementation is changed. if this is just
for the two constants below, please make them be public static members instead.

[afakhry, 2016/10/22 00:28:48]

It was actually more needed for ResumeDisplaysInternal(), since unit tests need
to provide a callback to know exactly when configuration was done. Here's what I
did based on your suggestion:
- Removed the friend.
- Moved the static constants to public.
- Made ResumeDisplaysInternal() --> ResumeDisplaysWithCallback() and moved it to
public.

+
+  // The delay to perform configuration after RRNotify. See the comment for
+  // |configure_timer_|.
+  static const int kConfigureDelayMs = 500;
+
+  // The delay to perform configuration after waking up from suspend when in
+  // multi display mode. Should be bigger than |kConfigureDelayMs|. Generally
+  // big enough for external displays to be detected and added.
+  // crbug.com/614624.
+  static const int kResumeConfigureMultiDisplayDelayMs = 2000;
 
   // Mapping a client to its protection request.
   typedef std::map<ContentProtectionClientId, ContentProtections>
       ProtectionRequests;
 
   // Updates |pending_*| members and applies the passed-in state. |callback| is
   // invoked (perhaps synchronously) on completion.
   void SetDisplayPowerInternal(chromeos::DisplayPowerState power_state,
                                int flags,
                                const ConfigurationCallback& callback);
@@ skipping 53 matching lines @@
                                   bool success);
   void OnContentProtectionClientUnregistered(bool success);
 
   // Callbacks used to signal when the native platform has released/taken
   // display control.
   void OnDisplayControlTaken(const DisplayControlCallback& callback,
                              bool success);
   void OnDisplayControlRelinquished(const DisplayControlCallback& callback,
                                     bool success);
 
+  void ResumeDisplaysInternal(const ConfigurationCallback& callback);

[Daniel Erat, 2016/10/21 21:09:45]

document this method (what it does and who calls it)

[afakhry, 2016/10/22 00:28:48]

Done. I made it public and moved it next to ResumeDisplays() and renamed it to
ResumeDisplaysWithCallback().

+
   StateController* state_controller_;
   SoftwareMirroringController* mirroring_controller_;
   std::unique_ptr<NativeDisplayDelegate> native_display_delegate_;
 
   // Used to enable modes which rely on panel fitting.
   bool is_panel_fitting_enabled_;
 
   // This is detected by the constructor to determine whether or not we should
   // be enabled.  If we aren't running on Chrome OS, we can't assume that the
   // Xrandr X11 extension or the Ozone underlying display hotplug system are
@@ skipping 83 matching lines @@
 
   // This must be the last variable.
   base::WeakPtrFactory<DisplayConfigurator> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DisplayConfigurator);
 };
 
 }  // namespace ui
 
 #endif  // UI_DISPLAY_CHROMEOS_DISPLAY_CONFIGURATOR_H_