Update Confirmation UI for saved password change

chrome/browser/password_manager/password_manager_browsertest.cc

Index: chrome/browser/password_manager/password_manager_browsertest.cc
diff --git a/chrome/browser/password_manager/password_manager_browsertest.cc b/chrome/browser/password_manager/password_manager_browsertest.cc
index cee06e8e68d9f15ddf227e50ffb08e0a0c25ea34..6a8bf2622e601a52233e2d9b7d26046014e9b47f 100644
--- a/chrome/browser/password_manager/password_manager_browsertest.cc
+++ b/chrome/browser/password_manager/password_manager_browsertest.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/login/login_prompt.h"
 #include "chrome/browser/ui/login/login_prompt_test_utils.h"
+#include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -1547,9 +1548,8 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
   observing_autofill_client.Wait();
 }
 
-// Passwords from change password forms should only be offered for saving when
-// it is certain that the username is correct.
-IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, ChangePwdCorrect) {
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
+                       ChangePwdMarkedWithUsername) {
   NavigateToFile("/password/password_form.html");
 
   NavigationObserver observer(WebContents());
@@ -1566,7 +1566,7 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, ChangePwdCorrect) {
   EXPECT_TRUE(prompt_observer->IsShowingPrompt());
 }
 
-IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, ChangePwdIncorrect) {
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, ChangePwdWithUsername) {
   NavigateToFile("/password/password_form.html");
 
   NavigationObserver observer(WebContents());
@@ -1580,13 +1580,13 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, ChangePwdIncorrect) {
       "document.getElementById('chg_submit_button').click()";
   ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
   observer.Wait();
-  EXPECT_FALSE(prompt_observer->IsShowingPrompt());
+  EXPECT_TRUE(prompt_observer->IsShowingPrompt());
 }
 
 // As the two ChangePwd* tests above, only with submitting through
 // history.pushState().
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
-                       ChangePwdPushStateCorrect) {
+                       ChangePwdPushStateMarkedWithUsername) {
   NavigateToFile("/password/password_push_state.html");
 
   NavigationObserver observer(WebContents());
@@ -1605,7 +1605,7 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
 }
 
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
-                       ChangePwdPushStateIncorrect) {
+                       ChangePwdPushStateWithUsername) {
   NavigateToFile("/password/password_push_state.html");
 
   NavigationObserver observer(WebContents());
@@ -1620,7 +1620,7 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
       "document.getElementById('chg_submit_button').click()";
   ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
   observer.Wait();
-  EXPECT_FALSE(prompt_observer->IsShowingPrompt());
+  EXPECT_TRUE(prompt_observer->IsShowingPrompt());
 }
 
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, NoPromptOnBack) {
@@ -2020,5 +2020,86 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
   iframe_killed.Wait();
 }
 
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
+                       ChangePwdNoAccountStored) {
+  ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
+  NavigateToFile("/password/password_form.html");
+
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      password_manager::switches::kEnablePasswordChangeSupport);
+
+  // Fill a form and submit through a <input type="submit"> button.
+  NavigationObserver observer(WebContents());
+  scoped_ptr<PromptObserver> prompt_observer(

[vabr (Chromium), 2015/07/31 12:48:13]

Looks unused.

[dvadym, 2015/08/03 15:44:07]

Not it's used after adding checking of credentials

+      PromptObserver::Create(WebContents()));
+  ManagePasswordsUIController* ui_controller =
+      ManagePasswordsUIController::FromWebContents(WebContents());
+
+  std::string fill_and_submit =
+      "document.getElementById('chg_password_wo_username_field').value = "
+      "'old_pw';"
+      "document.getElementById('chg_new_password_wo_username_1').value = "
+      "'new_pw';"
+      "document.getElementById('chg_new_password_wo_username_2').value = "
+      "'new_pw';"
+      "document.getElementById('chg_submit_wo_username_button').click()";
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
+  observer.Wait();
+  // No credentials stored, so save bubble is shown.
+  EXPECT_EQ(password_manager::ui::PENDING_PASSWORD_STATE,
+            ui_controller->state());
+}

