Implement NotificationManager

- Responsible for managing the internal notifications set and sending
  inline replies for notifications.

Bug: 1106937
Test: chromeos_components_unittests
Change-Id: I659f3b16c06005d213beb9c2cbe21618f35404c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2466980Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816893}

chromeos/components/phonehub/fake_notification_manager.cc

@@ -25,62 +25,10 @@ void FakeNotificationManager::SetNotification(
   SetNotificationsInternal(base::flat_set<Notification>{notification});
 }
 
-void FakeNotificationManager::SetNotificationsInternal(
-    const base::flat_set<Notification>& notifications) {
-  base::flat_set<int64_t> added_ids;
-  base::flat_set<int64_t> updated_ids;
-
-  for (const Notification& notification : notifications) {
-    int64_t id = notification.id();
-    auto it = id_to_notification_map_.find(id);
-
-    if (it == id_to_notification_map_.end()) {
-      id_to_notification_map_.emplace(id, notification);
-      added_ids.emplace(id);
-      continue;
-    }
-
-    it->second = notification;
-    updated_ids.emplace(id);
-  }
-
-  NotifyNotificationsAdded(added_ids);
-  NotifyNotificationsUpdated(updated_ids);
-}
-
 void FakeNotificationManager::RemoveNotification(int64_t id) {
   RemoveNotificationsInternal(base::flat_set<int64_t>{id});
 }
 
