[Autofill Assistant] Change slow warnings to use the status messages instead of the callouts.

Bug: b/175311081
Change-Id: Ibfce2d6df8033426fe54afee3ca511bcca13e948
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2642373
Commit-Queue: Michele Mancina <micantox@google.com>
Reviewed-by: Luca Hunkeler <hluca@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846181}

components/autofill_assistant/browser/client_settings.cc

@@ -118,7 +118,7 @@ void ClientSettings::UpdateFromProto(const ClientSettingsProto& proto) {
       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(
+      warning_delay = base::TimeDelta::FromMilliseconds(
           proto.slow_warning_settings().warning_delay_ms());
     }
     if (proto.slow_warning_settings().has_slow_roundtrip_threshold_ms()) {
@@ -137,6 +137,15 @@ void ClientSettings::UpdateFromProto(const ClientSettingsProto& proto) {
       slow_website_message =
           proto.slow_warning_settings().slow_website_message();
     }
+    if (proto.slow_warning_settings()
+            .has_minimum_warning_message_duration_ms()) {
+      minimum_warning_duration = base::TimeDelta::FromMilliseconds(
+          proto.slow_warning_settings().minimum_warning_message_duration_ms());
+    }
+    if (proto.slow_warning_settings().message_mode() !=
+        ClientSettingsProto::SlowWarningSettings::UNKNOWN) {
+      message_mode = proto.slow_warning_settings().message_mode();
+    }
   }
   // Test only settings.
   if (proto.has_integration_test_settings()) {

components/autofill_assistant/browser/client_settings.h

@@ -90,17 +90,45 @@ struct ClientSettings {
   // Snackbar.
   base::Optional<ClientSettingsProto::BackButtonSettings> back_button_settings;
 
+  // Whether to show warnings related to a slow connection to the user.
   bool enable_slow_connection_warnings = false;
+
+  // Whether to show warnings related to a slow website to the user.
   bool enable_slow_website_warnings = false;
-  bool only_show_warning_once = false;
-  base::TimeDelta timeout_warning_delay =
-      base::TimeDelta::FromMilliseconds(1000);
+
+  // If true, the slow connection or website warning will be only shown once.
+  bool only_show_warning_once = true;
+
+  // Defines the maximum wait on a dom find element operation before showing
+  // the slow website warning.
+  base::TimeDelta warning_delay = base::TimeDelta::FromMilliseconds(1500);
+
+  // Defines the number of consecutive slow roundtrips allowed before showing
+  // the slow connection warning.
   int max_consecutive_slow_roundtrips = 3;
+
+  // Defines the threshold above which a roundtrip is considered too slow.
   base::TimeDelta slow_roundtrip_threshold =
-      base::TimeDelta::FromMilliseconds(500);
+      base::TimeDelta::FromMilliseconds(1500);
+
+  // 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.
   std::string slow_connection_message = "";
+
+  // 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.
   std::string slow_website_message = "";
 
+  // The minimum duration that the message will be shown for (only applies to
+  // the slow connection messages).
+  base::TimeDelta minimum_warning_duration =
+      base::TimeDelta::FromMilliseconds(1500);
+
+  // Whether the warning message should replace the current status message or
+  // should be concatenated.
+  ClientSettingsProto::SlowWarningSettings::MessageMode message_mode =
+      ClientSettingsProto::SlowWarningSettings::REPLACE;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ClientSettings);
 };

components/autofill_assistant/browser/fake_script_executor_delegate.cc

@@ -78,6 +78,7 @@ void FakeScriptExecutorDelegate::SetTouchableElementArea(
     const ElementAreaProto& element) {}
 
 void FakeScriptExecutorDelegate::SetStatusMessage(const std::string& message) {
+  LOG(ERROR) << "SETTING STATUS MESSAGE TO: " << message;
   status_message_ = message;
 }
 

components/autofill_assistant/browser/script_executor.cc

@@ -313,9 +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->SetTimeoutWarningCallback(
+      base::BindOnce(&ScriptExecutor::MaybeShowSlowWebsiteWarning,
+                     weak_ptr_factory_.GetWeakPtr()));
   current_action_data_.wait_for_dom->Run();
 }
 
@@ -841,24 +841,50 @@ void ScriptExecutor::MaybeShowSlowWebsiteWarning() {
 }
 
 void ScriptExecutor::MaybeShowSlowConnectionWarning() {
-  MaybeShowSlowWarning(
+  base::TimeDelta delay = base::TimeDelta::FromMilliseconds(0);
+  bool warning_was_shown = MaybeShowSlowWarning(
       delegate_->GetSettings().slow_connection_message,
       delegate_->GetSettings().enable_slow_connection_warnings);
+  if (warning_was_shown) {
+    delay = delegate_->GetSettings().minimum_warning_duration;
+  }
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&ScriptExecutor::ProcessNextAction,
+                     weak_ptr_factory_.GetWeakPtr()),
+      delay);
 }
 
