Skip to content

Commit

Permalink
Reland "[Autofill Assistant] Forward trigger_ui_type to regular scrip…
Browse files Browse the repository at this point in the history
…t startup"

This is a reland of 7f3e0ef

An intermittent change from base::nullopt (and others) to absl::nullopt
was not properly reflected in the rebase and caused a compile error.

Original change's description:
> [Autofill Assistant] Forward trigger_ui_type to regular script startup
>
> This will let the backend know which type of trigger script the user
> saw and accepted.
>
> Bug: b/173495422
> Change-Id: If477bc562eba332bc412e513011e81e78a955966
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2879983
> Commit-Queue: Clemens Arbesser <arbesser@google.com>
> Reviewed-by: Stephane Zermatten <szermatt@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#883860}

Bug: b/173495422
Change-Id: I22e128db5a9a82682ecf8490d8cf716c7bb807cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2902965
Reviewed-by: Stephane Zermatten <szermatt@chromium.org>
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/master@{#883946}
  • Loading branch information
Clemens Arbesser authored and Chromium LUCI CQ committed May 18, 2021
1 parent 4ad6f45 commit f9ba709
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 15 deletions.
4 changes: 4 additions & 0 deletions components/autofill_assistant/browser/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ message ClientContextProto {
// triggered one.
optional bool is_in_chrome_triggered = 17;

// The type of trigger script that was shown and accepted at the beginning of
// this flow, if any.
optional TriggerUIType trigger_ui_type = 18;

message DeviceContextProto {
message VersionProto {
// The Android SDK version of the device.
Expand Down
42 changes: 27 additions & 15 deletions components/autofill_assistant/browser/starter_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,11 @@ class StarterTest : public content::RenderViewHostTestHarness {
// Returns a serialized GetTriggerScriptsResponseProto containing a single
// trigger script without any trigger conditions. As such, it will be shown
// immediately upon startup.
std::string CreateTriggerScriptResponseForTest() {
std::string CreateTriggerScriptResponseForTest(
TriggerUIType trigger_ui_type = UNSPECIFIED_TRIGGER_UI_TYPE) {
GetTriggerScriptsResponseProto get_trigger_scripts_response;
get_trigger_scripts_response.add_trigger_scripts();
get_trigger_scripts_response.add_trigger_scripts()->set_trigger_ui_type(
trigger_ui_type);
std::string serialized_get_trigger_scripts_response;
get_trigger_scripts_response.SerializeToString(
&serialized_get_trigger_scripts_response);
Expand Down Expand Up @@ -639,16 +641,21 @@ TEST_F(StarterTest, RpcTriggerScriptSucceeds) {
ASSERT_TRUE(request.ParseFromString(request_body));
EXPECT_THAT(request.url(), Eq(GURL(kExampleDeeplink)));
EXPECT_FALSE(request.client_context().is_in_chrome_triggered());
std::move(callback).Run(net::HTTP_OK,
CreateTriggerScriptResponseForTest());
std::move(callback).Run(
net::HTTP_OK,
CreateTriggerScriptResponseForTest(CART_FIRST_TIME_USER));
}));
GetTriggerScriptsResponseProto get_trigger_scripts_response;
get_trigger_scripts_response.ParseFromString(
CreateTriggerScriptResponseForTest());
EXPECT_CALL(mock_start_regular_script_callback_,
Run(GURL(kExampleDeeplink),
Pointee(Property(&TriggerContext::GetOnboardingShown, true)),
Optional(get_trigger_scripts_response.trigger_scripts(0))));
CreateTriggerScriptResponseForTest(CART_FIRST_TIME_USER));
EXPECT_CALL(
mock_start_regular_script_callback_,
Run(GURL(kExampleDeeplink),
Pointee(AllOf(Property(&TriggerContext::GetOnboardingShown, true),
Property(&TriggerContext::GetInChromeTriggered, false),
Property(&TriggerContext::GetTriggerUIType,
CART_FIRST_TIME_USER))),
Optional(get_trigger_scripts_response.trigger_scripts(0))));

starter_->Start(std::make_unique<TriggerContext>(
std::make_unique<ScriptParameters>(script_parameters), options));
Expand Down Expand Up @@ -984,19 +991,24 @@ TEST_F(StarterTest, ImplicitStartupOnSupportedDomain) {
EXPECT_THAT(request.url(),
Eq(GURL("https://www.some-website.com/cart")));
EXPECT_TRUE(request.client_context().is_in_chrome_triggered());
std::move(callback).Run(net::HTTP_OK,
CreateTriggerScriptResponseForTest());
std::move(callback).Run(
net::HTTP_OK,
CreateTriggerScriptResponseForTest(CART_RETURNING_USER));
}));
EXPECT_CALL(*mock_trigger_script_ui_delegate_, ShowTriggerScript)
.WillOnce([&]() {
ASSERT_TRUE(trigger_script_coordinator_ != nullptr);
trigger_script_coordinator_->PerformTriggerScriptAction(
TriggerScriptProto::ACCEPT);
});
EXPECT_CALL(mock_start_regular_script_callback_,
Run(GURL("https://www.some-website.com/cart"),
Pointee(Property(&TriggerContext::GetOnboardingShown, false)),
testing::Ne(absl::nullopt)));
EXPECT_CALL(
mock_start_regular_script_callback_,
Run(GURL("https://www.some-website.com/cart"),
Pointee(AllOf(Property(&TriggerContext::GetOnboardingShown, false),
Property(&TriggerContext::GetInChromeTriggered, true),
Property(&TriggerContext::GetTriggerUIType,
CART_RETURNING_USER))),
testing::Ne(absl::nullopt)));

// Implicit startup by navigating to an autofill-assistant-enabled site.
content::WebContentsTester::For(web_contents())
Expand Down
11 changes: 11 additions & 0 deletions components/autofill_assistant/browser/trigger_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ TriggerContext::TriggerContext(std::vector<const TriggerContext*> contexts)
if (initial_url_.empty()) {
initial_url_ = context->GetInitialUrl();
}
if (trigger_ui_type_ == UNSPECIFIED_TRIGGER_UI_TYPE) {
trigger_ui_type_ = context->GetTriggerUIType();
}
}
}

