[Password Generation] Save passwords even if heuristics determine submit failed

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 98a4af3b059d9c0c45ac48a0bb834cdc1b28f775..71a7533048471311b3c8f65fd3cdabe948fde095 100644
--- a/components/password_manager/core/browser/password_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -233,7 +233,9 @@ MATCHER_P(FormMatches, form, "") {
   return form.signon_realm == arg.signon_realm && form.origin == arg.origin &&
          form.action == arg.action &&
          form.username_element == arg.username_element &&
+         form.username_value == arg.username_value &&
          form.password_element == arg.password_element &&
+         form.password_value == arg.password_value &&
          form.new_password_element == arg.new_password_element &&
          form.submit_element == arg.submit_element;
 }
@@ -1028,4 +1030,177 @@ TEST_F(PasswordManagerTest, SubmitNotFetchedFromStoreForm) {
   form_to_save->Save();
 }
 
+TEST_F(PasswordManagerTest, PasswordGenerationForceSaving) {
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm());
+  observed.push_back(form);
+  // The initial load.

[vabr (Chromium), 2015/07/13 14:57:00]

optional nit: I'm not convinced that the comment here and on line 1039 is
actually useful, the method names are self-explanatory. In fact, I'm just
putting together a clean-up CL to remove these comments, so I suggest dropping
them here as well.

Similarly on lines 1062 and 1064 and in the rest of the tests.

[Garrett Casto, 2015/07/13 17:20:13]

Done.

+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  // The initial layout.
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  manager()->SetHasGeneratedPasswordForForm(&driver_, form, true);
+
+  // The user should not be presented with an infobar as they have already given

[vabr (Chromium), 2015/07/13 14:57:00]

nit: Alternative phrasing:
"be presented with an infobar" -> "need to confirm saving"
as not everywhere the UI is an infobar.

[Garrett Casto, 2015/07/13 17:20:13]

True, done.

+  // consent by using the generated password. The form should be saved once
+  // navigation occurs. The client will be informed that automatic saving has
+  // occured.
+  EXPECT_CALL(client_,
+              PromptUserToSavePasswordPtr(
+                  _, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
+      .Times(Exactly(0));
+  EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
+  scoped_ptr<PasswordFormManager> saved_form_manager;
+  EXPECT_CALL(client_, AutomaticPasswordSavePtr(_))
+      .Times(Exactly(1))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&saved_form_manager)));
+
+  // Simulate submission failing, with the same form being visible after
+  // navigation.
+  manager()->ProvisionallySavePassword(form);
+  manager()->OnPasswordFormsParsed(&driver_,
+                                   observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(&driver_, observed,
+                                     true);  // The post-navigation layout.
+}
+
+TEST_F(PasswordManagerTest, PasswordGenerationPasswordEdited) {
+  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);
+
+  manager()->SetHasGeneratedPasswordForForm(&driver_, form, true);
+
+  // Simulate user editing and submitting a different password. Verify that
+  // the edited password is the one that is saved.
+  form.password_value = ASCIIToUTF16("different_password");
+  manager()->ProvisionallySavePassword(form);
+
+  // The user should not be presented with an infobar as they have already given
+  // consent by using the generated password. The form should be saved once
+  // navigation occurs. The client will be informed that automatic saving has
+  // occured.

[vabr (Chromium), 2015/07/13 14:57:00]

typo: occurred

[Garrett Casto, 2015/07/13 17:20:13]

Done.

