[Password Generation] Add negative votes for crowdsourcing

components/password_manager/core/browser/password_form_manager_unittest.cc

Index: components/password_manager/core/browser/password_form_manager_unittest.cc
diff --git a/components/password_manager/core/browser/password_form_manager_unittest.cc b/components/password_manager/core/browser/password_form_manager_unittest.cc
index 30b7e325daeac7c635ff8627f91dfc9ba0776955..98d126e5e49e7fe941a918e1fc5f3a4694389007 100644
--- a/components/password_manager/core/browser/password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_form_manager_unittest.cc
@@ -242,6 +242,43 @@ class PasswordFormManagerTest : public testing::Test {
     return p->ShouldIgnoreResult(*form);
   }
 
+  void AccountCreationUploadTest(const autofill::FormData& observed_form_data,

[vabr (Chromium), 2015/02/12 10:08:27]

Please add a comment about what this method does.

[Garrett Casto, 2015/02/12 22:15:36]

Done.

+                                 int times_used,
+                                 PasswordForm::GenerationUploadStatus status,
+                                 autofill::ServerFieldType* field_type) {

[vabr (Chromium), 2015/02/12 10:08:27]

nit: This should be const, to make it clear the pointer is here to allow nulls,
not to allow modification.

[Garrett Casto, 2015/02/12 22:15:35]

Done.

+    TestPasswordManagerClient client_with_store(mock_store());
+    TestPasswordManager password_manager(&client_with_store);
+    EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord())

[vabr (Chromium), 2015/02/12 10:08:27]

nit: Unless you really care about seeing this call, you should write
ON_CALL(...).WillByDefault(Return(false));

[Garrett Casto, 2015/02/12 22:15:35]

Done.

+        .WillRepeatedly(Return(false));
+
+    PasswordForm form(*observed_form());
+
+    form.form_data = observed_form_data;
+
+    PasswordFormManager form_manager(&password_manager, &client_with_store,
+                                     client_with_store.driver(), form, false);
+    std::vector<PasswordForm*> result;
+    result.push_back(CreateSavedMatch(false));
+    result[0]->generation_upload_status = status;
+    result[0]->times_used = times_used;
+
+    PasswordForm form_to_save(form);
+    form_to_save.preferred = true;
+    form_to_save.username_value = result[0]->username_value;
+    form_to_save.password_value = result[0]->password_value;
+
+    SimulateFetchMatchingLoginsFromPasswordStore(&form_manager);
+    SimulateResponseFromPasswordStore(&form_manager, result);

[vabr (Chromium), 2015/02/12 10:08:27]

If you rebase to ToT, this method was already replaced by just calling
OnGetPasswordStoreResults on form_manager. Also note that |result| now needs to
be a ScopedVector.

[Garrett Casto, 2015/02/12 22:15:35]

Done.

