[Password Manager] Fix to recognise failed login attempt for sites where content server pushes new …

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 e628923a135ea80ddcadd137103a16188e0ad70d..4138105ba877657bb3553ebec4aeced2def3d16b 100644
--- a/components/password_manager/core/browser/password_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -758,4 +758,42 @@ TEST_F(PasswordManagerTest, SyncCredentialsNotSaved) {
   manager()->OnPasswordFormsRendered(observed, true);
 }
 
+// Verify that the form submission resulting into concent-server pushing

[vabr (Chromium), 2014/08/26 09:42:59]

nit: Although generally correct, the sentence is a bit long and confusing (e.g.,
it uses "prompt" as a verb, while in most of the text in the code it's used as a
noun). Suggested reformulation, split into two sentences:

"On failed login attempts, the retry-form can have action scheme changed from
HTTP to HTTPS (see http://crbug.com/400769). Check that such retry-form is
considered equal to the original login form, and the attempt recognised as a
failure."

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+// seperate form where actions differing only in their schemes shall not prompt
+// password save popup dialog.
+TEST_F(PasswordManagerTest,
+       FormSubmitWithNavigateToNewFormDifferingActionsOnlyBySchemes) {

[vabr (Chromium), 2014/08/26 09:42:59]

nit: The test name does not state what is tested, and describes the situation in
a little bit confusing way. What about:

SeeingFormActionWithOnlyHttpHttpsChangeIsLoginFailure

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+  std::vector<PasswordForm*> result;  // Empty password store.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_.get(), GetLogins(_, _, _))
+      .WillRepeatedly(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
+  PasswordForm first_form(MakeSimpleForm());
+  first_form.origin = GURL("http://www.xda-developers.com/");
+  first_form.action = GURL("http://forum.xda-developers.com/login.php");
+  first_form.signon_realm = "http://www.xda-developers.com/";
+  PasswordForm second_form(MakeSimpleForm());

[vabr (Chromium), 2014/08/26 09:42:59]

nit: Create this form from the previous, to emphasise they are almost the same
(and shorten the initialisation below):

PasswordForm second_form(first_form);

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+  second_form.origin = GURL("http://forum.xda-developers.com/login.php");

[vabr (Chromium), 2014/08/26 09:42:59]

The origin is not related to the tested change, so although in the real example
the origin differed in this way, here you can keep it the same. As a bonus, it
will be clearer, what the change is.

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+  second_form.action = GURL("https://forum.xda-developers.com/login.php");

[vabr (Chromium), 2014/08/26 09:42:59]

nit: Please add a comment emphasising that the change is in the scheme (it's
hard to spot).

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+  second_form.signon_realm = "http://www.xda-developers.com/";

[vabr (Chromium), 2014/08/26 09:42:59]

You can drop this if you create second_form based on first_form.

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+
+  std::vector<PasswordForm> observed;
+  observed.push_back(first_form);
+  manager()->OnPasswordFormsParsed(observed);
+  observed.clear();
+  manager()->OnPasswordFormsRendered(observed, true);

[vabr (Chromium), 2014/08/26 09:42:59]

Why do you clear |observed| before calling OnPasswordFormsRendered? If we
pretend the |first_form| to be the one submitted, it should be among the ones
which were visible (i.e., reported to OnPasswordFormsRendered).

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+
+  // Now submit the first form.
+  OnPasswordFormSubmitted(first_form);
+
+  // This page contains a form with the same action, but on a different scheme,

[vabr (Chromium), 2014/08/26 09:42:59]

nit: No need to comment on the difference, it should be clear from the
definition of second_form above. Instead, perhaps mention what putting it to
|observed| means:

// Simulate loading a page, which contains |second_form| instead of
|first_form|.

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+  // i.e. "http://" to "https://".
+  observed.push_back(second_form);
+
+  // Verify that not to prompt with password save popup dialog.

[vabr (Chromium), 2014/08/26 09:42:59]

grammar nit: "Verify that no prompt to save the password is shown."

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+  EXPECT_CALL(client_, PromptUserToSavePasswordPtr(_)).Times(Exactly(0));
+  manager()->OnPasswordFormsParsed(observed);
+  manager()->OnPasswordFormsRendered(observed, true);
+  observed.clear();
+}
+
 }  // namespace password_manager