Skip to content

GitLab

T
tangled
Commit e54136de authored by Clemens Arbesser's avatar Clemens Arbesser Committed by Commit Bot
Browse files
Options

[Autofill Assistant] Refactor lite script notification for ui-shown 

This is done in preparation for an upcoming new UKM where we need to
know not just when the UI was shown, but also when the lite script
started. I opted to rename the existing callback instead of introducing
a new one.

This is a refactoring only and should have no user-facing changes.

Bug: b/169052568
Change-Id: I44d4033e20a8c7dfd4cca96be5a3c36388c8b1f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424319
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: default avatarSandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#810548}
parent 1117bd9f
Hide whitespace changes
Inline Side-by-side
Showing with 45 additions and 31 deletions
chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantLiteScriptCoordinator.java
View file @ e54136de
......@@ -53,11 +53,13 @@ class AutofillAssistantLiteScriptCoordinator {
}
@Override
public void onUiShown() {
// The prompt was displayed on screen, hence we mark them as returning user
// from now on.
AutofillAssistantPreferencesUtil
.setAutofillAssistantReturningLiteScriptUser();
public void onScriptRunning(boolean uiShown) {
if (uiShown) {
// The prompt was displayed on screen, hence we mark them as returning
// user from now on.
AutofillAssistantPreferencesUtil
.setAutofillAssistantReturningLiteScriptUser();
}
}
});
AutofillAssistantServiceInjector.setServiceToInject(liteService);
......
chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantLiteService.java
View file @ e54136de
......@@ -19,8 +19,11 @@ public class AutofillAssistantLiteService
interface Delegate {
/** The lite script has finished with {@code state}. */
void onFinished(@LiteScriptFinishedState int state);
/** The UI was shown for the first time to the user. */
void onUiShown();
/**
* The lite script has started and is now running. In the first stage of the script,
* @code{uiShown} is expected to be false, in the second stage, it is expected to be true.
*/
void onScriptRunning(boolean uiShown);
}
private final WebContents mWebContents;
private final String mTriggerScriptPath;
......@@ -51,9 +54,9 @@ public class AutofillAssistantLiteService
}
@CalledByNative
private void onUiShown() {
private void onScriptRunning(boolean uiShown) {
if (mDelegate != null) {
mDelegate.onUiShown();
mDelegate.onScriptRunning(uiShown);
}
}
......
chrome/browser/android/autofill_assistant/lite_service_bridge.cc
View file @ e54136de
......@@ -22,9 +22,12 @@ void OnFinished(base::android::ScopedJavaGlobalRef<jobject> java_lite_service,
static_cast<int>(state));
}
void OnUiShown(base::android::ScopedJavaGlobalRef<jobject> java_lite_service) {
void OnScriptRunning(
base::android::ScopedJavaGlobalRef<jobject> java_lite_service,
bool ui_shown) {
JNIEnv* env = base::android::AttachCurrentThread();
Java_AutofillAssistantLiteService_onUiShown(env, java_lite_service);
Java_AutofillAssistantLiteService_onScriptRunning(env, java_lite_service,
ui_shown);
}
// static
......@@ -51,8 +54,9 @@ jlong JNI_AutofillAssistantLiteService_CreateLiteService(
base::android::ConvertJavaStringToUTF8(env, jtrigger_script_path),
base::BindOnce(&OnFinished, base::android::ScopedJavaGlobalRef<jobject>(
java_lite_service)),
base::BindOnce(&OnUiShown, base::android::ScopedJavaGlobalRef<jobject>(
java_lite_service))));
base::BindRepeating(
&OnScriptRunning,
base::android::ScopedJavaGlobalRef<jobject>(java_lite_service))));
}
} // namespace autofill_assistant
components/autofill_assistant/browser/lite_service.cc
View file @ e54136de
......@@ -20,11 +20,12 @@ LiteService::LiteService(
const std::string& trigger_script_path,
base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
notify_finished_callback,
base::OnceCallback<void()> notify_ui_shown_callback)
base::RepeatingCallback<void(bool)> notify_script_running_callback)
: service_impl_(std::move(service_impl)),
trigger_script_path_(trigger_script_path),
notify_finished_callback_(std::move(notify_finished_callback)),
notify_ui_shown_callback_(std::move(notify_ui_shown_callback)) {}
notify_script_running_callback_(
std::move(notify_script_running_callback)) {}
LiteService::~LiteService() {
if (notify_finished_callback_) {
......@@ -131,6 +132,7 @@ void LiteService::OnGetActions(ResponseCallback callback,
std::string serialized_first_part;
split_actions->first.SerializeToString(&serialized_first_part);
std::move(callback).Run(result, serialized_first_part);
notify_script_running_callback_.Run(/*ui_shown = */ false);
}
void LiteService::GetNextActions(
......@@ -177,9 +179,7 @@ void LiteService::GetNextActions(
trigger_script_second_part_->SerializeToString(&serialized_second_part);
trigger_script_second_part_.reset();
std::move(callback).Run(true, serialized_second_part);
if (notify_ui_shown_callback_) {
std::move(notify_ui_shown_callback_).Run();
}
notify_script_running_callback_.Run(/*ui_shown = */ true);
return;
}
} else {
......
components/autofill_assistant/browser/lite_service.h
View file @ e54136de
......@@ -29,7 +29,7 @@ class LiteService : public Service {
const std::string& trigger_script_path,
base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
notify_finished_callback,
base::OnceCallback<void()> notify_ui_shown_callback);
base::RepeatingCallback<void(bool)> notify_script_running_callback);
// If the destructor is called before |GetNextActions|, the script was
// terminated before finishing (user cancelled, closed the tab, etc.).
~LiteService() override;
......@@ -96,8 +96,9 @@ class LiteService : public Service {
// controller will terminate gracefully with an explicit stop action.
base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
notify_finished_callback_;
// Notifies the java bridge that the UI was shown for the first time.
base::OnceCallback<void(void)> notify_ui_shown_callback_;
// Notifies the java bridge that the script is running. The bool parameter
// indicates whether the UI is being shown or not.
base::RepeatingCallback<void(bool)> notify_script_running_callback_;
// The second part of the trigger script, i.e., the actions that should be run
// after a successful prompt(browse) action in the first part of the script.
......
components/autofill_assistant/browser/lite_service_unittest.cc
View file @ e54136de
......@@ -40,7 +40,7 @@ class LiteServiceTest : public testing::Test {
lite_service_ = std::make_unique<LiteService>(
std::move(service_impl), kFakeScriptPath, mock_finished_callback_.Get(),
mock_ui_shown_callback_.Get());
mock_script_running_callback_.Get());
EXPECT_CALL(*mock_native_service_, OnGetScriptsForUrl).Times(0);
EXPECT_CALL(*mock_native_service_, OnGetNextActions).Times(0);
......@@ -78,7 +78,8 @@ class LiteServiceTest : public testing::Test {
base::MockCallback<base::OnceCallback<void(Metrics::LiteScriptFinishedState)>>
mock_finished_callback_;
base::MockCallback<base::OnceCallback<void(void)>> mock_ui_shown_callback_;
base::MockCallback<base::RepeatingCallback<void(bool)>>
mock_script_running_callback_;
base::MockCallback<base::OnceCallback<void(bool, const std::string&)>>
mock_response_callback_;
MockService* mock_native_service_ = nullptr;
......@@ -89,14 +90,15 @@ class LiteServiceTest : public testing::Test {
TEST_F(LiteServiceTest, RunsNotificationOnDelete) {
base::MockCallback<base::OnceCallback<void(Metrics::LiteScriptFinishedState)>>
notification_callback;
base::MockCallback<base::OnceCallback<void()>> ui_shown_callback;
base::MockCallback<base::RepeatingCallback<void(bool)>>
script_running_callback;
EXPECT_CALL(
notification_callback,
Run(Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED));
{
LiteService lite_service(std::make_unique<MockService>(), kFakeScriptPath,
notification_callback.Get(),
ui_shown_callback.Get());
script_running_callback.Get());
}
}
......@@ -164,7 +166,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsFailed) {
std::move(callback).Run(false, std::string());
});
EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
EXPECT_CALL(mock_script_running_callback_, Run).Times(0);
lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
TriggerContextImpl(), "", "",
mock_response_callback_.Get());
......@@ -183,7 +185,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsParsingError) {
Service::ResponseCallback& callback) {
std::move(callback).Run(true, std::string("invalid proto"));
});
EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
EXPECT_CALL(mock_script_running_callback_, Run).Times(0);
lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
TriggerContextImpl(), "", "",
mock_response_callback_.Get());
......@@ -195,7 +197,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsContainsUnsafeActions) {
get_actions_response_.add_actions()->mutable_prompt();
ExpectStopWithFinishedState(
Metrics::LiteScriptFinishedState::LITE_SCRIPT_UNSAFE_ACTIONS);
EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
EXPECT_CALL(mock_script_running_callback_, Run).Times(0);
lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
TriggerContextImpl(), "", "",
mock_response_callback_.Get());
......@@ -205,6 +207,7 @@ TEST_F(LiteServiceTest, GetActionsSucceedsForMinimalViableScript) {
get_actions_response_.add_actions()->mutable_prompt()->set_browse_mode(true);
get_actions_response_.add_actions()->mutable_prompt();
EXPECT_CALL(mock_response_callback_, Run(true, _));
EXPECT_CALL(mock_script_running_callback_, Run(/*ui_shown =*/false)).Times(1);
lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
TriggerContextImpl(), "", "",
mock_response_callback_.Get());
......@@ -261,7 +264,7 @@ TEST_F(LiteServiceTest, GetActionsSplitsActionsResponseAtLastBrowse) {
processed_actions.back().mutable_prompt_choice()->set_server_payload(
proto.actions(3).prompt().choices(0).server_payload());
});
EXPECT_CALL(mock_ui_shown_callback_, Run).Times(1);
EXPECT_CALL(mock_script_running_callback_, Run(/*ui_shown =*/false)).Times(1);
lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
TriggerContextImpl(), "", "",
mock_response_callback_.Get());
......@@ -277,6 +280,7 @@ TEST_F(LiteServiceTest, GetActionsSplitsActionsResponseAtLastBrowse) {
EXPECT_FALSE(proto.actions(0).prompt().browse_mode());
});
EXPECT_CALL(mock_script_running_callback_, Run(/*ui_shown =*/true)).Times(1);
lite_service_->GetNextActions(TriggerContextImpl(), "", "", processed_actions,
mock_response_callback_.Get());
}
......@@ -290,7 +294,7 @@ TEST_F(LiteServiceTest, GetNextActionsFirstPartStopsOnUserNavigateAway) {
ExpectStopWithFinishedState(
Metrics::LiteScriptFinishedState::LITE_SCRIPT_BROWSE_FAILED_NAVIGATE);
EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
EXPECT_CALL(mock_script_running_callback_, Run(/*ui_shown =*/true)).Times(0);
lite_service_->GetNextActions(TriggerContextImpl(), "", "", processed_actions,
mock_response_callback_.Get());
}
......@@ -309,7 +313,7 @@ TEST_F(LiteServiceTest, GetNextActionsFirstPartSucceedsOnAutoSelectChoice) {
"payload");
EXPECT_CALL(mock_response_callback_, Run(true, ""));
EXPECT_CALL(mock_ui_shown_callback_, Run).Times(1);
EXPECT_CALL(mock_script_running_callback_, Run(/*ui_shown =*/true)).Times(1);
lite_service_->GetNextActions(TriggerContextImpl(), "", "", processed_actions,
mock_response_callback_.Get());
}
......
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