-void FakeNotificationManager::RemoveNotificationsInternal(
-    const base::flat_set<int64_t>& ids) {
-  for (int64_t id : ids) {
-    auto it = id_to_notification_map_.find(id);
-    DCHECK(it != id_to_notification_map_.end());
-    id_to_notification_map_.erase(it);
-  }
-
-  NotifyNotificationsRemoved(ids);
-}
-
-void FakeNotificationManager::ClearNotificationsInternal() {
-  base::flat_set<int64_t> removed_ids;
-  for (const auto& pair : id_to_notification_map_) {
-    removed_ids.emplace(pair.first);
-  }
-
-  id_to_notification_map_.clear();
-  NotifyNotificationsRemoved(removed_ids);
-}
-
-const Notification* FakeNotificationManager::GetNotification(
-    int64_t notification_id) const {
-  auto it = id_to_notification_map_.find(notification_id);
-  if (it == id_to_notification_map_.end())
-    return nullptr;
-  return &it->second;
-}
-
 void FakeNotificationManager::DismissNotification(int64_t notification_id) {
   DCHECK(base::Contains(id_to_notification_map_, notification_id));
   dismissed_notification_ids_.push_back(notification_id);

chromeos/components/phonehub/fake_notification_manager.h

@@ -20,14 +20,15 @@ class FakeNotificationManager : public NotificationManager {
   FakeNotificationManager();
   ~FakeNotificationManager() override;
 
+  using NotificationManager::SetNotificationsInternal;
+
+  using NotificationManager::RemoveNotificationsInternal;
+
+  using NotificationManager::ClearNotificationsInternal;
+
   void SetNotification(const Notification& notification);
-  void SetNotificationsInternal(
-      const base::flat_set<Notification>& notifications) override;
 
   void RemoveNotification(int64_t id);
-  void RemoveNotificationsInternal(const base::flat_set<int64_t>& ids) override;
-
-  void ClearNotificationsInternal() override;
 
   const std::vector<int64_t>& dismissed_notification_ids() const {
     return dismissed_notification_ids_;
@@ -50,12 +51,10 @@ class FakeNotificationManager : public NotificationManager {
 
  private:
   // NotificationManager:
-  const Notification* GetNotification(int64_t notification_id) const override;
   void DismissNotification(int64_t notification_id) override;
   void SendInlineReply(int64_t notification_id,
                        const base::string16& inline_reply_text) override;
 
-  std::unordered_map<int64_t, Notification> id_to_notification_map_;
   std::vector<int64_t> dismissed_notification_ids_;
   std::vector<InlineReplyMetadata> inline_replies_;
 };

chromeos/components/phonehub/notification_manager.cc

@@ -4,6 +4,8 @@
 
 #include "chromeos/components/phonehub/notification_manager.h"
 
+#include "chromeos/components/multidevice/logging/logging.h"
+
 namespace chromeos {
 namespace phonehub {
 
@@ -37,5 +39,63 @@ void NotificationManager::NotifyNotificationsRemoved(
     observer.OnNotificationsRemoved(notification_ids);
 }
 
+void NotificationManager::SetNotificationsInternal(
+    const base::flat_set<Notification>& notifications) {
+  PA_LOG(INFO) << "Setting notifications internally.";
+
+  base::flat_set<int64_t> added_ids;
+  base::flat_set<int64_t> updated_ids;
+
+  for (const Notification& notification : notifications) {
+    int64_t id = notification.id();
+    auto it = id_to_notification_map_.find(id);
+
+    if (it == id_to_notification_map_.end()) {
+      id_to_notification_map_.emplace(id, notification);
+      added_ids.emplace(id);
+      continue;
+    }
+
+    it->second = notification;
+    updated_ids.emplace(id);
+  }
+
+  NotifyNotificationsAdded(added_ids);
+  NotifyNotificationsUpdated(updated_ids);
+}
+
+void NotificationManager::RemoveNotificationsInternal(
+    const base::flat_set<int64_t>& notification_ids) {
+  PA_LOG(INFO) << "Removing notifications internally.";
+
+  for (int64_t id : notification_ids) {
+    auto it = id_to_notification_map_.find(id);
+    DCHECK(it != id_to_notification_map_.end());
+    id_to_notification_map_.erase(it);
+  }
+
+  NotifyNotificationsRemoved(notification_ids);
+}
+
+void NotificationManager::ClearNotificationsInternal() {
+  PA_LOG(INFO) << "Clearing notification internally.";
+
+  base::flat_set<int64_t> removed_ids;
+  for (const auto& pair : id_to_notification_map_) {
+    removed_ids.emplace(pair.first);
+  }
+
+  id_to_notification_map_.clear();
+  NotifyNotificationsRemoved(removed_ids);
+}
+
+const Notification* NotificationManager::GetNotification(
+    int64_t notification_id) const {
+  auto it = id_to_notification_map_.find(notification_id);
+  if (it == id_to_notification_map_.end())
+    return nullptr;
+  return &it->second;
+}
+
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/notification_manager.h

@@ -6,6 +6,7 @@
 #define CHROMEOS_COMPONENTS_PHONEHUB_NOTIFICATION_MANAGER_H_
 
 #include <stdint.h>
+#include <unordered_map>
 
 #include "base/containers/flat_set.h"
 #include "base/observer_list.h"
@@ -45,8 +46,7 @@ class NotificationManager {
   // Returns null if no notification exists with the given ID. Pointers returned
   // by this function should not be cached, since the underlying Notification
   // object may be deleted by a future update.
-  virtual const Notification* GetNotification(
-      int64_t notification_id) const = 0;
+  const Notification* GetNotification(int64_t notification_id) const;
 
   // Dismisses the notification with the given ID; if no notification exists
   // with this ID, this function is a no-op.
@@ -62,23 +62,24 @@ class NotificationManager {
 
  protected:
   friend class PhoneStatusProcessor;
+  friend class NotificationManagerImplTest;
 
   NotificationManager();
 
   // Sets the internal collection of notifications. This does not send any
   // requests to the remote phone device.
-  virtual void SetNotificationsInternal(
-      const base::flat_set<Notification>& notifications) = 0;
+  void SetNotificationsInternal(
+      const base::flat_set<Notification>& notifications);
 
   // Removes the dismissed notifications from the internal collection of
   // notifications. Does not send a request to remove notifications to the
   // remote device.
-  virtual void RemoveNotificationsInternal(
-      const base::flat_set<int64_t>& notification_ids) = 0;
+  void RemoveNotificationsInternal(
+      const base::flat_set<int64_t>& notification_ids);
 
   // Clears the underlying internal collection of notifications. This does not
   // send any requests to clear the phone's notifications.
-  virtual void ClearNotificationsInternal() = 0;
+  void ClearNotificationsInternal();
 
   void NotifyNotificationsAdded(
       const base::flat_set<int64_t>& notification_ids);
@@ -87,6 +88,8 @@ class NotificationManager {
   void NotifyNotificationsRemoved(
       const base::flat_set<int64_t>& notification_ids);
 
+  std::unordered_map<int64_t, Notification> id_to_notification_map_;
+
  private:
   base::ObserverList<Observer> observer_list_;
 };

chromeos/components/phonehub/notification_manager_impl.cc

@@ -4,47 +4,41 @@
 
 #include "chromeos/components/phonehub/notification_manager_impl.h"
 
+#include "base/containers/flat_set.h"
 #include "chromeos/components/multidevice/logging/logging.h"
+#include "chromeos/components/phonehub/message_sender.h"
 #include "chromeos/components/phonehub/notification.h"
 
 namespace chromeos {
 namespace phonehub {
 
-NotificationManagerImpl::NotificationManagerImpl() = default;
-
-NotificationManagerImpl::~NotificationManagerImpl() = default;
-
-const Notification* NotificationManagerImpl::GetNotification(
-    int64_t notification_id) const {
-  return nullptr;
-}
-
-void NotificationManagerImpl::SetNotificationsInternal(
-    const base::flat_set<Notification>& notifications) {
-  PA_LOG(INFO) << "Setting notifications internally.";
-  // TODO(jimmyxong): Implement this stub function.
+NotificationManagerImpl::NotificationManagerImpl(MessageSender* message_sender)
+    : message_sender_(message_sender) {
+  DCHECK(message_sender_);
 }
 
-void NotificationManagerImpl::RemoveNotificationsInternal(
-    const base::flat_set<int64_t>& notification_ids) {
-  PA_LOG(INFO) << "Removing notifications internally.";
-  // TODO(jimmyxgong): Implement this stub function.
-}
+NotificationManagerImpl::~NotificationManagerImpl() = default;
 
 void NotificationManagerImpl::DismissNotification(int64_t notification_id) {
   PA_LOG(INFO) << "Dismissing notification with ID " << notification_id << ".";
-}
 
-void NotificationManagerImpl::ClearNotificationsInternal() {
-  PA_LOG(INFO) << "Clearing notification internally.";
-  // TODO(jimmyxgong): Implement this stub function.
+  RemoveNotificationsInternal(base::flat_set<int64_t>{notification_id});
+  message_sender_->SendDismissNotificationRequest(notification_id);
 }
 
 void NotificationManagerImpl::SendInlineReply(
     int64_t notification_id,
     const base::string16& inline_reply_text) {
+  if (!GetNotification(notification_id)) {
+    PA_LOG(INFO) << "Could not send inline reply for notification with ID "
+                 << notification_id << ".";
+    return;
+  }
+
   PA_LOG(INFO) << "Sending inline reply for notification with ID "
                << notification_id << ".";
+  message_sender_->SendNotificationInlineReplyRequest(notification_id,
+                                                      inline_reply_text);
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/notification_manager_impl.h

@@ -5,30 +5,28 @@
 #ifndef CHROMEOS_COMPONENTS_PHONEHUB_NOTIFICATION_MANAGER_IMPL_H_
 #define CHROMEOS_COMPONENTS_PHONEHUB_NOTIFICATION_MANAGER_IMPL_H_
 
-#include "base/containers/flat_set.h"
+#include <unordered_map>
+
 #include "chromeos/components/phonehub/notification.h"
 #include "chromeos/components/phonehub/notification_manager.h"
 
 namespace chromeos {
 namespace phonehub {
 
-// TODO(https://crbug.com/1106937): Add real implementation.
+class MessageSender;
+
 class NotificationManagerImpl : public NotificationManager {
  public:
-  NotificationManagerImpl();
+  NotificationManagerImpl(MessageSender* message_sender);
   ~NotificationManagerImpl() override;
 
  private:
   // NotificationManager:
-  const Notification* GetNotification(int64_t notification_id) const override;
-  void SetNotificationsInternal(
-      const base::flat_set<Notification>& notifications) override;
-  void RemoveNotificationsInternal(
-      const base::flat_set<int64_t>& notification_ids) override;
-  void ClearNotificationsInternal() override;
   void DismissNotification(int64_t notification_id) override;
   void SendInlineReply(int64_t notification_id,
                        const base::string16& inline_reply_text) override;
+
+  MessageSender* message_sender_;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/notification_manager_impl_unittest.cc

@@ -8,14 +8,33 @@
 
 #include "base/containers/flat_map.h"
 #include "base/optional.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chromeos/components/phonehub/fake_message_sender.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
 namespace phonehub {
 namespace {
 
+const char kAppName[] = "Test App";
+const char kPackageName[] = "com.google.testapp";
+
+const char kTitle[] = "Test notification";
+const char kTextContent[] = "This is a test notification";
+
 enum class NotificationState { kAdded, kUpdated, kRemoved };
 
+Notification CreateNotification(int64_t id) {
+  return chromeos::phonehub::Notification(
+      id,
+      chromeos::phonehub::Notification::AppMetadata(base::UTF8ToUTF16(kAppName),
+                                                    kPackageName,
+                                                    /*icon=*/gfx::Image()),
+      base::Time::Now(), Notification::Importance::kDefault,
+      /*inline_reply_id=*/0, base::UTF8ToUTF16(kTitle),
+      base::UTF8ToUTF16(kTextContent));
+}
+
 class FakeObserver : public NotificationManager::Observer {
  public:
   FakeObserver() = default;
@@ -63,23 +82,151 @@ class NotificationManagerImplTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
-    manager_ = std::make_unique<NotificationManagerImpl>();
+    fake_message_sender_ = std::make_unique<FakeMessageSender>();
+    manager_ =
+        std::make_unique<NotificationManagerImpl>(fake_message_sender_.get());
     manager_->AddObserver(&fake_observer_);
   }
 
   void TearDown() override { manager_->RemoveObserver(&fake_observer_); }
 
   NotificationManager& manager() { return *manager_; }
+  FakeMessageSender& fake_message_sender() { return *fake_message_sender_; }
+
+  void SetNotificationsInternal(
+      const base::flat_set<Notification>& notifications) {
+    manager_->SetNotificationsInternal(notifications);
+  }
+
+  void ClearNotificationsInternal() { manager_->ClearNotificationsInternal(); }
+
+  size_t GetNumNotifications() {
+    return manager_->id_to_notification_map_.size();
+  }
+
+  base::Optional<NotificationState> GetNotificationState(
+      int64_t notification_id) {
+    return fake_observer_.GetState(notification_id);
+  }
 
  private:
   FakeObserver fake_observer_;
+  std::unique_ptr<FakeMessageSender> fake_message_sender_;
   std::unique_ptr<NotificationManager> manager_;
 };
 
-// TODO(khorimoto): Remove this test once we have real functionality to test.
-TEST_F(NotificationManagerImplTest, Initialize) {
-  EXPECT_FALSE(manager().GetNotification(/*notification_id=*/0));
+TEST_F(NotificationManagerImplTest, SetAndClearNotificationsInternal) {
+  EXPECT_EQ(0u, GetNumNotifications());
+  const int64_t expected_id1 = 0;
+  const int64_t expected_id2 = 1;
+
+  SetNotificationsInternal(base::flat_set<Notification>{
+      CreateNotification(expected_id1), CreateNotification(expected_id2)});
+  EXPECT_EQ(2u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id2));
+
+  ClearNotificationsInternal();
+  EXPECT_EQ(0u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kRemoved, GetNotificationState(expected_id1));
+  EXPECT_EQ(NotificationState::kRemoved, GetNotificationState(expected_id2));
 }
 
+TEST_F(NotificationManagerImplTest, GetNotification) {
+  EXPECT_EQ(0u, GetNumNotifications());
+  const int64_t expected_id1 = 0;
+
+  SetNotificationsInternal(
+      base::flat_set<Notification>{CreateNotification(expected_id1)});
+  EXPECT_EQ(1u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
+
+  // Call GetNotification() on an existent notification id. Expect a non-null
+  // pointer.
+  EXPECT_TRUE(manager().GetNotification(expected_id1));
+
+  // Call GetNotification() on a non-existent notification id. Expect a null
+  // pointer.
+  EXPECT_FALSE(manager().GetNotification(/*notification_id=*/4));
+
+  // Remove |expected_id1| and expect that GetNotification() returns a null
+  // pointer.
+  manager().DismissNotification(expected_id1);
+  EXPECT_EQ(1u, fake_message_sender().GetDismissNotificationRequestCallCount());
+  EXPECT_EQ(expected_id1,
+            fake_message_sender().GetRecentDismissNotificationRequest());
+  EXPECT_FALSE(manager().GetNotification(expected_id1));
+}
+
+TEST_F(NotificationManagerImplTest, DismissNotifications) {
+  EXPECT_EQ(0u, GetNumNotifications());
+  const int64_t expected_id1 = 0;
+  const int64_t expected_id2 = 1;
+
+  SetNotificationsInternal(base::flat_set<Notification>{
+      CreateNotification(expected_id1), CreateNotification(expected_id2)});
+  EXPECT_EQ(2u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id2));
+
+  manager().DismissNotification(expected_id2);
+  EXPECT_EQ(1u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
+  EXPECT_EQ(NotificationState::kRemoved, GetNotificationState(expected_id2));
+  EXPECT_EQ(1u, fake_message_sender().GetDismissNotificationRequestCallCount());
+  EXPECT_EQ(expected_id2,
+            fake_message_sender().GetRecentDismissNotificationRequest());
+}
+
+TEST_F(NotificationManagerImplTest, UpdatedNotification) {
+  EXPECT_EQ(0u, GetNumNotifications());
+  const int64_t expected_id1 = 0;
+  const int64_t expected_id2 = 1;
+
+  SetNotificationsInternal(base::flat_set<Notification>{
+      CreateNotification(expected_id1), CreateNotification(expected_id2)});
+  EXPECT_EQ(2u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id2));
+
+  // Simulate updating a notification.
+  SetNotificationsInternal(
+      base::flat_set<Notification>{CreateNotification(expected_id1)});
+  EXPECT_EQ(2u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kUpdated, GetNotificationState(expected_id1));
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id2));
+}
+
+TEST_F(NotificationManagerImplTest, SendInlineReply) {
+  EXPECT_EQ(0u, GetNumNotifications());
+  const int64_t expected_id1 = 0;
+
+  SetNotificationsInternal(
+      base::flat_set<Notification>{CreateNotification(expected_id1)});
+  EXPECT_EQ(1u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
+
+  // Simulate sending an inline reply to a notification.
+  const base::string16& expected_reply(base::UTF8ToUTF16("test reply"));
+  manager().SendInlineReply(expected_id1, expected_reply);
+  EXPECT_EQ(1u, GetNumNotifications());
+  EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
+  EXPECT_EQ(1u,
+            fake_message_sender().GetNotificationInlineReplyRequestCallCount());
+  std::pair<int64_t, base::string16> pair =
+      fake_message_sender().GetRecentNotificationInlineReplyRequest();
+  EXPECT_EQ(expected_id1, pair.first);
+  EXPECT_EQ(expected_reply, pair.second);
+
+  // Simulate sending an inline reply to a non-existent notification. Expect
+  // that no new reply calls were called and that the most recent reply is the
+  // same as the previous inline reply call.
+  manager().SendInlineReply(/*notification_id=*/5, /*reply=*/base::string16());
+  EXPECT_EQ(1u,
+            fake_message_sender().GetNotificationInlineReplyRequestCallCount());
+  pair = fake_message_sender().GetRecentNotificationInlineReplyRequest();
+  EXPECT_EQ(expected_id1, pair.first);
+  EXPECT_EQ(expected_reply, pair.second);
+}
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/phone_hub_manager_impl.cc

@@ -63,7 +63,8 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
               feature_status_provider_.get(),
               message_sender_.get(),
               connection_scheduler_.get())),
-      notification_manager_(std::make_unique<NotificationManagerImpl>()),
+      notification_manager_(
+          std::make_unique<NotificationManagerImpl>(message_sender_.get())),
       onboarding_ui_tracker_(std::make_unique<OnboardingUiTrackerImpl>(
           pref_service,
           feature_status_provider_.get(),

chromeos/components/phonehub/phone_status_processor.cc

@@ -255,11 +255,14 @@ void PhoneStatusProcessor::OnPhoneStatusUpdateReceived(
   SetReceivedNotifications(phone_status_update.updated_notifications());
   SetReceivedPhoneStatusModelStates(phone_status_update.properties());
 
-  base::flat_set<int64_t> removed_notification_ids;
-  for (auto& id : phone_status_update.removed_notification_ids()) {
-    removed_notification_ids.emplace(id);
+  if (!phone_status_update.removed_notification_ids().empty()) {
+    base::flat_set<int64_t> removed_notification_ids;
+    for (auto& id : phone_status_update.removed_notification_ids()) {
+      removed_notification_ids.emplace(id);
+    }
+    notification_manager_->RemoveNotificationsInternal(
+        removed_notification_ids);
   }
-  notification_manager_->RemoveNotificationsInternal(removed_notification_ids);
 }
 
 void PhoneStatusProcessor::OnHostStatusChanged(