[Passwords] AreAllFieldsEmpty() should deal with FormData

With the new parser in place, FormData should be used to check empty fields instead of PasswordForm Change-Id: I8242cc82524d14de4a82c2ffde6d8c059c7d3ee2 Bug: 949519,1017129, 1008798 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849894 Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#708676}

[Passwords] AreAllFieldsEmpty() should deal with FormData
With the new parser in place, FormData should be used to check empty fields instead of PasswordForm Change-Id: I8242cc82524d14de4a82c2ffde6d8c059c7d3ee2 Bug: 949519,1017129, 1008798 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849894 Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#708676}
733e377c · Mohamed Amir Yosef · Commit Bot · 16980f14 · 733e377c · 733e377c
Commit 733e377c authored Oct 23, 2019 by Mohamed Amir Yosef Committed by Commit Bot Oct 23, 2019
5 changed files
--- a/chrome/browser/password_manager/password_manager_browsertest.cc
+++ b/chrome/browser/password_manager/password_manager_browsertest.cc
--- a/chrome/test/data/password/update_form_empty_fields.html
+++ b/chrome/test/data/password/update_form_empty_fields.html
+<html>
+
+<body>
+  Navigation complete. Below is the password update form again with empty fields.
+  <form action="update_form_empty_fields.html" id="chg_testform_wo_username">
+    <input type="password" id="password" name="password">
+    <input type="password" id="new_password_1" name="new_password_1">
+    <input type="password" id="new_password_2" name="new_password_2">
+    <input type="submit" id="chg_submit_wo_username_button" name="chg_submit_button">
+  </form>
+
+</body>
+
+</html>
--- a/components/autofill/content/renderer/form_autofill_util.cc
+++ b/components/autofill/content/renderer/form_autofill_util.cc
@@ -1768,7 +1768,8 @@ bool WebFormElementToFormData(
  form->unique_renderer_id = form_element.UniqueRendererFormId();
  form->url = GetCanonicalOriginForDocument(frame->GetDocument());
  form->action = GetCanonicalActionForForm(form_element);
-  form->is_action_empty = form_element.Action().IsNull();
+  form->is_action_empty =
+      form_element.Action().IsNull() || form_element.Action().IsEmpty();
  if (IsAutofillFieldMetadataEnabled()) {
    SCOPED_UMA_HISTOGRAM_TIMER(
        "PasswordManager.ButtonTitlePerformance.HasFormTag");

--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -127,14 +127,6 @@ uint32_t FindFormsDifferences(const FormData& lhs, const FormData& rhs) {
  return differences_bitmask;
 }

-// Since empty or unspecified form's action is automatically set to the page
-// origin, this function checks if a form's action is empty by comparing it to
-// its origin.
-bool HasNonEmptyAction(const FormData& form) {
-  // TODO(crbug.com/1008798): The logic isn't accurate and should be fixed.
-  return form.action != form.url;
-}
-
 bool FormContainsFieldWithName(const FormData& form,
                               const base::string16& element) {
  if (element.empty())
@@ -240,8 +232,8 @@ bool PasswordFormManager::IsEqualToSubmittedForm(
  if (IsHttpAuth())
    return false;

-  if (form.action.is_valid() && HasNonEmptyAction(form) &&
-      HasNonEmptyAction(submitted_form_) &&
+  if (form.action.is_valid() && !form.is_action_empty &&
+      !submitted_form_.is_action_empty &&
      submitted_form_.action == form.action) {
    return true;
  }

--- a/components/password_manager/core/browser/password_manager.cc
+++ b/components/password_manager/core/browser/password_manager.cc
@@ -59,9 +59,13 @@ namespace {
 // already.
 using Logger = autofill::SavePasswordProgressLogger;

-bool AreAllFieldsEmpty(const PasswordForm& form) {
-  return form.username_value.empty() && form.password_value.empty() &&
-         form.new_password_value.empty();
+bool AreAllFieldsEmpty(const FormData& form_data) {
+  for (const auto& field : form_data.fields) {
+    if (!field.value.empty())
+      return false;
+  }
+
+  return true;
 }

 // Returns true if the user needs to be prompted before a password can be
@@ -741,8 +745,7 @@ void PasswordManager::OnPasswordFormsRendered(
  }

  // Record all visible forms from the frame.
-  all_visible_forms_.insert(all_visible_forms_.end(),
-                            visible_forms.begin(),
+  all_visible_forms_.insert(all_visible_forms_.end(), visible_forms.begin(),
                            visible_forms.end());

  if (!did_stop_loading &&
@@ -768,7 +771,7 @@ void PasswordManager::OnPasswordFormsRendered(
    for (const PasswordForm& form : all_visible_forms_) {
      if (submitted_manager->IsEqualToSubmittedForm(form.form_data)) {
        if (submitted_manager->IsPossibleChangePasswordFormWithoutUsername() &&
-            AreAllFieldsEmpty(form)) {
+            AreAllFieldsEmpty(form.form_data)) {
          continue;
        }
        submitted_manager->GetMetricsRecorder()->LogSubmitFailed();