[PasswordGeneration] Improve change/confirm password field detection.

components/autofill/content/renderer/password_generation_agent.cc

Index: components/autofill/content/renderer/password_generation_agent.cc
diff --git a/components/autofill/content/renderer/password_generation_agent.cc b/components/autofill/content/renderer/password_generation_agent.cc
index bf50f220493544252111f122723c9c80668fe832..0e2619289d1ffc47cb02e893a72d01f9cece1278 100644
--- a/components/autofill/content/renderer/password_generation_agent.cc
+++ b/components/autofill/content/renderer/password_generation_agent.cc
@@ -4,7 +4,9 @@
 
 #include "components/autofill/content/renderer/password_generation_agent.h"
 
+#include <algorithm>
 #include <memory>
+#include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ -39,17 +41,17 @@ namespace {
 
 using Logger = autofill::SavePasswordProgressLogger;
 
-// Returns true if we think that this form is for account creation. |passwords|
-// is filled with the password field(s) in the form.
+// Returns true if we think that this form is for account creation. Password
+// field(s) of the form are pushed back to |passwords|.
 bool GetAccountCreationPasswordFields(
     const std::vector<blink::WebFormControlElement>& control_elements,
     std::vector<blink::WebInputElement>* passwords) {
-  for (size_t i = 0; i < control_elements.size(); i++) {
+  for (const auto& control_element : control_elements) {
     const blink::WebInputElement* input_element =
-        ToWebInputElement(&control_elements[i]);
-    if (input_element && input_element->IsTextField()) {
-      if (input_element->IsPasswordField())
-        passwords->push_back(*input_element);
+        ToWebInputElement(&control_element);
+    if (input_element && input_element->IsTextField() &&
+        input_element->IsPasswordField()) {
+      passwords->push_back(*input_element);
     }
   }
   return !passwords->empty();
@@ -71,10 +73,35 @@ const PasswordFormGenerationData* FindFormGenerationData(
   return nullptr;
 }
 
-// This function returns a vector of password fields into which Chrome should
-// fill the generated password. It assumes that |field_signature| describes the
-// field where Chrome shows the password generation prompt. It returns no more
-// than 2 elements.
+// Returns a vector of up to 2 password fields with autocomplete attribute set
+// to "new-password". These will be filled with the generated password.
+std::vector<blink::WebInputElement> FindNewPasswordElementsForGeneration(

[dvadym, 2017/06/07 17:16:43]

It's not clear by names what's the different between this function and next
function.
Probably it's better to call this function something like
FindMarkedBySiteNewPasswordElements

[pkalinnikov, 2017/06/08 12:33:14]

Done.

+    const std::vector<blink::WebInputElement>& all_password_elements) {
+  auto is_new_password_field = [](const blink::WebInputElement& element) {
+    CR_DEFINE_STATIC_LOCAL(blink::WebString, kAutocomplete, ("autocomplete"));

[dvadym, 2017/06/07 17:16:42]

It's better to reuse function HasAutocompleteAttributeValue
https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas...

[pkalinnikov, 2017/06/08 12:33:14]

Done.

+    CR_DEFINE_STATIC_LOCAL(blink::WebString, kNewPassword, ("new-password"));
+    return element.GetAttribute(kAutocomplete) == kNewPassword;
+  };
+
+  std::vector<blink::WebInputElement> passwords;
+
+  auto field_iter =
+      std::find_if(all_password_elements.begin(), all_password_elements.end(),
+                   is_new_password_field);
+  if (field_iter != all_password_elements.end()) {
+    passwords.push_back(*field_iter++);
+    field_iter = std::find_if(field_iter, all_password_elements.end(),
+                              is_new_password_field);
+    if (field_iter != all_password_elements.end())
+      passwords.push_back(*field_iter);
+  }
+
+  return passwords;
+}
+
+// Returns a vector of up to 2 password fields into which Chrome should fill the
+// generated password. It assumes that |field_signature| describes the field
+// where Chrome shows the password generation prompt.
 std::vector<blink::WebInputElement> FindPasswordElementsForGeneration(
     const std::vector<blink::WebInputElement>& all_password_elements,
     const PasswordFormGenerationData& generation_data) {
@@ -85,11 +112,12 @@ std::vector<blink::WebInputElement> FindPasswordElementsForGeneration(
     const blink::WebInputElement& input = *iter;
     FieldSignature signature = CalculateFieldSignatureByNameAndType(
         input.NameForAutofill().Utf16(), input.FormControlType().Utf8());
-    if (signature == generation_data.field_signature)
+    if (signature == generation_data.field_signature) {
       generation_field_iter = iter;
-    else if (generation_data.confirmation_field_signature &&
-             signature == *generation_data.confirmation_field_signature)
+    } else if (generation_data.confirmation_field_signature &&
+               signature == *generation_data.confirmation_field_signature) {
       confirmation_field_iter = iter;
+    }
   }
 
   std::vector<blink::WebInputElement> passwords;
@@ -115,7 +143,7 @@ void CopyElementValueToOtherInputElements(
 }
 
 bool AutocompleteAttributesSetForGeneration(const PasswordForm& form) {

[dvadym, 2017/06/07 17:16:42]

Nit: This function is pretty trivial, we can remove it.

[pkalinnikov, 2017/06/08 12:33:14]

Done.

-  return form.username_marked_by_site && form.new_password_marked_by_site;
+  return form.new_password_marked_by_site;
 }
 
 }  // namespace
@@ -123,8 +151,7 @@ bool AutocompleteAttributesSetForGeneration(const PasswordForm& form) {
 PasswordGenerationAgent::AccountCreationFormData::AccountCreationFormData(
     linked_ptr<PasswordForm> password_form,
     std::vector<blink::WebInputElement> passwords)
-    : form(password_form),
-      password_elements(passwords) {}
+    : form(password_form), password_elements(std::move(passwords)) {}
 
 PasswordGenerationAgent::AccountCreationFormData::AccountCreationFormData(
     const AccountCreationFormData& other) = default;
@@ -277,9 +304,8 @@ void PasswordGenerationAgent::FindPossibleGenerationForm() {
             &passwords)) {
       if (form_classifier_enabled_)
         RunFormClassifierAndSaveVote(forms[i], *password_form);
-      AccountCreationFormData ac_form_data(
-          make_linked_ptr(password_form.release()), passwords);
-      possible_account_creation_forms_.push_back(ac_form_data);
+      possible_account_creation_forms_.emplace_back(
+          make_linked_ptr(password_form.release()), std::move(passwords));
     }
   }
 
@@ -389,8 +415,11 @@ void PasswordGenerationAgent::DetermineGenerationElement() {
   for (auto& possible_form_data : possible_account_creation_forms_) {
     PasswordForm* possible_password_form = possible_form_data.form.get();
     const PasswordFormGenerationData* generation_data = nullptr;
+
+    std::vector<blink::WebInputElement> password_elements;
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kLocalHeuristicsOnlyForPasswordGeneration)) {
+      password_elements = possible_form_data.password_elements;

[pkalinnikov, 2017/06/06 16:49:46]

This line is equivalent to what we had before the CL.

However, shouldn't we do lines 435-445 here as well? My understanding is that it
is also a "local heuristic".

[dvadym, 2017/06/07 17:16:42]

Don't worry about these lines, they are not used. Probably we should remove
switch at all.

[pkalinnikov, 2017/06/08 12:33:14]

Acknowledged.

       VLOG(2) << "Bypassing additional checks.";
     } else if (!ContainsURL(not_blacklisted_password_form_origins_,
                             possible_password_form->origin)) {
@@ -399,28 +428,31 @@ void PasswordGenerationAgent::DetermineGenerationElement() {
     } else {
       generation_data = FindFormGenerationData(generation_enabled_forms_,
                                                *possible_password_form);
-      if (!generation_data) {
-        if (AutocompleteAttributesSetForGeneration(*possible_password_form)) {
-          LogMessage(Logger::STRING_GENERATION_RENDERER_AUTOCOMPLETE_ATTRIBUTE);
-          password_generation::LogPasswordGenerationEvent(
-              password_generation::AUTOCOMPLETE_ATTRIBUTES_ENABLED_GENERATION);
-        } else {
+      if (generation_data) {
+        password_elements = FindPasswordElementsForGeneration(
+            possible_form_data.password_elements, *generation_data);
+      } else {
+        if (!AutocompleteAttributesSetForGeneration(*possible_password_form)) {
           LogMessage(Logger::STRING_GENERATION_RENDERER_NO_SERVER_SIGNAL);
           continue;
         }
+
+        LogMessage(Logger::STRING_GENERATION_RENDERER_AUTOCOMPLETE_ATTRIBUTE);
+        password_generation::LogPasswordGenerationEvent(
+            password_generation::AUTOCOMPLETE_ATTRIBUTES_ENABLED_GENERATION);
+
+        password_elements = FindNewPasswordElementsForGeneration(
+            possible_form_data.password_elements);
       }
     }
+
     LogMessage(Logger::STRING_GENERATION_RENDERER_ELIGIBLE_FORM_FOUND);
-    std::vector<blink::WebInputElement> password_elements =
-        generation_data
-            ? FindPasswordElementsForGeneration(
-                  possible_form_data.password_elements, *generation_data)
-            : possible_form_data.password_elements;
     if (password_elements.empty()) {
       // It might be if JavaScript changes field names.
       LogMessage(Logger::STRING_GENERATION_RENDERER_NO_FIELD_FOUND);
       return;
     }
+
     generation_form_data_.reset(new AccountCreationFormData(
         possible_form_data.form, std::move(password_elements)));
     generation_element_ = generation_form_data_->password_elements[0];