Delay display configuration after waking up from suspend with multi displays

ui/display/chromeos/display_configurator.h

Index: ui/display/chromeos/display_configurator.h
diff --git a/ui/display/chromeos/display_configurator.h b/ui/display/chromeos/display_configurator.h
index 9fa5b41b987fccc7c31d10713fcacbf0944b6d8b..05a98b5be61c43b3874bcd895880857cf2dfa133 100644
--- a/ui/display/chromeos/display_configurator.h
+++ b/ui/display/chromeos/display_configurator.h
@@ -42,6 +42,10 @@ 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:
@@ -167,15 +171,6 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   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);
@@ -184,13 +179,6 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   // 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(
@@ -282,10 +270,6 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   // 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,
@@ -293,8 +277,40 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
                           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>
@@ -368,6 +384,8 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   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_;