[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..3bf8b3dbc1b5e0b82d41250e56ef02a1d9491542 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"

[Ilya Sherman, 2015/03/19 21:25:50]

Existing Autofill code uses ICU for regexes.  I don't think we should have the
component depend on multiple regex libraries.

[vabr (Chromium), 2015/03/20 13:07:00]

Thanks for pointing this out, I did not know about it. Changed to ICU.

 
 using blink::WebDocument;
 using blink::WebFormControlElement;
@@ -22,6 +23,21 @@ 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) {
+  // NPN+P.* corresponds to a form which starts with a login section (NP) and
+  // continues with a sign-up section (N+PP.*). The aim is to distinguish such

[Ilya Sherman, 2015/03/19 21:25:50]

Is the end of the regex supposed to be "PP.* or "P.*"?  You use both.

[vabr (Chromium), 2015/03/20 13:07:00]

PP was a typo, thanks for pointing it out.

+  // forms from change password-forms (N*PPP?) and forms which use password

[Ilya Sherman, 2015/03/19 21:25:50]

Is there intentionally not a .* at the end of this regex?

[vabr (Chromium), 2015/03/20 13:07:00]

Not intentionally. It was meant as an example. Added .* to be consistent.

+  // fields to store private but non-password data (could look like, e.g.,
+  // PN+P.*). This is likely not bullet-proof, and if we see a form which is
+  // misclassified by this, we should consider improving the classification.
+  if (RE2::FullMatch(layout_sequence, "NPN+P.*"))
+    return PasswordForm::Layout::LAYOUT_LOGIN_AND_SIGNUP;
+  return PasswordForm::Layout::LAYOUT_OTHER;
+}

[Ilya Sherman, 2015/03/19 21:25:50]

Honestly, I do not think this code is very clear when written using regexes.  I
think it would be much more readable if you passed a vector of form field types,
and had the matching logic written more explicitly.  You could still keep the
high-level motivational comment written in terms of regexes if you want to,
though please keep in mind that it's a lot of effort for most human readers to
unpack a single-line regex into the logical pieces that underly it.

[vabr (Chromium), 2015/03/20 13:07:00]

I'm not sure I follow you here. What do you mean by a vector of form field
types? A vector of some kind of enum? Also, what do you mean by a more explicit
matching logic? Something like a finite-state automaton seems to me to be much
harder to read than a single regexp.
This is too vague for me -- I don't know how most human readers work, and I do
not understand what unpacking a regex into logical pieces mean. I tried to
improve the comment above the pattern definition, please let me know what you
think about it.

[Ilya Sherman, 2015/03/20 22:43:07]

After trying to write this as a C++ method, I withdraw my objection to using
regexes -- it's just too fiddly to get an equivalent C++ implementation written.
What I mean is, it's hard to parse "NPN+P.*" as a regex that consists of three
parts: "NP" (login section), "N+P" (sign-up section), ".*" (other fields that we
don't care about).  And, hence, it's easy to make a typo that people will miss. 
I'm a big fan of Python's verbose regex syntax [1], which allows you to write
regexes split into their logical parts, instantly making the regexes much easier
to read.

[1] https://docs.python.org/2/library/re.html#re.VERBOSE

[vabr (Chromium), 2015/03/23 14:23:44]

Thanks for explanation. I tried to emulate the verbose mode by using the
implicit string literal concatenation and C++ comments. Please let me know if
you see further room for improvement.

+
 // 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 +141,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 +152,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 +219,7 @@ void GetPasswordForm(
       }
     }
   }
+  password_form->layout = SequenceToLayout(layout_sequence);
 
   if (!username_element.isNull()) {
     password_form->username_element = username_element.nameForAutofill();