Filter deletions in PasswordSyncBridge::GetUnsyncedPasswordsStorageKeys

Previous to this CL, the method returned storage keys for deletion
entries. This was not a problem for the existing callers, but this CL
makes it sound by filtering those out and adding more tests.

Bug: 1060132
Change-Id: Id7f30cc25aebcbc4ab2df2c54af9f796f56c929e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141915
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757378}

components/password_manager/core/browser/sync/password_sync_bridge.cc

@@ -846,7 +846,9 @@ std::set<int> PasswordSyncBridge::GetUnsyncedPasswordsStorageKeys() {
   std::unique_ptr<syncer::MetadataBatch> batch =
       metadata_store->GetAllSyncMetadata();
   for (const auto& metadata_entry : batch->GetAllMetadata()) {
-    if (change_processor()->IsEntityUnsynced(metadata_entry.first)) {
+    // Ignore unsynced deletions.
+    if (!metadata_entry.second->is_deleted() &&
+        change_processor()->IsEntityUnsynced(metadata_entry.first)) {
       storage_keys.insert(ParsePrimaryKey(metadata_entry.first));
     }
   }

components/password_manager/core/browser/sync/password_sync_bridge_unittest.cc

@@ -902,32 +902,53 @@ TEST_F(PasswordSyncBridgeTest, ShouldNotifyUnsyncedCredentialsIfAccountStore) {
   ON_CALL(*mock_password_store_sync(), IsAccountStore())
       .WillByDefault(Return(true));
 
-  const int kPrimaryKeyUnsyncedEntry = 1000;
-  const int kPrimaryKeySyncedEntry = 1001;
-  const std::string kPrimaryKeyUnsyncedEntryStr = "1000";
-  const std::string kPrimaryKeySyncedEntryStr = "1001";
-  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeyUnsyncedEntryStr))
+  const int kPrimaryKeyUnsyncedCredential = 1000;
+  const int kPrimaryKeySyncedCredential = 1001;
+  const int kPrimaryKeyUnsyncedDeletion = 1002;
+  const std::string kPrimaryKeyUnsyncedCredentialStr = "1000";
+  const std::string kPrimaryKeySyncedCredentialStr = "1001";
+  const std::string kPrimaryKeyUnsyncedDeletionStr = "1002";
+  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeyUnsyncedCredentialStr))
       .WillByDefault(Return(true));
-  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeySyncedEntryStr))
+  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeySyncedCredentialStr))
       .WillByDefault(Return(false));
+  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeyUnsyncedDeletionStr))
+      .WillByDefault(Return(true));
+
+  sync_pb::EntityMetadata is_deletion_metadata;
+  is_deletion_metadata.set_is_deleted(true);
+  sync_pb::EntityMetadata is_not_deletion_metadata;
+  is_not_deletion_metadata.set_is_deleted(false);
   ON_CALL(*mock_sync_metadata_store_sync(), GetAllSyncMetadata())
       .WillByDefault([&]() {
         auto batch = std::make_unique<syncer::MetadataBatch>();
-        batch->AddMetadata(kPrimaryKeyUnsyncedEntryStr,
-                           std::make_unique<sync_pb::EntityMetadata>());
-        batch->AddMetadata(kPrimaryKeySyncedEntryStr,
-                           std::make_unique<sync_pb::EntityMetadata>());
+        batch->AddMetadata(kPrimaryKeyUnsyncedCredentialStr,
+                           std::make_unique<sync_pb::EntityMetadata>(
+                               is_not_deletion_metadata));
+        batch->AddMetadata(kPrimaryKeySyncedCredentialStr,
+                           std::make_unique<sync_pb::EntityMetadata>(
+                               is_not_deletion_metadata));
+        batch->AddMetadata(
+            kPrimaryKeyUnsyncedDeletionStr,
+            std::make_unique<sync_pb::EntityMetadata>(is_deletion_metadata));
         return batch;
       });
 