+  EXPECT_CALL(client_,
+              PromptUserToSavePasswordPtr(
+                  _, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
+      .Times(Exactly(0));
+  EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
+  scoped_ptr<PasswordFormManager> saved_form_manager;
+  EXPECT_CALL(client_, AutomaticPasswordSavePtr(_))
+      .Times(Exactly(1))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&saved_form_manager)));
+
+  // Simulate submission failing, with the same form being visible after
+  // navigation.
+  manager()->OnPasswordFormsParsed(&driver_,
+                                   observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(&driver_, observed,
+                                     true);  // The post-navigation layout.
+}
+
+TEST_F(PasswordManagerTest,
+       PasswordGenerationNoLongerGeneratedPasswordNotForceSaved_FailedSubmit) {

[vabr (Chromium), 2015/07/13 14:57:00]

In this test and the next one, it is not clear, what a "failed submit" means.
Looking just at the comments, these tests are the same, but only the second one
provisionally saves the form. Could you please clarify which types of submit
failures this deals with?

[Garrett Casto, 2015/07/13 17:20:13]

I only mention failed submit here. I can expound upon it in a comment.

+  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);
+
+  manager()->SetHasGeneratedPasswordForForm(&driver_, form, true);
+
+  // Simulate user removing generated password and adding a new one.
+  form.new_password_value = ASCIIToUTF16("different_password");
+  manager()->SetHasGeneratedPasswordForForm(&driver_, form, false);
+
+  // No infobar or prompt is shown if submission fails.
+  EXPECT_CALL(client_,
+              PromptUserToSavePasswordPtr(
+                  _, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
+      .Times(Exactly(0));
+  EXPECT_CALL(client_, AutomaticPasswordSavePtr(_)).Times(Exactly(0));
+
+  // Simulate submission failing, with the same form being visible after
+  // navigation.
+  manager()->OnPasswordFormsParsed(&driver_,
+                                   observed);  // The post-navigation load.

[vabr (Chromium), 2015/07/13 14:57:00]

Here we give a second reason (after not provisionally saving the form) for the
saving to fail. That sounds fishy, the tests should check one thing every time.

[Garrett Casto, 2015/07/13 17:20:13]

They are testing two different things. The two tests verify that if you generate
a password and then make it no longer generated 1) you are still prompted
normally if a submit occurs and 2) if submit fails we don't automatically save
(unlike with a generated password). I'll try to clear this up with comments.

+  manager()->OnPasswordFormsRendered(&driver_, observed,
+                                     true);  // The post-navigation layout.
+}
+
+TEST_F(PasswordManagerTest,
+       PasswordGenerationNoLongerGeneratedPasswordNotForceSaved) {
+  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);
+
+  manager()->SetHasGeneratedPasswordForForm(&driver_, form, true);
+
+  // Simulate user removing generated password and adding a new one.
+  form.password_value = ASCIIToUTF16("different_password");
+  manager()->SetHasGeneratedPasswordForForm(&driver_, form, false);
+
+  manager()->ProvisionallySavePassword(form);
+
+  // Verify that a normal prompt is shown instead of the force saving UI.
+  scoped_ptr<PasswordFormManager> form_to_save;
+  EXPECT_CALL(client_,
+              PromptUserToSavePasswordPtr(
+                  _, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
+  EXPECT_CALL(client_, AutomaticPasswordSavePtr(_)).Times(Exactly(0));
+
+  // Simulate submission failing, with the same form being visible after
+  // navigation.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(&driver_,
+                                   observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(&driver_, observed,
+                                     true);  // The post-navigation layout.
+}
+
+TEST_F(PasswordManagerTest, PasswordGenerationUsernameChanged) {
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm());
+  form.username_value = ASCIIToUTF16("");
+  observed.push_back(form);
+  // The initial load.
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  // The initial layout.
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  manager()->SetHasGeneratedPasswordForForm(&driver_, form, true);
+
+  // Simulate user changing the password and username, without every completely

[vabr (Chromium), 2015/07/13 14:57:00]

typo: every -> ever

[Garrett Casto, 2015/07/13 17:20:13]

Done.

+  // deleting the password.
+  form.password_value = ASCIIToUTF16("different_password");
+  form.username_value = ASCIIToUTF16("new_username");
+  manager()->ProvisionallySavePassword(form);
+
+  EXPECT_CALL(client_,
+              PromptUserToSavePasswordPtr(
+                  _, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
+      .Times(Exactly(0));
+  EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
+  scoped_ptr<PasswordFormManager> saved_form_manager;
+  EXPECT_CALL(client_, AutomaticPasswordSavePtr(_))
+      .Times(Exactly(1))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&saved_form_manager)));
+
+  observed.clear();
+  manager()->OnPasswordFormsParsed(&driver_,
+                                   observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(&driver_, observed,
+                                     true);  // The post-navigation layout.
+}
+
 }  // namespace password_manager