[Password manager] Recognise squashed login+sign-up forms

components/autofill/content/renderer/password_form_conversion_utils.cc

Index: components/autofill/content/renderer/password_form_conversion_utils.cc
diff --git a/components/autofill/content/renderer/password_form_conversion_utils.cc b/components/autofill/content/renderer/password_form_conversion_utils.cc
index dd97e9aaba7621fba147504501066f4e459358d6..38b30e8dd48e9938f30571cf5771222b9b319f62 100644
--- a/components/autofill/content/renderer/password_form_conversion_utils.cc
+++ b/components/autofill/content/renderer/password_form_conversion_utils.cc
@@ -11,6 +11,7 @@
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFormControlElement.h"
 #include "third_party/WebKit/public/web/WebInputElement.h"
+#include "third_party/re2/re2/re2.h"
 
 using blink::WebDocument;
 using blink::WebFormControlElement;
@@ -22,6 +23,15 @@ using blink::WebVector;
 namespace autofill {
 namespace {
 
+// Given the sequence of non-password and password text input fields of a form,
+// represented as a string of Ns (non-password) and Ps (password), computes the
+// layout type of that form.
+PasswordForm::Layout SequenceToLayout(re2::StringPiece layout_sequence) {
+  if (RE2::FullMatch(layout_sequence, "NPN+PP"))

[Garrett Casto, 2015/03/17 22:49:37]

As written this is over constrained and doesn't catch either godaddy.com or
nordstrom.com (godaddy has only one password field for the signup portion and
nordstrom has a text field after the last password field).

We want this to be as simple as possible and still allow sites like godaddy to
work but not negatively impact more traditional forms. The best regexp that I
can think of that makes both broken sites work is "NPN+P.*". I can't think of
examples right now that this would match that aren't combined forms. I could
imagine a signup form that could match this ("username, password, random
information, security question, security answer (password field)"), but I would
think such a construct would be rare. Even if it were to accidentally get
labeled as LAYOUT_LOGIN_AND_SIGNUP, filling saved credentials on a signup form,
while bad, isn't a disaster.

Does that regexp change seem reasonable to you?

[vabr (Chromium), 2015/03/18 16:51:03]

Ah, I forgot to get back from my distilled toy examples to the real world.
Thanks for pointing this out!
It does, I agree with your view of the severity of the potential breakage, and
cannot think of a page I know is already broken by that.

+    return PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP;
+  return PasswordForm::Layout::LAYOUT_OTHER;
+}
+
 // Checks in a case-insensitive way if the autocomplete attribute for the given
 // |element| is present and has the specified |value_in_lowercase|.
 bool HasAutocompleteAttributeValue(const WebInputElement& element,
@@ -125,6 +135,8 @@ void GetPasswordForm(
   WebVector<WebFormControlElement> control_elements;
   form.getFormControlElements(control_elements);
 
+  std::string layout_sequence;
+  layout_sequence.reserve(control_elements.size());
   for (size_t i = 0; i < control_elements.size(); ++i) {
     WebFormControlElement control_element = control_elements[i];
     if (control_element.isActivatedSubmit())
@@ -134,6 +146,13 @@ void GetPasswordForm(
     if (!input_element || !input_element->isEnabled())
       continue;
 
+    if (input_element->isTextField()) {
+      if (input_element->isPasswordField())
+        layout_sequence.push_back('P');
+      else
+        layout_sequence.push_back('N');
+    }
+
     if (input_element->isPasswordField()) {
       passwords.push_back(*input_element);
       // If we have not yet considered any element to be the username so far,
@@ -194,6 +213,7 @@ void GetPasswordForm(
       }
     }
   }
+  password_form->layout = SequenceToLayout(layout_sequence);
 
   if (!username_element.isNull()) {
     password_form->username_element = username_element.nameForAutofill();