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

[CrOS PhoneHub] Log metrics via UserActionRecorder

This CL instantiates UserActionRecorderImpl and hooks it up to various
user actions, causing user engagement and retention metrics to be
emitted.

Bug: 1150634, 1106937
Change-Id: I64498e7585d2be4c0536dc33cd5486781376c961
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2576404
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: default avatarRegan Hsu <hsuregan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835919}
parent 175544a0
......@@ -16,6 +16,7 @@
#include "base/strings/string_number_conversions.h"
#include "base/strings/utf_string_conversions.h"
#include "chromeos/components/multidevice/logging/logging.h"
#include "chromeos/components/phonehub/user_action_recorder.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/gfx/paint_vector_icon.h"
#include "ui/views/controls/highlight_path_generator.h"
......@@ -40,12 +41,14 @@ constexpr int kTitleMaxLines = 2;
ContinueBrowsingChip::ContinueBrowsingChip(
const chromeos::phonehub::BrowserTabsModel::BrowserTabMetadata& metadata,
int index,
size_t total_count)
size_t total_count,
chromeos::phonehub::UserActionRecorder* user_action_recorder)
: views::Button(base::BindRepeating(&ContinueBrowsingChip::ButtonPressed,
base::Unretained(this))),
url_(metadata.url),
index_(index),
total_count_(total_count) {
total_count_(total_count),
user_action_recorder_(user_action_recorder) {
auto* color_provider = AshColorProvider::Get();
SetFocusBehavior(FocusBehavior::ALWAYS);
focus_ring()->SetColor(color_provider->GetControlsLayerColor(
......@@ -134,6 +137,7 @@ const char* ContinueBrowsingChip::GetClassName() const {
void ContinueBrowsingChip::ButtonPressed() {
PA_LOG(INFO) << "Opening browser tab: " << url_;
phone_hub_metrics::LogTabContinuationChipClicked(index_);
user_action_recorder_->RecordBrowserTabOpened();
NewWindowDelegate::GetInstance()->NewTabWithUrl(
url_, /*from_user_interaction=*/true);
......
......@@ -10,6 +10,12 @@
#include "ui/gfx/canvas.h"
#include "ui/views/controls/button/button.h"
namespace chromeos {
namespace phonehub {
class UserActionRecorder;
} // namespace phonehub
} // namespace chromeos
namespace ash {
// A chip containing a web page info (title, web URL, etc.) that users left off
......@@ -19,7 +25,8 @@ class ASH_EXPORT ContinueBrowsingChip : public views::Button {
ContinueBrowsingChip(
const chromeos::phonehub::BrowserTabsModel::BrowserTabMetadata& metadata,
int index,
size_t total_count);
size_t total_count,
chromeos::phonehub::UserActionRecorder* user_action_recorder);
~ContinueBrowsingChip() override;
ContinueBrowsingChip(ContinueBrowsingChip&) = delete;
......@@ -40,6 +47,8 @@ class ASH_EXPORT ContinueBrowsingChip : public views::Button {
// The total number of chips in the parent view.
size_t total_count_;
chromeos::phonehub::UserActionRecorder* user_action_recorder_ = nullptr;
};
} // namespace ash
......
......@@ -48,8 +48,8 @@ PhoneConnectedView::PhoneConnectedView(
auto* phone_model = phone_hub_manager->GetPhoneModel();
if (phone_model) {
setup_layered_view(
AddChildView(std::make_unique<TaskContinuationView>(phone_model)));
setup_layered_view(AddChildView(std::make_unique<TaskContinuationView>(
phone_model, phone_hub_manager->GetUserActionRecorder())));
}
}
......
......@@ -148,8 +148,7 @@ void PhoneHubTray::ShowBubble(bool show_by_click) {
if (bubble_)
return;
// Attempts a connection to the phone if needed.
ui_controller_->MaybeRequestConnection();
ui_controller_->HandleBubbleOpened();
TrayBubbleView::InitParams init_params;
init_params.delegate = this;
......
......@@ -15,6 +15,7 @@
#include "base/logging.h"
#include "chromeos/components/phonehub/connection_scheduler.h"
#include "chromeos/components/phonehub/phone_hub_manager.h"
#include "chromeos/components/phonehub/user_action_recorder.h"
using FeatureStatus = chromeos::phonehub::FeatureStatus;
......@@ -76,15 +77,16 @@ std::unique_ptr<PhoneHubContentView> PhoneHubUiController::CreateContentView(
}
}
void PhoneHubUiController::MaybeRequestConnection() {
void PhoneHubUiController::HandleBubbleOpened() {
if (!phone_hub_manager_)
return;
auto feature_status =
phone_hub_manager_->GetFeatureStatusProvider()->GetStatus();
if (feature_status == FeatureStatus::kEnabledButDisconnected)
phone_hub_manager_->GetConnectionScheduler()->ScheduleConnectionNow();
phone_hub_manager_->GetUserActionRecorder()->RecordUiOpened();
}
void PhoneHubUiController::AddObserver(Observer* observer) {
......
......@@ -71,9 +71,9 @@ class ASH_EXPORT PhoneHubUiController
std::unique_ptr<views::View> CreateStatusHeaderView(
PhoneStatusView::Delegate* delegate);
// Request a connection to the phone if there is no current connection and
// PhoneHub is set up.
void MaybeRequestConnection();
// Handler for when the bubble is opened. Requests a connection to the phone
// if there is no current connection, and records metrics.
void HandleBubbleOpened();
// Observer functions.
void AddObserver(Observer* observer);
......
......@@ -71,8 +71,9 @@ class HeaderView : public views::Label {
} // namespace
TaskContinuationView::TaskContinuationView(
chromeos::phonehub::PhoneModel* phone_model)
: phone_model_(phone_model) {
chromeos::phonehub::PhoneModel* phone_model,
chromeos::phonehub::UserActionRecorder* user_action_recorder)
: phone_model_(phone_model), user_action_recorder_(user_action_recorder) {
SetID(PhoneHubViewID::kTaskContinuationView);
phone_model_->AddObserver(this);
......@@ -183,7 +184,8 @@ void TaskContinuationView::Update() {
for (const BrowserTabsModel::BrowserTabMetadata& metadata :
browser_tabs.most_recent_tabs()) {
chips_view_->AddTaskChip(new ContinueBrowsingChip(
metadata, index, browser_tabs.most_recent_tabs().size()));
metadata, index, browser_tabs.most_recent_tabs().size(),
user_action_recorder_));
index++;
}
......
......@@ -10,6 +10,12 @@
#include "ui/views/view.h"
#include "ui/views/view_model.h"
namespace chromeos {
namespace phonehub {
class UserActionRecorder;
} // namespace phonehub
} // namespace chromeos
namespace ash {
// A view in Phone Hub bubble that allows user to pick up unfinished task left
......@@ -18,7 +24,9 @@ class ASH_EXPORT TaskContinuationView
: public views::View,
public chromeos::phonehub::PhoneModel::Observer {
public:
explicit TaskContinuationView(chromeos::phonehub::PhoneModel* phone_model);
TaskContinuationView(
chromeos::phonehub::PhoneModel* phone_model,
chromeos::phonehub::UserActionRecorder* user_action_recorder);
~TaskContinuationView() override;
TaskContinuationView(TaskContinuationView&) = delete;
TaskContinuationView operator=(TaskContinuationView&) = delete;
......@@ -60,6 +68,7 @@ class ASH_EXPORT TaskContinuationView
void Update();
chromeos::phonehub::PhoneModel* phone_model_ = nullptr;
chromeos::phonehub::UserActionRecorder* user_action_recorder_ = nullptr;
TaskChipsView* chips_view_ = nullptr;
};
......
......@@ -8,6 +8,7 @@
#include "ash/system/phonehub/continue_browsing_chip.h"
#include "ash/test/ash_test_base.h"
#include "base/test/scoped_feature_list.h"
#include "chromeos/components/phonehub/fake_user_action_recorder.h"
#include "chromeos/components/phonehub/mutable_phone_model.h"
#include "chromeos/components/phonehub/phone_model_test_util.h"
#include "chromeos/constants/chromeos_features.h"
......@@ -46,8 +47,8 @@ class TaskContinuationViewTest : public AshTestBase {
feature_list_.InitAndEnableFeature(chromeos::features::kPhoneHub);
AshTestBase::SetUp();
task_continuation_view_ =
std::make_unique<TaskContinuationView>(&phone_model_);
task_continuation_view_ = std::make_unique<TaskContinuationView>(
&phone_model_, &fake_user_action_recorder_);
}
void TearDown() override {
......@@ -62,6 +63,7 @@ class TaskContinuationViewTest : public AshTestBase {
private:
std::unique_ptr<TaskContinuationView> task_continuation_view_;
chromeos::phonehub::FakeUserActionRecorder fake_user_action_recorder_;
chromeos::phonehub::MutablePhoneModel phone_model_;
base::test::ScopedFeatureList feature_list_;
MockNewWindowDelegate new_window_delegate_;
......
......@@ -20,6 +20,7 @@
#include "chromeos/components/phonehub/notification_access_manager_impl.h"
#include "chromeos/components/phonehub/onboarding_ui_tracker_impl.h"
#include "chromeos/components/phonehub/phone_hub_manager_impl.h"
#include "chromeos/components/phonehub/user_action_recorder_impl.h"
#include "chromeos/constants/chromeos_features.h"
#include "chromeos/services/multidevice_setup/public/cpp/prefs.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
......
......@@ -6,13 +6,16 @@
#include "chromeos/components/multidevice/logging/logging.h"
#include "chromeos/components/phonehub/message_sender.h"
#include "chromeos/components/phonehub/user_action_recorder.h"
namespace chromeos {
namespace phonehub {
DoNotDisturbControllerImpl::DoNotDisturbControllerImpl(
MessageSender* message_sender)
: message_sender_(message_sender) {
MessageSender* message_sender,
UserActionRecorder* user_action_recorder)
: message_sender_(message_sender),
user_action_recorder_(user_action_recorder) {
DCHECK(message_sender_);
}
......@@ -51,6 +54,7 @@ void DoNotDisturbControllerImpl::RequestNewDoNotDisturbState(bool enabled) {
return;
PA_LOG(INFO) << "Attempting to set DND state; new value: " << enabled;
user_action_recorder_->RecordDndAttempt();
message_sender_->SendUpdateNotificationModeRequest(enabled);
}
......
......@@ -11,12 +11,14 @@ namespace chromeos {
namespace phonehub {
class MessageSender;
class UserActionRecorder;
// Responsible for sending and receiving states in regards to the DoNotDisturb
// feature of the user's remote phone.
class DoNotDisturbControllerImpl : public DoNotDisturbController {
public:
explicit DoNotDisturbControllerImpl(MessageSender* message_sender);
DoNotDisturbControllerImpl(MessageSender* message_sender,
UserActionRecorder* user_action_recorder);
~DoNotDisturbControllerImpl() override;
private:
......@@ -29,9 +31,11 @@ class DoNotDisturbControllerImpl : public DoNotDisturbController {
void RequestNewDoNotDisturbState(bool enabled) override;
bool CanRequestNewDndState() const override;
MessageSender* message_sender_;
UserActionRecorder* user_action_recorder_;
bool is_dnd_enabled_ = false;
bool can_request_new_dnd_state_ = false;
MessageSender* message_sender_;
};
} // namespace phonehub
......
......@@ -7,6 +7,7 @@
#include <memory>
#include "chromeos/components/phonehub/fake_message_sender.h"
#include "chromeos/components/phonehub/fake_user_action_recorder.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace chromeos {
......@@ -40,9 +41,8 @@ class DoNotDisturbControllerImplTest : public testing::Test {
// testing::Test:
void SetUp() override {
fake_message_sender_ = std::make_unique<FakeMessageSender>();
controller_ = std::make_unique<DoNotDisturbControllerImpl>(
fake_message_sender_.get());
&fake_message_sender_, &fake_user_action_recorder_);
controller_->AddObserver(&fake_observer_);
}
......@@ -65,18 +65,22 @@ class DoNotDisturbControllerImplTest : public testing::Test {
}
bool GetRecentUpdateNotificationModeRequest() {
return fake_message_sender_->GetRecentUpdateNotificationModeRequest();
return fake_message_sender_.GetRecentUpdateNotificationModeRequest();
}
size_t GetUpdateNotificationModeRequestCallCount() {
return fake_message_sender_->GetUpdateNotificationModeRequestCallCount();
return fake_message_sender_.GetUpdateNotificationModeRequestCallCount();
}
size_t GetNumObserverCalls() const { return fake_observer_.num_calls(); }
FakeUserActionRecorder fake_user_action_recorder_;
private:
FakeObserver fake_observer_;
std::unique_ptr<FakeMessageSender> fake_message_sender_;
FakeMessageSender fake_message_sender_;
std::unique_ptr<DoNotDisturbControllerImpl> controller_;
};
......@@ -127,6 +131,7 @@ TEST_F(DoNotDisturbControllerImplTest, SetInternalStatesWithObservers) {
TEST_F(DoNotDisturbControllerImplTest, RequestNewDoNotDisturbState) {
RequestNewDoNotDisturbState(/*enabled=*/true);
EXPECT_EQ(1u, fake_user_action_recorder_.num_dnd_attempts());
EXPECT_TRUE(GetRecentUpdateNotificationModeRequest());
EXPECT_EQ(1u, GetUpdateNotificationModeRequestCallCount());
// Simulate receiving a response and setting the internal value.
......@@ -134,6 +139,7 @@ TEST_F(DoNotDisturbControllerImplTest, RequestNewDoNotDisturbState) {
/*can_request_new_dnd_state=*/true);
RequestNewDoNotDisturbState(/*enabled=*/false);
EXPECT_EQ(2u, fake_user_action_recorder_.num_dnd_attempts());
EXPECT_FALSE(GetRecentUpdateNotificationModeRequest());
EXPECT_EQ(2u, GetUpdateNotificationModeRequestCallCount());
// Simulate receiving a response and setting the internal value.
......@@ -142,6 +148,7 @@ TEST_F(DoNotDisturbControllerImplTest, RequestNewDoNotDisturbState) {
// Requesting for a the same state as the currently set state is a no-op.
RequestNewDoNotDisturbState(/*enabled=*/false);
EXPECT_EQ(2u, fake_user_action_recorder_.num_dnd_attempts());
EXPECT_FALSE(GetRecentUpdateNotificationModeRequest());
EXPECT_EQ(2u, GetUpdateNotificationModeRequestCallCount());
}
......
......@@ -47,5 +47,9 @@ ConnectionScheduler* FakePhoneHubManager::GetConnectionScheduler() {
return &fake_connection_scheduler_;
}
UserActionRecorder* FakePhoneHubManager::GetUserActionRecorder() {
return &fake_user_action_recorder_;
}
} // namespace phonehub
} // namespace chromeos
......@@ -15,6 +15,7 @@
#include "chromeos/components/phonehub/fake_notification_manager.h"
#include "chromeos/components/phonehub/fake_onboarding_ui_tracker.h"
#include "chromeos/components/phonehub/fake_tether_controller.h"
#include "chromeos/components/phonehub/fake_user_action_recorder.h"
#include "chromeos/components/phonehub/mutable_phone_model.h"
#include "chromeos/components/phonehub/phone_hub_manager.h"
......@@ -61,6 +62,10 @@ class FakePhoneHubManager : public PhoneHubManager {
return &fake_connection_scheduler_;
}
FakeUserActionRecorder* fake_user_action_recorder() {
return &fake_user_action_recorder_;
}
private:
// PhoneHubManager:
DoNotDisturbController* GetDoNotDisturbController() override;
......@@ -72,6 +77,7 @@ class FakePhoneHubManager : public PhoneHubManager {
PhoneModel* GetPhoneModel() override;
TetherController* GetTetherController() override;
ConnectionScheduler* GetConnectionScheduler() override;
UserActionRecorder* GetUserActionRecorder() override;
FakeDoNotDisturbController fake_do_not_disturb_controller_;
FakeFeatureStatusProvider fake_feature_status_provider_;
......@@ -82,6 +88,7 @@ class FakePhoneHubManager : public PhoneHubManager {
MutablePhoneModel mutable_phone_model_;
FakeTetherController fake_tether_controller_;
FakeConnectionScheduler fake_connection_scheduler_;
FakeUserActionRecorder fake_user_action_recorder_;
};
} // namespace phonehub
......
......@@ -6,15 +6,18 @@
#include "chromeos/components/multidevice/logging/logging.h"
#include "chromeos/components/phonehub/message_sender.h"
#include "chromeos/components/phonehub/user_action_recorder.h"
namespace chromeos {
namespace phonehub {
FindMyDeviceControllerImpl::FindMyDeviceControllerImpl(
DoNotDisturbController* do_not_disturb_controller,
MessageSender* message_sender)
MessageSender* message_sender,
UserActionRecorder* user_action_recorder)
: do_not_disturb_controller_(do_not_disturb_controller),
message_sender_(message_sender) {
message_sender_(message_sender),
user_action_recorder_(user_action_recorder) {
DCHECK(do_not_disturb_controller_);
DCHECK(message_sender_);
......@@ -45,6 +48,7 @@ void FindMyDeviceControllerImpl::RequestNewPhoneRingingState(bool ringing) {
PA_LOG(INFO) << "Attempting to set Find My Device phone ring state; new "
<< "value: " << ringing;
user_action_recorder_->RecordFindMyDeviceAttempt();
message_sender_->SendRingDeviceRequest(ringing);
}
......
......@@ -12,6 +12,7 @@ namespace chromeos {
namespace phonehub {
class MessageSender;
class UserActionRecorder;
// Responsible for sending and receiving updates in regards to the Find My
// Device feature which involves ringing the user's remote phone.
......@@ -19,7 +20,8 @@ class FindMyDeviceControllerImpl : public FindMyDeviceController,
public DoNotDisturbController::Observer {
public:
FindMyDeviceControllerImpl(DoNotDisturbController* do_not_disturb_controller,
MessageSender* message_sender);
MessageSender* message_sender,
UserActionRecorder* user_action_recorder);
~FindMyDeviceControllerImpl() override;
private:
......@@ -41,6 +43,7 @@ class FindMyDeviceControllerImpl : public FindMyDeviceController,
DoNotDisturbController* do_not_disturb_controller_;
MessageSender* message_sender_;
UserActionRecorder* user_action_recorder_;
};
} // namespace phonehub
......
......@@ -8,6 +8,7 @@
#include "chromeos/components/phonehub/fake_do_not_disturb_controller.h"
#include "chromeos/components/phonehub/fake_message_sender.h"
#include "chromeos/components/phonehub/fake_user_action_recorder.h"
#include "chromeos/components/phonehub/find_my_device_controller.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -42,11 +43,9 @@ class FindMyDeviceControllerImplTest : public testing::Test {
// testing::Test:
void SetUp() override {
fake_do_not_disturb_controller_ =
std::make_unique<FakeDoNotDisturbController>();
fake_message_sender_ = std::make_unique<FakeMessageSender>();
controller_ = std::make_unique<FindMyDeviceControllerImpl>(
fake_do_not_disturb_controller_.get(), fake_message_sender_.get());
&fake_do_not_disturb_controller_, &fake_message_sender_,
&fake_user_action_recorder_);
controller_->AddObserver(&fake_observer_);
}
......@@ -67,8 +66,9 @@ class FindMyDeviceControllerImplTest : public testing::Test {
size_t GetNumObserverCalls() const { return fake_observer_.num_calls(); }
protected:
std::unique_ptr<FakeDoNotDisturbController> fake_do_not_disturb_controller_;
std::unique_ptr<FakeMessageSender> fake_message_sender_;
FakeDoNotDisturbController fake_do_not_disturb_controller_;
FakeMessageSender fake_message_sender_;
FakeUserActionRecorder fake_user_action_recorder_;
private:
std::unique_ptr<FindMyDeviceControllerImpl> controller_;
......@@ -81,7 +81,7 @@ TEST_F(FindMyDeviceControllerImplTest, RingingStateChanges) {
// Simulate flipping DoNotDisturb mode to enabled, this should set the
// FindMyPhone status to kRingingNotAvailable.
fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
fake_do_not_disturb_controller_.SetDoNotDisturbStateInternal(
/*is_dnd_enabled=*/true, /*can_request_new_dnd_state=*/true);
EXPECT_EQ(FindMyDeviceController::Status::kRingingNotAvailable,
GetPhoneRingingStatus());
......@@ -94,7 +94,7 @@ TEST_F(FindMyDeviceControllerImplTest, RingingStateChanges) {
// Flip DoNotDisturb back to disabled, expect status to reset back to its
// previous state.
fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
fake_do_not_disturb_controller_.SetDoNotDisturbStateInternal(
/*is_dnd_enabled=*/false, /*can_request_new_dnd_state=*/true);
// Since we previously recorded that the phone should be ringing during
// DoNotDisturb mode was enabled, we return to that state once DoNotDisturb
......@@ -111,32 +111,35 @@ TEST_F(FindMyDeviceControllerImplTest, RingingStateChanges) {
TEST_F(FindMyDeviceControllerImplTest, RequestNewRingStatus) {
RequestNewPhoneRingingState(/*ringing=*/true);
EXPECT_EQ(1u, fake_message_sender_->GetRingDeviceRequestCallCount());
EXPECT_TRUE(fake_message_sender_->GetRecentRingDeviceRequest());
EXPECT_EQ(1u, fake_user_action_recorder_.num_find_my_device_attempts());
EXPECT_EQ(1u, fake_message_sender_.GetRingDeviceRequestCallCount());
EXPECT_TRUE(fake_message_sender_.GetRecentRingDeviceRequest());
// Simulate flipping DoNotDisturb mode to enabled, this should set the
// FindMyPhone status to kRingingNotAvailable and not send any new messages.
fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
fake_do_not_disturb_controller_.SetDoNotDisturbStateInternal(
/*is_dnd_enabled=*/true, /*can_request_new_dnd_state=*/true);
EXPECT_EQ(FindMyDeviceController::Status::kRingingNotAvailable,
GetPhoneRingingStatus());
RequestNewPhoneRingingState(/*ringing=*/false);
EXPECT_EQ(1u, fake_message_sender_->GetRingDeviceRequestCallCount());
EXPECT_EQ(1u, fake_user_action_recorder_.num_find_my_device_attempts());
EXPECT_EQ(1u, fake_message_sender_.GetRingDeviceRequestCallCount());
// No new messages were sent, expect that the last request was still the
// previous "true" value.
EXPECT_TRUE(fake_message_sender_->GetRecentRingDeviceRequest());
EXPECT_TRUE(fake_message_sender_.GetRecentRingDeviceRequest());
// Flip DoNotDisturb mode to disabled, expect that messages are able to be
// sent again.
fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
fake_do_not_disturb_controller_.SetDoNotDisturbStateInternal(
/*is_dnd_enabled=*/false, /*can_request_new_dnd_state=*/true);
EXPECT_EQ(FindMyDeviceController::Status::kRingingOff,
GetPhoneRingingStatus());
RequestNewPhoneRingingState(/*ringing=*/false);
EXPECT_EQ(2u, fake_message_sender_->GetRingDeviceRequestCallCount());
EXPECT_FALSE(fake_message_sender_->GetRecentRingDeviceRequest());
EXPECT_EQ(2u, fake_user_action_recorder_.num_find_my_device_attempts());
EXPECT_EQ(2u, fake_message_sender_.GetRingDeviceRequestCallCount());
EXPECT_FALSE(fake_message_sender_.GetRecentRingDeviceRequest());
}
} // namespace phonehub
......
......@@ -8,6 +8,7 @@
#include "chromeos/components/multidevice/logging/logging.h"
#include "chromeos/components/phonehub/message_sender.h"
#include "chromeos/components/phonehub/notification.h"
#include "chromeos/components/phonehub/user_action_recorder.h"
namespace chromeos {
namespace phonehub {
......@@ -17,8 +18,10 @@ using multidevice_setup::mojom::FeatureState;
NotificationManagerImpl::NotificationManagerImpl(
MessageSender* message_sender,
UserActionRecorder* user_action_recorder,
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client)
: message_sender_(message_sender),
user_action_recorder_(user_action_recorder),
multidevice_setup_client_(multidevice_setup_client) {
DCHECK(message_sender_);
DCHECK(multidevice_setup_client_);
......@@ -39,6 +42,7 @@ void NotificationManagerImpl::DismissNotification(int64_t notification_id) {
return;
}
user_action_recorder_->RecordNotificationDismissAttempt();
RemoveNotificationsInternal(base::flat_set<int64_t>{notification_id});
message_sender_->SendDismissNotificationRequest(notification_id);
}
......@@ -54,6 +58,7 @@ void NotificationManagerImpl::SendInlineReply(
PA_LOG(INFO) << "Sending inline reply for notification with ID "
<< notification_id << ".";
user_action_recorder_->RecordNotificationReplyAttempt();
message_sender_->SendNotificationInlineReplyRequest(notification_id,
inline_reply_text);
}
......
......@@ -16,6 +16,7 @@ namespace chromeos {
namespace phonehub {
class MessageSender;
class UserActionRecorder;
class NotificationManagerImpl
: public NotificationManager,
......@@ -23,6 +24,7 @@ class NotificationManagerImpl
public:
NotificationManagerImpl(
MessageSender* message_sender,
UserActionRecorder* user_action_recorder,
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client);
~NotificationManagerImpl() override;
......@@ -38,6 +40,7 @@ class NotificationManagerImpl
feature_states_map) override;
MessageSender* message_sender_;
UserActionRecorder* user_action_recorder_;
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
multidevice_setup::mojom::FeatureState notifications_feature_status_;
};
......
......@@ -10,6 +10,7 @@
#include "base/optional.h"
#include "base/strings/utf_string_conversions.h"
#include "chromeos/components/phonehub/fake_message_sender.h"
#include "chromeos/components/phonehub/fake_user_action_recorder.h"
#include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -86,18 +87,16 @@ class NotificationManagerImplTest : public testing::Test {
// testing::Test:
void SetUp() override {
fake_message_sender_ = std::make_unique<FakeMessageSender>();
fake_multidevice_setup_client_ =
std::make_unique<multidevice_setup::FakeMultiDeviceSetupClient>();
manager_ = std::make_unique<NotificationManagerImpl>(
fake_message_sender_.get(), fake_multidevice_setup_client_.get());
&fake_message_sender_, &fake_user_action_recorder_,
&fake_multidevice_setup_client_);
manager_->AddObserver(&fake_observer_);
}
void TearDown() override { manager_->RemoveObserver(&fake_observer_); }
NotificationManager& manager() { return *manager_; }
FakeMessageSender& fake_message_sender() { return *fake_message_sender_; }
FakeMessageSender& fake_message_sender() { return fake_message_sender_; }
void SetNotificationsInternal(
const base::flat_set<Notification>& notifications) {
......@@ -116,15 +115,18 @@ class NotificationManagerImplTest : public testing::Test {
}
void SetNotificationFeatureStatus(FeatureState feature_state) {
fake_multidevice_setup_client_->SetFeatureState(
fake_multidevice_setup_client_.SetFeatureState(
Feature::kPhoneHubNotifications, feature_state);
}
FakeUserActionRecorder fake_user_action_recorder_;
private:
FakeObserver fake_observer_;
std::unique_ptr<FakeMessageSender> fake_message_sender_;
std::unique_ptr<multidevice_setup::FakeMultiDeviceSetupClient>
fake_multidevice_setup_client_;
FakeMessageSender fake_message_sender_;
multidevice_setup::FakeMultiDeviceSetupClient fake_multidevice_setup_client_;
std::unique_ptr<NotificationManager> manager_;
};
......@@ -183,6 +185,7 @@ TEST_F(NotificationManagerImplTest, DismissNotifications) {
EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id2));
manager().DismissNotification(expected_id2);
EXPECT_EQ(1u, fake_user_action_recorder_.num_notification_dismissals());
EXPECT_EQ(1u, GetNumNotifications());
EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
EXPECT_EQ(NotificationState::kRemoved, GetNotificationState(expected_id2));
......@@ -192,6 +195,7 @@ TEST_F(NotificationManagerImplTest, DismissNotifications) {
// Dismiss the same notification again, verify nothing happens.
manager().DismissNotification(expected_id2);
EXPECT_EQ(1u, fake_user_action_recorder_.num_notification_dismissals());
EXPECT_EQ(1u, GetNumNotifications());
EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
EXPECT_EQ(NotificationState::kRemoved, GetNotificationState(expected_id2));
......@@ -231,6 +235,7 @@ TEST_F(NotificationManagerImplTest, SendInlineReply) {
// 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, fake_user_action_recorder_.num_notification_replies());
EXPECT_EQ(1u, GetNumNotifications());
EXPECT_EQ(NotificationState::kAdded, GetNotificationState(expected_id1));
EXPECT_EQ(1u,
......@@ -244,6 +249,7 @@ TEST_F(NotificationManagerImplTest, SendInlineReply) {
// 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_user_action_recorder_.num_notification_replies());
EXPECT_EQ(1u,
fake_message_sender().GetNotificationInlineReplyRequestCallCount());
pair = fake_message_sender().GetRecentNotificationInlineReplyRequest();
......
......@@ -17,6 +17,7 @@ class NotificationManager;
class OnboardingUiTracker;
class PhoneModel;
class TetherController;
class UserActionRecorder;
// Responsible for the core logic of the Phone Hub feature and exposes
// interfaces via its public API. This class is intended to be a singleton.
......@@ -37,6 +38,7 @@ class PhoneHubManager {
virtual OnboardingUiTracker* GetOnboardingUiTracker() = 0;
virtual PhoneModel* GetPhoneModel() = 0;
virtual TetherController* GetTetherController() = 0;
virtual UserActionRecorder* GetUserActionRecorder() = 0;
protected:
PhoneHubManager() = default;
......
......@@ -23,6 +23,7 @@
#include "chromeos/components/phonehub/phone_model.h"
#include "chromeos/components/phonehub/phone_status_processor.h"
#include "chromeos/components/phonehub/tether_controller_impl.h"
#include "chromeos/components/phonehub/user_action_recorder_impl.h"
#include "chromeos/dbus/power/power_manager_client.h"
#include "components/session_manager/core/session_manager.h"
......@@ -36,7 +37,8 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
chromeos::secure_channel::SecureChannelClient* secure_channel_client,
std::unique_ptr<BrowserTabsModelProvider> browser_tabs_model_provider,
const base::RepeatingClosure& show_multidevice_setup_dialog_callback)
: connection_manager_(
: user_action_recorder_(std::make_unique<UserActionRecorderImpl>()),
connection_manager_(
std::make_unique<ConnectionManagerImpl>(multidevice_setup_client,
device_sync_client,
secure_channel_client)),
......@@ -54,14 +56,16 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
std::make_unique<CrosStateSender>(message_sender_.get(),
connection_manager_.get(),
multidevice_setup_client)),
do_not_disturb_controller_(
std::make_unique<DoNotDisturbControllerImpl>(message_sender_.get())),
do_not_disturb_controller_(std::make_unique<DoNotDisturbControllerImpl>(
message_sender_.get(),
user_action_recorder_.get())),
connection_scheduler_(std::make_unique<ConnectionSchedulerImpl>(
connection_manager_.get(),
feature_status_provider_.get())),
find_my_device_controller_(std::make_unique<FindMyDeviceControllerImpl>(
do_not_disturb_controller_.get(),
message_sender_.get())),
message_sender_.get(),
user_action_recorder_.get())),
notification_access_manager_(
std::make_unique<NotificationAccessManagerImpl>(
pref_service,
......@@ -70,6 +74,7 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
connection_scheduler_.get())),
notification_manager_(
std::make_unique<NotificationManagerImpl>(message_sender_.get(),
user_action_recorder_.get(),
multidevice_setup_client)),
onboarding_ui_tracker_(std::make_unique<OnboardingUiTrackerImpl>(
pref_service,
......@@ -88,6 +93,7 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
phone_model_.get())),
tether_controller_(
std::make_unique<TetherControllerImpl>(phone_model_.get(),
user_action_recorder_.get(),
multidevice_setup_client)),
browser_tabs_model_provider_(std::move(browser_tabs_model_provider)),
browser_tabs_model_controller_(
......@@ -139,6 +145,10 @@ TetherController* PhoneHubManagerImpl::GetTetherController() {
return tether_controller_.get();
}
UserActionRecorder* PhoneHubManagerImpl::GetUserActionRecorder() {
return user_action_recorder_.get();
}
// These should be destroyed in the opposite order of how these objects are
// initialized in the constructor.
void PhoneHubManagerImpl::Shutdown() {
......@@ -159,6 +169,7 @@ void PhoneHubManagerImpl::Shutdown() {
message_receiver_.reset();
feature_status_provider_.reset();
connection_manager_.reset();
user_action_recorder_.reset();
}
} // namespace phonehub
......
......@@ -38,6 +38,7 @@ class MessageReceiver;
class MultideviceSetupStateUpdater;
class MutablePhoneModel;
class PhoneStatusProcessor;
class UserActionRecorder;
// Implemented as a KeyedService which is keyed by the primary Profile.
class PhoneHubManagerImpl : public PhoneHubManager, public KeyedService {
......@@ -62,11 +63,13 @@ class PhoneHubManagerImpl : public PhoneHubManager, public KeyedService {
OnboardingUiTracker* GetOnboardingUiTracker() override;
PhoneModel* GetPhoneModel() override;
TetherController* GetTetherController() override;
UserActionRecorder* GetUserActionRecorder() override;
private:
// KeyedService:
void Shutdown() override;
std::unique_ptr<UserActionRecorder> user_action_recorder_;
std::unique_ptr<ConnectionManager> connection_manager_;
std::unique_ptr<FeatureStatusProvider> feature_status_provider_;
std::unique_ptr<MessageReceiver> message_receiver_;
......
......@@ -6,6 +6,7 @@
#include "chromeos/components/multidevice/logging/logging.h"
#include "chromeos/components/phonehub/phone_status_model.h"
#include "chromeos/components/phonehub/user_action_recorder.h"
#include "chromeos/components/phonehub/util/histogram_util.h"
#include "chromeos/services/network_config/in_process_instance.h"
......@@ -54,17 +55,21 @@ void TetherControllerImpl::TetherNetworkConnector::GetNetworkStateList(
TetherControllerImpl::TetherControllerImpl(
PhoneModel* phone_model,
UserActionRecorder* user_action_recorder,
MultiDeviceSetupClient* multidevice_setup_client)
: TetherControllerImpl(
phone_model,
user_action_recorder,
multidevice_setup_client,
std::make_unique<TetherControllerImpl::TetherNetworkConnector>()) {}
TetherControllerImpl::TetherControllerImpl(
PhoneModel* phone_model,
UserActionRecorder* user_action_recorder,
MultiDeviceSetupClient* multidevice_setup_client,
std::unique_ptr<TetherControllerImpl::TetherNetworkConnector> connector)
: phone_model_(phone_model),
user_action_recorder_(user_action_recorder),
multidevice_setup_client_(multidevice_setup_client),
connector_(std::move(connector)) {
// Receive updates when devices (e.g., Tether, Ethernet, Wi-Fi) go on/offline
......@@ -114,6 +119,7 @@ void TetherControllerImpl::AttemptConnection() {
}
PA_LOG(INFO) << "Attempting connection; current status is " << status_;
user_action_recorder_->RecordTetherConnectionAttempt();
util::LogTetherConnectionResult(
util::TetherConnectionResult::kAttemptConnection);
......
......@@ -18,6 +18,8 @@ namespace {
using multidevice_setup::MultiDeviceSetupClient;
} // namespace
class UserActionRecorder;
// TetherController implementation which utilizes MultiDeviceSetupClient and
// CrosNetworkConfig in order to interact with Instant Tethering. If Instant
// Tethering is user disabled, AttemptConnection() will first enable the feature
......@@ -36,8 +38,9 @@ class TetherControllerImpl
public multidevice_setup::MultiDeviceSetupClient::Observer,
public chromeos::network_config::mojom::CrosNetworkConfigObserver {
public:
explicit TetherControllerImpl(
TetherControllerImpl(
PhoneModel* phone_model,
UserActionRecorder* user_action_recorder,
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client);
~TetherControllerImpl() override;
......@@ -114,6 +117,7 @@ class TetherControllerImpl
// parameter constructor calls this constructor.
TetherControllerImpl(
PhoneModel* phone_model,
UserActionRecorder* user_action_recorder,
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
std::unique_ptr<TetherControllerImpl::TetherNetworkConnector> connector);
......@@ -154,6 +158,7 @@ class TetherControllerImpl
TetherController::Status ComputeStatus() const;
PhoneModel* phone_model_;
UserActionRecorder* user_action_recorder_;
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
ConnectDisconnectStatus connect_disconnect_status_ =
ConnectDisconnectStatus::kIdle;
......
......@@ -10,6 +10,7 @@
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/task_environment.h"
#include "chromeos/components/phonehub/fake_user_action_recorder.h"
#include "chromeos/components/phonehub/mutable_phone_model.h"
#include "chromeos/components/phonehub/phone_status_model.h"
#include "chromeos/network/network_state_handler.h"
......@@ -173,7 +174,8 @@ class TetherControllerImplTest : public testing::Test {
controller_ =
base::WrapUnique<TetherControllerImpl>(new TetherControllerImpl(
&fake_phone_model_, &fake_multidevice_setup_client_,
&fake_phone_model_, &fake_user_action_recorder_,
&fake_multidevice_setup_client_,
std::make_unique<FakeTetherNetworkConnector>()));
controller_->AddObserver(&fake_observer_);
}
......@@ -266,7 +268,13 @@ class TetherControllerImplTest : public testing::Test {
/*expected_enabled=*/expected_enabled, base::nullopt, success);
}
void AttemptConnection() { controller_->AttemptConnection(); }
void AttemptConnection() {
size_t num_recorded_connection_attempts_before_call =
fake_user_action_recorder_.num_tether_attempts();
controller_->AttemptConnection();
EXPECT_EQ(num_recorded_connection_attempts_before_call + 1,
fake_user_action_recorder_.num_tether_attempts());
}
void Disconnect() { controller_->Disconnect(); }
......@@ -286,6 +294,7 @@ class TetherControllerImplTest : public testing::Test {
std::string service_path_;
multidevice_setup::FakeMultiDeviceSetupClient fake_multidevice_setup_client_;
MutablePhoneModel fake_phone_model_;
FakeUserActionRecorder fake_user_action_recorder_;
FakeObserver fake_observer_;
std::unique_ptr<TetherControllerImpl> controller_;
};
......
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