Commit 3129eaf9 authored by Richard Knoll's avatar Richard Knoll Committed by Commit Bot

Add metrics for muted notifications

This adds metrics for actions of the muted notification as well as stats
once the screen capture session ends.

Bug: 1131375
Change-Id: I47a5fc9bf6677f3cc248d727b9a95c4048983870
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2467941
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: default avatarJesse Doherty <jwd@chromium.org>
Reviewed-by: default avatarPeter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822681}
parent c0f5fce4
......@@ -36,7 +36,13 @@ class NotificationBlocker {
const message_center::Notification& notification) = 0;
// Called when |notification| got blocked because this blocker is active.
// |replaced| is true if the |notification| replaces a previously blocked one.
virtual void OnBlockedNotification(
const message_center::Notification& notification,
bool replaced) {}
// Called when a previously blocked |notification| got closed.
virtual void OnClosedNotification(
const message_center::Notification& notification) {}
// Observer methods.
......
......@@ -40,26 +40,20 @@ void NotificationDisplayQueue::EnqueueNotification(
NotificationHandler::Type notification_type,
const message_center::Notification& notification,
std::unique_ptr<NotificationCommon::Metadata> metadata) {
RemoveQueuedNotification(notification.id());
bool replaced =
DoRemoveQueuedNotification(notification.id(), /*notify=*/false);
queued_notifications_.emplace_back(notification_type, notification,
std::move(metadata));
// Notify blockers that a new notification has been blocked.
for (auto& blocker : blockers_) {
if (blocker->ShouldBlockNotification(notification))
blocker->OnBlockedNotification(notification);
blocker->OnBlockedNotification(notification, replaced);
}
}
void NotificationDisplayQueue::RemoveQueuedNotification(
const std::string& notification_id) {
auto it =
std::find_if(queued_notifications_.begin(), queued_notifications_.end(),
[&notification_id](const QueuedNotification& queued) {
return queued.notification.id() == notification_id;
});
if (it != queued_notifications_.end())
queued_notifications_.erase(it);
DoRemoveQueuedNotification(notification_id, /*notify=*/true);
}
std::set<std::string> NotificationDisplayQueue::GetQueuedNotificationIds()
......@@ -92,6 +86,29 @@ void NotificationDisplayQueue::AddNotificationBlocker(
blockers_.push_back(std::move(blocker));
}
bool NotificationDisplayQueue::DoRemoveQueuedNotification(
const std::string& notification_id,
bool notify) {
auto it =
std::find_if(queued_notifications_.begin(), queued_notifications_.end(),
[&notification_id](const QueuedNotification& queued) {
return queued.notification.id() == notification_id;
});
if (it == queued_notifications_.end())
return false;
if (notify) {
for (auto& blocker : blockers_) {
if (blocker->ShouldBlockNotification(it->notification))
blocker->OnClosedNotification(it->notification);
}
}
queued_notifications_.erase(it);
return true;
}
void NotificationDisplayQueue::MaybeDisplayQueuedNotifications() {
auto show_begin = std::stable_partition(
queued_notifications_.begin(), queued_notifications_.end(),
......
......@@ -66,6 +66,12 @@ class NotificationDisplayQueue : public NotificationBlocker::Observer {
void AddNotificationBlocker(std::unique_ptr<NotificationBlocker> blocker);
private:
// Removes a queued notification by its |notification_id| and returns if there
// was a queued notification with that id. If |notify| is true this will
// notify all relevant blockers about the removal.
bool DoRemoveQueuedNotification(const std::string& notification_id,
bool notify);
// Called when the state of a notification blocker changes. Will display and
// free all queued notifications if no blocker is active anymore.
void MaybeDisplayQueuedNotifications();
......
......@@ -40,6 +40,10 @@ class FakeNotificationBlocker : public NotificationBlocker {
}
MOCK_METHOD(void,
OnBlockedNotification,
(const message_center::Notification&, bool),
(override));
MOCK_METHOD(void,
OnClosedNotification,
(const message_center::Notification&),
(override));
......@@ -330,3 +334,31 @@ TEST_F(NotificationDisplayQueueTest, MultipleBlockersNotifyBlocked) {
queue().EnqueueNotification(NotificationHandler::Type::TRANSIENT,
CreateNotification("id2"), /*metadata=*/nullptr);
}
TEST_F(NotificationDisplayQueueTest, NotifiesReplacedNotification) {
message_center::Notification notification = CreateNotification("id");
notification_blocker().SetShouldBlockNotifications(true);
EXPECT_CALL(notification_blocker(),
OnBlockedNotification(testing::_, /*replaced=*/false));
queue().EnqueueNotification(NotificationHandler::Type::TRANSIENT,
notification, /*metadata=*/nullptr);
EXPECT_CALL(notification_blocker(),
OnBlockedNotification(testing::_, /*replaced=*/true));
EXPECT_CALL(notification_blocker(), OnClosedNotification).Times(0);
queue().EnqueueNotification(NotificationHandler::Type::TRANSIENT,
notification, /*metadata=*/nullptr);
}
TEST_F(NotificationDisplayQueueTest, NotifiesClosedNotification) {
message_center::Notification notification = CreateNotification("id");
notification_blocker().SetShouldBlockNotifications(true);
EXPECT_CALL(notification_blocker(), OnBlockedNotification);
queue().EnqueueNotification(NotificationHandler::Type::TRANSIENT,
notification, /*metadata=*/nullptr);
EXPECT_CALL(notification_blocker(), OnClosedNotification);
queue().RemoveQueuedNotification(notification.id());
}
......@@ -6,6 +6,8 @@
#include <algorithm>
#include "base/metrics/histogram_functions.h"
#include "base/strings/strcat.h"
#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
#include "chrome/browser/notifications/notification_display_service.h"
#include "chrome/grit/generated_resources.h"
......@@ -16,7 +18,27 @@
#include "url/origin.h"
namespace {
const char kMuteNotificationId[] = "notifications_muted";
// Suffix for a mute notification action. Should match suffix
// NotificationMuteAction in histogram_suffixes_list.xml.
std::string MutedActionSuffix(MutedNotificationHandler::Action action) {
switch (action) {
case MutedNotificationHandler::Action::kUserClose:
return "Close";
case MutedNotificationHandler::Action::kBodyClick:
return "Body";
case MutedNotificationHandler::Action::kShowClick:
return "Show";
}
}
void RecordScreenCaptureCount(const std::string& suffix, int count) {
base::UmaHistogramCounts100(
base::StrCat({"Notifications.Blocker.ScreenCapture.", suffix}), count);
}
} // namespace
ScreenCaptureNotificationBlocker::ScreenCaptureNotificationBlocker(
......@@ -50,16 +72,27 @@ bool ScreenCaptureNotificationBlocker::ShouldBlockNotification(
}
void ScreenCaptureNotificationBlocker::OnBlockedNotification(
const message_center::Notification& notification) {
++muted_notification_count_;
const message_center::Notification& notification,
bool replaced) {
if (replaced)
++replaced_notification_count_;
else
++muted_notification_count_;
if (state_ == NotifyState::kNotifyMuted)
DisplayMuteNotification();
}
void ScreenCaptureNotificationBlocker::OnClosedNotification(
const message_center::Notification& notification) {
++closed_notification_count_;
}
void ScreenCaptureNotificationBlocker::OnAction(
MutedNotificationHandler::Action action) {
DCHECK(state_ == NotifyState::kNotifyMuted);
CloseMuteNotification();
ReportMuteNotificationAction(action);
switch (action) {
case MutedNotificationHandler::Action::kUserClose:
......@@ -69,6 +102,7 @@ void ScreenCaptureNotificationBlocker::OnAction(
case MutedNotificationHandler::Action::kShowClick:
state_ = NotifyState::kShowAll;
NotifyBlockingStateChanged();
ReportSessionMetrics();
break;
}
}
......@@ -82,7 +116,11 @@ void ScreenCaptureNotificationBlocker::OnIsCapturingDisplayChanged(
capturing_web_contents_.erase(web_contents);
if (capturing_web_contents_.empty()) {
ReportSessionMetrics();
muted_notification_count_ = 0;
replaced_notification_count_ = 0;
closed_notification_count_ = 0;
reported_session_metrics_ = false;
state_ = NotifyState::kNotifyMuted;
CloseMuteNotification();
}
......@@ -90,16 +128,37 @@ void ScreenCaptureNotificationBlocker::OnIsCapturingDisplayChanged(
NotifyBlockingStateChanged();
}
void ScreenCaptureNotificationBlocker::ReportSessionMetrics() {
if (reported_session_metrics_)
return;
RecordScreenCaptureCount("MutedCount", muted_notification_count_);
RecordScreenCaptureCount("ReplacedCount", replaced_notification_count_);
RecordScreenCaptureCount("ClosedCount", closed_notification_count_);
reported_session_metrics_ = true;
}
void ScreenCaptureNotificationBlocker::ReportMuteNotificationAction(
MutedNotificationHandler::Action action) {
RecordScreenCaptureCount(
base::StrCat({"Action.", MutedActionSuffix(action)}),
muted_notification_count_ + replaced_notification_count_);
}
void ScreenCaptureNotificationBlocker::DisplayMuteNotification() {
int total_notification_count =
muted_notification_count_ + replaced_notification_count_;
message_center::RichNotificationData rich_notification_data;
rich_notification_data.renotify = true;
rich_notification_data.buttons.emplace_back(l10n_util::GetPluralStringFUTF16(
IDS_NOTIFICATION_MUTED_ACTION_SHOW, muted_notification_count_));
IDS_NOTIFICATION_MUTED_ACTION_SHOW, total_notification_count));
message_center::Notification notification(
message_center::NOTIFICATION_TYPE_SIMPLE, kMuteNotificationId,
l10n_util::GetPluralStringFUTF16(IDS_NOTIFICATION_MUTED_TITLE,
muted_notification_count_),
total_notification_count),
l10n_util::GetStringUTF16(IDS_NOTIFICATION_MUTED_MESSAGE),
/*icon=*/gfx::Image(),
/*display_source=*/base::string16(),
......
......@@ -39,7 +39,9 @@ class ScreenCaptureNotificationBlocker
// NotificationBlocker:
bool ShouldBlockNotification(
const message_center::Notification& notification) override;
void OnBlockedNotification(
void OnBlockedNotification(const message_center::Notification& notification,
bool replaced) override;
void OnClosedNotification(
const message_center::Notification& notification) override;
// MutedNotificationHandler::Delegate:
......@@ -53,6 +55,8 @@ class ScreenCaptureNotificationBlocker
FRIEND_TEST_ALL_PREFIXES(ScreenCaptureNotificationBlockerTest,
ObservesMediaStreamCaptureIndicator);
void ReportSessionMetrics();
void ReportMuteNotificationAction(MutedNotificationHandler::Action action);
void DisplayMuteNotification();
void CloseMuteNotification();
......@@ -65,9 +69,14 @@ class ScreenCaptureNotificationBlocker
NotifyState state_ = NotifyState::kNotifyMuted;
// Counter for the number of notifications that have been muted during the
// current screen capturing session.
// Number of notifications muted during the current screen capture session.
int muted_notification_count_ = 0;
// Number of notifications replaced during the current screen capture session.
int replaced_notification_count_ = 0;
// Number of notifications closed during the current screen capture session.
int closed_notification_count_ = 0;
// Flag if metrics have been reported for the current screen capture session.
bool reported_session_metrics_ = false;
// The |notification_display_service_| owns a NotificationDisplayQueue which
// owns |this| so a raw pointer is safe here.
......
......@@ -6,6 +6,7 @@
#include "base/optional.h"
#include "base/scoped_observer.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/browser_features.h"
#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
......@@ -27,15 +28,20 @@
namespace {
message_center::Notification CreateNotification(const GURL& origin) {
message_center::Notification CreateNotification(const GURL& origin,
const std::string& id) {
return message_center::Notification(
message_center::NOTIFICATION_TYPE_SIMPLE, /*id=*/"id",
message_center::NOTIFICATION_TYPE_SIMPLE, id,
/*title=*/base::string16(),
/*message=*/base::string16(), /*icon=*/gfx::Image(),
/*display_source=*/base::string16(), origin, message_center::NotifierId(),
message_center::RichNotificationData(), /*delegate=*/nullptr);
}
message_center::Notification CreateNotification(const GURL& origin) {
return CreateNotification(origin, /*id=*/"id");
}
} // namespace
namespace {
......@@ -87,6 +93,15 @@ class ScreenCaptureNotificationBlockerTest : public testing::Test {
return contents;
}
void SimulateClose(bool by_user) {
base::Optional<message_center::Notification> notification =
GetMutedNotification();
ASSERT_TRUE(notification);
notification_service_->RemoveNotification(
NotificationHandler::Type::NOTIFICATIONS_MUTED, notification->id(),
by_user, /*silent=*/false);
}
void SimulateClick(const base::Optional<int>& action_index) {
base::Optional<message_center::Notification> notification =
GetMutedNotification();
......@@ -202,7 +217,7 @@ TEST_F(ScreenCaptureNotificationBlockerTest, ShowsMutedNotification) {
blocker().OnIsCapturingDisplayChanged(
CreateWebContents(GURL("https://example1.com")), true);
blocker().OnBlockedNotification(
CreateNotification(GURL("https://example2.com")));
CreateNotification(GURL("https://example2.com")), /*replaced*/ false);
base::Optional<message_center::Notification> notification =
GetMutedNotification();
......@@ -228,7 +243,7 @@ TEST_F(ScreenCaptureNotificationBlockerTest, UpdatesMutedNotification) {
for (int i = 0; i < kCount; ++i) {
blocker().OnBlockedNotification(
CreateNotification(GURL("https://example2.com")));
CreateNotification(GURL("https://example2.com")), /*replaced*/ false);
}
base::Optional<message_center::Notification> notification =
......@@ -255,7 +270,7 @@ TEST_F(ScreenCaptureNotificationBlockerTest, ClosesMutedNotification) {
// Expect a notification once we block one.
blocker().OnBlockedNotification(
CreateNotification(GURL("https://example2.com")));
CreateNotification(GURL("https://example2.com")), /*replaced*/ false);
EXPECT_TRUE(GetMutedNotification());
// Expect notification to be closed when capturing stops.
......@@ -268,7 +283,7 @@ TEST_F(ScreenCaptureNotificationBlockerTest,
blocker().OnIsCapturingDisplayChanged(
CreateWebContents(GURL("https://example1.com")), true);
blocker().OnBlockedNotification(
CreateNotification(GURL("https://example2.com")));
CreateNotification(GURL("https://example2.com")), /*replaced*/ false);
// Expect notification to be closed after clicking on its body.
SimulateClick(/*action_index=*/base::nullopt);
......@@ -279,13 +294,13 @@ TEST_F(ScreenCaptureNotificationBlockerTest, ShowsMutedNotificationAfterClose) {
blocker().OnIsCapturingDisplayChanged(
CreateWebContents(GURL("https://example1.com")), true);
blocker().OnBlockedNotification(
CreateNotification(GURL("https://example2.com")));
CreateNotification(GURL("https://example2.com")), /*replaced*/ false);
// Blocking another notification after closing the muted one should show a new
// one with an updated message.
SimulateClick(/*action_index=*/base::nullopt);
blocker().OnBlockedNotification(
CreateNotification(GURL("https://example2.com")));
CreateNotification(GURL("https://example2.com")), /*replaced*/ false);
base::Optional<message_center::Notification> notification =
GetMutedNotification();
......@@ -308,7 +323,7 @@ TEST_F(ScreenCaptureNotificationBlockerTest, ShowAction) {
message_center::Notification notification =
CreateNotification(GURL("https://example2.com"));
blocker().OnBlockedNotification(notification);
blocker().OnBlockedNotification(notification, /*replaced*/ false);
EXPECT_TRUE(blocker().ShouldBlockNotification(notification));
// Showing should close the "Notifications muted" notification and allow
......@@ -320,3 +335,100 @@ TEST_F(ScreenCaptureNotificationBlockerTest, ShowAction) {
EXPECT_FALSE(GetMutedNotification());
EXPECT_FALSE(blocker().ShouldBlockNotification(notification));
}
TEST_F(ScreenCaptureNotificationBlockerTest, CloseHistogram) {
base::HistogramTester histogram_tester;
const char kHistogram[] = "Notifications.Blocker.ScreenCapture.Action.Close";
blocker().OnIsCapturingDisplayChanged(
CreateWebContents(GURL("https://example1.com")), true);
message_center::Notification notification =
CreateNotification(GURL("https://example2.com"));
blocker().OnBlockedNotification(notification, /*replaced*/ false);
SimulateClose(/*by_user=*/true);
histogram_tester.ExpectBucketCount(kHistogram, /*sample=*/1, /*count=*/1);
histogram_tester.ExpectTotalCount(kHistogram, /*count=*/1);
blocker().OnBlockedNotification(notification, /*replaced*/ false);
SimulateClose(/*by_user=*/true);
histogram_tester.ExpectBucketCount(kHistogram, /*sample=*/2, /*count=*/1);
histogram_tester.ExpectTotalCount(kHistogram, /*count=*/2);
blocker().OnBlockedNotification(notification, /*replaced*/ false);
SimulateClose(/*by_user=*/false);
histogram_tester.ExpectTotalCount(kHistogram, /*count=*/2);
}
TEST_F(ScreenCaptureNotificationBlockerTest, BodyClickHistogram) {
base::HistogramTester histogram_tester;
const char kHistogram[] = "Notifications.Blocker.ScreenCapture.Action.Body";
blocker().OnIsCapturingDisplayChanged(
CreateWebContents(GURL("https://example1.com")), true);
message_center::Notification notification =
CreateNotification(GURL("https://example2.com"));
blocker().OnBlockedNotification(notification, /*replaced*/ false);
SimulateClick(/*action_index=*/base::nullopt);
histogram_tester.ExpectBucketCount(kHistogram, /*sample=*/1, /*count=*/1);
histogram_tester.ExpectTotalCount(kHistogram, /*count=*/1);
blocker().OnBlockedNotification(notification, /*replaced*/ false);
SimulateClick(/*action_index=*/base::nullopt);
histogram_tester.ExpectBucketCount(kHistogram, /*sample=*/2, /*count=*/1);
histogram_tester.ExpectTotalCount(kHistogram, /*count=*/2);
}
TEST_F(ScreenCaptureNotificationBlockerTest, ShowClickHistogram) {
base::HistogramTester histogram_tester;
blocker().OnIsCapturingDisplayChanged(
CreateWebContents(GURL("https://example1.com")), true);
message_center::Notification notification =
CreateNotification(GURL("https://example2.com"));
blocker().OnBlockedNotification(notification, /*replaced*/ false);
SimulateClick(kShowActionIndex);
histogram_tester.ExpectUniqueSample(
"Notifications.Blocker.ScreenCapture.Action.Show", /*sample=*/1,
/*count=*/1);
}
TEST_F(ScreenCaptureNotificationBlockerTest, SessionEndHistograms) {
base::HistogramTester histogram_tester;
content::WebContents* contents =
CreateWebContents(GURL("https://example1.com"));
blocker().OnIsCapturingDisplayChanged(contents, true);
message_center::Notification notification1 =
CreateNotification(GURL("https://example2.com"), "id1");
message_center::Notification notification2 =
CreateNotification(GURL("https://example2.com"), "id2");
message_center::Notification notification3 =
CreateNotification(GURL("https://example2.com"), "id3");
// Mute 3 unique notifications.
blocker().OnBlockedNotification(notification1, /*replaced*/ false);
blocker().OnBlockedNotification(notification2, /*replaced*/ false);
blocker().OnBlockedNotification(notification3, /*replaced*/ false);
// Replace 2 of the muted notifications.
blocker().OnBlockedNotification(notification1, /*replaced*/ true);
blocker().OnBlockedNotification(notification2, /*replaced*/ true);
// Close 1 of the muted notifications.
blocker().OnClosedNotification(notification1);
blocker().OnIsCapturingDisplayChanged(contents, false);
histogram_tester.ExpectUniqueSample(
"Notifications.Blocker.ScreenCapture.MutedCount", /*sample=*/3,
/*count=*/1);
histogram_tester.ExpectUniqueSample(
"Notifications.Blocker.ScreenCapture.ReplacedCount", /*sample=*/2,
/*count=*/1);
histogram_tester.ExpectUniqueSample(
"Notifications.Blocker.ScreenCapture.ClosedCount", /*sample=*/1,
/*count=*/1);
}
......@@ -85,6 +85,58 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
</summary>
</histogram>
<histogram name="Notifications.Blocker.ScreenCapture.Action.{Action}"
units="notifications" expires_after="M91">
<owner>knollr@chromium.org</owner>
<owner>peter@chromium.org</owner>
<summary>
Records the number of times the 'Notifications muted' notification got shown
before the user took an action. Clicking on 'Show' can only be recorded once
per session while 'Body' and 'Close' may be recorded multiple times. Logged
each time the user interacts with the notification. {Action}
</summary>
<token key="Action">
<variant name="Body"/>
<variant name="Close"/>
<variant name="Show"/>
</token>
</histogram>
<histogram name="Notifications.Blocker.ScreenCapture.ClosedCount"
units="notifications" expires_after="M91">
<owner>knollr@chromium.org</owner>
<owner>peter@chromium.org</owner>
<summary>
The number of muted notifications that got closed programmatically (e.g. by
the developer via JS) during a screen capture session before they were shown
to the user. Logged once after each screen capture session ends.
</summary>
</histogram>
<histogram name="Notifications.Blocker.ScreenCapture.MutedCount"
units="notifications" expires_after="M91">
<owner>knollr@chromium.org</owner>
<owner>peter@chromium.org</owner>
<summary>
The number of notifications that got muted during a screen capture session
instead of being shown to the user. This number might be higher than the
actual number of queued notifications at the end of a session as some
notifications might have been closed programmatically before that. Logged
once after each screen capture session ends.
</summary>
</histogram>
<histogram name="Notifications.Blocker.ScreenCapture.ReplacedCount"
units="notifications" expires_after="M91">
<owner>knollr@chromium.org</owner>
<owner>peter@chromium.org</owner>
<summary>
The number of muted notifications that got replaced during a screen capture
session. The user will only see the final state of the notification without
the previous versions. Logged once after each screen capture session ends.
</summary>
</histogram>
<histogram name="Notifications.Chime.Android.Events" enum="ChimeEvent"
expires_after="never">
<owner>hesen@chromium.org</owner>
......
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