Remove MessageCenter's NotificationChangeQueue.

This was added due to TetherNotificationPresenter being a
MessageCenterObserver that modifies the message center. That was only
the case due to a misunderstanding (it should have used
NotificationDelegate) and is now fixed. No MessageCenterObserver instance
should modify the MC during iteration. The change queue made that
possible, but that just allowed abuse of MessageCenterObserver.

Bug: 795331
Change-Id: I25f94853e0c8b29f3929d7459ed3cd3bfd2ed780
Reviewed-on: https://chromium-review.googlesource.com/829983
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Mitsuru Oshima (In Tokyo) <oshima@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537196}

ui/message_center/BUILD.gn

@@ -62,8 +62,6 @@ jumbo_component("message_center") {
       "//build/config/compiler:no_size_t_to_int_warning",
     ]
     sources = [
-      "change_queue.cc",
-      "change_queue.h",
       "cocoa/notification_controller.h",
       "cocoa/notification_controller.mm",
       "cocoa/opaque_views.h",
@@ -196,7 +194,6 @@ if (enable_message_center) {
 
   test("message_center_unittests") {
     sources = [
-      "change_queue_unittest.cc",
       "cocoa/notification_controller_unittest.mm",
       "cocoa/popup_collection_unittest.mm",
       "cocoa/popup_controller_unittest.mm",

ui/message_center/change_queue.cc

-// Copyright 2017 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.
-
-#include "ui/message_center/change_queue.h"
-
-#include "ui/message_center/message_center_impl.h"
-#include "ui/message_center/public/cpp/notification.h"
-
-namespace message_center {
-
-// Change represents an operation made on a notification. Since it contains
-// the final state of the notification, except complex cases, we generally
-// optimize the list and keep only the last change for a particular notification
-// that is in the notification list around. There are two ids; |id_| is the
-// post-update notification id that has been assigned by an update, and
-// |previous_id| is the previous id of the notification before the Change.
-// The two ids are same unless the Change changes the id of the notification.
-// See the comments of id() and previous_id() for reference.
-class ChangeQueue::Change {
- public:
-  Change(ChangeType type,
-         const std::string& id,
-         std::unique_ptr<Notification> notification);
-  ~Change();
-
-  // Used to transfer ownership of the contained notification.
-  std::unique_ptr<Notification> PassNotification();
-
-  Notification* notification() const { return notification_.get(); }
-  // Returns the post-update ID. It means:
-  // - ADD event: ID of the notification to be added. In this case, this must be
-  //   same as |previous_id()|.
-  // - UPDATE event: ID of the notification after the change. If the change
-  //   doesn't update its ID, this value is same as |previous_id()|.
-  // - DELETE event: ID of the notification to be deleted. This must be same as
-  //   |previous_id()|.
-  const std::string& id() const { return id_; }
-  ChangeType type() const { return type_; }
-  bool by_user() const { return by_user_; }
-  void set_by_user(bool by_user) { by_user_ = by_user; }
-  // Returns the ID which is used in the notification list. In other word, it
-  // means the ID before the change.
-  const std::string& previous_id() const { return previous_id_; }
-  void set_type(const ChangeType new_type) { type_ = new_type; }
-  void ReplaceNotification(std::unique_ptr<Notification> new_notification);
-
- private:
-  ChangeType type_;
-  std::string id_;
-  std::string previous_id_;
-  bool by_user_;
-  std::unique_ptr<Notification> notification_;
-
-  DISALLOW_COPY_AND_ASSIGN(Change);
-};
-
-////////////////////////////////////////////////////////////////////////////////
-// ChangeFinder
-
-struct ChangeFinder {
-  explicit ChangeFinder(const std::string& id) : id(id) {}
-  bool operator()(const std::unique_ptr<ChangeQueue::Change>& change) {
-    return change->id() == id;
-  }
-
-  std::string id;
-};
-
-////////////////////////////////////////////////////////////////////////////////
-// ChangeQueue::Change
-
-ChangeQueue::Change::Change(ChangeType type,
-                            const std::string& id,
-                            std::unique_ptr<Notification> notification)
-    : type_(type),
-      previous_id_(id),
-      by_user_(false),
-      notification_(std::move(notification)) {
-  DCHECK(!id.empty());
-  DCHECK(type != CHANGE_TYPE_DELETE || !notification_);
-
-  id_ = notification_ ? notification_->id() : previous_id_;
-}
-
-ChangeQueue::Change::~Change() {}
-
-std::unique_ptr<Notification> ChangeQueue::Change::PassNotification() {
-  return std::move(notification_);
-}
-
-void ChangeQueue::Change::ReplaceNotification(
-    std::unique_ptr<Notification> new_notification) {
-  id_ = new_notification ? new_notification->id() : previous_id_;
-  notification_.swap(new_notification);
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// ChangeQueue
-
-ChangeQueue::ChangeQueue() = default;
-
-ChangeQueue::~ChangeQueue() = default;
-
-void ChangeQueue::ApplyChanges(MessageCenterImpl* message_center) {
-  while (!changes_.empty()) {
-    auto iter = changes_.begin();
-    std::unique_ptr<Change> change(std::move(*iter));
-    changes_.erase(iter);
-    ApplyChangeInternal(message_center, std::move(change));
-  }
-}
-
-void ChangeQueue::AddNotification(std::unique_ptr<Notification> notification) {
-  std::string id = notification->id();
-  changes_.push_back(
-      std::make_unique<Change>(CHANGE_TYPE_ADD, id, std::move(notification)));
-}
-
-void ChangeQueue::UpdateNotification(
-    const std::string& old_id,
-    std::unique_ptr<Notification> notification) {
-  changes_.push_back(std::make_unique<Change>(CHANGE_TYPE_UPDATE, old_id,
-                                              std::move(notification)));
-}
-
-void ChangeQueue::RemoveNotification(const std::string& id, bool by_user) {
-  auto change = std::make_unique<Change>(CHANGE_TYPE_DELETE, id, nullptr);
-  change->set_by_user(by_user);
-  changes_.push_back(std::move(change));
-}
-
-Notification* ChangeQueue::GetLatestNotification(const std::string& id) const {
-  auto iter =
-      std::find_if(changes_.rbegin(), changes_.rend(), ChangeFinder(id));
-  if (iter == changes_.rend())
-    return nullptr;
-
-  if ((*iter)->type() == CHANGE_TYPE_DELETE)
-    return nullptr;
-
-  return (*iter)->notification();
-}
-
-void ChangeQueue::ApplyChangeInternal(MessageCenterImpl* message_center,
-                                      std::unique_ptr<Change> change) {
-  switch (change->type()) {
-    case CHANGE_TYPE_ADD:
-      message_center->AddNotificationImmediately(change->PassNotification());
-      break;
-    case CHANGE_TYPE_UPDATE:
-      message_center->UpdateNotificationImmediately(change->previous_id(),
-                                                    change->PassNotification());
-      break;
-    case CHANGE_TYPE_DELETE:
-      message_center->RemoveNotificationImmediately(change->previous_id(),
-                                                    change->by_user());
-      break;
-    default:
-      NOTREACHED();
-  }
-}
-
-}  // namespace message_center

ui/message_center/change_queue.h

-// Copyright 2017 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 UI_MESSAGE_CENTER_CHANGE_QUEUE_H_
-#define UI_MESSAGE_CENTER_CHANGE_QUEUE_H_
-
-#include <vector>
-
-#include "base/macros.h"
-#include "base/memory/ptr_util.h"
-#include "ui/message_center/message_center_export.h"
-
-namespace message_center {
-
-class MessageCenterImpl;
-class Notification;
-
-// ChangeQueue keeps track of all the changes. This class can be used for
-// queuing changes during the message center can't be updated.
-class MESSAGE_CENTER_EXPORT ChangeQueue {
- public:
-  enum ChangeType {
-    CHANGE_TYPE_ADD = 0,
-    CHANGE_TYPE_UPDATE,
-    CHANGE_TYPE_DELETE
-  };
-
-  // Change represents an operation made on a notification.  Since it contains
-  // the final state of the notification, we only keep the last change for a
-  // particular notification that is in the notification list around.  There are
-  // two ids; |id_| is the newest notification id that has been assigned by an
-  // update, and |notification_list_id_| is the id of the notification it should
-  // be updating as it exists in the notification list.
-  class Change;
-
-  ChangeQueue();
-  ~ChangeQueue();
-
-  // Called when the message center has appropriate visibility.  Modifies
-  // |message_center| but does not retain it.  This also causes the queue to
-  // empty itself.
-  void ApplyChanges(MessageCenterImpl* message_center);
-
-  // Causes a TYPE_ADD change to be added to the queue.
-  void AddNotification(std::unique_ptr<Notification> notification);
-
-  // Causes a TYPE_UPDATE change to be added to the queue.
-  void UpdateNotification(const std::string& old_id,
-                          std::unique_ptr<Notification> notification);
-
-  // Causes a TYPE_DELETE change to be added to the queue.
-  void RemoveNotification(const std::string& id, bool by_user);
-
-  // Returns the pointer to the Notification in the latest Change which has the
-  // given ID. If the Change is UPDATE, the ID after the change is searched.
-  // If the Change id REMOVE, this returns nullptr.
-  // ChangeQueue keeps retaining ownership of the Notification. The returned
-  // notification can be modified or be copied by caller.
-  Notification* GetLatestNotification(const std::string& id) const;
-
- private:
-  void ApplyChangeInternal(MessageCenterImpl* message_center,
-                           std::unique_ptr<Change> change);
-
-  std::vector<std::unique_ptr<Change>> changes_;
-
-  DISALLOW_COPY_AND_ASSIGN(ChangeQueue);
-};
-
-}  // namespace message_center
-
-#endif  // UI_MESSAGE_CENTER_CHANGE_QUEUE_H_

ui/message_center/change_queue_unittest.cc

-// Copyright 2017 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.
-
-#include "ui/message_center/change_queue.h"
-
-#include <utility>
-
-#include "base/strings/utf_string_conversions.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "ui/message_center/message_center_impl.h"
-
-using base::UTF8ToUTF16;
-
-namespace message_center {
-
-namespace {
-
-Notification* CreateSimpleNotification(const std::string& id) {
-  RichNotificationData optional_fields;
-  optional_fields.buttons.push_back(ButtonInfo(UTF8ToUTF16("foo")));
-  optional_fields.buttons.push_back(ButtonInfo(UTF8ToUTF16("foo")));
-  return new Notification(
-      NOTIFICATION_TYPE_SIMPLE, id, UTF8ToUTF16("title"), UTF8ToUTF16(id),
-      gfx::Image() /* icon */, base::string16() /* display_source */, GURL(),
-      NotifierId(NotifierId::APPLICATION, "change_queue_test"), optional_fields,
-      NULL);
-}
-
-}  // namespace
-
-class ChangeQueueTest : public testing::Test {
- public:
-  ChangeQueueTest() = default;
-  ~ChangeQueueTest() override = default;
-
-  MessageCenterImpl* message_center() { return &message_center_; }
-
- private:
-  MessageCenterImpl message_center_;
-};
-
-TEST_F(ChangeQueueTest, Queueing) {
-  ChangeQueue queue;
-  std::string id("id1");
-  std::string id2("id2");
-  NotifierId notifier_id1(NotifierId::APPLICATION, "app1");
-
-  // First, add and update a notification to ensure updates happen
-  // normally.
-  std::unique_ptr<Notification> notification(CreateSimpleNotification(id));
-  message_center()->AddNotification(std::move(notification));
-  notification.reset(CreateSimpleNotification(id2));
-  message_center()->UpdateNotification(id, std::move(notification));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id2));
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
-
-  // Then update a notification; nothing should have happened.
-  notification.reset(CreateSimpleNotification(id));
-  queue.UpdateNotification(id2, std::move(notification));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id2));
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
-
-  // Apply the changes from the queue.
-  queue.ApplyChanges(message_center());
-
-  // Close the message center; then the update should have propagated.
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id2));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
-}
-
-TEST_F(ChangeQueueTest, SimpleQueueing) {
-  ChangeQueue queue;
-  std::string ids[6] = {"0", "1", "2", "3", "4", "5"};
-  NotifierId notifier_id1(NotifierId::APPLICATION, "app1");
-
-  std::unique_ptr<Notification> notification;
-  // Prepare initial notifications on NotificationList.
-  // NL: ["0", "1", "2", "4"]
-  int i = 0;
-  for (; i < 3; i++) {
-    notification.reset(CreateSimpleNotification(ids[i]));
-    message_center()->AddNotification(std::move(notification));
-  }
-  for (i = 0; i < 3; i++) {
-    EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[i]));
-  }
-  for (; i < 6; i++) {
-    EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[i]));
-  }
-
-  // This should update notification "1" to have id "4".
-  notification.reset(CreateSimpleNotification(ids[4]));
-  queue.AddNotification(std::move(notification));
-
-  // Change the ID: "2" -> "5", "5" -> "3".
-  notification.reset(CreateSimpleNotification(ids[5]));
-  queue.UpdateNotification(ids[2], std::move(notification));
-  notification.reset(CreateSimpleNotification(ids[3]));
-  queue.UpdateNotification(ids[5], std::move(notification));
-
-  // This should update notification "4" to "5".
-  notification.reset(CreateSimpleNotification(ids[5]));
-  queue.UpdateNotification(ids[4], std::move(notification));
-
-  // This should create a new "4", that doesn't overwrite the update from 4
-  // before.
-  notification.reset(CreateSimpleNotification(ids[4]));
-  queue.AddNotification(std::move(notification));
-
-  // The NL should still be the same: ["0", "1", "2", "4"]
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[0]));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[1]));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[2]));
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[3]));
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[4]));
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[5]));
-  EXPECT_EQ(message_center()->GetVisibleNotifications().size(), 3u);
-
-  // Apply the changes from the queue.
-  queue.ApplyChanges(message_center());
-
-  // Changes should be applied.
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[0]));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[1]));
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[2]));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[3]));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[4]));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[5]));
-  EXPECT_EQ(message_center()->GetVisibleNotifications().size(), 5u);
-}
-
-}  // namespace message_center

ui/message_center/message_center_impl.h

@@ -16,7 +16,6 @@
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
-#include "ui/message_center/change_queue.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/message_center_observer.h"
 #include "ui/message_center/message_center_stats_collector.h"
@@ -27,10 +26,6 @@
 
 namespace message_center {
 
-namespace internal {
-class ScopedNotificationsIterationLock;
-};
-
 // The default implementation of MessageCenter.
 class MESSAGE_CENTER_EXPORT MessageCenterImpl
     : public MessageCenter,
@@ -88,23 +83,10 @@ class MESSAGE_CENTER_EXPORT MessageCenterImpl
   // NotificationBlocker::Observer overrides:
   void OnBlockingStateChanged(NotificationBlocker* blocker) override;
 
-  // Unexposed methods:
-  void AddNotificationImmediately(std::unique_ptr<Notification> notification);
-  void UpdateNotificationImmediately(
-      const std::string& old_id,
-      std::unique_ptr<Notification> new_notification);
-  void RemoveNotificationImmediately(const std::string& id, bool by_user);
-
  protected:
   void DisableTimersForTest() override;
 
  private:
-  friend internal::ScopedNotificationsIterationLock;
-  class ScopedNotificationsLock;
-
-  Notification* GetLatestNotificationIncludingQueued(
-      const std::string& id) const;
-
   THREAD_CHECKER(thread_checker_);
 
   std::unique_ptr<NotificationList> notification_list_;
@@ -113,13 +95,9 @@ class MESSAGE_CENTER_EXPORT MessageCenterImpl
   std::unique_ptr<PopupTimersController> popup_timers_controller_;
   std::unique_ptr<base::OneShotTimer> quiet_mode_timer_;
   std::vector<NotificationBlocker*> blockers_;
-  std::unique_ptr<ChangeQueue> notification_change_queue_;
 
   bool visible_ = false;
 
-  // modified by ScopedNotificationsIterationLock.
-  bool iterating_ = false;
-
   base::string16 system_notification_app_name_;
 
   MessageCenterStatsCollector stats_collector_;

ui/message_center/message_center_impl_unittest.cc

@@ -782,36 +782,5 @@ TEST_F(MessageCenterImplTest, RemoveWhileMessageCenterVisible) {
   EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
 }
 
-TEST_F(MessageCenterImplTest, RemoveWhileIteratingObserver) {
-  std::string id("id1");
-  CheckObserver check1(message_center(), id);
-  CheckObserver check2(message_center(), id);
-  RemoveObserver remove(message_center(), id);
-
-  // Prepare a notification
-  std::unique_ptr<Notification> notification(CreateSimpleNotification(id));
-  message_center()->AddNotification(std::move(notification));
-  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
-
-  // Install the test handlers
-  message_center()->AddObserver(&check1);
-  message_center()->AddObserver(&remove);
-  message_center()->AddObserver(&check2);
-
-  // Update the notification. The notification will be removed in the observer,
-  // but the actual removal will be done at the end of the iteration.
-  // Notification keeps alive during iteration. This is checked by
-  // CheckObserver.
-  notification.reset(CreateSimpleNotification(id));
-  message_center()->UpdateNotification(id, std::move(notification));
-
-  // Notification is removed correctly.
-  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
-
-  message_center()->RemoveObserver(&check1);
-  message_center()->RemoveObserver(&remove);
-  message_center()->RemoveObserver(&check2);
-}
-
 }  // namespace internal
 }  // namespace message_center