-void ScriptExecutor::MaybeShowSlowWarning(const std::string& message,
+bool ScriptExecutor::MaybeShowSlowWarning(const std::string& message,
                                           bool enabled) {
   if (message.empty() || !enabled || !delegate_->ShouldShowWarning()) {
-    return;
+    return false;
   }
 
   if (delegate_->GetSettings().only_show_warning_once &&
       warning_callout_already_shown_) {
-    return;
+    return false;
   }
 
   warning_callout_already_shown_ = true;
-  SetBubbleMessage(message);
+  const std::string& previous_message = GetStatusMessage();
+  switch (delegate_->GetSettings().message_mode) {
+    case ClientSettingsProto::SlowWarningSettings::CONCATENATE:
+      LOG(ERROR) << "CHECKING IF SHOULD CONCATENATE WARNING";
+      if (previous_message.find(message) == std::string::npos) {
+        LOG(ERROR) << "CONCATENATING WARNING";
+        SetStatusMessage(previous_message + message);
+      }
+      break;
+    case ClientSettingsProto::SlowWarningSettings::REPLACE:
+      LOG(ERROR) << "REPLACING WARNING";
+      SetStatusMessage(message);
+      break;
+    case ClientSettingsProto::SlowWarningSettings::UNKNOWN:
+      return false;
+  }
+
+  return true;
 }
 
 base::WeakPtr<ActionDelegate> ScriptExecutor::GetWeakPtr() const {
@@ -890,18 +916,29 @@ void ScriptExecutor::OnGetActions(base::TimeTicks start_time,
     return;
   }
 
+  if (roundtrip_duration < delegate_->GetSettings().slow_roundtrip_threshold) {
+    consecutive_slow_roundtrip_counter_ = 0;
+    LOG(ERROR) << "RESETTING COUNTER";
+  } else {
+    consecutive_slow_roundtrip_counter_++;
+    LOG(ERROR) << "COUNTER INCREASED TO "
+               << consecutive_slow_roundtrip_counter_;
+  }
+
   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();
-      }
+    if (consecutive_slow_roundtrip_counter_ >=
+        delegate_->GetSettings().max_consecutive_slow_roundtrips) {
+      LOG(ERROR) << "SHOWING WARNING";
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE,
+          base::BindOnce(&ScriptExecutor::MaybeShowSlowConnectionWarning,
+                         weak_ptr_factory_.GetWeakPtr()),
+          delegate_->GetSettings().minimum_warning_duration);
     } else {
-      consecutive_slow_roundtrip_counter_ = 0;
+      LOG(ERROR) << "NOT SHOWING WARNING";
+
+      ProcessNextAction();
     }
-    ProcessNextAction();
     return;
   }
 
@@ -1122,7 +1159,7 @@ ScriptExecutor::WaitForDomOperation::WaitForDomOperation(
       check_elements_(std::move(check_elements)),
       callback_(std::move(callback)),
       timeout_warning_period_(
-          main_script->delegate_->GetSettings().timeout_warning_delay),
+          main_script->delegate_->GetSettings().warning_delay),
       retry_timer_(main_script->delegate_->GetSettings()
                        .periodic_element_check_interval) {}
 

components/autofill_assistant/browser/script_executor.h

@@ -459,7 +459,7 @@ class ScriptExecutor : public ActionDelegate,
 
   // Maybe shows the message specified in a callout, depending on the current
   // state and client settings.
-  void MaybeShowSlowWarning(const std::string& message, bool enabled);
+  bool MaybeShowSlowWarning(const std::string& message, bool enabled);
 
   const std::string script_path_;
   std::unique_ptr<TriggerContext> additional_context_;

components/autofill_assistant/browser/script_executor_unittest.cc

@@ -268,11 +268,17 @@ ACTION_P2(Delay, env, delay) {
   env->FastForwardBy(base::TimeDelta::FromMilliseconds(delay));
 }
 
