Refactored FormData, FormFieldData comparison operators.

This is a cleanup CL that avoids any semantic changes, with one
exception noted below.

* Used std::tie() and tuples to improve maintainability in FormFieldData.

* Removed FormData::operator==() and FormFieldData::operator==()
  They were only used in tests, and they are relatively difficult to maintain.
  (Case in point: FormFieldData::operator==() did not compare several members,
  and behaved awkwardly for |FormFieldData::label|.)
  Replaced the usage in the tests with !(a < b) && !(b < a).

* Moved FormData::operator<(), FormFieldData::operator<() to comparators.
  These only exist to make STL containers happy, namely an std::set member of
  FormCache. The style guide advises not to implement operator<() in such
  cases.

* FormData::operator<() and FormFieldData::operator<() (now the comparators)
  did and do not compare all members. This seems unintended with the use case
  in FormCache.
  However, this change is deferred to a follow-up CL.
  This CL only makes minor modifications to the lexicographic orderings in
  that the order in which the members are compared is slightly changed.
  This should have no repercussions since FormCache makes no assumptions about
  the ordering.

Bug: 1007974
Change-Id: I5109aa37edccfd858993e1f0c6992b9988c8cf46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1884790Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Christoph Schwering <schwering@google.com>
Cr-Commit-Position: refs/heads/master@{#714276}

components/autofill/content/renderer/form_cache.cc

@@ -559,23 +559,11 @@ bool FormCache::ShouldShowAutocompleteConsoleWarnings(
 
 void FormCache::PruneInitialValueCaches(
     const std::set<uint32_t>& ids_to_retain) {
-  // Prune initial_select_values_.
-  for (auto iter = initial_select_values_.begin();
-       iter != initial_select_values_.end();) {
-    if (!base::Contains(ids_to_retain, iter->first))
-      iter = initial_select_values_.erase(iter);
-    else
-      ++iter;
-  }
-
-  // Prune initial_checked_state_.
-  for (auto iter = initial_checked_state_.begin();
-       iter != initial_checked_state_.end();) {
-    if (!base::Contains(ids_to_retain, iter->first))
-      iter = initial_checked_state_.erase(iter);
-    else
-      ++iter;
-  }
+  auto should_not_retain = [&ids_to_retain](const auto& p) {
+    return !base::Contains(ids_to_retain, p.first);
+  };
+  base::EraseIf(initial_select_values_, should_not_retain);
+  base::EraseIf(initial_checked_state_, should_not_retain);
 }
 
 }  // namespace autofill

components/autofill/content/renderer/form_cache.h

@@ -86,7 +86,8 @@ class FormCache {
   blink::WebLocalFrame* frame_;
 
   // The cached forms. Used to prevent re-extraction of forms.
-  std::set<FormData> parsed_forms_;
+  // TODO(crbug/896689) Move to std::map<unique_rederer_id, FormData>.
+  std::set<FormData, FormData::IdentityComparator> parsed_forms_;
 
   // The synthetic FormData is for all the fieldsets in the document without a
   // form owner.

components/autofill/core/common/form_data.cc

@@ -123,28 +123,23 @@ bool FormData::DynamicallySameFormAs(const FormData& form) const {
   return true;
 }
 
-bool FormData::operator==(const FormData& form) const {
-  return name == form.name && id_attribute == form.id_attribute &&
-         name_attribute == form.name_attribute && url == form.url &&
-         action == form.action && is_action_empty == form.is_action_empty &&
-         unique_renderer_id == form.unique_renderer_id &&
-         submission_event == form.submission_event &&
-         is_form_tag == form.is_form_tag &&
-         is_formless_checkout == form.is_formless_checkout &&
-         fields == form.fields &&
-         username_predictions == form.username_predictions;
-}
-
-bool FormData::operator!=(const FormData& form) const {
-  return !(*this == form);
-}
-
-bool FormData::operator<(const FormData& form) const {
-  return std::tie(name, id_attribute, name_attribute, url, action, is_form_tag,
-                  is_formless_checkout, fields) <
-         std::tie(form.name, form.id_attribute, form.name_attribute, form.url,
-                  form.action, form.is_form_tag, form.is_formless_checkout,
-                  form.fields);
+bool FormData::IdentityComparator::operator()(const FormData& a,
+                                              const FormData& b) const {
+  // |unique_renderer_id| uniquely identifies the form, if and only if it is
+  // set; the other members compared below together uniquely identify the form
+  // as well.
+  auto tie = [](const FormData& f) {
+    return std::tie(f.unique_renderer_id, f.name, f.id_attribute,
+                    f.name_attribute, f.url, f.action, f.is_form_tag,
+                    f.is_formless_checkout);
+  };
+  if (tie(a) < tie(b))
+    return true;
+  if (tie(b) < tie(a))
+    return false;
+  return std::lexicographical_compare(a.fields.begin(), a.fields.end(),
+                                      b.fields.begin(), b.fields.end(),
+                                      FormFieldData::IdentityComparator());
 }
 
 std::ostream& operator<<(std::ostream& os, const FormData& form) {

components/autofill/core/common/form_data.h

@@ -29,6 +29,12 @@ using ButtonTitleList = std::vector<ButtonTitleInfo>;
 
 // Holds information about a form to be filled and/or submitted.
 struct FormData {
+  // Less-than relation for STL containers. Compares only members needed to
+  // uniquely identify a form.
+  struct IdentityComparator {
+    bool operator()(const FormData& a, const FormData& b) const;
+  };
+
   // TODO(https://crbug.com/875768): Rename this const to kNotSetRendererId, and
   // use it also for not set renderer ids in FormFieldData.
   static constexpr uint32_t kNotSetFormRendererId =
@@ -52,11 +58,6 @@ struct FormData {
   // If |form| is the same as this from the POV of dynamic refills.
   bool DynamicallySameFormAs(const FormData& form) const;
 
-  // Note: operator==() performs a full-field-comparison(byte by byte), this is
-  // different from SameFormAs(), which ignores comparison for those "values" of
-  // all form fields, just like what FormFieldData::SameFieldAs() ignores.
-  bool operator==(const FormData& form) const;
-  bool operator!=(const FormData& form) const;
   // Allow FormData to be a key in STL containers.
   bool operator<(const FormData& form) const;
 
@@ -66,6 +67,11 @@ struct FormData {
   // The name attribute of the form.
   base::string16 name_attribute;
 
+  // NOTE: update IdentityComparator                when adding new a member.
+  // NOTE: update SameFormAs()            if needed when adding new a member.
+  // NOTE: update SimilarFormAs()         if needed when adding new a member.
+  // NOTE: update DynamicallySameFormAs() if needed when adding new a member.
+
   // The name by which autofill knows this form. This is generally either the
   // name attribute or the id_attribute value, which-ever is non-empty with
   // priority given to the name_attribute. This value is used when computing
@@ -91,9 +97,9 @@ struct FormData {
   // and used if features::kAutofillRestrictUnownedFieldsToFormlessCheckout is
   // enabled, to prevent heuristics from running on formless non-checkout.
   bool is_formless_checkout = false;
-  //  Unique renderer id which is returned by function
-  //  WebFormElement::UniqueRendererFormId(). It is not persistant between page
-  //  loads, so it is not saved and not used in comparison in SameFormAs().
+  // Unique renderer id returned by WebFormElement::UniqueRendererFormId(). It
+  // is not persistent between page loads, so it is not saved and not used in
+  // comparison in SameFormAs().
   uint32_t unique_renderer_id = kNotSetFormRendererId;
   // The type of the event that was taken as an indication that this form is
   // being or has already been submitted. This field is filled only in Password

components/autofill/core/common/form_field_data.h

@@ -57,6 +57,12 @@ struct FormFieldData {
   using RoleAttribute = mojom::FormFieldData_RoleAttribute;
   using LabelSource = mojom::FormFieldData_LabelSource;
 
+  // Less-than relation for STL containers. Compares only members needed to
+  // uniquely identify a field.
+  struct IdentityComparator {
+    bool operator()(const FormFieldData& a, const FormFieldData& b) const;
+  };
+
   static constexpr uint32_t kNotSetFormControlRendererId =
       std::numeric_limits<uint32_t>::max();
 
@@ -67,20 +73,22 @@ struct FormFieldData {
   FormFieldData& operator=(FormFieldData&&);
   ~FormFieldData();
 
-  // Returns true if two form fields are the same, not counting the value.
+  // Returns true if both fields are identical, ignoring value- and
+  // parsing related members.
+  // See also SimilarFieldAs(), DynamicallySameFieldAs().
   bool SameFieldAs(const FormFieldData& field) const;
 
-  // SameFieldAs() is a little restricted when field's style changed
-  // dynamically, like css.
-  // This method only compares critical attributes of field to check whether
-  // they are similar enough to be considered as same field if form's
-  // other information isn't changed.
+  // Returns true if both fields are identical, ignoring members that
+  // are typically changed dynamically.
+  // Strictly weaker than SameFieldAs().
   bool SimilarFieldAs(const FormFieldData& field) const;
 
-  // If |field| is the same as this from the POV of dynamic refills.
+  // Returns true if both forms are equivalent from the POV of dynamic refills.
+  // Strictly weaker than SameFieldAs(): replaces equality of |is_focusable| and
+  // |role| with equality of IsVisible().
   bool DynamicallySameFieldAs(const FormFieldData& field) const;
 
-  // Returns true for all of textfield-looking types such as text, password,
+  // Returns true for all of textfield-looking types: text, password,
   // search, email, url, and number. It must work the same way as Blink function
   // WebInputElement::IsTextField(), and it returns false if |*this| represents
   // a textarea.
@@ -99,15 +107,6 @@ struct FormFieldData {
   bool HadFocus() const;
   bool WasAutofilled() const;
 
-  // Note: operator==() performs a full-field-comparison(byte by byte), this is
-  // different from SameFieldAs(), which ignores comparison for those "values"
-  // not regarded as part of identity of the field, such as is_autofilled and
-  // the option_values/contents etc.
-  bool operator==(const FormFieldData& field) const;
-  bool operator!=(const FormFieldData& field) const;
-  // Comparison operator exposed for STL map. Uses label, then name to sort.
-  bool operator<(const FormFieldData& field) const;
-
 #if defined(OS_IOS)
   // The identifier which uniquely addresses this field in the DOM. This is an
   // ephemeral value which is not guaranteed to be stable across page loads. It
@@ -122,6 +121,11 @@ struct FormFieldData {
 #define EXPECT_EQ_UNIQUE_ID(expected, actual)
 #endif
 
+  // NOTE: update IdentityComparator                 when adding new a member.
+  // NOTE: update SameFieldAs()            if needed when adding new a member.
+  // NOTE: update SimilarFieldAs()         if needed when adding new a member.
+  // NOTE: update DynamicallySameFieldAs() if needed when adding new a member.
+
   // The name by which autofill knows this field. This is generally either the
   // name attribute or the id_attribute value, which-ever is non-empty with
   // priority given to the name_attribute. This value is used when computing
@@ -129,8 +133,6 @@ struct FormFieldData {
   // TODO(crbug/896689): remove this and use attributes/unique_id instead.
   base::string16 name;
 
-  // If you add more, be sure to update the comparison operators, SameFieldAs,
-  // serializing functions (in the .cc file) and the constructor.
   base::string16 id_attribute;
   base::string16 name_attribute;
   base::string16 label;
@@ -142,10 +144,9 @@ struct FormFieldData {
   base::string16 aria_label;
   base::string16 aria_description;
 
-  // Unique renderer id which is returned by function
-  // WebFormControlElement::UniqueRendererFormControlId(). It is not persistant
-  // between page loads, so it is not saved and not used in comparison in
-  // SameFieldAs().
+  // Unique renderer id returned by WebFormElement::UniqueRendererFormId(). It
+  // is not persistent between page loads, so it is not saved and not used in
+  // comparison in SameFieldAs().
   uint32_t unique_renderer_id = kNotSetFormControlRendererId;
 
   // The ax node id of the form control in the accessibility tree.

components/autofill/core/common/mojom/autofill_types_mojom_traits_unittest.cc

@@ -28,6 +28,12 @@ const std::vector<const char*> kOptions = {"Option1", "Option2", "Option3",
                                            "Option4"};
 namespace {
 
+template <typename T>
+bool EquivalentData(const T& a, const T& b) {
+  typename T::IdentityComparator less;
+  return !less(a, b) && !less(b, a);
+}
+
 void CreateTestFieldDataPredictions(const std::string& signature,
                                     FormFieldDataPredictions* field_predict) {
   test::CreateTestSelectField("TestLabel", "TestName", "TestValue", kOptions,
@@ -129,8 +135,8 @@ void CheckEqualPasswordFormFillData(const PasswordFormFillData& expected,
   EXPECT_EQ(expected.form_renderer_id, actual.form_renderer_id);
   EXPECT_EQ(expected.origin, actual.origin);
   EXPECT_EQ(expected.action, actual.action);
-  EXPECT_EQ(expected.username_field, actual.username_field);
-  EXPECT_EQ(expected.password_field, actual.password_field);
+  EXPECT_TRUE(EquivalentData(expected.username_field, actual.username_field));
+  EXPECT_TRUE(EquivalentData(expected.password_field, actual.password_field));
   EXPECT_EQ(expected.preferred_realm, actual.preferred_realm);
   EXPECT_EQ(expected.uses_account_store, actual.uses_account_store);
 
@@ -234,7 +240,7 @@ class AutofillTypeTraitsTestImpl : public testing::Test,
 void ExpectFormFieldData(const FormFieldData& expected,
                          base::OnceClosure closure,
                          const FormFieldData& passed) {
-  EXPECT_EQ(expected, passed);
+  EXPECT_TRUE(EquivalentData(expected, passed));
   EXPECT_EQ(expected.value, passed.value);
   EXPECT_EQ(expected.typed_value, passed.typed_value);
   std::move(closure).Run();
@@ -243,7 +249,7 @@ void ExpectFormFieldData(const FormFieldData& expected,
 void ExpectFormData(const FormData& expected,
                     base::OnceClosure closure,
                     const FormData& passed) {
-  EXPECT_EQ(expected, passed);
+  EXPECT_TRUE(EquivalentData(expected, passed));
   std::move(closure).Run();
 }
 

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -136,7 +136,9 @@ void CheckPendingCredentials(const PasswordForm& expected,
   EXPECT_EQ(expected.username_element, actual.username_element);
   EXPECT_EQ(expected.password_element, actual.password_element);
   EXPECT_EQ(expected.blacklisted_by_user, actual.blacklisted_by_user);
-  EXPECT_EQ(expected.form_data, actual.form_data);
+  FormData::IdentityComparator less;
+  EXPECT_FALSE(less(expected.form_data, actual.form_data));
+  EXPECT_FALSE(less(actual.form_data, expected.form_data));
 }
 
 struct ExpectedGenerationUKM {