ui/message_center/message_center_observer.h

@@ -14,6 +14,8 @@ namespace message_center {
 class NotificationBlocker;
 
 // An observer class for the change of notifications in the MessageCenter.
+// WARNING: It is not safe to modify the message center from within these
+// callbacks.
 class MESSAGE_CENTER_EXPORT MessageCenterObserver {
  public:
   virtual ~MessageCenterObserver() {}

ui/message_center/ui_controller.cc

@@ -9,7 +9,6 @@
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/models/simple_menu_model.h"
@@ -49,13 +48,9 @@ bool UiController::ShowMessageCenterBubble(bool show_by_click) {
 }
 
 bool UiController::HideMessageCenterBubble() {
-#if defined(OS_CHROMEOS)
-  // TODO(yoshiki): Move the message center bubble related logic to ash/.
-  hide_empty_message_center_callback_.reset();
-#endif
-
   if (!message_center_visible_)
     return false;
+
   delegate_->HideMessageCenter();
   MarkMessageCenterHidden();
 
@@ -166,25 +161,8 @@ void UiController::OnBlockingStateChanged(NotificationBlocker* blocker) {
 }
 
 void UiController::OnMessageCenterChanged() {
-#if defined(OS_CHROMEOS)
-  // TODO(yoshiki): Move the message center bubble related logic to ash/.
-  if (message_center_visible_ && message_center_->NotificationCount() == 0) {
-    if (hide_empty_message_center_callback_)
-      return;
-
-    hide_empty_message_center_callback_ =
-        std::make_unique<base::CancelableClosure>(base::Bind(
-            base::IgnoreResult(&UiController::HideMessageCenterBubble),
-            base::Unretained(this)));
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, hide_empty_message_center_callback_->callback());
-    return;
-  }
-
-  // Cancel the callback if necessary.
-  if (hide_empty_message_center_callback_)
-    hide_empty_message_center_callback_.reset();
-#endif
+  if (message_center_visible_ && message_center_->NotificationCount() == 0)
+    HideMessageCenterBubble();
 
   if (popups_visible_ && !message_center_->HasPopupNotifications())
     HidePopupBubbleInternal();

ui/message_center/ui_controller.h

@@ -5,7 +5,6 @@
 #ifndef UI_MESSAGE_CENTER_UI_CONTROLLER_H_
 #define UI_MESSAGE_CENTER_UI_CONTROLLER_H_
 
-#include "base/cancelable_callback.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/strings/string16.h"
@@ -80,10 +79,6 @@ class MESSAGE_CENTER_EXPORT UiController : public MessageCenterObserver {
   bool popups_visible_ = false;
   UiDelegate* delegate_;
 
-#if defined(OS_CHROMEOS)
-  std::unique_ptr<base::CancelableClosure> hide_empty_message_center_callback_;
-#endif
-
   DISALLOW_COPY_AND_ASSIGN(UiController);
 };