Loosen up heuristics for detecting account creation forms.

chrome/renderer/autofill/password_generation_manager.cc

Index: chrome/renderer/autofill/password_generation_manager.cc
diff --git a/chrome/renderer/autofill/password_generation_manager.cc b/chrome/renderer/autofill/password_generation_manager.cc
index 11bd0b56d018a4722854fecc3547595e4d55a95f..f45d9a132054e38255b003788f554d5dacc026dc 100644
--- a/chrome/renderer/autofill/password_generation_manager.cc
+++ b/chrome/renderer/autofill/password_generation_manager.cc
@@ -33,22 +33,32 @@ bool GetAccountCreationPasswordFields(
   WebKit::WebVector<WebKit::WebFormControlElement> control_elements;
   form.getFormControlElements(control_elements);
 
+  int input_elements = 0;

[Ilya Sherman, 2012/09/10 21:23:49]

nit: size_t

[Ilya Sherman, 2012/09/10 21:23:49]

Optional nit: Perhaps |num_input_elements| or |input_element_count| or something
like that, to indicate that this is a count rather than a collection of
elements?

[Garrett Casto, 2012/09/10 21:44:23]

Done.

[Garrett Casto, 2012/09/10 21:44:23]

Done.

   for (size_t i = 0; i < control_elements.size(); i++) {
     WebKit::WebInputElement* input_element =
         toWebInputElement(&control_elements[i]);
     // Only pay attention to visible password fields.
     if (input_element &&
-        input_element->isPasswordField() &&
+        input_element->isTextField() &&
         input_element->hasNonEmptyBoundingBox()) {
-      passwords->push_back(*input_element);
+      input_elements++;
+      if (input_element->isPasswordField())
+        passwords->push_back(*input_element);
     }
   }
 
-  // For now, just assume that if there are two password fields in the
-  // form that this is meant for account creation.
-  // TODO(gcasto): Determine better heauristics for this.
-  if (passwords->size() == 2)
+  // This may be too lenient, but we assume that any form with at least three
+  // input elements where at least one of them is a password is an account
+  // creation form.
+  if (!passwords->empty() && input_elements >= 3) {
+    // We trim |passwords| because occasionally there are forms where the
+    // security question answers are put in password fields and we don't want
+    // to fill those.
+    if (passwords->size() > 2)
+      passwords->resize(2);
+
     return true;
+  }
 
   return false;
 }
@@ -103,7 +113,7 @@ void PasswordGenerationManager::DidFinishLoad(WebKit::WebFrame* frame) {
     scoped_ptr<webkit::forms::PasswordForm> password_form(
         webkit::forms::PasswordFormDomManager::CreatePasswordForm(forms[i]));
     if (!password_form.get()) {
-      DVLOG(2) << "Invalid action on form";
+      DVLOG(2) << "Skipping form as it would not be saved";
       continue;
     }
 
@@ -168,14 +178,15 @@ void PasswordGenerationManager::handleClick(WebKit::WebInputElement& element) {
   scoped_ptr<webkit::forms::PasswordForm> password_form(
       webkit::forms::PasswordFormDomManager::CreatePasswordForm(
           element.form()));
-  if (password_form.get()) {
-    Send(new AutofillHostMsg_ShowPasswordGenerationPopup(routing_id(),
-                                                         rect,
-                                                         element.maxLength(),
-                                                         *password_form));
-    password_generation::LogPasswordGenerationEvent(
-        password_generation::BUBBLE_SHOWN);
-  }
+  // We should not have shown the icon in this case.

[Ilya Sherman, 2012/09/10 21:23:49]

nit: What case does "in this case" refer to?  Could you reword this comment to
be a little bit more explicit?

[Garrett Casto, 2012/09/10 21:44:23]

Done.

+  DCHECK(password_form.get());
+
+  Send(new AutofillHostMsg_ShowPasswordGenerationPopup(routing_id(),
+                                                       rect,
+                                                       element.maxLength(),
+                                                       *password_form));
+  password_generation::LogPasswordGenerationEvent(
+      password_generation::BUBBLE_SHOWN);
 }
 
 void PasswordGenerationManager::willDetach(