[CrOS PhoneHub] Make notifications pop up when content is updated

Before this CL, we'd show a pop-up notification for any new
notifications (corresponding to new notification IDs), but we'd silently
update the notification content whenever it was updated.

However, many messaging apps use the same notification ID for a single
chat thread and just replace the text content with new content when a
new message is received. This meant that when users received a new
message from the same recipient, the new notification would not pop up.

This CL fixes the issue by always popping up updated notifications.

Fixed: 1159063
Bug: 1106937
Change-Id: I21aa76368d8de63068a12ad23c65fd4ecc4d3a4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2606109Reviewed-by: Regan Hsu <hsuregan@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839759}

ash/system/phonehub/phone_hub_notification_controller.cc

@@ -286,7 +286,8 @@ void PhoneHubNotificationController::OnFeatureStatusChanged() {
 void PhoneHubNotificationController::OnNotificationsAdded(
     const base::flat_set<int64_t>& notification_ids) {
   for (int64_t id : notification_ids) {
-    CreateOrUpdateNotification(manager_->GetNotification(id));
+    SetNotification(manager_->GetNotification(id),
+                    /*is_update=*/false);
   }
 
   LogNotificationCount();
@@ -295,7 +296,8 @@ void PhoneHubNotificationController::OnNotificationsAdded(
 void PhoneHubNotificationController::OnNotificationsUpdated(
     const base::flat_set<int64_t>& notification_ids) {
   for (int64_t id : notification_ids) {
-    CreateOrUpdateNotification(manager_->GetNotification(id));
+    SetNotification(manager_->GetNotification(id),
+                    /*is_update=*/true);
   }
 }
 
@@ -375,8 +377,9 @@ void PhoneHubNotificationController::LogNotificationCount() {
   phone_hub_metrics::LogNotificationCount(count);
 }
 
-void PhoneHubNotificationController::CreateOrUpdateNotification(
-    const chromeos::phonehub::Notification* notification) {
+void PhoneHubNotificationController::SetNotification(
+    const chromeos::phonehub::Notification* notification,
+    bool is_update) {
   int64_t phone_hub_id = notification->id();
   std::string cros_id = base::StrCat(
       {kNotifierId, kNotifierIdSeparator, base::NumberToString(phone_hub_id)});
@@ -389,7 +392,8 @@ void PhoneHubNotificationController::CreateOrUpdateNotification(
   }
   NotificationDelegate* delegate = notification_map_[phone_hub_id].get();
 
-  auto cros_notification = CreateNotification(notification, cros_id, delegate);
+  auto cros_notification =
+      CreateNotification(notification, cros_id, delegate, is_update);
   cros_notification->set_custom_view_type(kNotificationCustomViewType);
   shown_notification_ids_.insert(phone_hub_id);
 
@@ -404,7 +408,8 @@ std::unique_ptr<message_center::Notification>
 PhoneHubNotificationController::CreateNotification(
     const chromeos::phonehub::Notification* notification,
     const std::string& cros_id,
-    NotificationDelegate* delegate) {
+    NotificationDelegate* delegate,
+    bool is_update) {
   message_center::NotifierId notifier_id(
       message_center::NotifierType::PHONE_HUB, kNotifierId);
 
@@ -428,28 +433,14 @@ PhoneHubNotificationController::CreateNotification(
 
   const gfx::Image& icon = notification->contact_image().value_or(gfx::Image());
 
-  switch (notification->importance()) {
-    case chromeos::phonehub::Notification::Importance::kNone:
-      FALLTHROUGH;
-    case chromeos::phonehub::Notification::Importance::kLow:
-      optional_fields.priority = message_center::MIN_PRIORITY;
-      break;
-    case chromeos::phonehub::Notification::Importance::kUnspecified:
-      FALLTHROUGH;
-    case chromeos::phonehub::Notification::Importance::kMin:
-      FALLTHROUGH;
-    case chromeos::phonehub::Notification::Importance::kDefault:
-      optional_fields.priority = message_center::LOW_PRIORITY;
-      break;
-    case chromeos::phonehub::Notification::Importance::kHigh:
-      // If the notification has already been shown in the past (even across
-      // disconnects), then downgrade the priority so it's not a pop-up.
-      if (base::Contains(shown_notification_ids_, notification->id()))
-        optional_fields.priority = message_center::LOW_PRIORITY;
-      else
-        optional_fields.priority = message_center::MAX_PRIORITY;
-      break;
-  }
+  optional_fields.priority =
+      GetSystemPriorityForNotification(notification, is_update);
+
+  // If the notification was updated, set renotify to true so that the
+  // notification pops up again and is visible to the user. See
+  // https://crbug.com/1159063.
+  if (is_update)
+    optional_fields.renotify = true;
 
   message_center::ButtonInfo reply_button;
   reply_button.title = l10n_util::GetStringUTF16(
@@ -468,6 +459,39 @@ PhoneHubNotificationController::CreateNotification(
       delegate->AsScopedRefPtr());
 }
 
+int PhoneHubNotificationController::GetSystemPriorityForNotification(
+    const chromeos::phonehub::Notification* notification,
+    bool is_update) {
+  switch (notification->importance()) {
+    case chromeos::phonehub::Notification::Importance::kNone:
+      FALLTHROUGH;
+    case chromeos::phonehub::Notification::Importance::kMin:
+      return message_center::MIN_PRIORITY;
+
+    case chromeos::phonehub::Notification::Importance::kUnspecified:
+      FALLTHROUGH;
+    case chromeos::phonehub::Notification::Importance::kLow:
+      FALLTHROUGH;
+    case chromeos::phonehub::Notification::Importance::kDefault:
+      FALLTHROUGH;
+    case chromeos::phonehub::Notification::Importance::kHigh:
+      bool has_notification_been_shown =
+          base::Contains(shown_notification_ids_, notification->id());
+
+      // If the same notification was already shown and has not been updated,
+      // use LOW_PRIORITY so that the notification is silently added to the
+      // notification shade. This ensures that we don't spam users with the same
+      // information multiple times.
+      if (has_notification_been_shown && !is_update)
+        return message_center::LOW_PRIORITY;
+
+      // Use MAX_PRIORITY, which causes the notification to be shown in a popup
+      // so that users can see new messages come in as they are chatting. See
+      // https://crbug.com/1159063.
+      return message_center::MAX_PRIORITY;
+  }
+}
+
 // static
 std::unique_ptr<message_center::MessageView>
 PhoneHubNotificationController::CreateCustomNotificationView(

ash/system/phonehub/phone_hub_notification_controller.h

@@ -79,16 +79,21 @@ class ASH_EXPORT PhoneHubNotificationController
   // Logs the number of PhoneHub notifications.
   void LogNotificationCount();
 
-  // Creates or updates a ChromeOS notification for the given PhoneHub
-  // notification data.
-  void CreateOrUpdateNotification(
-      const chromeos::phonehub::Notification* notification);
+  // Shows a Chrome OS notification for the provided phonehub::Notification.
+  // If |is_update| is true, this function updates an existing notification;
+  // otherwise, a new notification is created.
+  void SetNotification(const chromeos::phonehub::Notification* notification,
+                       bool is_update);
 
   // Creates a message_center::Notification from the PhoneHub notification data.
   std::unique_ptr<message_center::Notification> CreateNotification(
       const chromeos::phonehub::Notification* notification,
       const std::string& cros_id,
-      NotificationDelegate* delegate);
+      NotificationDelegate* delegate,
+      bool is_update);
+  int GetSystemPriorityForNotification(
+      const chromeos::phonehub::Notification* notification,
+      bool is_update);
 
   static std::unique_ptr<message_center::MessageView>
   CreateCustomNotificationView(

ash/system/phonehub/phone_hub_notification_controller_unittest.cc

@@ -327,7 +327,7 @@ TEST_F(PhoneHubNotificationControllerTest, DoNotReshowPopupNotification) {
   ASSERT_TRUE(cros_notification);
   EXPECT_EQ(message_center::LOW_PRIORITY, cros_notification->priority());
 
-  // Disable the feature..
+  // Disable the feature.
   feature_status_provider_->SetStatus(
       chromeos::phonehub::FeatureStatus::kDisabled);
   notification_manager_->RemoveNotification(kPhoneHubNotificationId0);
@@ -340,6 +340,25 @@ TEST_F(PhoneHubNotificationControllerTest, DoNotReshowPopupNotification) {
   cros_notification = FindNotification(kCrOSNotificationId0);
   ASSERT_TRUE(cros_notification);
   EXPECT_EQ(message_center::MAX_PRIORITY, cros_notification->priority());
+
+  // Update the notification with some new text, but keep the notification ID
+  // the same.
+  chromeos::phonehub::Notification modified_fake_notification(
+      kPhoneHubNotificationId0,
+      chromeos::phonehub::Notification::AppMetadata(base::UTF8ToUTF16(kAppName),
+                                                    kPackageName,
+                                                    /*icon=*/gfx::Image()),
+      base::Time::Now(), chromeos::phonehub::Notification::Importance::kHigh,
+      /*inline_reply_id=*/0, base::UTF8ToUTF16(kTitle),
+      base::UTF8ToUTF16("New text"));
+
+  // Update the existingt notification; the priority should be MAX_PRIORITY, and
+  // renotify should be true.
+  notification_manager_->SetNotification(modified_fake_notification);
+  cros_notification = FindNotification(kCrOSNotificationId0);
+  ASSERT_TRUE(cros_notification);
+  EXPECT_EQ(message_center::MAX_PRIORITY, cros_notification->priority());
+  EXPECT_TRUE(cros_notification->renotify());
 }
 
 }  // namespace ash