Suppressed screen rotation notifications triggeres by the accelerometer

ash/system/chromeos/tray_display.cc

Index: ash/system/chromeos/tray_display.cc
diff --git a/ash/system/chromeos/tray_display.cc b/ash/system/chromeos/tray_display.cc
index b5814c1304f39c7bdc5835b17bd001905e128155..f84d6d7ceee8753d4063f5b08662946b6c8d5f64 100644
--- a/ash/system/chromeos/tray_display.cc
+++ b/ash/system/chromeos/tray_display.cc
@@ -14,6 +14,7 @@
 #include "ash/system/tray/system_tray_delegate.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_notification_view.h"
+#include "ash/wm/maximize_mode/maximize_mode_controller.h"
 #include "base/bind.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -125,6 +126,55 @@ void OpenSettings() {
   }
 }
 
+// A message centre notification blocker used to suppress display notifications.
+// NOTE: This class should NOT be inherited because the destructor causes
+// observers to call back in to the instance being destroyed.
+class ScopedNotificationBlocker
+    : public message_center::NotificationBlocker {
+ public:
+  ScopedNotificationBlocker();
+  virtual ~ScopedNotificationBlocker();
+
+  // Overrides from message_center::NotificationBlocker:

[jonross, 2014/05/14 01:00:39]

nit: use the newer style

// message_center::NotificationBlocker:

[bruthig, 2014/05/14 18:48:49]

Done.

+  virtual bool ShouldShowNotificationAsPopup(
+      const message_center::NotifierId& notifier_id) const OVERRIDE;
+  virtual bool ShouldShowNotification(
+      const message_center::NotifierId& notifier_id) const OVERRIDE;
+
+ private:
+  // When true notifications should be shown
+  bool should_show_notifications_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedNotificationBlocker);
+};
+
+ScopedNotificationBlocker::ScopedNotificationBlocker()
+    : message_center::NotificationBlocker(
+      message_center::MessageCenter::Get()),
+      should_show_notifications_(false) {
+  NotifyBlockingStateChanged();
+}
+
+ScopedNotificationBlocker::~ScopedNotificationBlocker() {
+  should_show_notifications_ = true;
+  NotifyBlockingStateChanged();

[jonross, 2014/05/14 01:00:39]

Does this trigger calls to the subsequent methods?

If not there is no need for should_show_notifications.

[bruthig, 2014/05/14 18:48:49]

Yes it does.  NotifyBlockingStateChanged will cause
ScopedNotificationBlocker::ShouldShowNotificationAsPopup to be called.

+}
+
+bool ScopedNotificationBlocker::ShouldShowNotificationAsPopup(
+    const message_center::NotifierId& notifier_id) const {
+  if (notifier_id.id == system_notifier::kNotifierDisplay)
+    return should_show_notifications_;
+  return true;
+}
+
+bool ScopedNotificationBlocker::ShouldShowNotification(
+    const message_center::NotifierId& notifier_id) const {
+  if (notifier_id.id == system_notifier::kNotifierDisplay)
+    return should_show_notifications_;
+  return message_center::NotificationBlocker::ShouldShowNotification(
+      notifier_id);
+}
+
 }  // namespace
 
 const char TrayDisplay::kNotificationId[] = "chrome://settings/display";
@@ -388,7 +438,22 @@ void TrayDisplay::CreateOrUpdateNotification(
       message_center::RichNotificationData(),
       new message_center::HandleNotificationClickedDelegate(
           base::Bind(&OpenSettings))));
-  message_center::MessageCenter::Get()->AddNotification(notification.Pass());
+
+  message_center::MessageCenter* message_center =
+      message_center::MessageCenter::Get();
+  ash::MaximizeModeController* maximize_mode_controller =

[jonross, 2014/05/14 01:00:39]

nit: This is in the namespace of ash, don't need to specify it.

[bruthig, 2014/05/14 18:48:49]

Done.

+      ash::Shell::GetInstance()->maximize_mode_controller();
+  // Check to see if the display notification popups should be blocked.  This
+  // is used for things like suppressing redundant notifications for screen
+  // rotations caused by accelerometer events.
+  if (maximize_mode_controller &&
+      !maximize_mode_controller->show_display_notifications()) {
+    ScopedNotificationBlocker notification_blocker;
+    message_center->AddNotification(notification.Pass());
+    message_center->MarkSinglePopupAsShown(kNotificationId, false);

[jonross, 2014/05/14 01:00:39]

nit: comment on the meaning of false. In general for ambiguous cases like this
an enum is preferred. However I don't see it in the scope of this change to
update that method.

[flackr, 2014/05/14 01:48:33]

Not sure that we prefer enums for this on or off only two state kind of member.
The comment style is typically /* mark_notification_as_read */ false or the
other way around. Example:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/message_center/...

That being said, can we just remove the current display notification if it's
there and not add any notifications when they're accelerometer triggered?

[bruthig, 2014/05/14 18:48:49]

Done.

+  } else {
+    message_center->AddNotification(notification.Pass());
+  }
 }
 
 views::View* TrayDisplay::CreateDefaultView(user::LoginStatus status) {