Loosen up heuristics for detecting account creation forms.

chrome/renderer/autofill/password_generation_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/autofill/password_generation_manager.h"
 
 #include "base/logging.h"
 #include "chrome/common/autofill_messages.h"
 #include "chrome/common/password_generation_util.h"
 #include "content/public/renderer/render_view.h"
@@ skipping 15 matching lines @@
 
 // Returns true if we think that this form is for account creation. |passwords|
 // is filled with the password field(s) in the form.
 bool GetAccountCreationPasswordFields(
     const WebKit::WebFormElement& form,
     std::vector<WebKit::WebInputElement>* passwords) {
   // Grab all of the passwords for the form.
   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;
 }
 
 } // namespace
 
 PasswordGenerationManager::PasswordGenerationManager(
     content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
       render_view_(render_view),
@@ skipping 34 matching lines @@
     // the future, as we only want to avoid creating passwords if the signin
     // form has autocomplete turned off.
     if (forms[i].isNull() || !forms[i].autoComplete())
       continue;
 
     // If we can't get a valid PasswordForm, we skip this form because the
     // the password won't get saved even if we generate it.
     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;
     }
 
     // Do not generate password for GAIA since it is used to retrieve the
     // generated paswords.
     GURL realm(password_form->signon_realm);
     if (realm == GURL(GaiaUrls::GetInstance()->gaia_login_form_realm()))
       continue;
 
     std::vector<WebKit::WebInputElement> passwords;
@@ skipping 44 matching lines @@
 
 WebKit::WebCString PasswordGenerationManager::imageNameForReadOnlyState() {
   return imageNameForNormalState();
 }
 
 void PasswordGenerationManager::handleClick(WebKit::WebInputElement& element) {
   gfx::Rect rect(element.decorationElementFor(this).boundsInViewportSpace());
   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(
     const WebKit::WebInputElement& element) {
   // No implementation
 }
 
 bool PasswordGenerationManager::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(PasswordGenerationManager, message)
@@ skipping 46 matching lines @@
       passwords_[0].decorationElementFor(this).setAttribute("style",
                                                             "display:block");
       password_generation::LogPasswordGenerationEvent(
           password_generation::ICON_SHOWN);
       return;
     }
   }
 }
 
 }  // namespace autofill