[Passwords] Properly update password in both stores

This CL implements the agreed upon proposal in this doc
https://docs.google.com/document/d/1ZJnlpeqdhd_a-ypovOmHLrIPYbbdlK8h3OIggGuj5G4/edit?usp=sharing

By making sure the following fields are preserved when updating password
in MultiStorePasswordSaveManager

date_created
date_synced
times_used
moving_blocked_for_list

times_used is incremented upon update.

Bug: 1012203
Change-Id: Ife4731381f317efddab032fe416d835c0ec86228
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257932
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781290}

components/password_manager/core/browser/multi_store_password_save_manager.cc

@@ -94,6 +94,20 @@ PendingCredentialsState ResolvePendingCredentialsStates(
   return PendingCredentialsState::NONE;
 }
 
+// Returns a PasswordForm that has all fields taken from |update| except
+// date_created, date_synced, times_used and moving_blocked_for_list that are
+// taken from |original_form|.
+PasswordForm UpdateFormPreservingDifferentFieldsAcrossStores(
+    const PasswordForm& original_form,
+    const PasswordForm& update) {
+  PasswordForm result(update);
+  result.date_created = original_form.date_created;
+  result.date_synced = original_form.date_synced;
+  result.times_used = original_form.times_used;
+  result.moving_blocked_for_list = original_form.moving_blocked_for_list;
+  return result;
+}
+
 }  // namespace
 
 MultiStorePasswordSaveManager::MultiStorePasswordSaveManager(
@@ -152,11 +166,18 @@ void MultiStorePasswordSaveManager::SavePendingToStoreImpl(
     case PendingCredentialsState::EQUAL_TO_SAVED_MATCH: {
       // If the submitted credentials exists in both stores,
       // |pending_credentials_| might be from the account store (and thus not
-      // have a moving_blocked_for_list). We need to preserve any existing list,
-      // so explicitly copy it over from the profile store match.
-      PasswordForm form_to_update(pending_credentials_);
-      form_to_update.moving_blocked_for_list =
-          states.similar_saved_form_from_profile_store->moving_blocked_for_list;
+      // have a moving_blocked_for_list). We need to preserve any existing list.
+      // Same applies for other fields. Check the comment on
+      // UpdateFormPreservingDifferentFieldsAcrossStores().
+      PasswordForm form_to_update =
+          UpdateFormPreservingDifferentFieldsAcrossStores(
+              *states.similar_saved_form_from_profile_store,
+              pending_credentials_);
+      // For other cases, |pending_credentials_.times_used| is updated in
+      // UpdateMetadataForUsage() invoked from UploadVotesAndMetrics().
+      // UpdateFormPreservingDifferentFieldsAcrossStores() preserved the
+      // original times_used, and hence we should increment it here.
+      form_to_update.times_used++;
       form_saver_->Update(form_to_update, profile_matches,
                           old_profile_password);
     } break;
@@ -179,10 +200,18 @@ void MultiStorePasswordSaveManager::SavePendingToStoreImpl(
       case PendingCredentialsState::EQUAL_TO_SAVED_MATCH: {
         // If the submitted credentials exists in both stores,
         // .|pending_credentials_| might be from the profile store (and thus
-        // has a moving_blocked_for_list). We need to clear it before storing to
-        // the account store.
-        PasswordForm form_to_update(pending_credentials_);
-        form_to_update.moving_blocked_for_list.clear();
+        // has a moving_blocked_for_list). We need to preserve any existing
+        // values. Same applies for other fields. Check the comment on
+        // UpdateFormPreservingDifferentFieldsAcrossStores().
+        PasswordForm form_to_update =
+            UpdateFormPreservingDifferentFieldsAcrossStores(
+                *states.similar_saved_form_from_account_store,
+                pending_credentials_);
+        // For other cases, |pending_credentials_.times_used| is updated in
+        // UpdateMetadataForUsage() invoked from UploadVotesAndMetrics().
+        // UpdateFormPreservingDifferentFieldsAcrossStores() preserved the
+        // original times_used, and hence we should increment it here.
+        form_to_update.times_used++;
         account_store_form_saver_->Update(form_to_update, account_matches,
                                           old_account_password);
       } break;

components/password_manager/core/browser/multi_store_password_save_manager_unittest.cc

@@ -406,15 +406,26 @@ TEST_F(MultiStorePasswordSaveManagerTest, UpdateInBothStores) {
 TEST_F(MultiStorePasswordSaveManagerTest, AutomaticSaveInBothStores) {
   SetAccountStoreEnabled(/*is_enabled=*/true);
 
+  // Set different values for the fields that should be preserved per store
+  // (namely: date_created, date_synced, times_used, moving_blocked_for_list)
   PasswordForm saved_match_in_profile_store(saved_match_);
   saved_match_in_profile_store.username_value =
       parsed_submitted_form_.username_value;
   saved_match_in_profile_store.password_value =
       parsed_submitted_form_.password_value;
   saved_match_in_profile_store.in_store = PasswordForm::Store::kProfileStore;
+  saved_match_in_profile_store.date_created =
+      base::Time::Now() - base::TimeDelta::FromDays(10);
+  saved_match_in_profile_store.times_used = 10;
+  saved_match_in_profile_store.moving_blocked_for_list.push_back(
+      autofill::GaiaIdHash::FromGaiaId("email@gmail.com"));
 
   PasswordForm saved_match_in_account_store(saved_match_in_profile_store);
   saved_match_in_account_store.in_store = PasswordForm::Store::kAccountStore;
+  saved_match_in_account_store.date_created = base::Time::Now();
+  saved_match_in_account_store.date_synced = base::Time::Now();
+  saved_match_in_account_store.times_used = 5;
+  saved_match_in_account_store.moving_blocked_for_list.clear();
 
   SetNonFederatedAndNotifyFetchCompleted(
       {&saved_match_in_profile_store, &saved_match_in_account_store});
@@ -430,6 +441,7 @@ TEST_F(MultiStorePasswordSaveManagerTest, AutomaticSaveInBothStores) {
 
   // We still should update both credentials to update the |date_last_used| and
   // |times_used|. Note that |in_store| is irrelevant since it's not persisted.
+  // All other fields should be preserved.
   PasswordForm expected_profile_update_form(saved_match_in_profile_store);
   expected_profile_update_form.times_used++;
   expected_profile_update_form.date_last_used =