-TEST_F(ScriptExecutorTest, ShowsSlowConnectionWarning) {
+TEST_F(ScriptExecutorTest, ShowsSlowConnectionWarningReplace) {
   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;
+  client_settings->slow_roundtrip_threshold =
+      base::TimeDelta::FromMilliseconds(100);
+  client_settings->minimum_warning_duration =
+      base::TimeDelta::FromMilliseconds(100);
+  client_settings->message_mode =
+      ClientSettingsProto::SlowWarningSettings::REPLACE;
   ActionsResponseProto initial_actions_response;
   initial_actions_response.add_actions()->mutable_tell()->set_message("1");
   EXPECT_CALL(mock_service_, OnGetActions(StrEq(kScriptPath), _, _, _, _, _))
@@ -283,15 +289,56 @@ TEST_F(ScriptExecutorTest, ShowsSlowConnectionWarning) {
   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(next_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());
+  task_environment_.FastForwardBy(
+      task_environment_.NextMainThreadPendingTaskDelay());
+  EXPECT_EQ(delegate_.GetStatusMessage(), "slow");
+  task_environment_.FastForwardBy(
+      task_environment_.NextMainThreadPendingTaskDelay());
+  EXPECT_EQ(delegate_.GetStatusMessage(), "2");
+}
+
+TEST_F(ScriptExecutorTest, ShowsSlowConnectionWarningConcatenate) {
+  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;
+  client_settings->slow_roundtrip_threshold =
+      base::TimeDelta::FromMilliseconds(100);
+  client_settings->minimum_warning_duration =
+      base::TimeDelta::FromMilliseconds(100);
+  client_settings->message_mode =
+      ClientSettingsProto::SlowWarningSettings::CONCATENATE;
+  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))))
+                                         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(next_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");
+  task_environment_.FastForwardBy(
+      task_environment_.NextMainThreadPendingTaskDelay());
+  EXPECT_EQ(delegate_.GetStatusMessage(), "1... slow");
+  task_environment_.FastForwardBy(
+      task_environment_.NextMainThreadPendingTaskDelay());
+  EXPECT_EQ(delegate_.GetStatusMessage(), "2");
 }
 
 TEST_F(ScriptExecutorTest, SlowConnectionWarningNotShowingIfNotConsecutive) {
@@ -319,7 +366,7 @@ TEST_F(ScriptExecutorTest, SlowConnectionWarningNotShowingIfNotConsecutive) {
               Run(Field(&ScriptExecutor::Result::success, true)));
   executor_->Run(&user_data_, executor_callback_.Get());
 
-  EXPECT_NE(delegate_.GetBubbleMessage(), "slow");
+  EXPECT_NE(delegate_.GetStatusMessage(), "slow");
 }
 
 TEST_F(ScriptExecutorTest, SlowConnectionWarningNotShowingIfOnCompleted) {
@@ -343,7 +390,7 @@ TEST_F(ScriptExecutorTest, SlowConnectionWarningNotShowingIfOnCompleted) {
               Run(Field(&ScriptExecutor::Result::success, true)));
   executor_->Run(&user_data_, executor_callback_.Get());
 
-  EXPECT_NE(delegate_.GetBubbleMessage(), "slow");
+  EXPECT_NE(delegate_.GetStatusMessage(), "slow");
 }
 
 TEST_F(ScriptExecutorTest, UnsupportedAction) {
@@ -621,11 +668,36 @@ TEST_F(ScriptExecutorTest, WaitForDomWaitUntil) {
   EXPECT_EQ(ACTION_APPLIED, processed_actions_capture[0].status());
 }
 
-TEST_F(ScriptExecutorTest, WaitForDomSlowWarning) {
+TEST_F(ScriptExecutorTest, WaitForDomSlowWarningReplace) {
   ClientSettings* client_settings = delegate_.GetMutableSettings();
   client_settings->slow_website_message = "slow";
   client_settings->enable_slow_website_warnings = true;
   ActionsResponseProto actions_response;
+  actions_response.add_actions()->mutable_tell()->set_message("1");
+  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_.GetStatusMessage(), "slow");
+}
+
+TEST_F(ScriptExecutorTest, WaitForDomSlowWarningConcatenate) {
+  ClientSettings* client_settings = delegate_.GetMutableSettings();
+  client_settings->slow_website_message = "... slow";
+  client_settings->enable_slow_website_warnings = true;
+  client_settings->message_mode =
+      ClientSettingsProto::SlowWarningSettings::CONCATENATE;
+  ActionsResponseProto actions_response;
+  actions_response.add_actions()->mutable_tell()->set_message("1");
   auto* wait_for_dom = actions_response.add_actions()->mutable_wait_for_dom();
   *wait_for_dom->mutable_wait_condition()->mutable_match() =
       ToSelectorProto("element");
@@ -639,7 +711,7 @@ TEST_F(ScriptExecutorTest, WaitForDomSlowWarning) {
       .WillOnce(Delay(&task_environment_, 2000));
   executor_->Run(&user_data_, executor_callback_.Get());
 
-  EXPECT_EQ(delegate_.GetBubbleMessage(), "slow");
+  EXPECT_EQ(delegate_.GetStatusMessage(), "1... slow");
 }
 
 TEST_F(ScriptExecutorTest, RunInterrupt) {

components/autofill_assistant/browser/service.proto

@@ -235,6 +235,13 @@ message ClientSettingsProto {
   // Settings to define the behavior for showing warnings about slow connection
   // and/or websites to users.
   message SlowWarningSettings {
+    enum MessageMode {
+      UNKNOWN = 0;
+      // The warning message will be appended to the existing status message.
+      CONCATENATE = 1;
+      // The warning message will replace the current status message.
+      REPLACE = 2;
+    }
     // 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.
@@ -255,6 +262,12 @@ message ClientSettingsProto {
     // 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;
+    // The minimum duration that the message will be shown for (only applies to
+    // the slow connection messages).
+    optional int32 minimum_warning_message_duration_ms = 9;
+    // Whether the warning message should replace the current status message or
+    // should be concatenated.
+    optional MessageMode message_mode = 10;
   }
   optional SlowWarningSettings slow_warning_settings = 21;