Remove PasswordForm::layout

As described in the associated bug, the |layout| data member of
|PasswordForm| contributes less benefit than it causes complexity. It
is not used in the new FormData -> PasswordForm parser, and this CL
also remvoes it from the old one, as well as removing all related
code.

Bug: 852772
Change-Id: Icdc6979a9c0a57093a459f2dfc2e27766f9e9142
Reviewed-on: https://chromium-review.googlesource.com/1209582Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589507}

components/autofill/content/common/autofill_types.mojom

@@ -32,12 +32,6 @@ enum GenerationUploadStatus {
   UNKNOWN_STATUS = 10
 };
 
-// autofill::PasswordForm::Layout
-enum PasswordFormLayout {
-  LAYOUT_OTHER,
-  LAYOUT_LOGIN_AND_SIGNUP
-};
-
 // autofill::PasswordForm::Type
 enum PasswordFormType {
   TYPE_MANUAL,
@@ -244,7 +238,6 @@ struct PasswordForm {
   url.mojom.Url icon_url;
   url.mojom.Origin federation_origin;
   bool skip_zero_click;
-  PasswordFormLayout layout;
   bool was_parsed_using_autofill_predictions;
   bool is_public_suffix_match;
   bool is_affiliation_based_match;

components/autofill/content/common/autofill_types_struct_traits.cc

@@ -132,40 +132,6 @@ bool EnumTraits<autofill::mojom::GenerationUploadStatus,
   return false;
 }
 
-// static
-autofill::mojom::PasswordFormLayout EnumTraits<
-    autofill::mojom::PasswordFormLayout,
-    autofill::PasswordForm::Layout>::ToMojom(autofill::PasswordForm::Layout
-                                                 input) {
-  switch (input) {
-    case autofill::PasswordForm::Layout::LAYOUT_OTHER:
-      return autofill::mojom::PasswordFormLayout::LAYOUT_OTHER;
-    case autofill::PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP:
-      return autofill::mojom::PasswordFormLayout::LAYOUT_LOGIN_AND_SIGNUP;
-  }
-
-  NOTREACHED();
-  return autofill::mojom::PasswordFormLayout::LAYOUT_OTHER;
-}
-
-// static
-bool EnumTraits<autofill::mojom::PasswordFormLayout,
-                autofill::PasswordForm::Layout>::
-    FromMojom(autofill::mojom::PasswordFormLayout input,
-              autofill::PasswordForm::Layout* output) {
-  switch (input) {
-    case autofill::mojom::PasswordFormLayout::LAYOUT_OTHER:
-      *output = autofill::PasswordForm::Layout::LAYOUT_OTHER;
-      return true;
-    case autofill::mojom::PasswordFormLayout::LAYOUT_LOGIN_AND_SIGNUP:
-      *output = autofill::PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP;
-      return true;
-  }
-
-  NOTREACHED();
-  return false;
-}
-
 // static
 autofill::mojom::PasswordFormType EnumTraits<
     autofill::mojom::PasswordFormType,
@@ -815,9 +781,6 @@ bool StructTraits<
 
   out->skip_zero_click = data.skip_zero_click();
 
-  if (!data.ReadLayout(&out->layout))
-    return false;
-
   out->was_parsed_using_autofill_predictions =
       data.was_parsed_using_autofill_predictions();
   out->is_public_suffix_match = data.is_public_suffix_match();

components/autofill/content/common/autofill_types_struct_traits.h

@@ -57,15 +57,6 @@ struct EnumTraits<autofill::mojom::GenerationUploadStatus,
                         autofill::PasswordForm::GenerationUploadStatus* output);
 };
 
-template <>
-struct EnumTraits<autofill::mojom::PasswordFormLayout,
-                  autofill::PasswordForm::Layout> {
-  static autofill::mojom::PasswordFormLayout ToMojom(
-      autofill::PasswordForm::Layout input);
-  static bool FromMojom(autofill::mojom::PasswordFormLayout input,
-                        autofill::PasswordForm::Layout* output);
-};
-
 template <>
 struct EnumTraits<autofill::mojom::PasswordFormType,
                   autofill::PasswordForm::Type> {
@@ -598,11 +589,6 @@ struct StructTraits<autofill::mojom::PasswordFormDataView,
     return r.skip_zero_click;
   }
 