Expand Down Expand Up @@ -121,4 +124,12 @@ bool TriggerContext::GetInChromeTriggered() const {
return is_in_chrome_triggered_;
}

TriggerUIType TriggerContext::GetTriggerUIType() const {
return trigger_ui_type_;
}

void TriggerContext::SetTriggerUIType(TriggerUIType trigger_ui_type) {
trigger_ui_type_ = trigger_ui_type;
}

} // namespace autofill_assistant
8 changes: 8 additions & 0 deletions components/autofill_assistant/browser/trigger_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ class TriggerContext {
// i.e., a button or link on a website, or whether this is from within Chrome.
virtual bool GetInChromeTriggered() const;

// Returns the trigger type of the trigger script that was shown and accepted
// at the beginning of the flow, if any.
virtual TriggerUIType GetTriggerUIType() const;

// Sets the trigger type of the shown trigger script.
virtual void SetTriggerUIType(TriggerUIType trigger_ui_type);

private:
std::unique_ptr<ScriptParameters> script_parameters_;

Expand All @@ -113,6 +120,7 @@ class TriggerContext {

// The initial url at the time of triggering.
std::string initial_url_;
TriggerUIType trigger_ui_type_ = UNSPECIFIED_TRIGGER_UI_TYPE;
};

} // namespace autofill_assistant
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,12 @@ TEST(TriggerContextTest, Create) {
EXPECT_TRUE(context.GetDirectAction());
EXPECT_EQ(context.GetInitialUrl(), "https://www.example.com");
EXPECT_TRUE(context.GetInChromeTriggered());
EXPECT_EQ(context.GetTriggerUIType(), UNSPECIFIED_TRIGGER_UI_TYPE);

context.SetOnboardingShown(false);
EXPECT_FALSE(context.GetOnboardingShown());
context.SetTriggerUIType(CART_FIRST_TIME_USER);
EXPECT_EQ(context.GetTriggerUIType(), CART_FIRST_TIME_USER);
}

TEST(TriggerContextTest, MergeEmpty) {
Expand All @@ -58,6 +61,7 @@ TEST(TriggerContextTest, MergeEmpty) {
EXPECT_FALSE(merged.GetOnboardingShown());
EXPECT_FALSE(merged.GetDirectAction());
EXPECT_FALSE(merged.GetInChromeTriggered());
EXPECT_EQ(merged.GetTriggerUIType(), UNSPECIFIED_TRIGGER_UI_TYPE);
}

TEST(TriggerContextTest, MergeEmptyWithNonEmpty) {
Expand All @@ -76,6 +80,8 @@ TEST(TriggerContextTest, MergeEmptyWithNonEmpty) {
EXPECT_FALSE(merged.GetCCT());
EXPECT_FALSE(merged.GetOnboardingShown());
EXPECT_FALSE(merged.GetDirectAction());
EXPECT_FALSE(merged.GetInChromeTriggered());
EXPECT_EQ(merged.GetTriggerUIType(), UNSPECIFIED_TRIGGER_UI_TYPE);
}

TEST(TriggerContextTest, MergeNonEmptyWithNonEmpty) {
Expand All @@ -94,6 +100,7 @@ TEST(TriggerContextTest, MergeNonEmptyWithNonEmpty) {
/* is_direct_action = */ true,
/* initial_url = */ "https://www.example.com",
/* is_in_chrome_triggered = */ true};
context2.SetTriggerUIType(CHECKOUT_FIRST_TIME_USER);

// Adding empty to make sure empty contexts are properly skipped.
TriggerContext empty;
Expand All @@ -107,6 +114,7 @@ TEST(TriggerContextTest, MergeNonEmptyWithNonEmpty) {
EXPECT_TRUE(merged.GetDirectAction());
EXPECT_EQ(merged.GetInitialUrl(), "https://www.example.com");
EXPECT_TRUE(merged.GetInChromeTriggered());
EXPECT_EQ(merged.GetTriggerUIType(), CHECKOUT_FIRST_TIME_USER);
}

TEST(TriggerContextTest, HasExperimentId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ void TriggerScriptCoordinator::RunCallback(
Metrics::RecordTriggerScriptFinished(ukm_recorder_, ukm_source_id_,
trigger_ui_type, state);
}
trigger_context_->SetTriggerUIType(trigger_ui_type);
std::move(callback_).Run(state, std::move(trigger_context_), trigger_script);
}

Expand Down

0 comments on commit f9ba709

Please sign in to comment.