Fix errors in display resolution change notifications

ash/display/resolution_notification_controller.h

Index: ash/display/resolution_notification_controller.h
diff --git a/ash/display/resolution_notification_controller.h b/ash/display/resolution_notification_controller.h
index 8ac70473323293d1bb03ea4661039c57e0916c18..a445f39206d3ad86295b0fcc9782b659ce28378d 100644
--- a/ash/display/resolution_notification_controller.h
+++ b/ash/display/resolution_notification_controller.h
@@ -31,23 +31,34 @@ class ASH_EXPORT ResolutionNotificationController
   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.