Update Confirmation UI for saved password change

components/password_manager/core/browser/password_form_manager_unittest.cc

Index: components/password_manager/core/browser/password_form_manager_unittest.cc
diff --git a/components/password_manager/core/browser/password_form_manager_unittest.cc b/components/password_manager/core/browser/password_form_manager_unittest.cc
index 500d81f3b1426d0c70d6212c6d749cfd39830468..a38b064ff9f4011e4defe6285ba1b4f1b3a4516b 100644
--- a/components/password_manager/core/browser/password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_form_manager_unittest.cc
@@ -104,7 +104,8 @@ class TestPasswordManagerClient : public StubPasswordManagerClient {
  public:
   explicit TestPasswordManagerClient(PasswordStore* password_store)
       : password_store_(password_store),
-        driver_(new NiceMock<MockPasswordManagerDriver>) {
+        driver_(new NiceMock<MockPasswordManagerDriver>),
+        is_update_password_ui_enabled_(false) {
     prefs_.registry()->RegisterBooleanPref(prefs::kPasswordManagerSavingEnabled,
                                            true);
   }
@@ -132,12 +133,20 @@ class TestPasswordManagerClient : public StubPasswordManagerClient {
 
   void KillDriver() { driver_.reset(); }
 
+  bool IsUpdatePasswordUIEnabled() const override {
+    return is_update_password_ui_enabled_;
+  }
+  void set_is_update_password_ui_enabled(bool value) {
+    is_update_password_ui_enabled_ = value;
+  }
+
  private:
   autofill::PasswordForm form_to_filter_;
 
   TestingPrefServiceSimple prefs_;
   PasswordStore* password_store_;
   scoped_ptr<MockPasswordManagerDriver> driver_;
+  bool is_update_password_ui_enabled_;
 };
 
 class TestPasswordManager : public PasswordManager {
@@ -1572,6 +1581,63 @@ TEST_F(PasswordFormManagerTest, TestSuggestingPasswordChangeForms) {
   EXPECT_TRUE(password_manager.GetLatestWaitForUsername());
 }
 
+TEST_F(PasswordFormManagerTest, TestUpdateMethod) {
+  // Add a new password field to the test form. The PasswordFormManager should
+  // save the password from this field, instead of the current password field.
+  observed_form()->new_password_element = ASCIIToUTF16("NewPasswd");
+
+  // Given that |observed_form| was most likely a change password form, it
+  // should not serve as a source for updating meta-information stored with the
+  // old credentials, such as element names, as they are likely going to be
+  // different between change password and login forms. To test this in depth,
+  // forcibly wipe |submit_element|, which should normally trigger updating this
+  // field from |observed_form| in the UpdateLogin() step as a special case. We
+  // will verify in the end that this did not happen.
+  saved_match()->submit_element.clear();
+
+  TestPasswordManagerClient client_with_store(mock_store());
+  client_with_store.set_is_update_password_ui_enabled(true);
+  PasswordFormManager manager(nullptr, &client_with_store,
+                              client_with_store.driver(), *observed_form(),
+                              false);
+  SimulateMatchingPhase(&manager, RESULT_MATCH_FOUND);
+
+  // User submits current and new credentials to the observed form.
+  PasswordForm credentials(*observed_form());
+  credentials.username_element.clear();
+  credentials.password_value = saved_match()->password_value;
+  credentials.new_password_value = ASCIIToUTF16("test2");
+  credentials.preferred = true;
+  manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  // Successful login. The PasswordManager would instruct PasswordFormManager
+  // to save, and since this is an update, it should know not to save as a new
+  // login.
+  EXPECT_FALSE(manager.IsNewLogin());
+
+  // By now, the PasswordFormManager should have promoted the new password value

[vasilii, 2015/07/31 14:38:54]

The comment says that it didn't happen in fact.

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

I took this comment from line 563, since I've copied part of that tests. But it
seems to be correct, |credentials| is initial credentials with new passsword,
GetPendingCredentials(&manager) returns credentials from PasswordFormManager
which are mentioned in the comment. 

+  // already to be the current password, and should no longer maintain any info
+  // about the new password.
+  EXPECT_EQ(credentials.new_password_value,
+            GetPendingCredentials(&manager)->password_value);
+  EXPECT_TRUE(GetPendingCredentials(&manager)->new_password_element.empty());
+  EXPECT_TRUE(GetPendingCredentials(&manager)->new_password_value.empty());
+
+  // Trigger saving to exercise some special case handling in UpdateLogin().
+  PasswordForm new_credentials;
+  EXPECT_CALL(*mock_store(), UpdateLogin(_))
+      .WillOnce(testing::SaveArg<0>(&new_credentials));
+  manager.Update(*saved_match());
+  Mock::VerifyAndClearExpectations(mock_store());
+
+  // No meta-information should be updated, only the password.
+  EXPECT_EQ(credentials.new_password_value, new_credentials.password_value);
+  EXPECT_EQ(saved_match()->username_element, new_credentials.username_element);
+  EXPECT_EQ(saved_match()->password_element, new_credentials.password_element);
+  EXPECT_EQ(saved_match()->submit_element, new_credentials.submit_element);

[vasilii, 2015/07/31 14:38:54]

Do I understand right that the new credential has the only different member
which is the password value. If this is the case just write
EXPECT_CALL(*mock_store(), UpdateLogin(new_credentials)); with the correct
|new_credentials|

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

No, it can contain a lot of different members, that are not related for my
changes and the behavior how they are changed can be changed in future CLs. For
example it will contain update of usage statistics (you can see the method
PasswordFormManager::UpdateLogin() which is called as result of Update()). So I
think it's better to check only relevant fields.

+}
+
 TEST_F(PasswordFormManagerTest, WipeStoreCopyIfOutdated_BeforeStoreCallback) {
   PasswordForm form(*saved_match());
   ASSERT_FALSE(form.password_value.empty());