Processing USERNAME reply from Autofill server in Password Manager

components/password_manager/core/browser/password_manager_unittest.cc

Index: components/password_manager/core/browser/password_manager_unittest.cc
diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc
index a3cc8fc2cd5e185535fcbbe8030a4773d8b52119..06d8ebf67b8da20699c3cd67a9bf19728b83cc88 100644
--- a/components/password_manager/core/browser/password_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -1037,4 +1037,41 @@ TEST_F(PasswordManagerTest, SavingSignupForms_NoActionMatch) {
   form_to_save->Save();
 }
 
+TEST_F(PasswordManagerTest, FormSubmittedChangedWithAutofillResponse) {

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

The test name is a bit unclear, and the comment below does not help (it says
nothing about changing usernames based on username predictions).

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

Comment was fixed to 
  // Test that observing that a newly submitted form differs from provisionally
  // saved one due to autoflll predictions caused another field to be identified
as username.

+  // Test that observing a newly submitted form shows the save password bar.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(Exactly(0));
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm());
+  observed.push_back(form);
+  // The initial load.
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  // The initial layout.
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  // We simulate that |username_element| is changed due to autofill response.

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

The comment does not completely explain what is being done.
What about:
// Simulate that based on autofill server username prediction, the username of
the form changed from the default candidate ("Email") to something else. Set the
parsed_using_autofill_predictions bit to true to make sure that choice of
username is trusted further in the code.

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

Done.

+  form.username_element = ASCIIToUTF16("Username");
+  form.parsed_using_autofill_predictions = true;
+  // And the form submit contract is to call ProvisionallySavePassword.
+  manager()->ProvisionallySavePassword(form);
+
+  scoped_ptr<PasswordFormManager> form_to_save;
+  EXPECT_CALL(client_,
+              PromptUserToSavePasswordPtr(
+                  _, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
+
+  // Now the password manager waits for the navigation to complete.
+  observed.clear();
+  // The post-navigation load.
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  // The post-navigation layout.
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  ASSERT_TRUE(form_to_save.get());

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

nit: You can drop ".get()".

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

Done.

+  EXPECT_CALL(*store_, AddLogin(FormMatches(form)));

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

At this point, it is still not clear to me what this test tests. If not for
parsed_using_autofill_predictions being true, would this fail? Maybe this
warrants an explanation in a code comment.

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

I think that other added comments explain this. Namely that the form will be
rejected if this bit is not set

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

Acknowledged.

+
+  // Simulate saving the form, as if the info bar was accepted.
+  form_to_save->Save();
+}
+
 }  // namespace password_manager