Commit cc36f66a authored by Xing Liu's avatar Xing Liu Committed by Commit Bot

Notification scheduler: Improve user action plumbing.

This CL wraps all user action data into a struct, which loweres the
difficulty for maintenance and simplifies the code.

Also plumb the button id through the user
action pipeline.

Bug: 979770
Change-Id: I91b9e8c9d35ff413051d9cc937ef6c2283112464
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1717312Reviewed-by: default avatarHesen Zhang <hesen@chromium.org>
Reviewed-by: default avatarDavid Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682551}
parent 7665bc13
......@@ -48,6 +48,8 @@ public class DisplayAgent {
"org.chromium.chrome.browser.notifications.scheduler.EXTRA_GUID";
private static final String EXTRA_ACTION_BUTTON_TYPE =
"org.chromium.chrome.browser.notifications.scheduler.EXTRA_ACTION_BUTTON_TYPE";
private static final String EXTRA_ACTION_BUTTON_ID =
"org.chromium.chrome.browser.notifications.scheduler.EXTRA_ACTION_BUTTON_ID";
private static final String EXTRA_SCHEDULER_CLIENT_TYPE =
"org.chromium.chrome.browser.notifications.scheduler.EXTRA_SCHEDULER_CLIENT_TYPE ";
......@@ -150,16 +152,19 @@ public class DisplayAgent {
case NotificationIntentInterceptor.IntentType.UNKNOWN:
break;
case NotificationIntentInterceptor.IntentType.CONTENT_INTENT:
nativeOnContentClick(Profile.getLastUsedProfile(), clientType, guid);
nativeOnUserAction(Profile.getLastUsedProfile(), clientType, UserActionType.CLICK,
guid, ActionButtonType.UNKNOWN_ACTION, null);
break;
case NotificationIntentInterceptor.IntentType.DELETE_INTENT:
nativeOnDismiss(Profile.getLastUsedProfile(), clientType, guid);
nativeOnUserAction(Profile.getLastUsedProfile(), clientType, UserActionType.DISMISS,
guid, ActionButtonType.UNKNOWN_ACTION, null);
break;
case NotificationIntentInterceptor.IntentType.ACTION_INTENT:
int actionButtonType = IntentUtils.safeGetIntExtra(
intent, EXTRA_ACTION_BUTTON_TYPE, ActionButtonType.UNKNOWN_ACTION);
nativeOnActionButton(
Profile.getLastUsedProfile(), clientType, guid, actionButtonType);
String buttonId = IntentUtils.safeGetStringExtra(intent, EXTRA_ACTION_BUTTON_ID);
nativeOnUserAction(Profile.getLastUsedProfile(), clientType,
UserActionType.BUTTON_CLICK, guid, actionButtonType, buttonId);
break;
}
}
......@@ -228,6 +233,7 @@ public class DisplayAgent {
Intent actionIntent = buildIntent(
context, NotificationIntentInterceptor.IntentType.ACTION_INTENT, systemData);
actionIntent.putExtra(EXTRA_ACTION_BUTTON_TYPE, button.type);
actionIntent.putExtra(EXTRA_ACTION_BUTTON_ID, button.id);
// TODO(xingliu): Support button icon. See https://crbug.com/983354
builder.addAction(0 /*icon_id*/, button.text,
......@@ -257,10 +263,7 @@ public class DisplayAgent {
private DisplayAgent() {}
private static native void nativeOnContentClick(
Profile profile, @SchedulerClientType int type, String guid);
private static native void nativeOnDismiss(
Profile profile, @SchedulerClientType int type, String guid);
private static native void nativeOnActionButton(Profile profile,
@SchedulerClientType int clientType, String guid, @ActionButtonType int type);
private static native void nativeOnUserAction(Profile profile,
@SchedulerClientType int clientType, @UserActionType int actionType, String guid,
@ActionButtonType int type, String buttonId);
}
......@@ -4,6 +4,9 @@
#include "chrome/browser/notifications/scheduler/display_agent_android.h"
#include <string>
#include <utility>
#include "base/android/jni_string.h"
#include "base/logging.h"
#include "chrome/android/chrome_jni_headers/DisplayAgent_jni.h"
......@@ -15,6 +18,7 @@
using base::android::ConvertUTF16ToJavaString;
using base::android::ConvertUTF8ToJavaString;
using base::android::JavaParamRef;
using base::android::ScopedJavaLocalRef;
namespace {
......@@ -30,38 +34,30 @@ notifications::UserActionHandler* GetUserActionHandler(
} // namespace
// static
void JNI_DisplayAgent_OnContentClick(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& j_profile,
jint j_client_type,
const base::android::JavaParamRef<jstring>& j_guid) {
GetUserActionHandler(j_profile)->OnClick(
static_cast<notifications::SchedulerClientType>(j_client_type),
ConvertJavaStringToUTF8(env, j_guid));
}
// static
void JNI_DisplayAgent_OnDismiss(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& j_profile,
void JNI_DisplayAgent_OnUserAction(JNIEnv* env,
const JavaParamRef<jobject>& j_profile,
jint j_client_type,
const base::android::JavaParamRef<jstring>& j_guid) {
GetUserActionHandler(j_profile)->OnDismiss(
jint j_action_type,
const JavaParamRef<jstring>& j_guid,
jint j_button_type,
const JavaParamRef<jstring>& j_button_id) {
auto user_action_type =
static_cast<notifications::UserActionType>(j_action_type);
notifications::UserActionData action_data(
static_cast<notifications::SchedulerClientType>(j_client_type),
ConvertJavaStringToUTF8(env, j_guid));
}
user_action_type, ConvertJavaStringToUTF8(env, j_guid));
// Attach button click data.
if (user_action_type == notifications::UserActionType::kButtonClick) {
notifications::ButtonClickInfo button_click_info;
button_click_info.button_id = ConvertJavaStringToUTF8(env, j_button_id);
button_click_info.type =
static_cast<notifications::ActionButtonType>(j_button_type);
action_data.button_click_info =
base::make_optional(std::move(button_click_info));
}
// static
void JNI_DisplayAgent_OnActionButton(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& j_profile,
jint j_client_type,
const base::android::JavaParamRef<jstring>& j_guid,
jint type) {
GetUserActionHandler(j_profile)->OnActionClick(
static_cast<notifications::SchedulerClientType>(j_client_type),
ConvertJavaStringToUTF8(env, j_guid),
static_cast<notifications::ActionButtonType>(type));
GetUserActionHandler(j_profile)->OnUserAction(action_data);
}
DisplayAgentAndroid::DisplayAgentAndroid() = default;
......
......@@ -109,20 +109,22 @@ void ImpressionHistoryTrackerImpl::GetImpressionDetail(
std::move(callback).Run(std::move(detail));
}
void ImpressionHistoryTrackerImpl::OnClick(SchedulerClientType type,
const std::string& guid) {
OnClickInternal(guid, true /*update_db*/);
}
void ImpressionHistoryTrackerImpl::OnActionClick(SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) {
OnButtonClickInternal(guid, button_type, true /*update_db*/);
}
void ImpressionHistoryTrackerImpl::OnDismiss(SchedulerClientType type,
const std::string& guid) {
OnDismissInternal(guid, true /*update_db*/);
void ImpressionHistoryTrackerImpl::OnUserAction(
const UserActionData& action_data) {
auto button_type = action_data.button_click_info.has_value()
? action_data.button_click_info->type
: ActionButtonType::kUnknownAction;
switch (action_data.action_type) {
case UserActionType::kClick:
OnClickInternal(action_data.guid, true /*update_db*/);
break;
case UserActionType::kButtonClick:
OnButtonClickInternal(action_data.guid, button_type, true /*update_db*/);
break;
case UserActionType::kDismiss:
OnDismissInternal(action_data.guid, true /*update_db*/);
break;
}
}
void ImpressionHistoryTrackerImpl::OnStoreInitialized(
......
......@@ -95,11 +95,7 @@ class ImpressionHistoryTrackerImpl : public ImpressionHistoryTracker {
void GetImpressionDetail(
SchedulerClientType type,
ImpressionDetail::ImpressionDetailCallback callback) override;
void OnClick(SchedulerClientType type, const std::string& guid) override;
void OnActionClick(SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) override;
void OnDismiss(SchedulerClientType type, const std::string& guid) override;
void OnUserAction(const UserActionData& action_data) override;
// Called after |store_| is initialized.
void OnStoreInitialized(InitCallback callback,
......
......@@ -24,6 +24,7 @@ namespace notifications {
namespace {
const char kGuid1[] = "guid1";
const char kButtonId[] = "button_id_1";
const char kTimeStr[] = "04/25/20 01:00:00 AM";
struct TestCase {
......@@ -252,7 +253,9 @@ TEST_F(ImpressionHistoryTrackerTest, ClickNoImpression) {
InitTrackerWithData(test_case);
EXPECT_CALL(*store(), Update(_, _, _)).Times(0);
EXPECT_CALL(*delegate(), OnImpressionUpdated()).Times(0);
tracker()->OnClick(SchedulerClientType::kTest1, kGuid1);
UserActionData action_data(SchedulerClientType::kTest1,
UserActionType::kClick, kGuid1);
tracker()->OnUserAction(action_data);
VerifyClientStates(test_case);
}
......@@ -331,12 +334,22 @@ TEST_P(ImpressionHistoryTrackerUserActionTest, UserAction) {
// Trigger user action.
if (GetParam().user_feedback == UserFeedback::kClick) {
tracker()->OnClick(SchedulerClientType::kTest1, kGuid1);
UserActionData action_data(SchedulerClientType::kTest1,
UserActionType::kClick, kGuid1);
tracker()->OnUserAction(action_data);
} else if (GetParam().button_type.has_value()) {
tracker()->OnActionClick(SchedulerClientType::kTest1, kGuid1,
GetParam().button_type.value());
ButtonClickInfo button_click_info;
button_click_info.button_id = kButtonId;
button_click_info.type = GetParam().button_type.value();
UserActionData action_data(SchedulerClientType::kTest1,
UserActionType::kButtonClick, kGuid1);
action_data.button_click_info =
base::make_optional(std::move(button_click_info));
tracker()->OnUserAction(action_data);
} else if (GetParam().user_feedback == UserFeedback::kDismiss) {
tracker()->OnDismiss(SchedulerClientType::kTest1, kGuid1);
UserActionData action_data(SchedulerClientType::kTest1,
UserActionType::kDismiss, kGuid1);
tracker()->OnUserAction(action_data);
}
VerifyClientStates(test_case);
......
......@@ -79,39 +79,15 @@ void InitAwareNotificationScheduler::OnStopTask(SchedulerTaskTime task_time) {
weak_ptr_factory_.GetWeakPtr(), task_time));
}
void InitAwareNotificationScheduler::OnClick(SchedulerClientType type,
const std::string& guid) {
void InitAwareNotificationScheduler::OnUserAction(
const UserActionData& action_data) {
if (IsReady()) {
impl_->OnClick(type, guid);
impl_->OnUserAction(action_data);
return;
}
MaybeCacheClosure(base::BindOnce(&InitAwareNotificationScheduler::OnClick,
weak_ptr_factory_.GetWeakPtr(), type, guid));
}
void InitAwareNotificationScheduler::OnActionClick(
SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) {
if (IsReady()) {
impl_->OnActionClick(type, guid, button_type);
return;
}
MaybeCacheClosure(
base::BindOnce(&InitAwareNotificationScheduler::OnActionClick,
weak_ptr_factory_.GetWeakPtr(), type, guid, button_type));
}
void InitAwareNotificationScheduler::OnDismiss(SchedulerClientType type,
const std::string& guid) {
if (IsReady()) {
impl_->OnDismiss(type, guid);
return;
}
MaybeCacheClosure(base::BindOnce(&InitAwareNotificationScheduler::OnDismiss,
weak_ptr_factory_.GetWeakPtr(), type, guid));
base::BindOnce(&InitAwareNotificationScheduler::OnUserAction,
weak_ptr_factory_.GetWeakPtr(), action_data));
}
void InitAwareNotificationScheduler::OnInitialized(InitCallback init_callback,
......
......@@ -41,11 +41,7 @@ class InitAwareNotificationScheduler : public NotificationScheduler {
void OnStartTask(SchedulerTaskTime task_time,
TaskFinishedCallback callback) override;
void OnStopTask(SchedulerTaskTime task_time) override;
void OnClick(SchedulerClientType type, const std::string& guid) override;
void OnActionClick(SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) override;
void OnDismiss(SchedulerClientType type, const std::string& guid) override;
void OnUserAction(const UserActionData& action_data) override;
// Called after initialization is done.
void OnInitialized(InitCallback init_callback, bool success);
......
......@@ -33,10 +33,7 @@ class MockNotificationScheduler : public NotificationScheduler {
ImpressionDetail::ImpressionDetailCallback callback));
MOCK_METHOD2(OnStartTask, void(SchedulerTaskTime, TaskFinishedCallback));
MOCK_METHOD1(OnStopTask, void(SchedulerTaskTime));
MOCK_METHOD2(OnClick, void(SchedulerClientType, const std::string&));
MOCK_METHOD3(OnActionClick,
void(SchedulerClientType, const std::string&, ActionButtonType));
MOCK_METHOD2(OnDismiss, void(SchedulerClientType, const std::string&));
MOCK_METHOD1(OnUserAction, void(const UserActionData&));
private:
DISALLOW_COPY_AND_ASSIGN(MockNotificationScheduler);
......
......@@ -37,15 +37,7 @@ void NoopNotificationScheduleService::OnStartTask(
void NoopNotificationScheduleService::OnStopTask(SchedulerTaskTime task_time) {}
void NoopNotificationScheduleService::OnClick(SchedulerClientType type,
const std::string& guid) {}
void NoopNotificationScheduleService::OnActionClick(
SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) {}
void NoopNotificationScheduleService::OnDismiss(SchedulerClientType type,
const std::string& guid) {}
void NoopNotificationScheduleService::OnUserAction(
const UserActionData& action_data) {}
} // namespace notifications
......@@ -39,11 +39,7 @@ class NoopNotificationScheduleService
void OnStopTask(SchedulerTaskTime task_time) override;
// UserActionHandler implementation.
void OnClick(SchedulerClientType type, const std::string& guid) override;
void OnActionClick(SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) override;
void OnDismiss(SchedulerClientType type, const std::string& guid) override;
void OnUserAction(const UserActionData& action_data) override;
DISALLOW_COPY_AND_ASSIGN(NoopNotificationScheduleService);
};
......
......@@ -58,21 +58,9 @@ void NotificationScheduleServiceImpl::OnStopTask(SchedulerTaskTime task_time) {
scheduler_->OnStopTask(task_time);
}
void NotificationScheduleServiceImpl::OnClick(SchedulerClientType type,
const std::string& guid) {
scheduler_->OnClick(type, guid);
}
void NotificationScheduleServiceImpl::OnActionClick(
SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) {
scheduler_->OnActionClick(type, guid, button_type);
}
void NotificationScheduleServiceImpl::OnDismiss(SchedulerClientType type,
const std::string& guid) {
scheduler_->OnDismiss(type, guid);
void NotificationScheduleServiceImpl::OnUserAction(
const UserActionData& action_data) {
scheduler_->OnUserAction(action_data);
}
void NotificationScheduleServiceImpl::OnInitialized(bool success) {
......
......@@ -45,11 +45,7 @@ class NotificationScheduleServiceImpl
void OnStopTask(SchedulerTaskTime task_time) override;
// UserActionHandler implementation.
void OnClick(SchedulerClientType type, const std::string& guid) override;
void OnActionClick(SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) override;
void OnDismiss(SchedulerClientType type, const std::string& guid) override;
void OnUserAction(const UserActionData& action_data) override;
// Called after initialization is done.
void OnInitialized(bool success);
......
......@@ -267,52 +267,22 @@ class NotificationSchedulerImpl : public NotificationScheduler,
std::move(notifications), std::move(client_states), task_start_time_);
}
void OnClick(SchedulerClientType type, const std::string& guid) override {
context_->impression_tracker()->OnClick(type, guid);
void OnUserAction(const UserActionData& action_data) override {
context_->impression_tracker()->OnUserAction(action_data);
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&NotificationSchedulerImpl::NotifyClientAfterUserAction,
weak_ptr_factory_.GetWeakPtr(), UserActionType::kClick,
type, base::nullopt));
weak_ptr_factory_.GetWeakPtr(), action_data));
}
void OnActionClick(SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) override {
context_->impression_tracker()->OnActionClick(type, guid, button_type);
ButtonClickInfo button_info;
// TODO(xingliu): Plumb the button id from platform.
button_info.button_id = std::string();
button_info.type = button_type;
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&NotificationSchedulerImpl::NotifyClientAfterUserAction,
weak_ptr_factory_.GetWeakPtr(),
UserActionType::kButtonClick, type,
std::move(button_info)));
}
void OnDismiss(SchedulerClientType type, const std::string& guid) override {
context_->impression_tracker()->OnDismiss(type, guid);
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&NotificationSchedulerImpl::NotifyClientAfterUserAction,
weak_ptr_factory_.GetWeakPtr(), UserActionType::kDismiss,
type, base::nullopt));
}
void NotifyClientAfterUserAction(
UserActionType action_type,
SchedulerClientType client_type,
base::Optional<ButtonClickInfo> button_info) {
auto* client = context_->client_registrar()->GetClient(client_type);
void NotifyClientAfterUserAction(const UserActionData& action_data) {
auto* client =
context_->client_registrar()->GetClient(action_data.client_type);
if (!client)
return;
client->OnUserAction(action_type, std::move(button_info));
client->OnUserAction(action_data);
}
std::unique_ptr<NotificationSchedulerContext> context_;
......
......@@ -25,8 +25,7 @@ void WebUIClient::OnSchedulerInitialized(bool success,
NOTIMPLEMENTED();
}
void WebUIClient::OnUserAction(UserActionType action_type,
base::Optional<ButtonClickInfo> button_info) {
void WebUIClient::OnUserAction(const UserActionData& action_data) {
NOTIMPLEMENTED();
}
......
......@@ -26,8 +26,7 @@ class WebUIClient : public NotificationSchedulerClient {
NotificationDataCallback callback) override;
void OnSchedulerInitialized(bool success,
std::set<std::string> guids) override;
void OnUserAction(UserActionType action_type,
base::Optional<ButtonClickInfo> button_info) override;
void OnUserAction(const UserActionData& action_data) override;
DISALLOW_COPY_AND_ASSIGN(WebUIClient);
};
......
......@@ -23,6 +23,7 @@ source_set("public") {
"notification_scheduler_client.h",
"notification_scheduler_client_registrar.cc",
"notification_scheduler_client_registrar.h",
"notification_scheduler_types.cc",
"notification_scheduler_types.h",
"schedule_params.cc",
"schedule_params.h",
......
......@@ -41,8 +41,7 @@ class NotificationSchedulerClient {
std::set<std::string> guids) = 0;
// Called when the user interacts with the notification.
virtual void OnUserAction(UserActionType action_type,
base::Optional<ButtonClickInfo> button_info) = 0;
virtual void OnUserAction(const UserActionData& action_data) = 0;
private:
DISALLOW_COPY_AND_ASSIGN(NotificationSchedulerClient);
......
// 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 "chrome/browser/notifications/scheduler/public/notification_scheduler_types.h"
namespace notifications {
UserActionData::UserActionData(SchedulerClientType client_type,
UserActionType action_type,
const std::string& guid)
: client_type(client_type), action_type(action_type), guid(guid) {}
UserActionData::UserActionData(const UserActionData& other) = default;
UserActionData::~UserActionData() = default;
} // namespace notifications
......@@ -5,8 +5,11 @@
#ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_PUBLIC_NOTIFICATION_SCHEDULER_TYPES_H_
#define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_PUBLIC_NOTIFICATION_SCHEDULER_TYPES_H_
#include <map>
#include <string>
#include "base/optional.h"
namespace notifications {
// Enum to describe the time to process scheduled notification data.
......@@ -73,6 +76,9 @@ enum class ImpressionResult {
};
// Defines user actions type.
// A Java counterpart will be generated for this enum.
// GENERATED_JAVA_ENUM_PACKAGE: (
// org.chromium.chrome.browser.notifications.scheduler)
enum class UserActionType {
// The user clicks on the notification body.
kClick = 0,
......@@ -107,6 +113,30 @@ struct ButtonClickInfo {
ActionButtonType type = ActionButtonType::kUnknownAction;
};
// Contains data associated with user actions.
struct UserActionData {
UserActionData(SchedulerClientType client_type,
UserActionType action_type,
const std::string& guid);
UserActionData(const UserActionData& other);
~UserActionData();
// The type of the client that sent the notification.
const SchedulerClientType client_type;
// The user action type.
const UserActionType action_type;
// The guid of the notification.
const std::string guid;
// The client defined custom data.
std::map<std::string, std::string> custom_data;
// The button click info, only available when the user clicked a button.
base::Optional<ButtonClickInfo> button_click_info;
};
} // namespace notifications
#endif // CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_PUBLIC_NOTIFICATION_SCHEDULER_TYPES_H_
......@@ -5,7 +5,6 @@
#ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_PUBLIC_USER_ACTION_HANDLER_H_
#define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_PUBLIC_USER_ACTION_HANDLER_H_
#include <string>
#include "base/macros.h"
#include "chrome/browser/notifications/scheduler/public/notification_scheduler_types.h"
......@@ -13,20 +12,10 @@
namespace notifications {
// An interface to plumb user actions events to notification scheduling system.
// Each event needs to provide an unique id of the notification shown.
class UserActionHandler {
public:
// Called when the user clicks on the notification. |guid| is the internal id
// to track the notification persist to disk.
virtual void OnClick(SchedulerClientType type, const std::string& guid) = 0;
// Called when the user clicks on a button on the notification.
virtual void OnActionClick(SchedulerClientType type,
const std::string& guid,
ActionButtonType button_type) = 0;
// Called when the user cancels or dismiss the notification.
virtual void OnDismiss(SchedulerClientType type, const std::string& guid) = 0;
// Called when the user interacts with the notification.
virtual void OnUserAction(const UserActionData& action_data) = 0;
~UserActionHandler() = default;
......
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