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..9e98f6d169f361314990812fbdeef6961666914c 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 @@
 

[vabr (Chromium), 2015/03/25 10:12:33]

Here you should #include <vector>.

[vabr (Chromium), 2015/03/26 09:43:55]

Ping.

[dvadym, 2015/03/26 12:29:12]

Done.

 #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,44 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
   return true;
 }
 
+void FindPredictedUsernameElement(
+    const WebFormElement& form,
+    const std::map<autofill::FormData, autofill::FormFieldData>*

[vabr (Chromium), 2015/03/25 10:12:33]

I suggest changing this to a const reference, and skip the call completely, if
the pointer to predictions is null.
That sounds like a better practice for performance, and will make the body of
this function shorter and clearer.

[dvadym, 2015/03/25 16:34:08]

Done.

+        form_predictions,
+    WebVector<WebFormControlElement>* control_elements,
+    WebInputElement& predicted_username_element) {
+  if (!form_predictions)
+    return;
+
+  FormData form_data;
+  if (!WebFormElementToFormData(form, WebFormControlElement(), REQUIRE_NONE,
+                                EXTRACT_NONE, &form_data, nullptr))
+    return;
+
+  // 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;
+  auto predictions_iterator = form_predictions->find(form_data);
+  if (predictions_iterator == form_predictions->end())
+    return;
+
+  std::vector<blink::WebFormControlElement> autofillable_elements =
+      ExtractAutofillableElementsFromSet(*control_elements, REQUIRE_NONE);
+  if (autofillable_elements.size() != form_data.fields.size()) {
+    NOTREACHED();

[vabr (Chromium), 2015/03/25 10:12:33]

Did you mean
DCHECK_EQ(autofillable_elements.size(), form_data.fields.size())
?

[dvadym, 2015/03/25 16:34:08]

Thanks, I didn't know about DCHECK_EQ

+  }
+
+  const autofill::FormFieldData& username_field = predictions_iterator->second;
+  autofill::FormFieldData form_field;
+  for (size_t i = 0; i < autofillable_elements.size(); ++i) {
+    if (form_data.fields[i].SameFieldAs(username_field)) {
+      predicted_username_element =
+          *toWebInputElement(&autofillable_elements[i]);
+      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 +154,9 @@ void GetPasswordForm(
     const WebFormElement& form,
     PasswordForm* password_form,
     const std::map<const blink::WebInputElement, blink::WebString>*
-        nonscript_modified_values) {
+        nonscript_modified_values,
+    const std::map<autofill::FormData, autofill::FormFieldData>*
+        form_predictions) {
   WebInputElement latest_input_element;
   WebInputElement username_element;
   password_form->username_marked_by_site = false;
@@ -195,6 +236,14 @@ void GetPasswordForm(
     }
   }
 
+  WebInputElement predicted_username_element;

[vabr (Chromium), 2015/03/25 10:12:33]

For performance reasons, I would also recommend putting the prediction check
before the for loop on lines 169-237, and making sure that if the username is
set already, we skip any parts of code trying to guess it.

[dvadym, 2015/03/25 16:34:07]

After offline discussing, it was decided to leave as now, namely sever side
prediction has priority over all other means to guess username (including
autocomplete='username'). 

+  FindPredictedUsernameElement(form, form_predictions, &control_elements,

[vabr (Chromium), 2015/03/25 10:12:33]

Following up on my comment on line 113, I suggest only calling this if
form_predictions are not null.

[dvadym, 2015/03/25 16:34:07]

Done.

+                               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 +322,15 @@ 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::map<autofill::FormData, autofill::FormFieldData>*
+        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>();