Creates notifications for display resolution change.

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 "ash/ash_export.h"
+#include "ash/display/display_controller.h"
+#include "base/callback.h"
+#include "base/gtest_prod_util.h"
+#include "base/timer/timer.h"
+#include "ui/gfx/display_observer.h"
+#include "ui/gfx/size.h"
+
+namespace views {
+class Label;
+class Widget;
+}  // namespace views
+
+namespace ash {
+namespace internal {
+// A class which manages the notification of display resolution change and
+// also manages the timeout if the resolution change is potentially dangerous.

[Daniel Erat, 2013/08/09 15:50:24]

nit: "potentially dangerous" seems like too-strong wording. it would be better
to say something like "if the new resolution is unusable".

[Jun Mukai, 2013/08/09 16:49:16]

done, but I think 'in case' would be better than 'if'. correct me if it's not.

[Daniel Erat, 2013/08/09 16:55:46]

"in case" seems more precise to me too :-)

+class ASH_EXPORT ResolutionNotificationController
+    : public gfx::DisplayObserver,
+      public DisplayController::Observer {
+ public:
+  ResolutionNotificationController();
+  virtual ~ResolutionNotificationController();
+
+  // Updates the display resolution for |display_id| to |new_resolution| and
+  // creates a notification for this change which offers a button to revert the
+  // change in case something goes wrong. The notification timeouts if the

[Daniel Erat, 2013/08/09 15:50:24]

nit: s/timeouts if/times out in case/

("timeout" is a noun; "time out" is a verb)

[Jun Mukai, 2013/08/09 16:49:16]

Done.

+  // display resolution change operation is potentially dangerous, i.e. there's

[Daniel Erat, 2013/08/09 15:50:24]

same here -- "potentially dangerous" makes it sound like choosing the wrong
resolution could damage hardware

[Jun Mukai, 2013/08/09 16:49:16]

just removed that sentence and write the condition directly.

+  // 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
+  // operations if something goes wrong.

[Daniel Erat, 2013/08/09 15:50:24]

nit: s/operations/changes/

[Jun Mukai, 2013/08/09 16:49:16]

Done.

+  void SetDisplayResolutionAndNotify(
+      int64 display_id,
+      const gfx::Size& old_resolution,
+      const gfx::Size& new_resolution,
+      const base::Closure& accept_callback);
+
+  // Returns true if the notification is visible or scheduled to be visible and
+  // the notification timeouts.

[Daniel Erat, 2013/08/09 15:50:24]

nit: s/timeouts/times out/

[Jun Mukai, 2013/08/09 16:49:16]

Done.

+  bool DoesNotificationTimeout();
+
+  // Called by the notification delegate when the user accepts the display
+  // resolution change.
+  void AcceptResolutionChange();
+
+  // Called by the notification delegate when the user wants to revert the
+  // display resolution change.
+  void RevertResolutionChange();
+
+ private:
+  friend class ResolutionNotificationControllerTest;
+  FRIEND_TEST_ALL_PREFIXES(ResolutionNotificationControllerTest, Timeout);
+
+  // A struct to bundle the data for a single resolution change.
+  struct ResolutionChangeInfo {
+    // The id of the display where the resolution change happens.
+    int64 display_id;
+
+    // The resolution before the change.
+    gfx::Size old_resolution;
+
+    // The new resolution after the change.
+    gfx::Size new_resolution;
+
+    // The callback when accept is chosen.
+    base::Closure accept_callback;
+
+    // The remaining timeout in seconds. 0 if the change does not timeout.

[Daniel Erat, 2013/08/09 15:50:24]

nit: s/does not timeout/does not time out/

[Jun Mukai, 2013/08/09 16:49:16]

Done.

+    uint8 timeout_count;
+
+    // The timer to invoke OnTimerTick() every second. This cannot be
+    // OneShotTimer since the message contains text "automatically closed in xx
+    // seconds..." which has to be updated every second.
+    base::RepeatingTimer<ResolutionNotificationController> timer;
+
+    ResolutionChangeInfo(int64 display_id,

[Daniel Erat, 2013/08/09 15:50:24]

nit: move c'tor and d'tor to top of members

[Jun Mukai, 2013/08/09 16:49:16]

Done.

+                         const gfx::Size& old_resolution,
+                         const gfx::Size& new_resolution,
+                         const base::Closure& accept_callback);
+    ~ResolutionChangeInfo();
+  };

[Daniel Erat, 2013/08/09 15:50:24]

nit: DISALLOW_COPY_AND_ASSIGN() (Timer isn't copyable)

[Jun Mukai, 2013/08/09 16:49:16]

Done.

+
+  // Create a new notification, or update its content if it already exists.
+  void CreateOrUpdateNotification();
+
+  // Called every second for timeout.
+  void OnTimerTick();
+
+  // gfx::DisplayObserver overrides:
+  virtual void OnDisplayBoundsChanged(const gfx::Display& display) OVERRIDE;
+  virtual void OnDisplayAdded(const gfx::Display& new_display) OVERRIDE;
+  virtual void OnDisplayRemoved(const gfx::Display& old_display) OVERRIDE;
+
+  // DisplayController::Observer overrides:
+  virtual void OnDisplayConfigurationChanged() OVERRIDE;
+
+  static void SuppressTimerForTest();
+
+  static const int kTimeoutInSec;

[Daniel Erat, 2013/08/09 15:50:24]

nit: static const members should be near the top of the private section
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...)

[Jun Mukai, 2013/08/09 16:49:16]

moved.

+  static const char kNotificationId[];
+
+  scoped_ptr<ResolutionChangeInfo> change_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(ResolutionNotificationController);
+};
+
+}  // namespace internal
+}  // namespace ash
+
+#endif  // ASH_DISPLAY_RESOLUTION_NOTIFICATION_CONTROLLER_H_