[Autofill] Refactored FormStructure::SectionenedFieldIndexes.

This CL refactors FormStructure::SectionenedFieldIndexes in some ways:
- it moves the class from the private part of the header to the body;
- it eliminates the std::vector copies in CurrentSection() and
  CurrentIndex();
- it eliminates the use of pointer arithmetic in
  FormStructure::RationalizeRepeatedFields();
- it eliminates casts of -1 to size_t, which is undefined behaviour;
- it names the member variables in a style-conformant way.

Bug: 1007974
Change-Id: Id6580c7962abd9d69bdb5c1f5a7b1bed3f9759c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587061
Auto-Submit: Christoph Schwering <schwering@google.com>
Commit-Queue: Matthias Körber <koerber@google.com>
Reviewed-by: Matthias Körber <koerber@google.com>
Cr-Commit-Position: refs/heads/master@{#836782}

components/autofill/core/browser/form_structure.cc

@@ -592,6 +592,55 @@ GetTypeRelationshipMap() {
 
 }  // namespace
 
+class FormStructure::SectionedFieldsIndexes {
+ public:
+  SectionedFieldsIndexes() = default;
+  ~SectionedFieldsIndexes() = default;
+
+  size_t LastFieldIndex() const {
+    if (sectioned_indexes_.empty())
+      return std::numeric_limits<size_t>::max();  // Shouldn't happen.
+    return sectioned_indexes_.back().back();
+  }
+
+  void AddFieldIndex(const size_t index, bool is_new_section) {
+    if (is_new_section || Empty()) {
+      sectioned_indexes_.emplace_back();
+    }
+    sectioned_indexes_.back().push_back(index);
+  }
+
+  void WalkForwardToTheNextSection() { current_section_ptr_++; }
+
+  bool IsFinished() const {
+    return current_section_ptr_ >= sectioned_indexes_.size();
+  }
+
+  size_t CurrentIndex() const {
+    return current_section_ptr_ < sectioned_indexes_.size()
+               ? sectioned_indexes_[current_section_ptr_].front()
+               : std::numeric_limits<size_t>::max();
+  }
+
+  const std::vector<size_t>* CurrentSection() const {
+    return current_section_ptr_ < sectioned_indexes_.size()
+               ? &sectioned_indexes_[current_section_ptr_]
+               : nullptr;
+  }
+
+  void Reset() { current_section_ptr_ = 0; }
+
+  bool Empty() const { return sectioned_indexes_.empty(); }
+
+ private:
+  // A vector of sections. Each section is a vector of some of the indexes
+  // that belong to the same section. The sections and indexes are sorted by
+  // their order of appearance on the form.
+  std::vector<std::vector<size_t>> sectioned_indexes_;
+  // Points to a vector of indexes that belong to the same section.
+  size_t current_section_ptr_ = 0;
+};
+
 FormStructure::FormStructure(const FormData& form)
     : id_attribute_(form.id_attribute),
       name_attribute_(form.name_attribute),
@@ -1480,17 +1529,13 @@ FormData FormStructure::ToFormData() const {
   data.is_formless_checkout = is_formless_checkout_;
   data.unique_renderer_id = unique_renderer_id_;
 
-  for (size_t i = 0; i < fields_.size(); ++i) {
-    data.fields.push_back(FormFieldData(*fields_[i]));
+  for (const auto& field : fields_) {
+    data.fields.push_back(*field);
   }
 
   return data;
 }
 
-FormStructure::SectionedFieldsIndexes::SectionedFieldsIndexes() {}
-
-FormStructure::SectionedFieldsIndexes::~SectionedFieldsIndexes() {}
-
 void FormStructure::RationalizeCreditCardFieldPredictions() {
   bool cc_first_name_found = false;
   bool cc_last_name_found = false;
@@ -1668,16 +1713,17 @@ void FormStructure::RationalizeAddressLineFields(
   for (sections_of_address_indexes->Reset();
        !sections_of_address_indexes->IsFinished();
        sections_of_address_indexes->WalkForwardToTheNextSection()) {
-    auto current_section = sections_of_address_indexes->CurrentSection();
+    auto* current_section = sections_of_address_indexes->CurrentSection();
 
     // The rationalization only applies to sections that have 2 or 3 visible
     // street address predictions.
-    if (current_section.size() != 2 && current_section.size() != 3) {
+    if (!current_section ||
+        (current_section->size() != 2 && current_section->size() != 3)) {
       continue;
     }
 
     int nb_address_rationalized = 0;
-    for (auto field_index : current_section) {
+    for (auto field_index : *current_section) {
       switch (nb_address_rationalized) {
         case 0:
           ApplyRationalizationsToFieldAndLog(field_index, ADDRESS_HOME_LINE1,
@@ -1808,10 +1854,6 @@ void FormStructure::RationalizeAddressStateCountry(
 
   while (!sections_of_state_indexes->IsFinished() ||
          !sections_of_country_indexes->IsFinished()) {
-    auto current_section_of_state_indexes =
-        sections_of_state_indexes->CurrentSection();
-    auto current_section_of_country_indexes =
-        sections_of_country_indexes->CurrentSection();
     // If there are still sections left with both country and state type, and
     // state and country current sections are equal, then that section has both
     // state and country. No rationalization needed.
@@ -1824,22 +1866,33 @@ void FormStructure::RationalizeAddressStateCountry(
       continue;
     }
 
-    size_t upper_index = 0, lower_index = 0;
+    size_t upper_index = 0;
+    size_t lower_index = 0;
+
+    auto* current_section_of_state_indexes =
+        sections_of_state_indexes->CurrentSection();
+    auto* current_section_of_country_indexes =
+        sections_of_country_indexes->CurrentSection();
+    DCHECK(current_section_of_state_indexes ||
+           current_section_of_country_indexes);
 
     // If country section is before the state ones, it means that that section
     // misses states, and the other way around.
-    if (current_section_of_state_indexes < current_section_of_country_indexes) {
+    if (!current_section_of_country_indexes ||
+        (current_section_of_state_indexes &&
+         *current_section_of_state_indexes <
+             *current_section_of_country_indexes)) {
       // We only rationalize when we have exactly two visible fields of a kind.
-      if (current_section_of_state_indexes.size() == 2) {
-        upper_index = current_section_of_state_indexes[0];
-        lower_index = current_section_of_state_indexes[1];
+      if (current_section_of_state_indexes->size() == 2) {
+        upper_index = (*current_section_of_state_indexes)[0];
+        lower_index = (*current_section_of_state_indexes)[1];
       }
       sections_of_state_indexes->WalkForwardToTheNextSection();
     } else {
       // We only rationalize when we have exactly two visible fields of a kind.
-      if (current_section_of_country_indexes.size() == 2) {
-        upper_index = current_section_of_country_indexes[0];
-        lower_index = current_section_of_country_indexes[1];
+      if (current_section_of_country_indexes->size() == 2) {
+        upper_index = (*current_section_of_country_indexes)[0];
+        lower_index = (*current_section_of_country_indexes)[1];
       }
       sections_of_country_indexes->WalkForwardToTheNextSection();
     }
@@ -1879,24 +1932,25 @@ void FormStructure::RationalizeRepeatedFields(
   // indexes of fields whose types are predicted as FULL_NAME by the server.
   SectionedFieldsIndexes sectioned_field_indexes_by_type[MAX_VALID_FIELD_TYPE];
 
-  for (const auto& field : fields_) {
+  for (size_t i = 0; i < fields_.size(); ++i) {
+    const AutofillField& field = *fields_[i];
     // The hidden fields are not considered when rationalizing.
-    if (!field->IsVisible())
+    if (!field.IsVisible())
       continue;
     // The billing and non-billing types are aggregated.
-    auto current_type = field->Type().GetStorableType();
+    auto current_type = field.Type().GetStorableType();
 
     if (current_type != UNKNOWN_TYPE && current_type < MAX_VALID_FIELD_TYPE) {
       // Look at the sectioned field indexes for the current type, if the
       // current field belongs to that section, then the field index should be
       // added to that same section, otherwise, start a new section.
       sectioned_field_indexes_by_type[current_type].AddFieldIndex(
-          &field - &fields_[0],
+          i,
           /*is_new_section*/ sectioned_field_indexes_by_type[current_type]
                   .Empty() ||
               fields_[sectioned_field_indexes_by_type[current_type]
                           .LastFieldIndex()]
-                      ->section != field->section);
+                      ->section != field.section);
     }
   }
 

components/autofill/core/browser/form_structure.h

@@ -411,51 +411,9 @@ class FormStructure {
   FRIEND_TEST_ALL_PREFIXES(ParameterizedFormStructureTest,
                            RationalizePhoneNumber_RunsOncePerSection);
 
-  class SectionedFieldsIndexes {
-   public:
-    SectionedFieldsIndexes();
-    ~SectionedFieldsIndexes();
-
-    size_t LastFieldIndex() const {
-      if (sectioned_indexes.empty())
-        return (size_t)-1;  // Shouldn't happen.
-      return sectioned_indexes.back().back();
-    }
-
-    void AddFieldIndex(const size_t index, bool is_new_section) {
-      if (is_new_section || Empty()) {
-        sectioned_indexes.push_back(std::vector<size_t>(1, index));
-        return;
-      }
-      sectioned_indexes.back().push_back(index);
-    }
-
-    void WalkForwardToTheNextSection() { current_section_ptr++; }
-
-    bool IsFinished() const {
-      return current_section_ptr >= sectioned_indexes.size();
-    }
-
-    size_t CurrentIndex() const { return CurrentSection()[0]; }
-
-    std::vector<size_t> CurrentSection() const {
-      if (current_section_ptr < sectioned_indexes.size())
-        return sectioned_indexes[current_section_ptr];
-      return std::vector<size_t>(1, (size_t)-1);  // To handle edge cases.
-    }
-
-    void Reset() { current_section_ptr = 0; }
-
-    bool Empty() const { return sectioned_indexes.empty(); }
-
-   private:
-    // A vector of sections. Each section is a vector of some of the indexes
-    // that belong to the same section. The sections and indexes are sorted by
-    // their order of appearance on the form.
-    std::vector<std::vector<size_t>> sectioned_indexes;
-    // Points to a vector of indexes that belong to the same section.
-    size_t current_section_ptr = 0;
-  };
+  // This class wraps a vector of vectors of field indices. The indices of a
+  // vector belong to the same group.
+  class SectionedFieldsIndexes;
 
   // Parses the field types from the server query response. |forms| must be the
   // same as the one passed to EncodeQueryRequest when constructing the query.