[Autofill] ImportFromSection returns true if profile is importable.

Currently, the return value is conditions on the actual import of
an address. However, the import can only fail if the incognito mode is
enabled, which is already checked beforehand. Therefore, the return
value for an importable profile is set to be always true if the profile
is importable to support a consistent behavior with future asynchronous
import flows.

Bug: 1135178
Change-Id: I7f062151706919439c82a6bd150ec8be7f219f1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632963
Commit-Queue: Matthias Körber <koerber@google.com>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845736}

components/autofill/core/browser/address_profiles/address_profile_save_manager.cc

@@ -17,9 +17,9 @@ AddressProfileSaveManager::AddressProfileSaveManager(
 
 AddressProfileSaveManager::~AddressProfileSaveManager() = default;
 
-bool AddressProfileSaveManager::SaveProfile(const AutofillProfile& profile) {
+void AddressProfileSaveManager::SaveProfile(const AutofillProfile& profile) {
   if (!personal_data_manager_)
-    return false;
+    return;
 
   if (base::FeatureList::IsEnabled(
           features::kAutofillAddressProfileSavePrompt)) {
@@ -27,18 +27,14 @@ bool AddressProfileSaveManager::SaveProfile(const AutofillProfile& profile) {
         profile,
         base::BindOnce(&AddressProfileSaveManager::SaveProfilePromptCallback,
                        weak_ptr_factory_.GetWeakPtr()));
-    // TODO(crbug.com/1135178): The semantics of the return value are that the
-    // profile has been saved. At this point, we cannot tell if the profile is
-    // going to be saved or not since it depends on the user action. Revisit
-    // once the intended behavior of save prompts is final.
-    return true;
+    return;
   }
-  return SaveProfileInternal(profile);
+  SaveProfileInternal(profile);
 }
 
-bool AddressProfileSaveManager::SaveProfileInternal(
+void AddressProfileSaveManager::SaveProfileInternal(
     const AutofillProfile& profile) {
-  return !personal_data_manager_->SaveImportedProfile(profile).empty();
+  personal_data_manager_->SaveImportedProfile(profile);
 }
 
 void AddressProfileSaveManager::SaveProfilePromptCallback(

components/autofill/core/browser/address_profiles/address_profile_save_manager.h

@@ -27,16 +27,14 @@ class AddressProfileSaveManager {
       delete;
   virtual ~AddressProfileSaveManager();
 
-  // Saves `profile` using the `personal_data_manager_`. Returns true
-  // if the profile was saved or updated, and false if no profile was
-  // saved.
-  bool SaveProfile(const AutofillProfile& profile);
+  // Saves `profile` using the `personal_data_manager_`.
+  void SaveProfile(const AutofillProfile& profile);
 
  private:
   void SaveProfilePromptCallback(
       AutofillClient::SaveAddressProfileOfferUserDecision user_decision,
       AutofillProfile profile);
-  bool SaveProfileInternal(const AutofillProfile& profile);
+  void SaveProfileInternal(const AutofillProfile& profile);
 
   AutofillClient* const client_;
 

components/autofill/core/browser/autofill_manager.cc

@@ -754,10 +754,6 @@ void AutofillManager::OnFormSubmittedImpl(const FormData& form,
     return;
   }
 
-  if (base::FeatureList::IsEnabled(features::kAutofillDisableImport)) {
-    return;
-  }
-
   // Update Personal Data with the form's submitted data.
   // Also triggers offering local/upload credit card save, if applicable.
   client_->GetFormDataImporter()->ImportFormData(*submitted_form,

components/autofill/core/browser/form_data_importer.cc

@@ -433,7 +433,10 @@ bool FormDataImporter::ImportFormData(
   // - ImportAddressProfiles may eventually save or update one or more address
   //   profiles.
   bool address_import = false;
-  if (profile_autofill_enabled) {
+
+  // Only import addresses if enabled.
+  if (profile_autofill_enabled &&
+      !base::FeatureList::IsEnabled(features::kAutofillDisableAddressImport)) {
     address_import = ImportAddressProfiles(submitted_form);
   }
 
@@ -455,7 +458,7 @@ bool FormDataImporter::ImportAddressProfiles(const FormStructure& form) {
   // We save a maximum of 2 profiles per submitted form (e.g. for shipping and
   // billing).
   static const size_t kMaxNumAddressProfilesSaved = 2;
-  size_t num_saved_profiles = 0;
+  size_t num_complete_profiles = 0;
 
   if (!form.field_count()) {
     import_log_buffer << LogMessage::kImportAddressProfileFromFormFailed
@@ -469,7 +472,7 @@ bool FormDataImporter::ImportAddressProfiles(const FormStructure& form) {
     }
 
     for (const std::string& section : sections) {
-      if (num_saved_profiles == kMaxNumAddressProfilesSaved)
+      if (num_complete_profiles == kMaxNumAddressProfilesSaved)
         break;
       // Log the output from a section in a separate div for readability.
       import_log_buffer << Tag{"div"}
@@ -478,38 +481,38 @@ bool FormDataImporter::ImportAddressProfiles(const FormStructure& form) {
                         << section << CTag{};
       // Try to import an address profile from the form fields of this section.
       if (ImportAddressProfileForSection(form, section, &import_log_buffer))
-        num_saved_profiles++;
+        num_complete_profiles++;
       // And close the div of the section import log.
       import_log_buffer << CTag{"div"};
     }
     // Run the import on the union of the section if the import was not
     // successful and if there is more than one section.
-    if (num_saved_profiles > 0) {
+    if (num_complete_profiles > 0) {
       AutofillMetrics::LogAddressFormImportStatustMetric(
           AutofillMetrics::AddressProfileImportStatusMetric::REGULAR_IMPORT);
     } else if (sections.size() > 1) {
       // Try to import by combining all sections.
       if (ImportAddressProfileForSection(form, "", &import_log_buffer)) {
-        num_saved_profiles++;
+        num_complete_profiles++;
         AutofillMetrics::LogAddressFormImportStatustMetric(
             AutofillMetrics::AddressProfileImportStatusMetric::
                 SECTION_UNION_IMPORT);
       }
     }
-    if (num_saved_profiles == 0) {
+    if (num_complete_profiles == 0) {
       AutofillMetrics::LogAddressFormImportStatustMetric(
           AutofillMetrics::AddressProfileImportStatusMetric::NO_IMPORT);
     }
   }
   import_log_buffer << LogMessage::kImportAddressProfileFromFormNumberOfImports
-                    << num_saved_profiles << CTag{};
+                    << num_complete_profiles << CTag{};
 
   // Write log buffer to autofill-internals.
   LogManager* log_manager = client_->GetLogManager();
   if (log_manager)
     log_manager->Log() << std::move(import_log_buffer);
 
-  return num_saved_profiles > 0;
+  return num_complete_profiles > 0;
 }
 
 bool FormDataImporter::ImportAddressProfileForSection(
@@ -716,7 +719,13 @@ bool FormDataImporter::ImportAddressProfileForSection(
   if (!candidate_profile.FinalizeAfterImport())
     return false;
 
-  return address_profile_save_manager_->SaveProfile(candidate_profile);
+  // At this stage, the saving of the profile can only be omitted by the
+  // incognito mode but the import is not triggered if the browser is in the
+  // incognito mode.
+  DCHECK(!personal_data_manager_->IsOffTheRecord());
+  address_profile_save_manager_->SaveProfile(candidate_profile);
+
+  return true;
 }
 
 bool FormDataImporter::ImportCreditCard(

components/autofill/core/browser/form_data_importer.h

@@ -112,7 +112,8 @@ class FormDataImporter {
   // Go through the |form| fields and attempt to extract and import valid
   // address profiles. Returns true on extraction success of at least one
   // profile. There are many reasons that extraction may fail (see
-  // implementation).
+  // implementation). The function returns true if at least one complete address
+  // profile was found.
   bool ImportAddressProfiles(const FormStructure& form);
 
   // Helper method for ImportAddressProfiles which only considers the fields for

components/autofill/core/browser/personal_data_manager.h

@@ -426,6 +426,9 @@ class PersonalDataManager : public KeyedService,
     client_profile_validator_ = validator;
   }
 
+  // Returns true if the PDM is in the off-the-record mode.
+  bool IsOffTheRecord() { return is_off_the_record_; }
+
  protected:
   // Only PersonalDataManagerFactory and certain tests can create instances of
   // PersonalDataManager.

components/autofill/core/common/autofill_features.cc

@@ -58,9 +58,9 @@ const base::Feature kAutofillCreateDataForTest{
 const base::Feature kAutofillDisableFilling{"AutofillDisableFilling",
                                             base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Kill switch for Autofill import.
-const base::Feature kAutofillDisableImport{"AutofillDisableImport",
-                                           base::FEATURE_DISABLED_BY_DEFAULT};
+// Kill switch for Autofill address import.
+const base::Feature kAutofillDisableAddressImport{
+    "AutofillDisableAddressImport", base::FEATURE_DISABLED_BY_DEFAULT};
 
 // Controls if Chrome support filling and importing apartment numbers.
 // TODO(crbug.com/1153715): Remove once launched.

components/autofill/core/common/autofill_features.h

@@ -28,7 +28,7 @@ extern const base::Feature kAutofillAllowNonHttpActivation;
 extern const base::Feature kAutofillCacheQueryResponses;
 extern const base::Feature kAutofillCreateDataForTest;
 extern const base::Feature kAutofillDisableFilling;
-extern const base::Feature kAutofillDisableImport;
+extern const base::Feature kAutofillDisableAddressImport;
 extern const base::Feature kAutofillEnableAccountWalletStorage;
 extern const base::Feature kAutofillEnableAugmentedPhoneCountryCode;
 extern const base::Feature kAutofillEnableDependentLocalityParsing;