[Autofill Assistant] Split AutofillAction into UseAddress and UseCard.

This CL splits the current AutofillAction into UseAddressAction and
UseCreditCardAction. Also splitting the unit tests.

Also fixing a small issue in client memory when checking
has_selected_address for an address key that exists but the contents
are a nullptr.

Pending a next CL: refactor the fallback and required fields to share
the logic. Right now after the split, the code will be temporarily
duplicated.

Bug: b/141362833
Change-Id: I34fae5e1234695e679e571ea0e888c2b0e6230bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1855960
Commit-Queue: Marian Fechete <marianfe@google.com>
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/master@{#705504}

components/autofill_assistant/browser/BUILD.gn

@@ -34,8 +34,6 @@ jumbo_static_library("browser") {
     "actions/action.cc",
     "actions/action.h",
     "actions/action_delegate.h",
-    "actions/autofill_action.cc",
-    "actions/autofill_action.h",
     "actions/click_action.cc",
     "actions/click_action.h",
     "actions/collect_user_data_action.cc",
@@ -78,6 +76,10 @@ jumbo_static_library("browser") {
     "actions/unsupported_action.h",
     "actions/upload_dom_action.cc",
     "actions/upload_dom_action.h",
+    "actions/use_address_action.cc",
+    "actions/use_address_action.h",
+    "actions/use_credit_card_action.cc",
+    "actions/use_credit_card_action.h",
     "actions/wait_for_document_action.cc",
     "actions/wait_for_document_action.h",
     "actions/wait_for_dom_action.cc",
@@ -185,7 +187,6 @@ jumbo_static_library("browser") {
 source_set("unit_tests") {
   testonly = true
   sources = [
-    "actions/autofill_action_unittest.cc",
     "actions/collect_user_data_action_unittest.cc",
     "actions/configure_bottom_sheet_action_unittest.cc",
     "actions/mock_action_delegate.cc",
@@ -194,6 +195,8 @@ source_set("unit_tests") {
     "actions/prompt_action_unittest.cc",
     "actions/set_form_field_value_action_unittest.cc",
     "actions/show_details_action_unittest.cc",
+    "actions/use_address_action_unittest.cc",
+    "actions/use_credit_card_action_unittest.cc",
     "actions/wait_for_document_action_unittest.cc",
     "actions/wait_for_dom_action_unittest.cc",
     "batch_element_checker_unittest.cc",

components/autofill_assistant/browser/actions/use_address_action.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_USE_ADDRESS_ACTION_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_USE_ADDRESS_ACTION_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "components/autofill_assistant/browser/actions/action.h"
+#include "components/autofill_assistant/browser/batch_element_checker.h"
+
+namespace autofill {
+class AutofillProfile;
+}  // namespace autofill
+
+namespace autofill_assistant {
+class ClientStatus;
+
+// An action to autofill a form using a local address.
+class UseAddressAction : public Action {
+ public:
+  explicit UseAddressAction(ActionDelegate* delegate, const ActionProto& proto);
+  ~UseAddressAction() override;
+
+ private:
+  enum FieldValueStatus { UNKNOWN, EMPTY, NOT_EMPTY };
+  struct RequiredField {
+    Selector selector;
+    bool simulate_key_presses = false;
+    int delay_in_millisecond = 0;
+    bool forced = false;
+    FieldValueStatus status = UNKNOWN;
+
+    // When filling in address, address_field must be set.
+    UseAddressProto::RequiredField::AddressField address_field =
+        UseAddressProto::RequiredField::UNDEFINED;
+
+    // Returns true if fallback is required for this field.
+    bool ShouldFallback(bool has_fallback_data) const {
+      return status == EMPTY || (forced && has_fallback_data);
+    }
+  };
+
+  // Data necessary for filling in the fallback fields. This is kept in a
+  // separate struct to make sure we don't keep it for longer than strictly
+  // necessary.
+  struct FallbackData {
+    FallbackData() = default;
+    ~FallbackData() = default;
+
+    // Profile for UseAddress fallback.
+    const autofill::AutofillProfile* profile = nullptr;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(FallbackData);
+  };
+
+  // Overrides Action:
+  void InternalProcessAction(ProcessActionCallback callback) override;
+
+  void EndAction(ProcessedActionStatusProto status);
+  void EndAction(const ClientStatus& status);
+
+  // Fill the form using data in client memory. Return whether filling succeeded
+  // or not through OnFormFilled.
+  void FillFormWithData();
+  void OnWaitForElement(bool element_found);
+
+  // Called when the address has been filled.
+  void OnFormFilled(std::unique_ptr<FallbackData> fallback_data,
+                    const ClientStatus& status);
+
+  // Check whether all required fields have a non-empty value. If it is the
+  // case, finish the action successfully. If it's not and |fallback_data|
+  // is null, fail the action. If |fallback_data| is non-null, use it to attempt
+  // to fill the failed fields without Autofill.
+  void CheckRequiredFields(std::unique_ptr<FallbackData> fallback_data);
+
+  // Triggers the check for a specific field.
+  void CheckRequiredFieldsSequentially(
+      bool allow_fallback,
+      size_t required_fields_index,
+      std::unique_ptr<FallbackData> fallback_data);
+
+  // Updates |required_fields_value_status_|.
+  void OnGetRequiredFieldValue(size_t required_fields_index,
+                               bool exists,
+                               const std::string& value);
+
+  // Called when all required fields have been checked.
+  void OnCheckRequiredFieldsDone(std::unique_ptr<FallbackData> fallback_data);
+
+  // Gets the fallback value.
+  std::string GetFallbackValue(const RequiredField& required_field,
+                               const FallbackData& fallback_data);
+
+  // Get the value of |address_field| associated to profile |profile|. Return
+  // empty string if there is no data available.
+  base::string16 GetAddressFieldValue(
+      const autofill::AutofillProfile* profile,
+      const UseAddressProto::RequiredField::AddressField& address_field);
+
+  // Sets fallback field values for empty fields from
+  // |required_fields_value_status_|.
+  void SetFallbackFieldValuesSequentially(
+      size_t required_fields_index,
+      std::unique_ptr<FallbackData> fallback_data);
+
+  // Called after trying to set form values without Autofill in case of fallback
+  // after failed validation.
+  void OnSetFallbackFieldValue(size_t required_fields_index,
+                               std::unique_ptr<FallbackData> fallback_data,
+                               const ClientStatus& status);
+
+  // Usage of the autofilled address.
+  std::string name_;
+  std::string prompt_;
+  Selector selector_;
+
+  std::vector<RequiredField> required_fields_;
+
+  std::unique_ptr<BatchElementChecker> batch_element_checker_;
+
+  ProcessActionCallback process_action_callback_;
+  base::WeakPtrFactory<UseAddressAction> weak_ptr_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(UseAddressAction);
+};
+
+}  // namespace autofill_assistant
+#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_AUTOFILL_ACTION_H_

components/autofill_assistant/browser/actions/autofill_action.h → components/autofill_assistant/browser/actions/use_credit_card_action.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_AUTOFILL_ACTION_H_
-#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_AUTOFILL_ACTION_H_
+#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_USE_CREDIT_CARD_ACTION_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_USE_CREDIT_CARD_ACTION_H_
 
 #include <memory>
 #include <string>
@@ -16,18 +16,18 @@
 #include "components/autofill_assistant/browser/batch_element_checker.h"
 
 namespace autofill {
-class AutofillProfile;
 class CreditCard;
 }  // namespace autofill
 
 namespace autofill_assistant {
 class ClientStatus;
 
-// An action to autofill a form using a local address or credit card.
-class AutofillAction : public Action {
+// An action to autofill a form using a credit card.
+class UseCreditCardAction : public Action {
  public:
-  explicit AutofillAction(ActionDelegate* delegate, const ActionProto& proto);
-  ~AutofillAction() override;
+  explicit UseCreditCardAction(ActionDelegate* delegate,
+                               const ActionProto& proto);
+  ~UseCreditCardAction() override;
 
  private:
   enum FieldValueStatus { UNKNOWN, EMPTY, NOT_EMPTY };
@@ -38,12 +38,9 @@ class AutofillAction : public Action {
     bool forced = false;
     FieldValueStatus status = UNKNOWN;
 
-    // When filling in credit card, card_field must be set. When filling in
-    // address, address_field must be set.
+    // When filling in credit card, card_field must be set.
     UseCreditCardProto::RequiredField::CardField card_field =
         UseCreditCardProto::RequiredField::UNDEFINED;
-    UseAddressProto::RequiredField::AddressField address_field =
-        UseAddressProto::RequiredField::UNDEFINED;
 
     // Returns true if fallback is required for this field.
     bool ShouldFallback(bool has_fallback_data) const {
@@ -58,9 +55,6 @@ class AutofillAction : public Action {
     FallbackData() = default;
     ~FallbackData() = default;
 
-    // Profile for UseAddress fallback.
-    const autofill::AutofillProfile* profile = nullptr;
-
     // Card information for UseCreditCard fallback.
     std::string cvc;
     int expiration_year = 0;
@@ -77,7 +71,7 @@ class AutofillAction : public Action {
   void EndAction(const ClientStatus& status);
 
   // Fill the form using data in client memory. Return whether filling succeeded
-  // or not through OnAddressFormFilled or OnCardFormFilled.
+  // or not through OnFormFilled.
   void FillFormWithData();
   void OnWaitForElement(bool element_found);
 
@@ -85,7 +79,7 @@ class AutofillAction : public Action {
   void OnGetFullCard(std::unique_ptr<autofill::CreditCard> card,
                      const base::string16& cvc);
 
-  // Called when the form, credit card or address, has been filled.
+  // Called when the form credit card has been filled.
   void OnFormFilled(std::unique_ptr<FallbackData> fallback_data,
                     const ClientStatus& status);
 
@@ -119,12 +113,6 @@ class AutofillAction : public Action {
       UseCreditCardProto::RequiredField::CardField field,
       const FallbackData& fallback_data);
 
-  // Get the value of |address_field| associated to profile |profile|. Return
-  // empty string if there is no data available.
-  base::string16 GetAddressFieldValue(
-      const autofill::AutofillProfile* profile,
-      const UseAddressProto::RequiredField::AddressField& address_field);
-
   // Sets fallback field values for empty fields from
   // |required_fields_value_status_|.
   void SetFallbackFieldValuesSequentially(
@@ -137,21 +125,17 @@ class AutofillAction : public Action {
                                std::unique_ptr<FallbackData> fallback_data,
                                const ClientStatus& status);
 
-  // Usage of the autofilled address. Ignored if autofilling a card.
-  std::string name_;
   std::string prompt_;
   Selector selector_;
 
-  // True if autofilling a card, otherwise we are autofilling an address.
-  bool is_autofill_card_;
   std::vector<RequiredField> required_fields_;
 
   std::unique_ptr<BatchElementChecker> batch_element_checker_;
 
   ProcessActionCallback process_action_callback_;
-  base::WeakPtrFactory<AutofillAction> weak_ptr_factory_{this};
+  base::WeakPtrFactory<UseCreditCardAction> weak_ptr_factory_{this};
 
-  DISALLOW_COPY_AND_ASSIGN(AutofillAction);
+  DISALLOW_COPY_AND_ASSIGN(UseCreditCardAction);
 };
 
 }  // namespace autofill_assistant

components/autofill_assistant/browser/client_memory.cc

@@ -20,7 +20,7 @@ const autofill::CreditCard* ClientMemory::selected_card() const {
 }
 
 bool ClientMemory::has_selected_card() const {
-  return selected_card_.has_value();
+  return selected_card() != nullptr;
 }
 
 const autofill::AutofillProfile* ClientMemory::selected_address(
@@ -44,7 +44,7 @@ void ClientMemory::set_selected_address(
 }
 
 bool ClientMemory::has_selected_address(const std::string& name) const {
-  return selected_addresses_.find(name) != selected_addresses_.end();
+  return selected_address(name) != nullptr;
 }
 
 void ClientMemory::set_selected_login(const WebsiteLoginFetcher::Login& login) {
@@ -52,7 +52,7 @@ void ClientMemory::set_selected_login(const WebsiteLoginFetcher::Login& login) {
 }
 
 bool ClientMemory::has_selected_login() const {
-  return selected_login_.has_value();
+  return selected_login() != nullptr;
 }
 
 const WebsiteLoginFetcher::Login* ClientMemory::selected_login() const {

components/autofill_assistant/browser/protocol_utils.cc

@@ -8,7 +8,6 @@
 
 #include "base/feature_list.h"
 #include "base/logging.h"
-#include "components/autofill_assistant/browser/actions/autofill_action.h"
 #include "components/autofill_assistant/browser/actions/click_action.h"
 #include "components/autofill_assistant/browser/actions/collect_user_data_action.h"
 #include "components/autofill_assistant/browser/actions/configure_bottom_sheet_action.h"
@@ -30,6 +29,8 @@
 #include "components/autofill_assistant/browser/actions/tell_action.h"
 #include "components/autofill_assistant/browser/actions/unsupported_action.h"
 #include "components/autofill_assistant/browser/actions/upload_dom_action.h"
+#include "components/autofill_assistant/browser/actions/use_address_action.h"
+#include "components/autofill_assistant/browser/actions/use_credit_card_action.h"
 #include "components/autofill_assistant/browser/actions/wait_for_document_action.h"
 #include "components/autofill_assistant/browser/actions/wait_for_dom_action.h"
 #include "components/autofill_assistant/browser/actions/wait_for_navigation_action.h"
@@ -209,8 +210,10 @@ bool ProtocolUtils::ParseActions(ActionDelegate* delegate,
         break;
       }
       case ActionProto::ActionInfoCase::kUseAddress:
+        client_action = std::make_unique<UseAddressAction>(delegate, action);
+        break;
       case ActionProto::ActionInfoCase::kUseCard: {
-        client_action = std::make_unique<AutofillAction>(delegate, action);
+        client_action = std::make_unique<UseCreditCardAction>(delegate, action);
         break;
       }
       case ActionProto::ActionInfoCase::kWaitForDom: {