Commit 62e653a5 authored by Jeroen Dhollander's avatar Jeroen Dhollander Committed by Commit Bot

Reland "Respect assistant notification expiration"

This is a reland of 31b40abf

The original push failed because there was an API change in
|ScopedTaskEnvironment| between the time I implemented this CL,
and the time it was submitted.

Original change's description:
> Respect assistant notification expiration
>
> Assistant notifications can now be given an expiration time through the
> |expiry_timestamp_ms| field.
>
> A new AssistantNotificationExpiryMonitor class will monitor all
> notifications and remove them (from the AssistantNotificationModel)
> when they expire.
>
> BUG=b/116618188
> TEST=unittests and manual create notification with expiry_timestamp_ms
>
> Change-Id: I5d0c0b3bcd3a26d772bccb883a5a8d5914d9c929
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1688019
> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
> Cr-Commit-Position: refs/heads/master@{#677864}

Bug: b/116618188
Change-Id: I93281eba85fb532877f409637d8f82820756d614

TBR=dcheng@chromium.org,xiaohuic@chromium.org,wutao@chromium.org

Change-Id: I93281eba85fb532877f409637d8f82820756d614
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1704674
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: default avatarXiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677995}
parent fd5efb2c
......@@ -190,6 +190,8 @@ component("ash") {
"assistant/assistant_interaction_controller.h",
"assistant/assistant_notification_controller.cc",
"assistant/assistant_notification_controller.h",
"assistant/assistant_notification_expiry_monitor.cc",
"assistant/assistant_notification_expiry_monitor.h",
"assistant/assistant_screen_context_controller.cc",
"assistant/assistant_screen_context_controller.h",
"assistant/assistant_settings.cc",
......@@ -1603,6 +1605,7 @@ test("ash_unittests") {
"app_menu/notification_menu_view_unittest.cc",
"app_menu/notification_overflow_view_unittest.cc",
"assistant/assistant_controller_unittest.cc",
"assistant/assistant_notification_controller_unittest.cc",
"assistant/assistant_screen_context_controller_unittest.cc",
"assistant/model/assistant_query_history_unittest.cc",
"assistant/ui/assistant_container_view_unittest.cc",
......
......@@ -8,6 +8,7 @@
#include <utility>
#include "ash/assistant/assistant_controller.h"
#include "ash/assistant/assistant_notification_expiry_monitor.h"
#include "ash/assistant/util/deep_link_util.h"
#include "ash/public/cpp/notification_utils.h"
#include "ash/public/cpp/vector_icons/vector_icons.h"
......@@ -81,6 +82,7 @@ AssistantNotificationController::AssistantNotificationController(
AssistantController* assistant_controller)
: assistant_controller_(assistant_controller),
binding_(this),
expiry_monitor_(this),
notifier_id_(GetNotifierId()) {
AddModelObserver(this);
assistant_controller_->AddObserver(this);
......
......@@ -9,6 +9,7 @@
#include "ash/ash_export.h"
#include "ash/assistant/assistant_controller_observer.h"
#include "ash/assistant/assistant_notification_expiry_monitor.h"
#include "ash/assistant/model/assistant_notification_model.h"
#include "ash/assistant/model/assistant_notification_model_observer.h"
#include "ash/assistant/model/assistant_ui_model_observer.h"
......@@ -97,6 +98,7 @@ class ASH_EXPORT AssistantNotificationController
mojo::Binding<mojom::AssistantNotificationController> binding_;
AssistantNotificationModel model_;
AssistantNotificationExpiryMonitor expiry_monitor_;
// Owned by AssistantController.
chromeos::assistant::mojom::Assistant* assistant_ = nullptr;
......
// Copyright 2019 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 "ash/assistant/assistant_notification_controller.h"
#include "ash/assistant/assistant_controller.h"
#include "ash/assistant/model/assistant_notification_model_observer.h"
#include "ash/shell.h"
#include "ash/test/ash_test_base.h"
#include "base/test/scoped_task_environment.h"
#include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace ash {
using chromeos::assistant::mojom::AssistantNotificationPtr;
using testing::_;
using testing::Eq;
using testing::Field;
using testing::StrictMock;
constexpr bool kAnyBool = false;
class AssistantNotificationModelObserverMock
: public AssistantNotificationModelObserver {
public:
AssistantNotificationModelObserverMock() = default;
~AssistantNotificationModelObserverMock() override = default;
MOCK_METHOD1(OnNotificationAdded,
void(const AssistantNotification* notification));
MOCK_METHOD1(OnNotificationUpdated,
void(const AssistantNotification* notification));
MOCK_METHOD2(OnNotificationRemoved,
void(const AssistantNotification* notification,
bool from_server));
MOCK_METHOD1(OnAllNotificationsRemoved, void(bool from_server));
private:
DISALLOW_COPY_AND_ASSIGN(AssistantNotificationModelObserverMock);
};
MATCHER_P(IdIs, expected_id, "") {
if (arg->client_id != expected_id) {
*result_listener << "Received notification with a wrong id.\n"
<< "Expected:\n '" << expected_id << "'\n"
<< "Actual:\n '" << arg->client_id << "'\n";
return false;
}
return true;
}
class AssistantNotificationControllerTest : public AshTestBase {
protected:
AssistantNotificationControllerTest() {
DestroyScopedTaskEnvironment();
scoped_task_environment_ =
std::make_unique<base::test::ScopedTaskEnvironment>(
base::test::ScopedTaskEnvironment::TimeSource::MOCK_TIME_AND_NOW,
base::test::ScopedTaskEnvironment::MainThreadType::UI);
}
~AssistantNotificationControllerTest() override = default;
void SetUp() override {
ASSERT_TRUE(chromeos::switches::IsAssistantEnabled());
AshTestBase::SetUp();
controller_ =
Shell::Get()->assistant_controller()->notification_controller();
DCHECK(controller_);
}
AssistantNotificationController& controller() { return *controller_; }
AssistantNotificationModelObserverMock& AddStrictObserverMock() {
observer_ =
std::make_unique<StrictMock<AssistantNotificationModelObserverMock>>();
controller().AddModelObserver(observer_.get());
return *observer_;
}
AssistantNotificationPtr CreateNotification(const std::string& id) {
auto notification =
chromeos::assistant::mojom::AssistantNotification::New();
notification->client_id = id;
return notification;
}
AssistantNotificationPtr CreateNotification(const std::string& id,
int timeout_ms) {
auto result = CreateNotification(id);
result->expiry_time =
base::Time::Now() + base::TimeDelta::FromMilliseconds(timeout_ms);
return result;
}
void AddNotification(const std::string& id, int timeout_ms) {
controller().AddOrUpdateNotification(CreateNotification(id, timeout_ms));
}
void AddNotification(const std::string& id) {
controller().AddOrUpdateNotification(CreateNotification(id));
}
void UpdateNotification(const std::string& id, int timeout_ms) {
controller().AddOrUpdateNotification(CreateNotification(id, timeout_ms));
}
void UpdateNotification(const std::string& id) {
controller().AddOrUpdateNotification(CreateNotification(id));
}
void RemoveNotification(const std::string& id) {
controller().RemoveNotificationById(id, kAnyBool);
}
void ForwardTimeInMs(int time_in_ms) {
scoped_task_environment_->FastForwardBy(
base::TimeDelta::FromMilliseconds(time_in_ms));
}
private:
AssistantNotificationController* controller_;
std::unique_ptr<AssistantNotificationModelObserverMock> observer_;
std::unique_ptr<base::test::ScopedTaskEnvironment> scoped_task_environment_;
DISALLOW_COPY_AND_ASSIGN(AssistantNotificationControllerTest);
};
TEST_F(AssistantNotificationControllerTest,
ShouldInformObserverOfNewNotifications) {
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationAdded(IdIs("id")));
controller().AddOrUpdateNotification(CreateNotification("id"));
}
TEST_F(AssistantNotificationControllerTest,
ShouldInformObserverOfUpdatedNotifications) {
const auto notification = CreateNotification("id");
controller().AddOrUpdateNotification(notification.Clone());
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationUpdated(IdIs("id")));
controller().AddOrUpdateNotification(notification.Clone());
}
TEST_F(AssistantNotificationControllerTest,
ShouldInformObserverOfRemovedNotifications) {
const auto notification = CreateNotification("id");
controller().AddOrUpdateNotification(notification.Clone());
constexpr bool from_server = kAnyBool;
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved(IdIs("id"), Eq(from_server)));
controller().RemoveNotificationById(notification->client_id, from_server);
}
TEST_F(AssistantNotificationControllerTest,
ShouldInformObserverOfRemoveAllNotifications) {
const auto notification = CreateNotification("id");
controller().AddOrUpdateNotification(notification.Clone());
constexpr bool from_server = !kAnyBool;
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnAllNotificationsRemoved(Eq(from_server)));
controller().RemoveAllNotifications(from_server);
}
TEST_F(AssistantNotificationControllerTest,
ShouldRemoveNotificationWhenItExpires) {
constexpr int timeout_ms = 1000;
AddNotification("id", timeout_ms);
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved(IdIs("id"), _));
ForwardTimeInMs(timeout_ms);
}
TEST_F(AssistantNotificationControllerTest,
ShouldNotRemoveNotificationsTooSoon) {
constexpr int timeout_ms = 1000;
AddNotification("id", timeout_ms);
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved).Times(0);
ForwardTimeInMs(timeout_ms - 1);
}
TEST_F(AssistantNotificationControllerTest,
ShouldUseFromServerFalseWhenNotificationExpires) {
constexpr int timeout_ms = 1000;
AddNotification("id", timeout_ms);
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved(_, Eq(false)));
ForwardTimeInMs(timeout_ms);
}
TEST_F(AssistantNotificationControllerTest,
ShouldRemoveEachNotificationAsItExpires) {
constexpr int first_timeout_ms = 1000;
constexpr int second_timeout_ms = 1500;
AddNotification("first", first_timeout_ms);
AddNotification("second", second_timeout_ms);
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved(IdIs("first"), _));
ForwardTimeInMs(first_timeout_ms);
EXPECT_CALL(observer, OnNotificationRemoved(IdIs("second"), _));
int delta_between_notifications = second_timeout_ms - first_timeout_ms;
ForwardTimeInMs(delta_between_notifications);
}
TEST_F(AssistantNotificationControllerTest,
ShouldSupport2NotificationsThatExpireAtTheSameTime) {
constexpr int timeout_ms = 1000;
AddNotification("first", timeout_ms);
AddNotification("at-same-time", timeout_ms);
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved(IdIs("first"), _));
EXPECT_CALL(observer, OnNotificationRemoved(IdIs("at-same-time"), _));
ForwardTimeInMs(timeout_ms);
}
TEST_F(AssistantNotificationControllerTest,
ShouldImmediateRemoveNotificationsThatAlreadyExpired) {
constexpr int negative_timeout = -1000;
AddNotification("expired", negative_timeout);
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved(IdIs("expired"), _));
}
TEST_F(AssistantNotificationControllerTest,
ShouldNotRemoveNotificationsThatWereManuallyRemoved) {
constexpr int timeout = 1000;
AddNotification("id", timeout);
RemoveNotification("id");
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved).Times(0);
ForwardTimeInMs(timeout);
}
TEST_F(AssistantNotificationControllerTest,
ShouldSupportExpiryTimeSetInUpdate) {
constexpr int timeout = 1000;
AddNotification("id");
UpdateNotification("id", timeout);
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved);
ForwardTimeInMs(timeout);
}
TEST_F(AssistantNotificationControllerTest,
ShouldNotRemoveNotificationIfExpiryTimeIsClearedInUpdate) {
constexpr int timeout = 1000;
AddNotification("id", timeout);
UpdateNotification("id");
auto& observer = AddStrictObserverMock();
EXPECT_CALL(observer, OnNotificationRemoved).Times(0);
ForwardTimeInMs(timeout);
}
} // namespace ash
// Copyright 2019 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 "ash/assistant/assistant_notification_expiry_monitor.h"
#include "ash/assistant/assistant_notification_controller.h"
#include "ash/assistant/model/assistant_notification_model.h"
#include "ash/assistant/model/assistant_notification_model_observer.h"
#include "base/bind.h"
#include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
namespace ash {
namespace {
bool HasExpired(const AssistantNotificationExpiryMonitor::AssistantNotification*
notification) {
return notification->expiry_time.has_value() &&
(notification->expiry_time.value() <= base::Time::Now());
}
// Returns the minimum of the base::Time instances that actually have a value.
base::Optional<base::Time> Min(base::Optional<base::Time> left,
base::Optional<base::Time> right) {
if (!left.has_value())
return right;
if (!right.has_value())
return left;
return std::min(left.value(), right.value());
}
} // namespace
class AssistantNotificationExpiryMonitor::Observer
: public AssistantNotificationModelObserver {
public:
Observer(AssistantNotificationExpiryMonitor* monitor) : monitor_(monitor) {}
~Observer() override = default;
void OnNotificationAdded(const AssistantNotification* notification) override {
monitor_->UpdateTimer();
}
void OnNotificationUpdated(
const AssistantNotification* notification) override {
monitor_->UpdateTimer();
}
void OnNotificationRemoved(const AssistantNotification* notification,
bool from_server) override {
monitor_->UpdateTimer();
}
void OnAllNotificationsRemoved(bool from_server) override {
monitor_->UpdateTimer();
}
private:
AssistantNotificationExpiryMonitor* const monitor_;
DISALLOW_COPY_AND_ASSIGN(Observer);
};
AssistantNotificationExpiryMonitor::AssistantNotificationExpiryMonitor(
AssistantNotificationController* controller)
: controller_(controller), observer_(std::make_unique<Observer>(this)) {
DCHECK(controller_);
controller_->AddModelObserver(observer_.get());
}
AssistantNotificationExpiryMonitor::~AssistantNotificationExpiryMonitor() =
default;
void AssistantNotificationExpiryMonitor::UpdateTimer() {
base::Optional<base::TimeDelta> timeout = GetTimerTimeout();
if (timeout) {
timer_.Start(
FROM_HERE, timeout.value(),
base::BindOnce(
&AssistantNotificationExpiryMonitor::RemoveExpiredNotifications,
base::Unretained(this)));
} else {
timer_.Stop();
}
}
base::Optional<base::TimeDelta>
AssistantNotificationExpiryMonitor::GetTimerTimeout() const {
base::Optional<base::Time> endtime = GetTimerEndTime();
if (endtime)
return endtime.value() - base::Time::Now();
return base::nullopt;
}
base::Optional<base::Time> AssistantNotificationExpiryMonitor::GetTimerEndTime()
const {
base::Optional<base::Time> result = base::nullopt;
for (const AssistantNotification* notification : GetNotifications())
result = Min(result, notification->expiry_time);
return result;
}
void AssistantNotificationExpiryMonitor::RemoveExpiredNotifications() {
for (const NotificationId& id : GetExpiredNotifications()) {
VLOG(1) << "Removing expired notification '" << id << "'";
controller_->RemoveNotificationById(id, /*from_server=*/false);
}
UpdateTimer();
}
std::vector<AssistantNotificationExpiryMonitor::NotificationId>
AssistantNotificationExpiryMonitor::GetExpiredNotifications() const {
std::vector<NotificationId> result;
for (const AssistantNotification* notification : GetNotifications()) {
if (HasExpired(notification))
result.push_back(notification->client_id);
}
return result;
}
std::vector<const AssistantNotificationExpiryMonitor::AssistantNotification*>
AssistantNotificationExpiryMonitor::GetNotifications() const {
return controller_->model()->GetNotifications();
}
} // namespace ash
// Copyright 2019 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 ASH_ASSISTANT_ASSISTANT_NOTIFICATION_EXPIRY_MONITOR_H_
#define ASH_ASSISTANT_ASSISTANT_NOTIFICATION_EXPIRY_MONITOR_H_
#include <memory>
#include <vector>
#include "ash/assistant/model/assistant_notification_model_observer.h"
#include "base/optional.h"
#include "base/timer/timer.h"
namespace ash {
class AssistantNotificationController;
// Will track all Assistant notifications by subscribing to the given
// |controller| and will call
// |AssistantNotificationController::RemoveNotificationById| when the
// notification expires (i.e. when the current time passes the value in the
// expiry_time| field).
class AssistantNotificationExpiryMonitor {
public:
using AssistantNotification =
chromeos::assistant::mojom::AssistantNotification;
explicit AssistantNotificationExpiryMonitor(
AssistantNotificationController* controller);
~AssistantNotificationExpiryMonitor();
private:
using NotificationId = std::string;
class Observer;
// Start/stop the timer waiting for the next expiry time.
// If the timer is already running this will start a new timer with the
// (new) expiry time that will expire first.
void UpdateTimer();
base::Optional<base::TimeDelta> GetTimerTimeout() const;
base::Optional<base::Time> GetTimerEndTime() const;
void RemoveExpiredNotifications();
std::vector<NotificationId> GetExpiredNotifications() const;
std::vector<const AssistantNotification*> GetNotifications() const;
base::OneShotTimer timer_;
AssistantNotificationController* const controller_;
std::unique_ptr<Observer> observer_;
DISALLOW_COPY_AND_ASSIGN(AssistantNotificationExpiryMonitor);
};
} // namespace ash
#endif // ASH_ASSISTANT_ASSISTANT_NOTIFICATION_EXPIRY_MONITOR_H_
......@@ -78,12 +78,17 @@ AssistantNotificationModel::GetNotificationById(const std::string& id) const {
return it != notifications_.end() ? it->second.get() : nullptr;
}
std::vector<const chromeos::assistant::mojom::AssistantNotification*>
AssistantNotificationModel::GetNotifications() const {
return GetNotificationsByType(base::nullopt);
}
std::vector<const chromeos::assistant::mojom::AssistantNotification*>
AssistantNotificationModel::GetNotificationsByType(
AssistantNotificationType type) const {
base::Optional<AssistantNotificationType> type) const {
std::vector<const AssistantNotification*> notifications;
for (const auto& notification : notifications_) {
if (notification.second->type == type)
if (!type || notification.second->type == type.value())
notifications.push_back(notification.second.get());
}
return notifications;
......
......@@ -58,8 +58,12 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantNotificationModel {
const AssistantNotification* GetNotificationById(const std::string& id) const;
// Returns all notifications matching the specified |type|.
// Use base::nullopt to return all notifications independent of their type.
std::vector<const AssistantNotification*> GetNotificationsByType(
AssistantNotificationType type) const;
base::Optional<AssistantNotificationType> type) const;
// Returns all notifications (that have not been removed).
std::vector<const AssistantNotification*> GetNotifications() const;
// Returns true if the model contains a notification uniquely identified by
// |id|, otherwise false.
......
......@@ -36,6 +36,7 @@
#include "libassistant/shared/internal_api/assistant_manager_delegate.h"
#include "libassistant/shared/internal_api/assistant_manager_internal.h"
#include "libassistant/shared/public/media_manager.h"
#include "mojo/public/mojom/base/time.mojom.h"
#include "services/media_session/public/mojom/constants.mojom.h"
#include "services/media_session/public/mojom/media_session.mojom.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
......@@ -609,6 +610,11 @@ void AssistantManagerServiceImpl::OnShowNotification(
notification_ptr->grouping_key = notification.grouping_key;
notification_ptr->obfuscated_gaia_id = notification.obfuscated_gaia_id;
if (notification.expiry_timestamp_ms) {
notification_ptr->expiry_time =
base::Time::FromJavaTime(notification.expiry_timestamp_ms);
}
// The server sometimes sends an empty |notification_id|, but our client
// requires a non-empty |client_id| for notifications. Known instances in
// which the server sends an empty |notification_id| are for Reminders.
......
......@@ -295,6 +295,10 @@ struct AssistantNotification {
// Whether this notification can turn on the display if it was off.
bool is_high_priority = false;
// When the notification should expire.
// Expressed as milliseconds since Unix Epoch.
mojo_base.mojom.Time? expiry_time;
};
// Models status of an app.
......
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