[Passwords] Replace PasswordFormManager.new_blacklisted_ with a boolean

Before this patch:
PasswordFormManager used own a |new_blacklisted_| form to signal that
the current form has been just blacklisted which was getting cleared
upon updates in underlying PasswordStore.

After this patch:
A boolean is introduce to book keep this information until the data
are refreshed from the store.
Follow up patches will clean things a little bit more to simplify the
Save/Update logic.

Change-Id: I775b799a90eaf9fd4964848154168f6ba7283dd0
Bug: 1004783
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807329Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698439}

components/password_manager/core/browser/form_saver.h

@@ -28,7 +28,7 @@ class FormSaver {
   // Blacklist the origin described by |digest|. Returns the PasswordForm pushed
   // to the store.
   virtual autofill::PasswordForm PermanentlyBlacklist(
-      PasswordStore::FormDigest digest) WARN_UNUSED_RESULT = 0;
+      PasswordStore::FormDigest digest) = 0;
 
   // Saves the |pending| form.
   // |matches| are relevant credentials for the site. After saving |pending|,

components/password_manager/core/browser/form_saver_impl.h

@@ -25,7 +25,7 @@ class FormSaverImpl : public FormSaver {
 
   // FormSaver:
   autofill::PasswordForm PermanentlyBlacklist(
-      PasswordStore::FormDigest digest) override WARN_UNUSED_RESULT;
+      PasswordStore::FormDigest digest) override;
   void Save(autofill::PasswordForm pending,
             const std::vector<const autofill::PasswordForm*>& matches,
             const base::string16& old_password) override;

components/password_manager/core/browser/password_form_manager.cc

@@ -252,7 +252,7 @@ base::span<const InteractionsStats> PasswordFormManager::GetInteractionsStats()
 }
 
 bool PasswordFormManager::IsBlacklisted() const {
-  return !blacklisted_matches_.empty();
+  return !blacklisted_matches_.empty() || newly_blacklisted_;
 }
 
 void PasswordFormManager::Save() {
@@ -264,6 +264,13 @@ void PasswordFormManager::Save() {
     form_saver_->Remove(**blacklisted_iterator);
     blacklisted_iterator = blacklisted_matches_.erase(blacklisted_iterator);
   }
+  if (newly_blacklisted_) {
+    PasswordForm newly_blacklisted_form =
+        password_manager_util::MakeNormalizedBlacklistedForm(
+            ConstructObservedFormDigest());
+    form_saver_->Remove(newly_blacklisted_form);
+    newly_blacklisted_ = false;
+  }
 
   // TODO(https://crbug.com/831123): Implement indicator event metrics.
   if (password_overridden_ &&
@@ -397,24 +404,24 @@ void PasswordFormManager::OnNoInteraction(bool is_update) {
 
 void PasswordFormManager::PermanentlyBlacklist() {
   DCHECK(!client_->IsIncognito());
-
-  if (!new_blacklisted_) {
-    new_blacklisted_ = std::make_unique<PasswordForm>();
-    if (observed_not_web_form_digest_) {
-      new_blacklisted_->origin = observed_not_web_form_digest_->origin;
-      // GetSignonRealm is not suitable for http auth credentials.
-      new_blacklisted_->signon_realm =
-          IsHttpAuth() ? observed_not_web_form_digest_->signon_realm
+  form_saver_->PermanentlyBlacklist(ConstructObservedFormDigest());
+  newly_blacklisted_ = true;
+}
+
+PasswordStore::FormDigest PasswordFormManager::ConstructObservedFormDigest() {
+  std::string signon_realm;
+  GURL origin;
+  if (observed_not_web_form_digest_) {
+    origin = observed_not_web_form_digest_->origin;
+    // GetSignonRealm is not suitable for http auth credentials.
+    signon_realm = IsHttpAuth()
+                       ? observed_not_web_form_digest_->signon_realm
                        : GetSignonRealm(observed_not_web_form_digest_->origin);
-    } else {
-      new_blacklisted_->origin = observed_form_.url;
-      new_blacklisted_->signon_realm = GetSignonRealm(observed_form_.url);
-    }
-    new_blacklisted_->scheme = GetScheme();
-    blacklisted_matches_.push_back(new_blacklisted_.get());
+  } else {
+    origin = observed_form_.url;
+    signon_realm = GetSignonRealm(observed_form_.url);
   }
-  *new_blacklisted_ = form_saver_->PermanentlyBlacklist(
-      PasswordStore::FormDigest(*new_blacklisted_));
+  return PasswordStore::FormDigest(GetScheme(), signon_realm, origin);
 }
 
 void PasswordFormManager::OnPasswordsRevealed() {
@@ -597,7 +604,7 @@ void PasswordFormManager::OnFetchCompleted() {
   received_stored_credentials_time_ = TimeTicks::Now();
 
   // Copy out blacklisted matches.
-  new_blacklisted_.reset();
+  newly_blacklisted_ = false;
   blacklisted_matches_ = form_fetcher_->GetBlacklistedMatches();
 
   autofills_left_ = kMaxTimesAutofill;

components/password_manager/core/browser/password_form_manager.h

@@ -281,6 +281,8 @@ class PasswordFormManager : public PasswordFormManagerInterface,
   // Save/update |pending_credentials_| to the password store.
   void SavePendingToStore(bool update);
 
+  PasswordStore::FormDigest ConstructObservedFormDigest();
+
   // The client which implements embedder-specific PasswordManager operations.
   PasswordManagerClient* client_;
 
@@ -298,14 +300,11 @@ class PasswordFormManager : public PasswordFormManagerInterface,
   // form. They are owned by |form_fetcher_|.
   std::vector<const autofill::PasswordForm*> blacklisted_matches_;
 
-  // If the observed form gets blacklisted through |this|, the blacklist entry
-  // gets stored in |new_blacklisted_| until data is potentially refreshed by
-  // reading from PasswordStore again. |blacklisted_matches_| will contain
-  // |new_blacklisted_.get()| in that case. The PasswordForm will usually get
-  // accessed via |blacklisted_matches_|, this unique_ptr is only used to store
-  // it (unlike the rest of forms being pointed to in |blacklisted_matches_|,
-  // which are owned by |form_fetcher_|).
-  std::unique_ptr<autofill::PasswordForm> new_blacklisted_;
+  // If the observed form gets blacklisted through |this|, we keep the
+  // information in this boolean flag until data is potentially refreshed by
+  // reading from PasswordStore again. Upon reading from the store again, we set
+  // this boolean to false again.
+  bool newly_blacklisted_ = false;
 
   // Takes care of recording metrics and events for |*this|.
   scoped_refptr<PasswordFormMetricsRecorder> metrics_recorder_;