-  static autofill::PasswordForm::Layout layout(
-      const autofill::PasswordForm& r) {
-    return r.layout;
-  }
-
   static bool was_parsed_using_autofill_predictions(
       const autofill::PasswordForm& r) {
     return r.was_parsed_using_autofill_predictions;

components/autofill/content/common/autofill_types_struct_traits_unittest.cc

@@ -99,7 +99,6 @@ void CreateTestPasswordForm(PasswordForm* form) {
   form->icon_url = GURL("https://foo.com/icon.png");
   form->federation_origin = url::Origin::Create(GURL("http://wwww.google.com"));
   form->skip_zero_click = false;
-  form->layout = PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP;
   form->was_parsed_using_autofill_predictions = false;
   form->is_public_suffix_match = true;
   form->is_affiliation_based_match = true;

components/autofill/content/renderer/password_form_conversion_utils.cc

@@ -139,57 +139,6 @@ enum class FieldFilteringLevel {
   USER_INPUT = 2
 };
 
-// Layout classification of password forms
-// A layout sequence of a form is the sequence of it's non-password and password
-// input fields, represented by "N" and "P", respectively. A form like this
-// <form>
-//   <input type='text' ...>
-//   <input type='hidden' ...>
-//   <input type='password' ...>
-//   <input type='submit' ...>
-// </form>
-// has the layout sequence "NP" -- "N" for the first field, and "P" for the
-// third. The second and fourth fields are ignored, because they are not text
-// fields.
-//
-// The code below classifies the layout (see PasswordForm::Layout) of a form
-// based on its layout sequence. This is done by assigning layouts regular
-// expressions over the alphabet {N, P}. LAYOUT_OTHER is implicitly the type
-// corresponding to all layout sequences not matching any other layout.
-//
-// LAYOUT_LOGIN_AND_SIGNUP is classified by NPN+P.*. This corresponds to a form
-// which starts with a login section (NP) and continues with a sign-up section
-// (N+P.*). The aim is to distinguish such forms from change password-forms
-// (N*PPP?.*) and forms which use password fields to store private but
-// non-password data (could look like, e.g., PN+P.*).
-const char kLoginAndSignupRegex[] =
-    "NP"   // Login section.
-    "N+P"  // Sign-up section.
-    ".*";  // Anything beyond that.
-
-struct LoginAndSignupLazyInstanceTraits
-    : public base::internal::DestructorAtExitLazyInstanceTraits<re2::RE2> {
-  static re2::RE2* New(void* instance) {
-    return CreateMatcher(instance, kLoginAndSignupRegex);
-  }
-};
-
-base::LazyInstance<re2::RE2, LoginAndSignupLazyInstanceTraits>
-    g_login_and_signup_matcher = LAZY_INSTANCE_INITIALIZER;
-
-// Given the sequence of non-password and password text input fields of a form,
-// represented as a string of Ns (non-password) and Ps (password), computes the
-// layout type of that form.
-PasswordForm::Layout SequenceToLayout(base::StringPiece layout_sequence) {
-  if (re2::RE2::FullMatch(
-          re2::StringPiece(layout_sequence.data(),
-                           base::checked_cast<int>(layout_sequence.size())),
-          g_login_and_signup_matcher.Get())) {
-    return PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP;
-  }
-  return PasswordForm::Layout::LAYOUT_OTHER;
-}
-
 // Helper to determine which password is the main (current) one, and which is
 // the new password (e.g., on a sign-up or change password form), if any. If the
 // new password is found and there is another password field with the same user
@@ -649,15 +598,6 @@ bool GetPasswordForm(
     }
   }
 
-  // Evaluate the structure of the form for determining the form type (e.g.,
-  // sign-up, sign-in, etc.).
-  std::string layout_sequence;
-  layout_sequence.reserve(plausible_inputs.size());
-  for (const FormFieldData* input : plausible_inputs) {
-    layout_sequence.push_back((input->form_control_type == "password") ? 'P'
-                                                                       : 'N');
-  }
-
   // Populate all_possible_passwords and form_has_autofilled_value in
   // |password_form|.
   // Contains the first password element for each non-empty password value.
@@ -826,7 +766,6 @@ bool GetPasswordForm(
   password_form->preferred = false;
   password_form->blacklisted_by_user = false;
   password_form->type = PasswordForm::TYPE_MANUAL;
-  password_form->layout = SequenceToLayout(layout_sequence);
 
   return true;
 }

components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc

@@ -1873,99 +1873,6 @@ TEST_F(MAYBE_PasswordFormConversionUtilsTest,
   }
 }
 
