Refactor detection of account creation forms and fix two minor issues.

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 "content/public/renderer/render_view.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebDocument.h"
@@ skipping 10 matching lines @@
 namespace autofill {
 
 PasswordGenerationManager::PasswordGenerationManager(
     content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
       enabled_(false) {
   render_view->GetWebView()->addTextFieldDecoratorClient(this);
 }
 PasswordGenerationManager::~PasswordGenerationManager() {}
 
-void PasswordGenerationManager::DidFinishDocumentLoad(WebKit::WebFrame* frame) {
+void PasswordGenerationManager::DidFinishLoad(WebKit::WebFrame* frame) {
+  // Clear previous state.
+  passwords_.clear();
+
   // We don't want to generate passwords if the browser won't store or sync
   // them.
   if (!enabled_)
     return;
 
   if (!ShouldAnalyzeDocument(frame->document()))
     return;
 
   WebKit::WebVector<WebKit::WebFormElement> forms;
   frame->document().forms(forms);
   for (size_t i = 0; i < forms.size(); ++i) {
-    const WebKit::WebFormElement& web_form = forms[i];
-    if (web_form.isNull() || !web_form.autoComplete())
-      continue;
-
-    // Grab all of the passwords for each form.
-    WebKit::WebVector<WebKit::WebFormControlElement> control_elements;
-    web_form.getFormControlElements(control_elements);
-
     std::vector<WebKit::WebInputElement> passwords;
-    for (size_t i = 0; i < control_elements.size(); i++) {
-      WebKit::WebInputElement* input_element =
-          toWebInputElement(&control_elements[i]);
-      if (input_element && 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. Also, we assume that there
-    // is only one account creation field per URL.
-    // TODO(gcasto): Determine better heauristics for this.
-    if (passwords.size() == 2) {
+    if (IsAccountCreationForm(forms[i], &passwords)) {
+      DVLOG(2) << "Account creation form detected";
       passwords_ = passwords;
       // Make the decoration visible for this element.
       passwords[0].decorationElementFor(this).setAttribute("style",
                                                            "display:block");
+      // We assume that there is only one account creation field per URL.
       return;
     }
   }
 }
 
 bool PasswordGenerationManager::ShouldAnalyzeDocument(
     const WebKit::WebDocument& document) const {
   // Make sure that this security origin is allowed to use password manager.
   // Generating a password that can't be saved is a bad idea.
   WebKit::WebSecurityOrigin origin = document.securityOrigin();
   if (!origin.canAccessPasswordManager()) {
     DVLOG(1) << "No PasswordManager access";
     return false;
   }
 
   return true;
 }
 
+bool PasswordGenerationManager::IsAccountCreationForm(
+    const WebKit::WebFormElement& form,
+    std::vector<WebKit::WebInputElement>* passwords) {
+  // Ignore forms with autocomplete turned off for now. We may remove this in
+  // the future, as we only want to avoid creating passwords if the signin
+  // form has autocomplete turned off.
+  if (form.isNull() || !form.autoComplete())
+    return false;
+
+  // 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.
+  webkit::forms::PasswordForm* password_form(
+      webkit::forms::PasswordFormDomManager::CreatePasswordForm(form));
+  if (!password_form) {
+    DVLOG(2) << "Invalid action on form";
+    return false;
+  }
+
+  // Grab all of the passwords for the form.
+  WebKit::WebVector<WebKit::WebFormControlElement> control_elements;
+  form.getFormControlElements(control_elements);
+
+  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->hasNonEmptyBoundingBox())
+      passwords->push_back(*input_element);

[Ilya Sherman, 2012/06/08 05:22:18]

nit: We usually do include curly braces for if-stmts whose conditions span
multiple lines.

[Garrett Casto, 2012/06/08 20:15:09]

Done.

+  }
+
+  // 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)
+    return true;
+
+  return false;
+}
+
 bool PasswordGenerationManager::shouldAddDecorationTo(
     const WebKit::WebInputElement& element) {
   return element.isPasswordField();
 }
 
 bool PasswordGenerationManager::visibleByDefault() {
   return false;
 }
 
 WebKit::WebCString PasswordGenerationManager::imageNameForNormalState() {
@@ skipping 44 matching lines @@
     it->setValue(password);
     it->setAutofilled(true);
   }
 }
 
 void PasswordGenerationManager::OnPasswordGenerationEnabled(bool enabled) {
   enabled_ = enabled;
 }
 
 }  // namespace autofill