-  autofill::PasswordForm unsynced_entry = MakePasswordForm(kSignonRealm1);
-  autofill::PasswordForm synced_entry = MakePasswordForm(kSignonRealm2);
-  fake_db()->AddLoginForPrimaryKey(kPrimaryKeyUnsyncedEntry, unsynced_entry);
-  fake_db()->AddLoginForPrimaryKey(kPrimaryKeySyncedEntry, synced_entry);
-
+  autofill::PasswordForm unsynced_credential = MakePasswordForm(kSignonRealm1);
+  autofill::PasswordForm synced_credential = MakePasswordForm(kSignonRealm2);
+  autofill::PasswordForm unsynced_deletion = MakePasswordForm(kSignonRealm3);
+  fake_db()->AddLoginForPrimaryKey(kPrimaryKeyUnsyncedCredential,
+                                   unsynced_credential);
+  fake_db()->AddLoginForPrimaryKey(kPrimaryKeySyncedCredential,
+                                   synced_credential);
+  fake_db()->AddLoginForPrimaryKey(kPrimaryKeyUnsyncedDeletion,
+                                   unsynced_deletion);
+
+  // The notification should only contain new credentials that are unsynced,
+  // ignoring both synced ones and deletion entries.
   EXPECT_CALL(*mock_password_store_sync(),
               NotifyUnsyncedCredentialsWillBeDeleted(
-                  UnorderedElementsAre(unsynced_entry)));
+                  UnorderedElementsAre(unsynced_credential)));
 
   // The content of the metadata change list does not matter in this case.
   bridge()->ApplyStopSyncChanges(bridge()->CreateMetadataChangeList());
@@ -938,28 +959,25 @@ TEST_F(PasswordSyncBridgeTest,
   ON_CALL(*mock_password_store_sync(), IsAccountStore())
       .WillByDefault(Return(false));
 
-  const int kPrimaryKeyUnsyncedEntry = 1000;
-  const int kPrimaryKeySyncedEntry = 1001;
-  const std::string kPrimaryKeyUnsyncedEntryStr = "1000";
-  const std::string kPrimaryKeySyncedEntryStr = "1001";
-  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeyUnsyncedEntryStr))
+  const int kPrimaryKeyUnsyncedCredential = 1000;
+  const std::string kPrimaryKeyUnsyncedCredentialStr = "1000";
+  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeyUnsyncedCredentialStr))
       .WillByDefault(Return(true));
-  ON_CALL(mock_processor(), IsEntityUnsynced(kPrimaryKeySyncedEntryStr))
-      .WillByDefault(Return(false));
+
+  sync_pb::EntityMetadata is_not_deletion_metadata;
+  is_not_deletion_metadata.set_is_deleted(false);
   ON_CALL(*mock_sync_metadata_store_sync(), GetAllSyncMetadata())
       .WillByDefault([&]() {
         auto batch = std::make_unique<syncer::MetadataBatch>();
-        batch->AddMetadata(kPrimaryKeyUnsyncedEntryStr,
-                           std::make_unique<sync_pb::EntityMetadata>());
-        batch->AddMetadata(kPrimaryKeySyncedEntryStr,
-                           std::make_unique<sync_pb::EntityMetadata>());
+        batch->AddMetadata(kPrimaryKeyUnsyncedCredentialStr,
+                           std::make_unique<sync_pb::EntityMetadata>(
+                               is_not_deletion_metadata));
         return batch;
       });
 
-  fake_db()->AddLoginForPrimaryKey(kPrimaryKeyUnsyncedEntry,
+  autofill::PasswordForm unsynced_deletion = MakePasswordForm(kSignonRealm3);
+  fake_db()->AddLoginForPrimaryKey(kPrimaryKeyUnsyncedCredential,
                                    MakePasswordForm(kSignonRealm1));
-  fake_db()->AddLoginForPrimaryKey(kPrimaryKeySyncedEntry,
-                                   MakePasswordForm(kSignonRealm2));
 
   EXPECT_CALL(*mock_password_store_sync(),
               NotifyUnsyncedCredentialsWillBeDeleted)