Fix errors in display resolution change notifications

ash/display/resolution_notification_controller.h

 // Copyright 2013 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 ASH_DISPLAY_RESOLUTION_NOTIFICATION_CONTROLLER_H_
 #define ASH_DISPLAY_RESOLUTION_NOTIFICATION_CONTROLLER_H_
 
 #include <stdint.h>
 
 #include "ash/ash_export.h"
@@ skipping 13 matching lines @@
 
 // A class which manages the notification of display resolution change and
 // also manages the timeout in case the new resolution is unusable.
 class ASH_EXPORT ResolutionNotificationController
     : public display::DisplayObserver,
       public WindowTreeHostManager::Observer {
  public:
   ResolutionNotificationController();
   ~ResolutionNotificationController() override;
 
-  // Prepare a resolution change notification for |display_id| from
-  // |old_resolution| to |new_resolution|, which offers a button to revert the
-  // change in case something goes wrong. The notification times out if there's
-  // only one display connected and the user is trying to modify its resolution.
-  // In that case, the timeout has to be set since the user cannot make any
-  // changes if something goes wrong.
+  // If |display_id| is not the internal display, Prepare a resolution change
+  // notification for |display_id| from |old_resolution| to |new_resolution|,
+  // which offers a button to revert the change in case something goes wrong.
+  // The notification times out if there's only one display connected and the
+  // user is trying to modify its resolution. In that case, the timeout has to
+  // be set since the user cannot make any changes if something goes wrong.
+  //
+  // Then call DisplayManager::SetDisplayMode() to apply the resolution change,
+  // and return the result; true if success, false otherwise.
+  // In case SetDisplayMode() fails, the prepared notification will be
+  // discarded.
+  //
+  // If |display_id| is the internal display, the resolution change is applied
+  // directly without preparing the confirm/revert notification (this kind of
+  // notification is only useful for external displays).
   //
   // This method does not create a notification itself. The notification will be
   // created the next OnDisplayConfigurationChanged(), which will be called
-  // asynchronously after the resolution change is requested. So typically this
-  // method will be combined with resolution change methods like
-  // DisplayManager::SetDisplayMode().
-  void PrepareNotification(
+  // asynchronously after the resolution change is requested by this method.
+  //
+  // |accept_callback| will be called when the user accepts the resoltion change
+  // by closing the notification bubble or clicking on the accept button (if
+  // any).
+  bool PrepareNotificationAndSetDisplayMode(
       int64_t display_id,
       const scoped_refptr<display::ManagedDisplayMode>& old_resolution,
       const scoped_refptr<display::ManagedDisplayMode>& new_resolution,
-      const base::Closure& accept_callback);
+      const base::Closure& accept_callback) WARN_UNUSED_RESULT;

[stevenjb, 2017/04/10 17:33:52]

I like this change and the explanation, but should we move this into a helper
file, e.g. display_util? (Maybe keep PrepareNotification more-or-less as it
was?)

[oshima, 2017/04/10 17:46:18]

I don't have strong opinion, and leave it to you guys.

[afakhry, 2017/04/10 23:26:21]

I also don't have a strong opinion about this, but one of the pros that I see
for keeping these changes here instead of display_util, is that now users are
forced to use this API correctly, no other way is provided.

Also, moving to display_util, we'll need to add a function to discard the
prepared notification data if SetDisplayMode() fails as opposed to keeping that
internal here.

Let me think about it more...

 
   // Returns true if the notification is visible or scheduled to be visible and
   // the notification times out.
   bool DoesNotificationTimeout();
 
   // Called by the notification delegate when the user accepts the display
   // resolution change. Set |close_notification| to true when the notification
   // should be removed.
   void AcceptResolutionChange(bool close_notification);
 
@@ skipping 33 matching lines @@
   static void SuppressTimerForTest();
 
   std::unique_ptr<ResolutionChangeInfo> change_info_;
 
   DISALLOW_COPY_AND_ASSIGN(ResolutionNotificationController);
 };
 
 }  // namespace ash
 
 #endif  // ASH_DISPLAY_RESOLUTION_NOTIFICATION_CONTROLLER_H_