-TEST_F(MAYBE_PasswordFormConversionUtilsTest, LayoutClassificationLogin) {
-  PasswordFormBuilder builder(kTestFormActionURL);
-  builder.AddHiddenField();
-  builder.AddTextField("username", "", nullptr);
-  builder.AddPasswordField("password", "", nullptr);
-  builder.AddSubmitButton("submit");
-  std::string login_html = builder.ProduceHTML();
-
-  std::unique_ptr<PasswordForm> login_form =
-      LoadHTMLAndConvertForm(login_html, nullptr, false);
-  ASSERT_TRUE(login_form);
-  EXPECT_FALSE(login_form->only_for_fallback_saving);
-  EXPECT_EQ(PasswordForm::Layout::LAYOUT_OTHER, login_form->layout);
-}
-
-TEST_F(MAYBE_PasswordFormConversionUtilsTest, LayoutClassificationSignup) {
-  PasswordFormBuilder builder(kTestFormActionURL);
-  builder.AddTextField("someotherfield", "", nullptr);
-  builder.AddTextField("username", "", nullptr);
-  builder.AddPasswordField("new_password", "", nullptr);
-  builder.AddHiddenField();
-  builder.AddPasswordField("new_password2", "", nullptr);
-  builder.AddSubmitButton("submit");
-  std::string signup_html = builder.ProduceHTML();
-
-  std::unique_ptr<PasswordForm> signup_form =
-      LoadHTMLAndConvertForm(signup_html, nullptr, false);
-  ASSERT_TRUE(signup_form);
-  EXPECT_FALSE(signup_form->only_for_fallback_saving);
-  EXPECT_EQ(PasswordForm::Layout::LAYOUT_OTHER, signup_form->layout);
-}
-
-TEST_F(MAYBE_PasswordFormConversionUtilsTest, LayoutClassificationChange) {
-  PasswordFormBuilder builder(kTestFormActionURL);
-  builder.AddTextField("username", "", nullptr);
-  builder.AddPasswordField("old_password", "", nullptr);
-  builder.AddHiddenField();
-  builder.AddPasswordField("new_password", "", nullptr);
-  builder.AddPasswordField("new_password2", "", nullptr);
-  builder.AddSubmitButton("submit");
-  std::string change_html = builder.ProduceHTML();
-
-  std::unique_ptr<PasswordForm> change_form =
-      LoadHTMLAndConvertForm(change_html, nullptr, false);
-  ASSERT_TRUE(change_form);
-  EXPECT_FALSE(change_form->only_for_fallback_saving);
-  EXPECT_EQ(PasswordForm::Layout::LAYOUT_OTHER, change_form->layout);
-}
-
-TEST_F(MAYBE_PasswordFormConversionUtilsTest,
-       LayoutClassificationLoginPlusSignup_A) {
-  PasswordFormBuilder builder(kTestFormActionURL);
-  builder.AddTextField("username", "", nullptr);
-  builder.AddHiddenField();
-  builder.AddPasswordField("password", "", nullptr);
-  builder.AddTextField("username2", "", nullptr);
-  builder.AddTextField("someotherfield", "", nullptr);
-  builder.AddPasswordField("new_password", "", nullptr);
-  builder.AddPasswordField("new_password2", "", nullptr);
-  builder.AddHiddenField();
-  builder.AddSubmitButton("submit");
-  std::string login_plus_signup_html = builder.ProduceHTML();
-
-  std::unique_ptr<PasswordForm> login_plus_signup_form =
-      LoadHTMLAndConvertForm(login_plus_signup_html, nullptr, false);
-  ASSERT_TRUE(login_plus_signup_form);
-  EXPECT_FALSE(login_plus_signup_form->only_for_fallback_saving);
-  EXPECT_EQ(PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP,
-            login_plus_signup_form->layout);
-}
-
-TEST_F(MAYBE_PasswordFormConversionUtilsTest,
-       LayoutClassificationLoginPlusSignup_B) {
-  PasswordFormBuilder builder(kTestFormActionURL);
-  builder.AddTextField("username", "", nullptr);
-  builder.AddHiddenField();
-  builder.AddPasswordField("password", "", nullptr);
-  builder.AddTextField("usrname2", "", nullptr);
-  builder.AddTextField("someotherfield", "", nullptr);
-  builder.AddPasswordField("new_password", "", nullptr);
-  builder.AddTextField("someotherfield2", "", nullptr);
-  builder.AddHiddenField();
-  builder.AddSubmitButton("submit");
-  std::string login_plus_signup_html = builder.ProduceHTML();
-
-  std::unique_ptr<PasswordForm> login_plus_signup_form =
-      LoadHTMLAndConvertForm(login_plus_signup_html, nullptr, false);
-  ASSERT_TRUE(login_plus_signup_form);
-  EXPECT_FALSE(login_plus_signup_form->only_for_fallback_saving);
-  EXPECT_EQ(PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP,
-            login_plus_signup_form->layout);
-}
-
 TEST_F(MAYBE_PasswordFormConversionUtilsTest,
        CreditCardNumberWithTypePasswordForm) {
   PasswordFormBuilder builder(kTestFormActionURL);

components/autofill/core/common/password_form.cc

@@ -60,9 +60,6 @@ void PasswordFormToJSON(const PasswordForm& form,
   target->SetString("icon_url", form.icon_url.possibly_invalid_spec());
   target->SetString("federation_origin", form.federation_origin.Serialize());
   target->SetBoolean("skip_next_zero_click", form.skip_zero_click);
-  std::ostringstream layout_string_stream;
-  layout_string_stream << form.layout;
-  target->SetString("layout", layout_string_stream.str());
   target->SetBoolean("was_parsed_using_autofill_predictions",
                      form.was_parsed_using_autofill_predictions);
   target->SetString("affiliated_web_realm", form.affiliated_web_realm);
@@ -89,7 +86,6 @@ PasswordForm::PasswordForm()
       times_used(0),
       generation_upload_status(NO_SIGNAL_SENT),
       skip_zero_click(false),
-      layout(Layout::LAYOUT_OTHER),
       was_parsed_using_autofill_predictions(false),
       is_public_suffix_match(false),
       is_affiliation_based_match(false),
@@ -108,8 +104,7 @@ PasswordForm& PasswordForm::operator=(const PasswordForm& form) = default;
 PasswordForm& PasswordForm::operator=(PasswordForm&& form) = default;
 
 bool PasswordForm::IsPossibleChangePasswordForm() const {
-  return !new_password_element.empty() &&
-         layout != PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP;
+  return !new_password_element.empty();
 }
 
 bool PasswordForm::IsPossibleChangePasswordFormWithoutUsername() const {
@@ -144,7 +139,7 @@ bool PasswordForm::operator==(const PasswordForm& form) const {
          // We compare the serialization of the origins here, as we want unique
          // origins to compare as '=='.
          federation_origin.Serialize() == form.federation_origin.Serialize() &&
-         skip_zero_click == form.skip_zero_click && layout == form.layout &&
+         skip_zero_click == form.skip_zero_click &&
          was_parsed_using_autofill_predictions ==
              form.was_parsed_using_autofill_predictions &&
          is_public_suffix_match == form.is_public_suffix_match &&
@@ -203,18 +198,6 @@ base::string16 ValueElementVectorToString(
   return base::JoinString(pairs, base::ASCIIToUTF16(", "));
 }
 
-std::ostream& operator<<(std::ostream& os, PasswordForm::Layout layout) {
-  switch (layout) {
-    case PasswordForm::Layout::LAYOUT_OTHER:
-      os << "LAYOUT_OTHER";
-      break;
-    case PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP:
-      os << "LAYOUT_LOGIN_AND_SIGNUP";
-      break;
-  }
-  return os;
-}
-
 std::ostream& operator<<(std::ostream& os, const PasswordForm& form) {
   base::DictionaryValue form_json;
   PasswordFormToJSON(form, &form_json);

components/autofill/core/common/password_form.h

@@ -70,21 +70,6 @@ struct PasswordForm {
     SCHEME_LAST = SCHEME_USERNAME_ONLY
   } scheme;
 
-  // During form parsing, Chrome tries to partly understand the type of the form
-  // based on the layout of its fields. The result of this analysis helps to
-  // treat the form correctly once the low-level information is lost by
-  // converting the web form into a PasswordForm. It is only used for observed
-  // HTML forms, not for stored credentials.
-  enum class Layout {
-    // Forms which either do not need to be classified, or cannot be classified
-    // meaningfully.
-    LAYOUT_OTHER,
-    // Login and signup forms combined in one <form>, to distinguish them from,
-    // e.g., change-password forms.
-    LAYOUT_LOGIN_AND_SIGNUP,
-    LAYOUT_LAST = LAYOUT_LOGIN_AND_SIGNUP
-  };
-
   // Events observed by the Password Manager that indicate either that a form is
   // potentially being submitted, or that a form has already been successfully
   // submitted. Recorded into a UMA histogram, so order of enumerators should
@@ -317,9 +302,6 @@ struct PasswordForm {
   // Once user selects this credential the flag is reseted.
   bool skip_zero_click;
 
-  // The layout as determined during parsing. Default value is LAYOUT_OTHER.
-  Layout layout;
-
   // If true, this form was parsed using Autofill predictions.
   bool was_parsed_using_autofill_predictions;
 
@@ -380,7 +362,6 @@ base::string16 ValueElementVectorToString(
     const ValueElementVector& value_element_pairs);
 
 // For testing.
-std::ostream& operator<<(std::ostream& os, PasswordForm::Layout layout);
 std::ostream& operator<<(std::ostream& os, const PasswordForm& form);
 std::ostream& operator<<(std::ostream& os, PasswordForm* form);
 std::ostream& operator<<(

components/password_manager/core/browser/password_form_manager.cc

@@ -473,9 +473,7 @@ void PasswordFormManager::SaveSubmittedFormTypeForMetrics(
 
   PasswordFormMetricsRecorder::SubmittedFormType type =
       PasswordFormMetricsRecorder::kSubmittedFormTypeUnspecified;
-  if (form.layout == PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP) {
-    type = PasswordFormMetricsRecorder::kSubmittedFormTypeLoginAndSignup;
-  } else if (is_change_password_form) {
+  if (is_change_password_form) {
     type = PasswordFormMetricsRecorder::kSubmittedFormTypeChangePasswordEnabled;
   } else if (is_signup_form) {
     if (no_username)

tools/metrics/histograms/enums.xml

@@ -37245,7 +37245,7 @@ Called by update_net_trust_anchors.py.-->
   <int value="4" label="Disabled change password form without username"/>
   <int value="5" label="Signup form"/>
   <int value="6" label="Signup form without username"/>
-  <int value="7" label="Combined login and signup form"/>
+  <int value="7" label="Combined login and signup form (obsolete)"/>
 </enum>
 
 <enum name="PasswordGenerationEvent">