Commit aaae5472 authored by Michele Mancina's avatar Michele Mancina Committed by Chromium LUCI CQ

[Autofill Assistant] Detects if the user connection or the website are slow.

User connection is evaluated based on the roundtrip times to our backend.
The website speed is based on the duration of single "active" cycle in the wait for dom operation.

Bug: b/175311081
Change-Id: Id7e57b8958af31d3e841b6d728fef16689b5edd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2614838
Commit-Queue: Michele Mancina <micantox@google.com>
Reviewed-by: default avatarMarian Fechete <marianfe@google.com>
Reviewed-by: default avatarLuca Hunkeler <hluca@google.com>
Cr-Commit-Position: refs/heads/master@{#843022}
parent dabedf59
......@@ -445,6 +445,14 @@ class ActionDelegate {
virtual void SetOverlayBehavior(
ConfigureUiStateProto::OverlayBehavior overlay_behavior) = 0;
// Maybe shows a warning letting the user know that the website is unusually
// slow, depending on the current settings.
virtual void MaybeShowSlowWebsiteWarning() = 0;
// Maybe shows a warning letting the user know that a slow connection was
// detected, depending on the current settings.
virtual void MaybeShowSlowConnectionWarning() = 0;
virtual base::WeakPtr<ActionDelegate> GetWeakPtr() const = 0;
protected:
......
......@@ -323,6 +323,9 @@ class MockActionDelegate : public ActionDelegate {
MOCK_METHOD1(SetOverlayBehavior,
void(ConfigureUiStateProto::OverlayBehavior));
MOCK_METHOD0(MaybeShowSlowWebsiteWarning, void());
MOCK_METHOD0(MaybeShowSlowConnectionWarning, void());
base::WeakPtr<ActionDelegate> GetWeakPtr() const override {
return weak_ptr_factory_.GetWeakPtr();
}
......
......@@ -105,7 +105,39 @@ void ClientSettings::UpdateFromProto(const ClientSettingsProto& proto) {
back_button_settings.reset();
}
}
if (proto.has_slow_warning_settings()) {
if (proto.slow_warning_settings().has_enable_slow_connection_warnings()) {
enable_slow_connection_warnings =
proto.slow_warning_settings().enable_slow_connection_warnings();
}
if (proto.slow_warning_settings().has_enable_slow_website_warnings()) {
enable_slow_website_warnings =
proto.slow_warning_settings().enable_slow_website_warnings();
}
if (proto.slow_warning_settings().has_show_only_once()) {
only_show_warning_once = proto.slow_warning_settings().show_only_once();
}
if (proto.slow_warning_settings().has_warning_delay_ms()) {
timeout_warning_delay = base::TimeDelta::FromMilliseconds(
proto.slow_warning_settings().warning_delay_ms());
}
if (proto.slow_warning_settings().has_slow_roundtrip_threshold_ms()) {
slow_roundtrip_threshold = base::TimeDelta::FromMilliseconds(
proto.slow_warning_settings().slow_roundtrip_threshold_ms());
}
if (proto.slow_warning_settings().has_max_consecutive_slow_roundtrips()) {
max_consecutive_slow_roundtrips =
proto.slow_warning_settings().max_consecutive_slow_roundtrips();
}
if (proto.slow_warning_settings().has_slow_connection_message()) {
slow_connection_message =
proto.slow_warning_settings().slow_connection_message();
}
if (proto.slow_warning_settings().has_slow_website_message()) {
slow_website_message =
proto.slow_warning_settings().slow_website_message();
}
}
// Test only settings.
if (proto.has_integration_test_settings()) {
integration_test_settings = proto.integration_test_settings();
......
......@@ -9,6 +9,7 @@
#include "base/optional.h"
#include "base/time/time.h"
#include "components/autofill_assistant/browser/service.pb.h"
#include "components/strings/grit/components_strings.h"
namespace autofill_assistant {
......@@ -89,6 +90,17 @@ struct ClientSettings {
// Snackbar.
base::Optional<ClientSettingsProto::BackButtonSettings> back_button_settings;
bool enable_slow_connection_warnings = false;
bool enable_slow_website_warnings = false;
bool only_show_warning_once = false;
base::TimeDelta timeout_warning_delay =
base::TimeDelta::FromMilliseconds(1000);
int max_consecutive_slow_roundtrips = 3;
base::TimeDelta slow_roundtrip_threshold =
base::TimeDelta::FromMilliseconds(500);
std::string slow_connection_message = "";
std::string slow_website_message = "";
private:
DISALLOW_COPY_AND_ASSIGN(ClientSettings);
};
......
......@@ -508,6 +508,11 @@ void Controller::SetBrowseModeInvisible(bool invisible) {
browse_mode_invisible_ = invisible;
}
bool Controller::ShouldShowWarning() {
return state_ == AutofillAssistantState::RUNNING ||
state_ == AutofillAssistantState::PROMPT;
}
void Controller::AddNavigationListener(
ScriptExecutorDelegate::NavigationListener* listener) {
navigation_listeners_.AddObserver(listener);
......
......@@ -154,6 +154,7 @@ class Controller : public ScriptExecutorDelegate,
view_inflation_finished_callback) override;
void ClearGenericUi() override;
void SetBrowseModeInvisible(bool invisible) override;
bool ShouldShowWarning() override;
// Show the UI if it's not already shown. This is only meaningful while in
// states where showing the UI is optional, such as RUNNING, in tracking mode.
......
......@@ -86,11 +86,11 @@ std::string FakeScriptExecutorDelegate::GetStatusMessage() const {
}
void FakeScriptExecutorDelegate::SetBubbleMessage(const std::string& message) {
status_message_ = message;
bubble_message_ = message;
}
std::string FakeScriptExecutorDelegate::GetBubbleMessage() const {
return status_message_;
return bubble_message_;
}
void FakeScriptExecutorDelegate::SetDetails(std::unique_ptr<Details> details,
......@@ -264,4 +264,8 @@ void FakeScriptExecutorDelegate::SetOverlayBehavior(
void FakeScriptExecutorDelegate::SetBrowseModeInvisible(bool invisible) {}
bool FakeScriptExecutorDelegate::ShouldShowWarning() {
return true;
}
} // namespace autofill_assistant
......@@ -101,6 +101,8 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
ConfigureUiStateProto::OverlayBehavior overlay_behavior) override;
void SetBrowseModeInvisible(bool invisible) override;
bool ShouldShowWarning() override;
ClientSettings* GetMutableSettings() { return &client_settings_; }
void SetCurrentURL(const GURL& url) { current_url_ = url; }
......@@ -120,10 +122,10 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
}
void SetUserModel(UserModel* user_model) { user_model_ = user_model; }
std::vector<AutofillAssistantState> GetStateHistory() {
return state_history_;
}
AutofillAssistantState GetState() const {
return state_history_.empty() ? AutofillAssistantState::INACTIVE
: state_history_.back();
......@@ -161,6 +163,7 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
std::unique_ptr<TriggerContext> trigger_context_;
std::vector<AutofillAssistantState> state_history_;
std::string status_message_;
std::string bubble_message_;
std::vector<Details> details_;
std::unique_ptr<InfoBox> info_box_;
std::unique_ptr<std::vector<UserAction>> user_actions_;
......
......@@ -296,6 +296,9 @@ void ScriptExecutor::ShortWaitForElement(
weak_ptr_factory_.GetWeakPtr(), selector),
base::BindOnce(&ScriptExecutor::OnShortWaitForElement,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
current_action_data_.wait_for_dom->SetTimeoutWarningCallback(
base::BindOnce(&ScriptExecutor::MaybeShowSlowWebsiteWarning,
weak_ptr_factory_.GetWeakPtr()));
current_action_data_.wait_for_dom->Run();
}
......@@ -310,6 +313,9 @@ void ScriptExecutor::WaitForDom(
this, delegate_, max_wait_time, allow_interrupt, check_elements,
base::BindOnce(&ScriptExecutor::OnWaitForElementVisibleWithInterrupts,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
current_action_data_.wait_for_dom->SetTimeoutWarningCallback(base::BindOnce(
&ScriptExecutor::SetBubbleMessage, weak_ptr_factory_.GetWeakPtr(),
delegate_->GetSettings().slow_website_message));
current_action_data_.wait_for_dom->Run();
}
......@@ -829,6 +835,32 @@ void ScriptExecutor::SetOverlayBehavior(
delegate_->SetOverlayBehavior(overlay_behavior);
}
void ScriptExecutor::MaybeShowSlowWebsiteWarning() {
MaybeShowSlowWarning(delegate_->GetSettings().slow_website_message,
delegate_->GetSettings().enable_slow_website_warnings);
}
void ScriptExecutor::MaybeShowSlowConnectionWarning() {
MaybeShowSlowWarning(
delegate_->GetSettings().slow_connection_message,
delegate_->GetSettings().enable_slow_connection_warnings);
}
void ScriptExecutor::MaybeShowSlowWarning(const std::string& message,
bool enabled) {
if (message.empty() || !enabled || !delegate_->ShouldShowWarning()) {
return;
}
if (delegate_->GetSettings().only_show_warning_once &&
warning_callout_already_shown_) {
return;
}
warning_callout_already_shown_ = true;
SetBubbleMessage(message);
}
base::WeakPtr<ActionDelegate> ScriptExecutor::GetWeakPtr() const {
return weak_ptr_factory_.GetWeakPtr();
}
......@@ -838,8 +870,10 @@ void ScriptExecutor::OnGetActions(base::TimeTicks start_time,
const std::string& response) {
VLOG(2) << __func__ << " http-status=" << http_status;
batch_start_time_ = base::TimeTicks::Now();
const base::TimeDelta& roundtrip_duration = batch_start_time_ - start_time;
// Doesn't trigger when the script is completed.
roundtrip_timing_stats_.set_roundtrip_time_ms(
(batch_start_time_ - start_time).InMilliseconds());
roundtrip_duration.InMilliseconds());
bool success =
http_status == net::HTTP_OK && ProcessNextActionResponse(response);
if (should_stop_script_) {
......@@ -857,6 +891,16 @@ void ScriptExecutor::OnGetActions(base::TimeTicks start_time,
}
if (!actions_.empty()) {
if (roundtrip_duration >
delegate_->GetSettings().slow_roundtrip_threshold) {
consecutive_slow_roundtrip_counter_++;
if (consecutive_slow_roundtrip_counter_ >=
delegate_->GetSettings().max_consecutive_slow_roundtrips) {
MaybeShowSlowConnectionWarning();
}
} else {
consecutive_slow_roundtrip_counter_ = 0;
}
ProcessNextAction();
return;
}
......@@ -1077,6 +1121,8 @@ ScriptExecutor::WaitForDomOperation::WaitForDomOperation(
allow_interrupt_(allow_interrupt),
check_elements_(std::move(check_elements)),
callback_(std::move(callback)),
timeout_warning_period_(
main_script->delegate_->GetSettings().timeout_warning_delay),
retry_timer_(main_script->delegate_->GetSettings()
.periodic_element_check_interval) {}
......@@ -1090,6 +1136,11 @@ void ScriptExecutor::WaitForDomOperation::Run() {
Start();
}
void ScriptExecutor::WaitForDomOperation::SetTimeoutWarningCallback(
base::OnceCallback<void()> timeout_warning) {
timeout_warning_callback_ = std::move(timeout_warning);
}
void ScriptExecutor::WaitForDomOperation::Start() {
retry_timer_.Start(
max_wait_time_,
......@@ -1137,8 +1188,19 @@ void ScriptExecutor::WaitForDomOperation::OnScriptListChanged(
main_script_->ReportScriptsUpdateToListener(std::move(scripts));
}
void ScriptExecutor::WaitForDomOperation::TimeoutWarning() {
if (timeout_warning_callback_) {
std::move(timeout_warning_callback_).Run();
}
}
void ScriptExecutor::WaitForDomOperation::RunChecks(
base::OnceCallback<void(const ClientStatus&)> report_attempt_result) {
warning_timer_ = std::make_unique<base::OneShotTimer>();
warning_timer_->Start(
FROM_HERE, timeout_warning_period_,
base::BindOnce(&ScriptExecutor::WaitForDomOperation::TimeoutWarning,
weak_ptr_factory_.GetWeakPtr()));
wait_time_total_ =
(wait_time_stopwatch_.TotalElapsed() < retry_timer_.period())
// It's the first run of the checks, set the total time waited to 0.
......@@ -1196,6 +1258,7 @@ void ScriptExecutor::WaitForDomOperation::OnElementCheckDone(
void ScriptExecutor::WaitForDomOperation::OnAllChecksDone(
base::OnceCallback<void(const ClientStatus&)> report_attempt_result) {
warning_timer_->Stop();
if (runnable_interrupts_.empty()) {
// Since no interrupts fired, allow previously-run interrupts to be run
// again in the next round. This is meant to give elements one round to
......
......@@ -273,6 +273,8 @@ class ScriptExecutor : public ActionDelegate,
void ClearGenericUi() override;
void SetOverlayBehavior(
ConfigureUiStateProto::OverlayBehavior overlay_behavior) override;
void MaybeShowSlowWebsiteWarning() override;
void MaybeShowSlowConnectionWarning() override;
base::WeakPtr<ActionDelegate> GetWeakPtr() const override;
private:
......@@ -304,6 +306,7 @@ class ScriptExecutor : public ActionDelegate,
void Run();
void Terminate();
void SetTimeoutWarningCallback(base::OnceCallback<void()> timeout_warning);
private:
void Start();
......@@ -342,6 +345,8 @@ class ScriptExecutor : public ActionDelegate,
// the original area.
void RestorePreInterruptScroll();
void TimeoutWarning();
ScriptExecutor* main_script_;
ScriptExecutorDelegate* delegate_;
const base::TimeDelta max_wait_time_;
......@@ -350,6 +355,9 @@ class ScriptExecutor : public ActionDelegate,
base::OnceCallback<void(const ClientStatus&)>)>
check_elements_;
WaitForDomOperation::Callback callback_;
base::OnceCallback<void()> timeout_warning_callback_;
std::unique_ptr<base::OneShotTimer> warning_timer_;
base::TimeDelta timeout_warning_period_;
std::unique_ptr<BatchElementChecker> batch_element_checker_;
......@@ -449,6 +457,10 @@ class ScriptExecutor : public ActionDelegate,
// that they do not overwrite the paused state.
bool ShouldInterruptOnPause(const ActionProto& proto);
// Maybe shows the message specified in a callout, depending on the current
// state and client settings.
void MaybeShowSlowWarning(const std::string& message, bool enabled);
const std::string script_path_;
std::unique_ptr<TriggerContext> additional_context_;
std::string last_global_payload_;
......@@ -525,6 +537,9 @@ class ScriptExecutor : public ActionDelegate,
base::TimeTicks batch_start_time_;
RoundtripTimingStats roundtrip_timing_stats_;
bool warning_callout_already_shown_ = false;
int consecutive_slow_roundtrip_counter_ = 0;
base::WeakPtrFactory<ScriptExecutor> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(ScriptExecutor);
};
......
......@@ -192,6 +192,10 @@ class ScriptExecutorDelegate {
// calling |EnterState(BROWSE)| to take effect.
virtual void SetBrowseModeInvisible(bool invisible) = 0;
// Whether the slow connection or website warning should be shown. Depends on
// the state at the moment of the invocation.
virtual bool ShouldShowWarning() = 0;
protected:
virtual ~ScriptExecutorDelegate() {}
};
......
......@@ -264,6 +264,88 @@ TEST_F(ScriptExecutorTest, RunMultipleActions) {
EXPECT_EQ(1u, processed_actions2_capture.size());
}
ACTION_P2(Delay, env, delay) {
env->FastForwardBy(base::TimeDelta::FromMilliseconds(delay));
}
TEST_F(ScriptExecutorTest, ShowsSlowConnectionWarning) {
ClientSettings* client_settings = delegate_.GetMutableSettings();
client_settings->slow_connection_message = "slow";
client_settings->enable_slow_connection_warnings = true;
client_settings->max_consecutive_slow_roundtrips = 2;
ActionsResponseProto initial_actions_response;
initial_actions_response.add_actions()->mutable_tell()->set_message("1");
EXPECT_CALL(mock_service_, OnGetActions(StrEq(kScriptPath), _, _, _, _, _))
.WillOnce(DoAll(Delay(&task_environment_, 600),
RunOnceCallback<5>(net::HTTP_OK,
Serialize(initial_actions_response))));
ActionsResponseProto next_actions_response;
next_actions_response.add_actions()->mutable_tell()->set_message("2");
EXPECT_CALL(mock_service_, OnGetNextActions(_, _, _, _, _, _))
.WillOnce(DoAll(Delay(&task_environment_, 600),
RunOnceCallback<5>(net::HTTP_OK,
Serialize(initial_actions_response))))
.WillOnce(RunOnceCallback<5>(net::HTTP_OK, ""));
EXPECT_CALL(executor_callback_,
Run(Field(&ScriptExecutor::Result::success, true)));
executor_->Run(&user_data_, executor_callback_.Get());
EXPECT_EQ(delegate_.GetBubbleMessage(), "slow");
}
TEST_F(ScriptExecutorTest, SlowConnectionWarningNotShowingIfNotConsecutive) {
ClientSettings* client_settings = delegate_.GetMutableSettings();
client_settings->slow_connection_message = "slow";
client_settings->enable_slow_connection_warnings = true;
client_settings->max_consecutive_slow_roundtrips = 2;
ActionsResponseProto initial_actions_response;
initial_actions_response.add_actions()->mutable_tell()->set_message("1");
EXPECT_CALL(mock_service_, OnGetActions(StrEq(kScriptPath), _, _, _, _, _))
.WillOnce(DoAll(Delay(&task_environment_, 600),
RunOnceCallback<5>(net::HTTP_OK,
Serialize(initial_actions_response))));
ActionsResponseProto next_actions_response;
next_actions_response.add_actions()->mutable_tell()->set_message("2");
EXPECT_CALL(mock_service_, OnGetNextActions(_, _, _, _, _, _))
.WillOnce(DoAll(RunOnceCallback<5>(net::HTTP_OK,
Serialize(initial_actions_response))))
.WillOnce(DoAll(Delay(&task_environment_, 600),
RunOnceCallback<5>(net::HTTP_OK,
Serialize(initial_actions_response))))
.WillOnce(RunOnceCallback<5>(net::HTTP_OK, ""));
EXPECT_CALL(executor_callback_,
Run(Field(&ScriptExecutor::Result::success, true)));
executor_->Run(&user_data_, executor_callback_.Get());
EXPECT_NE(delegate_.GetBubbleMessage(), "slow");
}
TEST_F(ScriptExecutorTest, SlowConnectionWarningNotShowingIfOnCompleted) {
ClientSettings* client_settings = delegate_.GetMutableSettings();
client_settings->slow_connection_message = "slow";
client_settings->enable_slow_connection_warnings = true;
client_settings->max_consecutive_slow_roundtrips = 2;
ActionsResponseProto initial_actions_response;
initial_actions_response.add_actions()->mutable_tell()->set_message("1");
EXPECT_CALL(mock_service_, OnGetActions(StrEq(kScriptPath), _, _, _, _, _))
.WillOnce(DoAll(Delay(&task_environment_, 600),
RunOnceCallback<5>(net::HTTP_OK,
Serialize(initial_actions_response))));
EXPECT_CALL(mock_service_, OnGetNextActions(_, _, _, _, _, _))
.WillOnce(
RunOnceCallback<5>(net::HTTP_OK, Serialize(initial_actions_response)))
.WillOnce(DoAll(Delay(&task_environment_, 600),
RunOnceCallback<5>(net::HTTP_OK, "")));
EXPECT_CALL(executor_callback_,
Run(Field(&ScriptExecutor::Result::success, true)));
executor_->Run(&user_data_, executor_callback_.Get());
EXPECT_NE(delegate_.GetBubbleMessage(), "slow");
}
TEST_F(ScriptExecutorTest, UnsupportedAction) {
ActionsResponseProto actions_response;
actions_response.add_actions(); // action definition missing
......@@ -539,6 +621,27 @@ TEST_F(ScriptExecutorTest, WaitForDomWaitUntil) {
EXPECT_EQ(ACTION_APPLIED, processed_actions_capture[0].status());
}
TEST_F(ScriptExecutorTest, WaitForDomSlowWarning) {
ClientSettings* client_settings = delegate_.GetMutableSettings();
client_settings->slow_website_message = "slow";
client_settings->enable_slow_website_warnings = true;
ActionsResponseProto actions_response;
auto* wait_for_dom = actions_response.add_actions()->mutable_wait_for_dom();
*wait_for_dom->mutable_wait_condition()->mutable_match() =
ToSelectorProto("element");
EXPECT_CALL(mock_service_, OnGetActions(_, _, _, _, _, _))
.WillOnce(RunOnceCallback<5>(net::HTTP_OK, Serialize(actions_response)));
// First check does not find the element, wait for dom waits 1s, then the
// element is found, and the action succeeds.
EXPECT_CALL(mock_web_controller_, OnFindElement(Selector({"element"}), _))
.WillOnce(Delay(&task_environment_, 2000));
executor_->Run(&user_data_, executor_callback_.Get());
EXPECT_EQ(delegate_.GetBubbleMessage(), "slow");
}
TEST_F(ScriptExecutorTest, RunInterrupt) {
// All elements exist, so first the interrupt should be run, then the element
// should be reported as found.
......@@ -1636,10 +1739,6 @@ TEST_F(ScriptExecutorTest, PauseAndResumeWithOngoingAction) {
EXPECT_EQ(AutofillAssistantState::PROMPT, delegate_.GetState());
}
ACTION_P2(Delay, env, delay) {
env->FastForwardBy(base::TimeDelta::FromMilliseconds(delay));
}
TEST_F(ScriptExecutorTest, RoundtripTimingStats) {
ActionsResponseProto actions_response;
ActionProto* action = actions_response.add_actions();
......
......@@ -8,6 +8,7 @@
#include "base/test/gmock_callback_support.h"
#include "base/test/mock_callback.h"
#include "base/test/task_environment.h"
#include "components/autofill_assistant/browser/fake_script_executor_delegate.h"
#include "components/autofill_assistant/browser/protocol_utils.h"
#include "components/autofill_assistant/browser/script_executor_delegate.h"
......@@ -126,6 +127,10 @@ class ScriptTrackerTest : public testing::Test, public ScriptTracker::Listener {
return output;
}
// task_environment_ must be first to guarantee other field
// creation run in that environment.
base::test::TaskEnvironment task_environment_;
GURL url_;
NiceMock<MockService> mock_service_;
NiceMock<MockWebController> mock_web_controller_;
......
......@@ -155,6 +155,7 @@ message OverlayImageProto {
optional ClientDimensionProto text_size = 7;
}
// Next ID: 22
message ClientSettingsProto {
message IntegrationTestSettings {
// Disables animations for the poodle and the progress bar.
......@@ -231,6 +232,32 @@ message ClientSettingsProto {
}
optional BackButtonSettings back_button_settings = 19;
// Settings to define the behavior for showing warnings about slow connection
// and/or websites to users.
message SlowWarningSettings {
// Whether to show warnings related to a slow connection to the user.
optional bool enable_slow_connection_warnings = 1;
// Whether to show warnings related to a slow website to the user.
optional bool enable_slow_website_warnings = 2;
// If true, the slow connection or website warning will be only shown once.
optional bool show_only_once = 3;
// Defines the maximum wait on a dom find element operation before showing
// the slow website warning.
optional int32 warning_delay_ms = 4;
// Defines the threshold above which a roundtrip is considered too slow.
optional int32 slow_roundtrip_threshold_ms = 5;
// Defines the number of consecutive slow roundtrips allowed before showing
// the slow connection warning.
optional int32 max_consecutive_slow_roundtrips = 6;
// The message to show as a warning to inform the user of a slow connection.
// If this is not set, no warning will be shown in case of slow connection.
optional string slow_connection_message = 7;
// The message to show as a warning to inform the user of a slow website.
// If this is not set, no warning will be shown in case of a slow website.
optional string slow_website_message = 8;
}
optional SlowWarningSettings slow_warning_settings = 21;
reserved 8 to 11;
}
......
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