Sending generated vote on password generation.

components/password_manager/core/browser/password_form_manager.cc

Index: components/password_manager/core/browser/password_form_manager.cc
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index 1a64f72688a33e5cc7c47eab90987332c898f515..4c70b66e7766a6fb3bcde925552969c67145cd2b 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -108,6 +108,7 @@ PasswordFormManager::PasswordFormManager(
               : std::vector<std::string>()),
       is_new_login_(true),
       has_generated_password_(false),
+      is_manual_generation_(false),
       password_overridden_(false),
       retry_password_form_password_update_(false),
       generation_available_(false),
@@ -590,7 +591,9 @@ void PasswordFormManager::SaveAsNewLogin() {
   // by password generation to help determine account creation sites.
   // Credentials that have been previously used (e.g. PSL matches) are checked
   // to see if they are valid account creation forms.
-  if (pending_credentials_.times_used == 0) {
+  if (has_generated_password_) {
+    UploadGeneratedVote();
+  } else if (pending_credentials_.times_used == 0) {
     if (!observed_form_.IsPossibleChangePasswordFormWithoutUsername())
       UploadPasswordForm(pending_credentials_.form_data, base::string16(),
                          autofill::PASSWORD, std::string());
@@ -661,7 +664,9 @@ void PasswordFormManager::UpdateLogin() {
   // Check to see if this form is a candidate for password generation.
   // Do not send votes on change password forms, since they were already sent in
   // Update() method.
-  if (!observed_form_.IsPossibleChangePasswordForm())
+  if (has_generated_password_) {
+    UploadGeneratedVote();
+  } else if (!observed_form_.IsPossibleChangePasswordForm())
     SendAutofillVotes(observed_form_, &pending_credentials_);
 
   UpdatePreferredLoginState(password_store);
@@ -792,6 +797,7 @@ bool PasswordFormManager::UploadPasswordForm(
   DCHECK(password_type == autofill::PASSWORD ||
          password_type == autofill::ACCOUNT_CREATION_PASSWORD ||
          autofill::NOT_ACCOUNT_CREATION_PASSWORD);
+  DCHECK(!has_generated_password_);
   autofill::AutofillManager* autofill_manager =
       client_->GetAutofillManagerForMainFrame();
   if (!autofill_manager || !autofill_manager->download_manager())
@@ -823,7 +829,7 @@ bool PasswordFormManager::UploadPasswordForm(
 
     autofill::ServerFieldTypeSet types;
     types.insert(type);
-    field->set_possible_types(types);
+    field->set_possible_types(std::move(types));

[vabr (Chromium), 2016/02/17 13:43:22]

The argument of set_possible_types is a const ref, so moving |types| does not
buy us anything. Recent discussions on chromium-dev [1] indicate that we will
not be moving towards moving arguments by default. Also, the std::move has the
danger that somebody might miss it and use |types| below later.

So I vote for reverting this line to not using std::move.

[1]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ZoAfW7GXUVI/fCYjk...

[dvadym, 2016/02/19 16:25:06]

Thanks for information about this discussions.

   }
   DCHECK(found_password_field);
   DCHECK(!should_find_username_field);
@@ -847,6 +853,7 @@ bool PasswordFormManager::UploadPasswordForm(
 bool PasswordFormManager::UploadChangePasswordForm(
     const autofill::ServerFieldType& password_type,
     const std::string& login_form_signature) {
+  DCHECK(!has_generated_password_);
   DCHECK(password_type == autofill::NEW_PASSWORD ||
          password_type == autofill::PROBABLY_NEW_PASSWORD ||
          autofill::NOT_NEW_PASSWORD);
@@ -906,7 +913,7 @@ bool PasswordFormManager::UploadChangePasswordForm(
 
     autofill::ServerFieldTypeSet types;
     types.insert(type);
-    field->set_possible_types(types);
+    field->set_possible_types(std::move(types));

[vabr (Chromium), 2016/02/17 13:43:22]

Ditto.

[dvadym, 2016/02/19 16:25:06]

Done.

   }
 
   // Force uploading as these events are relatively rare and we want to make
@@ -919,6 +926,58 @@ bool PasswordFormManager::UploadChangePasswordForm(
       login_form_signature, true /* observed_submission */);
 }
 
+bool PasswordFormManager::UploadGeneratedVote() {
+  DCHECK(has_generated_password_);
+  if (generation_element_.empty())
+    return false;
+  const autofill::FormData& form_data = observed_form_.form_data;
+  // Create FormStructure with field type information for uploading a vote.
+  FormStructure form_structure(form_data);

[vabr (Chromium), 2016/02/17 13:43:22]

nit: Why not inline form_data? It is short enough and not used below.

[dvadym, 2016/02/19 16:25:05]

Done.

+
+  autofill::AutofillManager* autofill_manager =
+      client_->GetAutofillManagerForMainFrame();
+  if (!autofill_manager->ShouldUploadForm(form_structure) ||
+      !form_structure.ShouldBeCrowdsourced())
+    return false;
+
+  autofill::ServerFieldTypeSet available_field_types;
+  available_field_types.insert(autofill::UNKNOWN_TYPE);
+  available_field_types.insert(autofill::PASSWORD);
+
+  autofill::PasswordGenerationEvent event = autofill::NO_GENERATION;
+  if (is_manual_generation_) {
+    event =
+        observed_form_.IsPossibleChangePasswordForm()
+            ? autofill::MANUALLY_TRIGGERED_GENERATION_ON_CHANGE_PASSWORD_FORM
+            : autofill::MANUALLY_TRIGGERED_GENERATION_ON_SIGN_UP_FORM;
+  } else {
+    event = observed_form_.IsPossibleChangePasswordForm()
+                ? autofill::
+                      AUTOMATICALLY_TRIGGERED_GENERATION_ON_CHANGE_PASSWORD_FORM
+                : autofill::AUTOMATICALLY_TRIGGERED_GENERATION_ON_SIGN_UP_FORM;
+  }
+
+  bool generation_field_found = false;
+  for (size_t i = 0; i < form_structure.field_count(); ++i) {
+    autofill::AutofillField* field = form_structure.field(i);
+    if (field->name == generation_element_) {
+      field->set_generation_event(event);
+      autofill::ServerFieldTypeSet types;
+      types.insert(autofill::PASSWORD);
+      field->set_possible_types(std::move(types));

[vabr (Chromium), 2016/02/17 13:43:22]

Remove std::move (see one of my comments above).

[dvadym, 2016/02/19 16:25:06]

Done.

+      generation_field_found = true;
+      break;
+    }
+  }
+
+  if (!generation_field_found)
+    return false;
+
+  return autofill_manager->download_manager()->StartUploadRequest(
+      form_structure, false /* was_autofilled */, available_field_types,
+      std::string(), true /* observed_submission */);
+}
+
 void PasswordFormManager::CreatePendingCredentials() {
   DCHECK(provisionally_saved_form_);
   base::string16 password_to_save(PasswordToSave(*provisionally_saved_form_));