+
+    if (field_type) {

[vabr (Chromium), 2015/02/12 10:08:25]

Should we explicitly add a similar EXPECT_CALL(...).Times(0) for cases when
field_type is null? While it might not be necessary due to default settings of
the mock, it would be clearer to know that this check is supposed to fail if
there are uploads and it is not told which value to expect.

[Garrett Casto, 2015/02/12 22:15:35]

I think that by default you only get a warning not a failure, so I'll explicitly
set this.

+      EXPECT_CALL(*client_with_store.mock_driver()->mock_autofill_manager(),
+                  UploadPasswordForm(_, *field_type)).Times(1);
+    }
+    form_manager.ProvisionallySave(
+        form_to_save, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+    form_manager.Save();
+  }
+
   PasswordForm* observed_form() { return &observed_form_; }
   PasswordForm* saved_match() { return &saved_match_; }
   PasswordForm* CreateSavedMatch(bool blacklisted) {
@@ -1208,59 +1245,42 @@ TEST_F(PasswordFormManagerTest, UploadFormData_NewPassword) {
   Mock::VerifyAndClearExpectations(&blacklist_form_manager);
 }
 
-TEST_F(PasswordFormManagerTest, UploadFormData_AccountCreationPassword) {
-  TestPasswordManagerClient client_with_store(mock_store());
-  TestPasswordManager password_manager(&client_with_store);
-  EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord())
-      .WillRepeatedly(Return(false));
-
-  PasswordForm form(*observed_form());
-
+TEST_F(PasswordFormManagerTest, UploadPasswordForm) {
+  autofill::FormData form_data;
   autofill::FormFieldData field;
   field.label = ASCIIToUTF16("Email");
   field.name = ASCIIToUTF16("Email");
   field.form_control_type = "text";
-  form.form_data.fields.push_back(field);
+  form_data.fields.push_back(field);
 
   field.label = ASCIIToUTF16("password");
   field.name = ASCIIToUTF16("password");
   field.form_control_type = "password";
-  form.form_data.fields.push_back(field);
-
-  PasswordFormManager form_manager(&password_manager, &client_with_store,
-                                   client_with_store.driver(), form, false);
-  std::vector<PasswordForm*> result;
-  result.push_back(CreateSavedMatch(false));
-
-  field.label = ASCIIToUTF16("full_name");
-  field.name = ASCIIToUTF16("full_name");
-  field.form_control_type = "text";
-  result[0]->form_data.fields.push_back(field);
-
-  field.label = ASCIIToUTF16("Email");
-  field.name = ASCIIToUTF16("Email");
-  field.form_control_type = "text";
-  result[0]->form_data.fields.push_back(field);
-
-  field.label = ASCIIToUTF16("password");
-  field.name = ASCIIToUTF16("password");
-  field.form_control_type = "password";
-  result[0]->form_data.fields.push_back(field);
-
-  PasswordForm form_to_save(form);
-  form_to_save.preferred = true;
-  form_to_save.username_value = result[0]->username_value;
-  form_to_save.password_value = result[0]->password_value;
-
-  SimulateFetchMatchingLoginsFromPasswordStore(&form_manager);
-  SimulateResponseFromPasswordStore(&form_manager, result);
-
-  EXPECT_CALL(*client_with_store.mock_driver()->mock_autofill_manager(),
-              UploadPasswordForm(_, autofill::ACCOUNT_CREATION_PASSWORD))
-      .Times(1);
-  form_manager.ProvisionallySave(
-      form_to_save, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
-  form_manager.Save();
+  form_data.fields.push_back(field);
+
+  // Form data is different than saved form data, account creation signal should
+  // be sent.
+  autofill::ServerFieldType field_type = autofill::ACCOUNT_CREATION_PASSWORD;
+  AccountCreationUploadTest(form_data, 0, PasswordForm::NO_SIGNAL_SENT,
+                            &field_type);
+
+  // Non-zero times used will not upload since we only upload a positive signal
+  // at most once.
+  AccountCreationUploadTest(form_data, 1, PasswordForm::NO_SIGNAL_SENT,
+                            nullptr);
+
+  // Same form data as saved match and POSITIVE_SIGNAL_SENT means there should
+  // be a negative autofill ping sent.
+  field_type = autofill::NOT_ACCOUNT_CREATION_PASSWORD;
+  AccountCreationUploadTest(saved_match()->form_data, 2,
+                            PasswordForm::POSITIVE_SIGNAL_SENT, &field_type);
+
+  // For any other GenerationUplaodStatus, no autofill upload should occur
+  // if the observed form data matches the saved form data.
+  AccountCreationUploadTest(saved_match()->form_data, 3,
+                            PasswordForm::NO_SIGNAL_SENT, nullptr);
+  AccountCreationUploadTest(saved_match()->form_data, 3,
+                            PasswordForm::NEGATIVE_SIGNAL_SENT, nullptr);
 }
 
 TEST_F(PasswordFormManagerTest, CorrectlySavePasswordWithoutUsernameFields) {