[vabr (Chromium), 2015/07/31 12:48:13]

Is there a way (here and in the other test) to check that the correct password
was saved?

[dvadym, 2015/08/03 15:44:07]

Good point, I've added check for these tests

+
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
+                       ChangePwd1AccountStored) {
+  ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      password_manager::switches::kEnablePasswordChangeSupport);
+  // At first let us save credentials to the PasswordManager.
+  NavigateToFile("/password/password_form.html");
+  NavigationObserver observer1(WebContents());
+  scoped_ptr<PromptObserver> prompt_observer(
+      PromptObserver::Create(WebContents()));
+  ManagePasswordsUIController* ui_controller =
+      ManagePasswordsUIController::FromWebContents(WebContents());
+  std::string fill_and_submit_signin =
+      "document.getElementById('username_field').value = 'temp';"
+      "document.getElementById('password_field').value = 'random';"
+      "document.getElementById('input_submit_button').click()";
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit_signin));
+  observer1.Wait();
+  EXPECT_EQ(password_manager::ui::PENDING_PASSWORD_STATE,
+            ui_controller->state());
+  prompt_observer->Accept();
+  // Check that credentials are stored.
+  password_manager::TestPasswordStore* password_store =

[vabr (Chromium), 2015/07/31 12:48:13]

Is it actually possible to just store the credentials to the store directly
without loading the page for that? I'm concerned about lengthy tests, which are
prone to flaking (also, I acknowledge that this is the case already for some
existing tests, but still would be nice to fix).

[vabr (Chromium), 2015/07/31 12:48:13]

Keeping the test password store in a naked pointer is not the best practice, but
since your CL did not start this you have a choice:
(1) Fix the new tests to use scoped_refptr, or
(2) at least insert a TODO(vabr) pointing to http://crbug.com/515870.

[dvadym, 2015/08/03 15:44:07]

Here we don't owe this TestPasswordStore, so it seems ok to use raw pointers.
Isn't it true?

[dvadym, 2015/08/03 15:44:07]

I've implemented such possibility and made direct adding PasswordForm to
PasswordStore

[vabr (Chromium), 2015/08/04 08:59:07]

For scoped_refptr (unlike scoped_ptr), there is no notion of a single owner --
anybody needing the object to live should increase its refcount. We might argue
whether some other piece of code keeps the store alive long enough for us not to
care, but this does not make the code clearer. Let's follow the standard
practice of keeping refcounted objects in scoped_refptr. (Note: the situation is
very different in Blink's refcounting, where performance is a big concern.)

[dvadym, 2015/08/04 14:34:46]

Done.

+      static_cast<password_manager::TestPasswordStore*>(
+          PasswordStoreFactory::GetForProfile(
+              browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS)
+              .get());
+  // Spin the message loop to make sure the password store had a chance to save
+  // the password.
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();
+  EXPECT_FALSE(password_store->IsEmpty());
+
+  // Check that password update bubble is shown.
+  NavigateToFile("/password/password_form.html");
+  NavigationObserver observer2(WebContents());
+  std::string fill_and_submit_change_password =
+      "document.getElementById('chg_password_wo_username_field').value = "
+      "'random';"
+      "document.getElementById('chg_new_password_wo_username_1').value = "
+      "'new_pw';"
+      "document.getElementById('chg_new_password_wo_username_2').value = "
+      "'new_pw';"
+      "document.getElementById('chg_submit_wo_username_button').click()";
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(),
+                                     fill_and_submit_change_password));
+  observer2.Wait();
+  EXPECT_EQ(password_manager::ui::PENDING_PASSWORD_UPDATE_STATE,
+            ui_controller->state());
+}
+
 }  // namespace password_manager