Commit 21ba15eb authored by Kyle Horimoto's avatar Kyle Horimoto Committed by Chromium LUCI CQ

[CrOS PhoneHub] Ensure conversation notifications are shown as popups

Previously, we'd look at the notification's "priority" field to
determine whether or not we'd show a popup or just add the notification
to the notification shade. This caused confusion to some users, since
some messaging apps would add this as a "minimum" or "no" priority
notification, which would cause us to silently add the notification
instead of adding it as a popup.

This CL updates the logic so that we always show notifications as a
popup.

Fixed: 1165646
Change-Id: Ic76bcb4eb448547ecc61c2d79828c07a3e3352b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626030
Auto-Submit: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: default avatarRegan Hsu <hsuregan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842781}
parent 0a8c6b81
......@@ -462,19 +462,6 @@ PhoneHubNotificationController::CreateNotification(
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());
......@@ -489,7 +476,6 @@ int PhoneHubNotificationController::GetSystemPriorityForNotification(
// so that users can see new messages come in as they are chatting. See
// https://crbug.com/1159063.
return message_center::MAX_PRIORITY;
}
}
// static
......
......@@ -361,4 +361,23 @@ TEST_F(PhoneHubNotificationControllerTest, DoNotReshowPopupNotification) {
EXPECT_TRUE(cros_notification->renotify());
}
// Regression test for https://crbug.com/1165646.
TEST_F(PhoneHubNotificationControllerTest, MinPriorityNotification) {
chromeos::phonehub::Notification fake_notification(
kPhoneHubNotificationId0,
chromeos::phonehub::Notification::AppMetadata(base::UTF8ToUTF16(kAppName),
kPackageName,
/*icon=*/gfx::Image()),
base::Time::Now(), chromeos::phonehub::Notification::Importance::kMin,
/*inline_reply_id=*/0, base::UTF8ToUTF16(kTitle),
base::UTF8ToUTF16(kTextContent));
// Adding the notification for the first time shows a pop-up (MAX_PRIORITY),
// even though the notification itself is Importance::kMin.
notification_manager_->SetNotification(fake_notification);
auto* cros_notification = FindNotification(kCrOSNotificationId0);
ASSERT_TRUE(cros_notification);
EXPECT_EQ(message_center::MAX_PRIORITY, cros_notification->priority());
}
} // namespace ash
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment