Processing USERNAME reply from Autofill server in Password Manager

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..8e7b1dd98be31d70c8242d4edc976ed3bb3eb3a2 100644
--- a/components/autofill/content/renderer/password_form_conversion_utils.cc
+++ b/components/autofill/content/renderer/password_form_conversion_utils.cc
@@ -6,6 +6,7 @@
 
 #include "base/strings/string_util.h"
 #include "components/autofill/content/renderer/form_autofill_util.h"
+#include "components/autofill/core/common/form_data_predictions.h"
 #include "components/autofill/core/common/password_form.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
@@ -107,6 +108,57 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
   return true;
 }
 
+void FindPredictedUsernameElement(
+    const WebFormElement& form,
+    const std::vector<autofill::FormDataPredictions>* form_predictions,
+    WebVector<WebFormControlElement>* control_elements,
+    WebInputElement& predicted_username_element) {
+  if (!form_predictions)
+    return;

[Garrett Casto, 2015/03/24 01:31:12]

Nit: Generally we give a line of whitespace after early returns like this.

[dvadym, 2015/03/24 16:38:49]

Done.

+  FormData form_data;
+  if (!WebFormElementToFormData(
+          form, WebFormControlElement(), REQUIRE_AUTOCOMPLETE,

[Garrett Casto, 2015/03/24 01:31:12]

Why REQUIRE_AUTOCOMPLETE and these particular ExtractMask options? Per
AutofillAgent::QueryAutofillSuggestions(), it's possible to get data from forms
even if certain elements have autocomplete="off", so I don't think that we want
to give ourselves this extra requirement. As for the extact mask, we don't
explicitly look at the value or option values, and none of these values affect
SameFormAs(), so I don't think that their extraction is necessary.

[dvadym, 2015/03/24 16:38:49]

Thanks, it makes sense, I didn't dig into details and took this code from
another place. I changed REQUIRE_AUTOCOMPLETE -> REQUIRE_NONE and mask to
EXTRACT_NONE

+          static_cast<ExtractMask>(EXTRACT_VALUE | EXTRACT_OPTION_TEXT |
+                                   EXTRACT_OPTIONS),
+          &form_data, nullptr))
+    return;

[Garrett Casto, 2015/03/24 01:31:12]

Nit: Newline.

[dvadym, 2015/03/24 16:38:49]

Done.

+  // Prediction forms are not user submitted, but |form| can be user submitted.
+  // We don't care about this flag for finding predictions, so set it to false.
+  form_data.user_submitted = false;

[Garrett Casto, 2015/03/24 01:31:12]

It's a little strange to me that user_submitted is considered form identifying.
It seems similar to the value of the field to me. We may want to ask whoever
reviews this from the Autofill side why this is.

[dvadym, 2015/03/24 16:38:49]

Yeah, for me it also seems strange. Anyway we need someone to review changes
from Autofill side and I'll ask him to review this issue.

+  const autofill::FormDataPredictions* prediction = nullptr;
+  for (auto& form_prediction : *form_predictions) {
+    if (form_prediction.data.SameFormAs(form_data)) {
+      prediction = &form_prediction;
+      break;
+    }
+  }
+  if (!prediction)
+    return;

[Garrett Casto, 2015/03/24 01:31:12]

Nit: Newline.

[dvadym, 2015/03/24 16:38:49]

Done.

+  std::vector<blink::WebFormControlElement> autofillable_elements =
+      ExtractAutofillableElementsFromSet(*control_elements, REQUIRE_NONE);

[Garrett Casto, 2015/03/24 01:31:12]

What do you think about changing this so that instead of the browser sending a
vector of FormDataPredictions, it sends a map of FormData->FormFieldData for
each form that has a username set. That means that the following code would
instead be something like 

for element in control_element:
WebFormControlElementToFormField(element, &form_field)
if form_field.SameFieldAs(username_field):
...

The advantages are we don't send superfluous data to the renderer (and don't
have to iterate through it repeatedly), we don't have to duplicate checking the
server type (in case we change the logic there), and I think that it makes the
code slightly simpler. What do you think?

[dvadym, 2015/03/24 16:38:49]

Yeah, I also think it's more clear. I'm surprised why map are rarely used in
such situations in Chrome, usually for by vector element is used. I've changed
to map<>.

+  if (autofillable_elements.size() != prediction->fields.size()) {
+    // Keep things simple.  Don't use predictions for forms that were modified
+    // between page load and the server's response to our query.
+    return;
+  }
+
+  for (size_t i = 0; i < autofillable_elements.size(); ++i) {
+    if (prediction->fields[i].server_type != "USERNAME" &&
+        prediction->fields[i].server_type != "USERNAME_AND_EMAIL_ADDRESS")
+      continue;
+    WebFormControlElement& element = autofillable_elements[i];

[Garrett Casto, 2015/03/24 01:31:12]

Nit: Move the whitespace up a line. |element| logically goes with the if
statement following it, not the one proceeding it.

[dvadym, 2015/03/24 16:38:49]

Done.

+
+    if (base::string16(element.nameForAutofill()) !=
+        prediction->data.fields[i].name) {
+      // Keep things simple.  Don't show predictions for elements whose names
+      // were modified between page load and the server's response to our query.
+      continue;
+    }
+    predicted_username_element = *toWebInputElement(&element);
+    break;
+  }
+}
+
 // Get information about a login form encapsulated in a PasswordForm struct.
 // If an element of |form| has an entry in |nonscript_modified_values|, the
 // associated string is used instead of the element's value to create
@@ -115,7 +167,8 @@ void GetPasswordForm(
     const WebFormElement& form,
     PasswordForm* password_form,
     const std::map<const blink::WebInputElement, blink::WebString>*
-        nonscript_modified_values) {
+        nonscript_modified_values,
+    const std::vector<autofill::FormDataPredictions>* form_predictions) {
   WebInputElement latest_input_element;
   WebInputElement username_element;
   password_form->username_marked_by_site = false;
@@ -195,6 +248,14 @@ void GetPasswordForm(
     }
   }
 
+  WebInputElement predicted_username_element;
+  FindPredictedUsernameElement(form, form_predictions, &control_elements,
+                               predicted_username_element);
+  if (!predicted_username_element.isNull()) {
+    username_element = predicted_username_element;
+    password_form->parsed_using_autofill_predictions = true;
+  }
+
   if (!username_element.isNull()) {
     password_form->username_element = username_element.nameForAutofill();
     base::string16 username_value = username_element.value();
@@ -273,12 +334,14 @@ void GetPasswordForm(
 scoped_ptr<PasswordForm> CreatePasswordForm(
     const WebFormElement& web_form,
     const std::map<const blink::WebInputElement, blink::WebString>*
-        nonscript_modified_values) {
+        nonscript_modified_values,
+    const std::vector<autofill::FormDataPredictions>* form_predictions) {
   if (web_form.isNull())
     return scoped_ptr<PasswordForm>();
 
   scoped_ptr<PasswordForm> password_form(new PasswordForm());
-  GetPasswordForm(web_form, password_form.get(), nonscript_modified_values);
+  GetPasswordForm(web_form, password_form.get(), nonscript_modified_values,
+                  form_predictions);
 
   if (!password_form->action.is_valid())
     return scoped_ptr<PasswordForm>();