Ignore autocomplete=off when password manager is saving the generated passwords during account crea…

chrome/browser/password_manager/password_manager_unittest.cc

Index: chrome/browser/password_manager/password_manager_unittest.cc
diff --git a/chrome/browser/password_manager/password_manager_unittest.cc b/chrome/browser/password_manager/password_manager_unittest.cc
index 1c0c97e7842b803f6c88824cbbb537907e344bfa..eb88b844d6b3847aae11720730da057fca8c15a4 100644
--- a/chrome/browser/password_manager/password_manager_unittest.cc
+++ b/chrome/browser/password_manager/password_manager_unittest.cc
@@ -75,7 +75,9 @@ class PasswordManagerTest : public ChromeRenderViewHostTestHarness {
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
-  PasswordForm MakeSimpleForm() {
+  // Make default value of |autocomplete| to be true, such that we only need to
+  // add tests in autocomplete=off cases.
+  PasswordForm MakeSimpleForm(bool autocomplete = true) {

[Ilya Sherman, 2012/12/06 22:55:59]

nit: Default function parameters are disallowed by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

The way I'd recommend doing this is not passing a parameter to this function,
and instead explicitly setting form.password_should_autocomplete = false on the
value returned from this function, for the tests that are testing that case.

[zysxqn, 2012/12/06 23:54:49]

Done.

     PasswordForm form;
     form.origin = GURL("http://www.google.com/a/LoginAuth");
     form.action = GURL("http://www.google.com/a/Login");
@@ -83,6 +85,7 @@ class PasswordManagerTest : public ChromeRenderViewHostTestHarness {
     form.password_element = ASCIIToUTF16("Passwd");
     form.username_value = ASCIIToUTF16("google");
     form.password_value = ASCIIToUTF16("password");
+    form.password_should_autocomplete = autocomplete;
     form.submit_element = ASCIIToUTF16("signIn");
     form.signon_realm = "http://www.google.com";
     return form;
@@ -107,6 +110,8 @@ MATCHER_P(FormMatches, form, "") {
          form.action == arg.action &&
          form.username_element == arg.username_element &&
          form.password_element == arg.password_element &&
+         form.password_should_autocomplete ==
+             arg.password_should_autocomplete &&
          form.submit_element == arg.submit_element;
 }
 
@@ -370,3 +375,58 @@ TEST_F(PasswordManagerTest, FillPasswordsOnDisabledManager) {
   manager()->OnPasswordFormsParsed(observed);
 }
 
+TEST_F(PasswordManagerTest, FormNotSubmitAutocompleteOff) {

[Ilya Sherman, 2012/12/06 22:55:59]

nit: Perhaps "Saved" rather than "Submit", or else omit "Not"?  (This also
applies on line 403 below.)

[zysxqn, 2012/12/06 23:54:49]

Done.

+  // Test password form with non-generated password will not be saved if
+  // autocomplete=off.
+  //
+  // This is mostly copied from FormSubmitEmptyStore.

[Ilya Sherman, 2012/12/06 22:55:59]

No need to include this line in the comment, though it suggests that extracting
shared functionality into a helper function might be useful for these tests.

[zysxqn, 2012/12/06 23:54:49]

Done.

+  std::vector<PasswordForm*> result;  // Empty password store.
+  EXPECT_CALL(delegate_, FillPasswordForm(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_, GetLogins(_,_))
+      .WillOnce(DoAll(WithArg<1>(InvokeConsumer(0, result)), Return(0)));
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm(false));
+  observed.push_back(form);
+  manager()->OnPasswordFormsParsed(observed);  // The initial load.
+  manager()->OnPasswordFormsRendered(observed);  // The initial layout.
+
+  // And the form submit contract is to call ProvisionallySavePassword.
+  manager()->ProvisionallySavePassword(form);
+
+  // Now the password manager waits for the navigation to complete. No call
+  // should be expected since the form will not be save.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(observed);  // The post-navigation layout.
+}

[Ilya Sherman, 2012/12/06 22:55:59]

This test isn't actually testing that the form isn't saved.  You probably want
to include the following lines:

EXPECT_CALL(delegate_,
            AddSavePasswordInfoBarIfPermitted(_)).Times(Exactly(0));
EXPECT_CALL(*store_, AddLogin(FormMatches(form))).Times(Exactly(0));

[zysxqn, 2012/12/06 23:54:49]

Done.

+
+TEST_F(PasswordManagerTest, GeneratedPasswordFormSubmitAutocompleteOff) {
+  // Test password form with generated password will still be saved if
+  // autocomplete=off.
+  //
+  // This is mostly copied from GenerartedPasswordFormSubmitEmptyStore.
+  std::vector<PasswordForm*> result;  // Empty password store.
+  EXPECT_CALL(delegate_, FillPasswordForm(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_, GetLogins(_,_))
+      .WillOnce(DoAll(WithArg<1>(InvokeConsumer(0, result)), Return(0)));
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm(false));
+  observed.push_back(form);
+  manager()->OnPasswordFormsParsed(observed);  // The initial load.
+  manager()->OnPasswordFormsRendered(observed);  // The initial layout.
+
+  // Simulate the user generating the password and submitting the form.
+  manager()->SetFormHasGeneratedPassword(form);
+  manager()->ProvisionallySavePassword(form);
+
+  // The user should not be presented with an infobar as they have already given
+  // consent. The form should be saved once navigation occurs.

[Ilya Sherman, 2012/12/06 22:55:59]

Where has the user given consent?  Does this test pass when run in isolation?

[Ilya Sherman, 2012/12/06 23:40:37]

Never mind, I think I understand what this means: The user has given consent by
asking Chrome to generate the password.  I think the comment could probably be
improved to make that a little clearer -- I read this to mean that the user had
already given consent by previously saving a password for this website/username
combo, or something like that.

[zysxqn, 2012/12/06 23:54:49]

This is copied from GeneratedPasswordFormSubmitEmptyStore. I think this
statement means user has given consent to save the password by using the
generated password. Yeah, for this test this is just "simulated" by calling
manager()->SetFormHasGeneratedpassword(form) above.

[zysxqn, 2012/12/06 23:54:49]

Done.

+  EXPECT_CALL(delegate_,
+              AddSavePasswordInfoBarIfPermitted(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
+
+  // Now the password manager waits for the navigation to complete.
+  observed.clear();
+  manager()->OnPasswordFormsParsed(observed);  // The post-navigation load.
+  manager()->OnPasswordFormsRendered(observed);  // The post-navigation layout.
+}