[Autofill Assistant] Resolve element once in SetFormFieldValueAction

Previously, each step resolved the element anew. Now the element is
resolved once and used throughout the action to perform actions on.

Bug: b/168107066
Change-Id: I4def429d89251cda5831cd5ca8268cf2bcf0ebab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485211
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/master@{#819805}

components/autofill_assistant/browser/actions/set_form_field_value_action.cc

@@ -13,6 +13,7 @@
 #include "components/autofill_assistant/browser/actions/action_delegate_util.h"
 #include "components/autofill_assistant/browser/client_status.h"
 #include "components/autofill_assistant/browser/user_data_util.h"
+#include "components/autofill_assistant/browser/web/element_finder.h"
 
 namespace autofill_assistant {
 namespace {
@@ -168,6 +169,20 @@ void SetFormFieldValueAction::OnWaitForElement(
     EndAction(element_status);
     return;
   }
+  delegate_->FindElement(selector_,
+                         base::BindOnce(&SetFormFieldValueAction::OnFindElement,
+                                        weak_ptr_factory_.GetWeakPtr()));
+}
+
+void SetFormFieldValueAction::OnFindElement(
+    const ClientStatus& element_status,
+    std::unique_ptr<ElementFinder::Result> element_result) {
+  if (!element_status.ok()) {
+    EndAction(element_status);
+    return;
+  }
+
+  element_ = std::move(element_result);
   SetFieldValueSequentially(/* field_index = */ 0, OkClientStatus());
 }
 
@@ -186,8 +201,8 @@ void SetFormFieldValueAction::SetFieldValueSequentially(
                      weak_ptr_factory_.GetWeakPtr(), field_index + 1);
   const auto& field_input = field_inputs_[field_index];
   if (field_input.keyboard_input) {
-    action_delegate_util::SendKeyboardInput(
-        delegate_, selector_, *field_input.keyboard_input, delay_in_millisecond,
+    action_delegate_util::PerformSendKeyboardInput(
+        delegate_, *field_input.keyboard_input, delay_in_millisecond, *element_,
         std::move(next_field_callback));
   } else if (field_input.password_type != PasswordValueType::NOT_SET) {
     switch (field_input.password_type) {
@@ -204,9 +219,9 @@ void SetFormFieldValueAction::SetFieldValueSequentially(
     }
   } else {
     auto fill_strategy = proto_.set_form_value().fill_strategy();
-    action_delegate_util::SetFieldValue(
-        delegate_, selector_, field_input.value, fill_strategy,
-        delay_in_millisecond,
+    action_delegate_util::PerformSetFieldValue(
+        delegate_, field_input.value, fill_strategy, delay_in_millisecond,
+        *element_,
         IsSimulatingKeyPresses(fill_strategy)
             ? std::move(next_field_callback)
             : base::BindOnce(
@@ -226,9 +241,9 @@ void SetFormFieldValueAction::OnGetStoredPassword(
     return;
   }
   auto fill_strategy = proto_.set_form_value().fill_strategy();
-  action_delegate_util::SetFieldValue(
-      delegate_, selector_, password, fill_strategy,
-      proto_.set_form_value().delay_in_millisecond(),
+  action_delegate_util::PerformSetFieldValue(
+      delegate_, password, fill_strategy,
+      proto_.set_form_value().delay_in_millisecond(), *element_,
       IsSimulatingKeyPresses(fill_strategy)
           ? std::move(next_field_callback)
           : base::BindOnce(
@@ -245,9 +260,8 @@ void SetFormFieldValueAction::OnSetFieldValueAndCheckFallback(
     EndAction(status);
     return;
   }
-  action_delegate_util::FindElementAndGetProperty(
-      delegate_, selector_,
-      base::BindOnce(&ActionDelegate::GetFieldValue, delegate_->GetWeakPtr()),
+  delegate_->GetFieldValue(
+      *element_,
       base::BindOnce(&SetFormFieldValueAction::OnGetFieldValue,
                      weak_ptr_factory_.GetWeakPtr(),
                      std::move(next_field_callback), requested_value));
@@ -273,9 +287,9 @@ void SetFormFieldValueAction::OnGetFieldValue(
 
     // Run |SetFieldValue| with keyboard simulation on and move on to next value
     // afterwards.
-    action_delegate_util::SetFieldValue(
-        delegate_, selector_, requested_value, SIMULATE_KEY_PRESSES,
-        proto_.set_form_value().delay_in_millisecond(),
+    action_delegate_util::PerformSetFieldValue(
+        delegate_, requested_value, SIMULATE_KEY_PRESSES,
+        proto_.set_form_value().delay_in_millisecond(), *element_,
         std::move(next_field_callback));
     return;
   }

components/autofill_assistant/browser/actions/set_form_field_value_action.h

@@ -15,6 +15,7 @@
 #include "components/autofill_assistant/browser/actions/action.h"
 #include "components/autofill_assistant/browser/string_conversions_util.h"
 #include "components/autofill_assistant/browser/user_data.h"
+#include "components/autofill_assistant/browser/web/element_finder.h"
 
 namespace autofill_assistant {
 
@@ -56,6 +57,8 @@ class SetFormFieldValueAction : public Action {
   void InternalProcessAction(ProcessActionCallback callback) override;
 
   void OnWaitForElement(const ClientStatus& element_status);
+  void OnFindElement(const ClientStatus& element_status,
+                     std::unique_ptr<ElementFinder::Result> element_result);
   void SetFieldValueSequentially(int field_index, const ClientStatus& status);
   void OnGetStoredPassword(
       base::OnceCallback<void(const ClientStatus&)> next_field_callback,
@@ -75,6 +78,7 @@ class SetFormFieldValueAction : public Action {
   void EndAction(const ClientStatus& status);
 
   Selector selector_;
+  std::unique_ptr<ElementFinder::Result> element_;
   std::vector<FieldInput> field_inputs_;
   ProcessActionCallback process_action_callback_;
   base::WeakPtrFactory<SetFormFieldValueAction> weak_ptr_factory_{this};

components/autofill_assistant/browser/actions/set_form_field_value_action_unittest.cc

@@ -145,7 +145,7 @@ TEST_F(SetFormFieldValueActionTest, Username) {
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
   const ElementFinder::Result& expected_element =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_, 2);
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(
       mock_action_delegate_,
       SetValueAttribute(kFakeUsername, EqualsElement(expected_element), _))
@@ -166,7 +166,7 @@ TEST_F(SetFormFieldValueActionTest, PasswordToFill) {
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
   const ElementFinder::Result& expected_element =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_, 2);
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(
       mock_action_delegate_,
       SetValueAttribute(kFakePassword, EqualsElement(expected_element), _))
@@ -265,7 +265,7 @@ TEST_F(SetFormFieldValueActionTest, Text) {
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
   const ElementFinder::Result& expected_element =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_, 2);
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(
       mock_action_delegate_,
       SetValueAttribute("SomeText𠜎", EqualsElement(expected_element), _))
@@ -353,13 +353,17 @@ TEST_F(SetFormFieldValueActionTest, MultipleValuesAndSimulateKeypress) {
   set_form_field_proto_->set_fill_strategy(SIMULATE_KEY_PRESSES);
 
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
+  const ElementFinder::Result& expected_element =
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(mock_action_delegate_,
-              OnSendKeyboardInput(UTF8ToUnicode("SomeText"), _, _, _))
+              OnSendKeyboardInput(UTF8ToUnicode("SomeText"), _,
+                                  EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<3>(OkClientStatus()));
   // The second entry, a deprecated keycode is transformed into a
   // field_input.keyboard_input.
   EXPECT_CALL(mock_action_delegate_,
-              OnSendKeyboardInput(std::vector<int>{13}, _, _, _))
+              OnSendKeyboardInput(std::vector<int>{13}, _,
+                                  EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<3>(OkClientStatus()));
 
   EXPECT_CALL(
@@ -377,7 +381,7 @@ TEST_F(SetFormFieldValueActionTest, ClientMemoryKey) {
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
   const ElementFinder::Result& expected_element =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_, 2);
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(
       mock_action_delegate_,
       SetValueAttribute("SomeText𠜎", EqualsElement(expected_element), _))
@@ -409,20 +413,15 @@ TEST_F(SetFormFieldValueActionTest, FallbackToSimulateKeystrokes) {
   value->set_text("123");
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
+  const ElementFinder::Result& expected_element =
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(mock_action_delegate_,
-              SetValueAttribute("123",
-                                EqualsElement(test_util::MockFindElement(
-                                    mock_action_delegate_, fake_selector_)),
-                                _))
+              SetValueAttribute("123", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
   EXPECT_CALL(mock_action_delegate_,
-              GetFieldValue(EqualsElement(test_util::MockFindElement(
-                                mock_action_delegate_, fake_selector_)),
-                            _))
+              GetFieldValue(EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()));
 
-  auto expected_element =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(mock_action_delegate_,
               SetValueAttribute("", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
@@ -448,20 +447,16 @@ TEST_F(SetFormFieldValueActionTest, FallbackForPassword) {
   value->set_use_password(true);
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
-  EXPECT_CALL(mock_action_delegate_,
-              SetValueAttribute(kFakePassword,
-                                EqualsElement(test_util::MockFindElement(
-                                    mock_action_delegate_, fake_selector_)),
-                                _))
+  const ElementFinder::Result& expected_element =
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
+  EXPECT_CALL(
+      mock_action_delegate_,
+      SetValueAttribute(kFakePassword, EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
   EXPECT_CALL(mock_action_delegate_,
-              GetFieldValue(EqualsElement(test_util::MockFindElement(
-                                mock_action_delegate_, fake_selector_)),
-                            _))
+              GetFieldValue(EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()));
 
-  auto expected_element =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(mock_action_delegate_,
               SetValueAttribute("", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
@@ -489,50 +484,40 @@ TEST_F(SetFormFieldValueActionTest, FallbackForMultipleValues) {
   enter->set_text("SomeOtherText");
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
+  const ElementFinder::Result& expected_element =
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
+
   // First value.
   EXPECT_CALL(mock_action_delegate_,
-              SetValueAttribute("SomeText",
-                                EqualsElement(test_util::MockFindElement(
-                                    mock_action_delegate_, fake_selector_)),
-                                _))
+              SetValueAttribute("SomeText", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
   EXPECT_CALL(mock_action_delegate_,
-              GetFieldValue(EqualsElement(test_util::MockFindElement(
-                                mock_action_delegate_, fake_selector_)),
-                            _))
+              GetFieldValue(EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()));
 
-  auto expected_element_1 =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(mock_action_delegate_,
-              SetValueAttribute("", EqualsElement(expected_element_1), _))
+              SetValueAttribute("", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
   EXPECT_CALL(mock_action_delegate_,
               OnSendKeyboardInput(UTF8ToUnicode("SomeText"), _,
-                                  EqualsElement(expected_element_1), _))
+                                  EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<3>(OkClientStatus()));
 
   // Second value.
-  EXPECT_CALL(mock_action_delegate_,
-              SetValueAttribute("SomeOtherText",
-                                EqualsElement(test_util::MockFindElement(
-                                    mock_action_delegate_, fake_selector_)),
-                                _))
+  EXPECT_CALL(
+      mock_action_delegate_,
+      SetValueAttribute("SomeOtherText", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
   EXPECT_CALL(mock_action_delegate_,
-              GetFieldValue(EqualsElement(test_util::MockFindElement(
-                                mock_action_delegate_, fake_selector_)),
-                            _))
+              GetFieldValue(EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()));
 
-  auto expected_element_2 =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(mock_action_delegate_,
-              SetValueAttribute("", EqualsElement(expected_element_2), _))
+              SetValueAttribute("", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));
   EXPECT_CALL(mock_action_delegate_,
               OnSendKeyboardInput(UTF8ToUnicode("SomeOtherText"), _,
-                                  EqualsElement(expected_element_2), _))
+                                  EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<3>(OkClientStatus()));
 
   EXPECT_CALL(callback_,
@@ -607,7 +592,7 @@ TEST_F(SetFormFieldValueActionTest, SetFieldFromProfileValue) {
   SetFormFieldValueAction action(&mock_action_delegate_, proto_);
 
   const ElementFinder::Result& expected_element =
-      test_util::MockFindElement(mock_action_delegate_, fake_selector_, 2);
+      test_util::MockFindElement(mock_action_delegate_, fake_selector_);
   EXPECT_CALL(mock_action_delegate_,
               SetValueAttribute("John", EqualsElement(expected_element), _))
       .WillOnce(RunOnceCallback<2>(OkClientStatus()));