Move NotifyUISignoutWillDeleteCredentials out of signin notifier

This CL reverts the changes to PasswordStoreSigninNotifier and moves the
API to new a dedicated class, owned by the PasswordStore. The class is
lazily initialized via a setter method (as it is done for the notifier),
and only for the account store, since that's the only place where it
should be exercised.

Bug: 1060132
Change-Id: I6d78fc5e2c061acb474fec4cf7be7418368fd8a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135750
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756607}

chrome/browser/password_manager/account_storage/account_password_store_factory.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -27,13 +28,16 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/browser/storage_partition.h"
+#include "content/public/browser/web_contents.h"
 
 #if !defined(OS_ANDROID)
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "content/public/browser/browser_task_traits.h"
 #endif  // !defined(OS_ANDROID)
@@ -59,6 +63,39 @@ void UpdateAllFormManagers(Profile* profile) {
   }
 }
 
+class UnsyncedCredentialsDeletionNotifierImpl
+    : public PasswordStore::UnsyncedCredentialsDeletionNotifier {
+ public:
+  explicit UnsyncedCredentialsDeletionNotifierImpl(Profile* profile);
+  ~UnsyncedCredentialsDeletionNotifierImpl() override = default;
+
+  // Finds the last active tab and notifies their ManagePasswordsUIController.
+  void Notify(const std::vector<autofill::PasswordForm>& credentials) override;
+
+ private:
+  Profile* const profile_;
+};
+
+UnsyncedCredentialsDeletionNotifierImpl::
+    UnsyncedCredentialsDeletionNotifierImpl(Profile* profile)
+    : profile_(profile) {}
+
+void UnsyncedCredentialsDeletionNotifierImpl::Notify(
+    const std::vector<autofill::PasswordForm>& credentials) {
+  Browser* browser = chrome::FindBrowserWithProfile(profile_);
+  if (!browser)
+    return;
+  content::WebContents* web_contents =
+      browser->tab_strip_model()->GetActiveWebContents();
+  if (!web_contents)
+    return;
+  auto* ui_controller =
+      ManagePasswordsUIController::FromWebContents(web_contents);
+  if (!ui_controller)
+    return;
+  ui_controller->NotifyUnsyncedCredentialsWillBeDeleted(credentials);
+}
+
 }  // namespace
 
 #endif  // !defined(OS_ANDROID)
@@ -138,6 +175,11 @@ AccountPasswordStoreFactory::BuildServiceInstanceFor(
   password_manager_util::RemoveUselessCredentials(ps, profile->GetPrefs(), 60,
                                                   network_context_getter);
 
+#if !defined(OS_ANDROID)
+  ps->SetUnsyncedCredentialsDeletionNotifier(
+      std::make_unique<UnsyncedCredentialsDeletionNotifierImpl>(profile));
+#endif  // !defined(OS_ANDROID)
+
   return ps;
 }
 

chrome/browser/password_manager/password_manager_browsertest.cc

@@ -440,23 +440,6 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
   EXPECT_TRUE(prompt_observer->IsSavePromptShownAutomatically());
 }
 
-#if defined(OS_WIN) || defined(OS_MACOSX) || \
-    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
-IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
-                       SignOutWithUnsyncedPasswords) {
-  std::vector<autofill::PasswordForm> credentials(1);
-  credentials[0].username_value = base::ASCIIToUTF16("unsynced_login");
-  credentials[0].password_value = base::ASCIIToUTF16("unsynced_password");
-  // TODO(crbug.com/1060132): Stop triggering the notifier directly once the
-  // full flow is available.
-  PasswordStoreSigninNotifierImpl notifier(browser()->profile());
-  notifier.NotifyUISignoutWillDeleteCredentials(credentials);
-  EXPECT_EQ(ManagePasswordsUIController::FromWebContents(WebContents())
-                ->GetUnsyncedCredentials(),
-            credentials);
-}
-#endif
-
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, PromptForDynamicForm) {
   // Adding a PSL matching form is a workaround explained later.
   scoped_refptr<password_manager::TestPasswordStore> password_store =

chrome/browser/password_manager/password_store_factory.cc

@@ -192,7 +192,7 @@ PasswordStoreFactory::BuildServiceInstanceFor(
     (defined(OS_LINUX) && !defined(OS_CHROMEOS))
   std::unique_ptr<password_manager::PasswordStoreSigninNotifier> notifier =
       std::make_unique<password_manager::PasswordStoreSigninNotifierImpl>(
-          profile);
+          IdentityManagerFactory::GetForProfile(profile));
   ps->SetPasswordStoreSigninNotifier(std::move(notifier));
 #endif
 

chrome/browser/password_manager/password_store_signin_notifier_impl.cc

@@ -4,20 +4,13 @@
 
 #include "chrome/browser/password_manager/password_store_signin_notifier_impl.h"
 
-#include "chrome/browser/signin/identity_manager_factory.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_finder.h"
-#include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
-#include "content/public/browser/web_contents.h"
 
 namespace password_manager {
 
 PasswordStoreSigninNotifierImpl::PasswordStoreSigninNotifierImpl(
-    Profile* profile)
-    : profile_(profile),
-      identity_manager_(IdentityManagerFactory::GetForProfile(profile)) {
+    signin::IdentityManager* identity_manager)
+    : identity_manager_(identity_manager) {
   DCHECK(identity_manager_);
 }
 
@@ -33,24 +26,6 @@ void PasswordStoreSigninNotifierImpl::UnsubscribeFromSigninEvents() {
   identity_manager_->RemoveObserver(this);
 }
 
-void PasswordStoreSigninNotifierImpl::NotifyUISignoutWillDeleteCredentials(
-    const std::vector<autofill::PasswordForm>& unsynced_credentials) {
-  // Find the last active tab and pass |unsynced_credentials| to the
-  // ManagePasswordsUIController attached to it.
-  Browser* browser = chrome::FindBrowserWithProfile(profile_);
-  if (!browser)
-    return;
-  content::WebContents* web_contents =
-      browser->tab_strip_model()->GetActiveWebContents();
-  if (!web_contents)
-    return;
-  auto* ui_controller =
-      ManagePasswordsUIController::FromWebContents(web_contents);
-  if (!ui_controller)
-    return;
-  ui_controller->NotifyUnsyncedCredentialsWillBeDeleted(unsynced_credentials);
-}
-
 void PasswordStoreSigninNotifierImpl::OnPrimaryAccountCleared(
     const CoreAccountInfo& account_info) {
   NotifySignedOut(account_info.email, /* primary_account= */ true);
@@ -60,8 +35,9 @@ void PasswordStoreSigninNotifierImpl::OnPrimaryAccountCleared(
 void PasswordStoreSigninNotifierImpl::OnExtendedAccountInfoRemoved(
     const AccountInfo& info) {
   // Only reacts to content area (non-primary) Gaia account sign-out event.
-  if (info.account_id != identity_manager_->GetPrimaryAccountId())
+  if (info.account_id != identity_manager_->GetPrimaryAccountId()) {
     NotifySignedOut(info.email, /* primary_account= */ false);
+  }
 }
 
 }  // namespace password_manager

chrome/browser/password_manager/password_store_signin_notifier_impl.h

@@ -5,18 +5,10 @@
 #ifndef CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_SIGNIN_NOTIFIER_IMPL_H_
 #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_SIGNIN_NOTIFIER_IMPL_H_
 
-#include <vector>
-
 #include "base/macros.h"
 #include "components/password_manager/core/browser/password_store_signin_notifier.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 
-namespace autofill {
-struct PasswordForm;
-}
-
-class Profile;
-
 namespace password_manager {
 
 // Responsible for subscribing to Chrome sign-in events and passing them to
@@ -25,21 +17,19 @@ class PasswordStoreSigninNotifierImpl
     : public PasswordStoreSigninNotifier,
       public signin::IdentityManager::Observer {
  public:
-  explicit PasswordStoreSigninNotifierImpl(Profile* profile);
+  explicit PasswordStoreSigninNotifierImpl(
+      signin::IdentityManager* identity_manager);
   ~PasswordStoreSigninNotifierImpl() override;
 
   // PasswordStoreSigninNotifier implementations.
   void SubscribeToSigninEvents(PasswordStore* store) override;
   void UnsubscribeFromSigninEvents() override;
-  void NotifyUISignoutWillDeleteCredentials(
-      const std::vector<autofill::PasswordForm>& unsynced_credentials) override;
 
   // IdentityManager::Observer implementations.
   void OnPrimaryAccountCleared(const CoreAccountInfo& account_info) override;
   void OnExtendedAccountInfoRemoved(const AccountInfo& info) override;
 
  private:
-  Profile* profile_ = nullptr;
   signin::IdentityManager* identity_manager_ = nullptr;
 };
 

chrome/browser/password_manager/password_store_signin_notifier_impl_unittest.cc

@@ -5,14 +5,11 @@
 #include "chrome/browser/password_manager/password_store_signin_notifier_impl.h"
 
 #include "base/bind.h"
-#include "chrome/browser/signin/identity_manager_factory.h"
-#include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
-#include "chrome/test/base/testing_profile.h"
+#include "base/test/task_environment.h"
 #include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/signin/public/identity_manager/accounts_mutator.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "components/signin/public/identity_manager/primary_account_mutator.h"
-#include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
@@ -22,19 +19,14 @@ namespace {
 
 class PasswordStoreSigninNotifierImplTest : public testing::Test {
  public:
-  PasswordStoreSigninNotifierImplTest()
-      : profile_(IdentityTestEnvironmentProfileAdaptor::
-                     CreateProfileForIdentityTestEnvironment()),
-        identity_test_env_adaptor_(profile_.get()) {
-    store_ = new MockPasswordStore();
-  }
+  PasswordStoreSigninNotifierImplTest() { store_ = new MockPasswordStore(); }
 
   ~PasswordStoreSigninNotifierImplTest() override {
     store_->ShutdownOnUIThread();
   }
 
   signin::IdentityTestEnvironment* identity_test_env() {
-    return identity_test_env_adaptor_.identity_test_env();
+    return &identity_test_env_;
   }
 
   signin::IdentityManager* identity_manager() {
@@ -42,16 +34,15 @@ class PasswordStoreSigninNotifierImplTest : public testing::Test {
   }
 
  protected:
-  content::BrowserTaskEnvironment task_environment_;
-  std::unique_ptr<TestingProfile> profile_;
-  IdentityTestEnvironmentProfileAdaptor identity_test_env_adaptor_;
+  base::test::TaskEnvironment task_environment_;
+  signin::IdentityTestEnvironment identity_test_env_;
   scoped_refptr<MockPasswordStore> store_;
 };
 
 // Checks that if a notifier is subscribed on sign-in events, then
 // a password store receives sign-in notifications.
 TEST_F(PasswordStoreSigninNotifierImplTest, Subscribed) {
-  PasswordStoreSigninNotifierImpl notifier(profile_.get());
+  PasswordStoreSigninNotifierImpl notifier(identity_manager());
   notifier.SubscribeToSigninEvents(store_.get());
   identity_test_env()->MakePrimaryAccountAvailable("test@example.com");
   testing::Mock::VerifyAndClearExpectations(store_.get());
@@ -63,7 +54,7 @@ TEST_F(PasswordStoreSigninNotifierImplTest, Subscribed) {
 // Checks that if a notifier is unsubscribed on sign-in events, then
 // a password store receives no sign-in notifications.
 TEST_F(PasswordStoreSigninNotifierImplTest, Unsubscribed) {
-  PasswordStoreSigninNotifierImpl notifier(profile_.get());
+  PasswordStoreSigninNotifierImpl notifier(identity_manager());
   notifier.SubscribeToSigninEvents(store_.get());
   notifier.UnsubscribeFromSigninEvents();
   EXPECT_CALL(*store_, ClearAllGaiaPasswordHash()).Times(0);
@@ -74,7 +65,7 @@ TEST_F(PasswordStoreSigninNotifierImplTest, Unsubscribed) {
 // Checks that if a notifier is unsubscribed on sign-in events, then
 // a password store receives no sign-in notifications.
 TEST_F(PasswordStoreSigninNotifierImplTest, SignOutContentArea) {
-  PasswordStoreSigninNotifierImpl notifier(profile_.get());
+  PasswordStoreSigninNotifierImpl notifier(identity_manager());
   notifier.SubscribeToSigninEvents(store_.get());
 
   identity_test_env()->MakePrimaryAccountAvailable("username");

components/password_manager/core/browser/password_store.cc

@@ -492,6 +492,14 @@ PasswordStore::CreateSyncControllerDelegate() {
           base::Unretained(this)));
 }
 
+void PasswordStore::SetUnsyncedCredentialsDeletionNotifier(
+    std::unique_ptr<PasswordStore::UnsyncedCredentialsDeletionNotifier>
+        notifier) {
+  DCHECK(!deletion_notifier_);
+  DCHECK(notifier);
+  deletion_notifier_ = std::move(notifier);
+}
+
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
 void PasswordStore::CheckReuse(const base::string16& input,
                                const std::string& domain,

components/password_manager/core/browser/password_store.h

@@ -107,6 +107,14 @@ class PasswordStore : protected PasswordStoreSync,
     virtual ~DatabaseCompromisedCredentialsObserver() = default;
   };
 
+  // Used to notify that unsynced credentials are about to be deleted.
+  class UnsyncedCredentialsDeletionNotifier {
+   public:
+    // Should be called from the UI thread.
+    virtual void Notify(const std::vector<autofill::PasswordForm>&) = 0;
+    virtual ~UnsyncedCredentialsDeletionNotifier() = default;
+  };
+
   // Represents a subset of autofill::PasswordForm needed for credential
   // retrievals.
   struct FormDigest {
@@ -320,6 +328,10 @@ class PasswordStore : protected PasswordStoreSync,
   std::unique_ptr<syncer::ProxyModelTypeControllerDelegate>
   CreateSyncControllerDelegate();
 
+  // Sets |deletion_notifier_|. Must not pass a nullptr.
+  void SetUnsyncedCredentialsDeletionNotifier(
+      std::unique_ptr<UnsyncedCredentialsDeletionNotifier> deletion_notifier);
+
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
   // Immediately called after |Init()| to retrieve password hash data for
   // reuse detection.
@@ -811,6 +823,8 @@ class PasswordStore : protected PasswordStoreSync,
   HashPasswordManager hash_password_manager_;
 #endif
 
+  std::unique_ptr<UnsyncedCredentialsDeletionNotifier> deletion_notifier_;
+
   bool shutdown_called_;
 
   InitStatus init_status_;

components/password_manager/core/browser/password_store_signin_notifier.h

@@ -6,25 +6,19 @@
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_STORE_SIGNIN_NOTIFIER_H_
 
 #include <string>
-#include <vector>
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 
-namespace autofill {
-struct PasswordForm;
-}
-
 namespace password_manager {
 
 class PasswordStore;
 
-// Abstract class used by PasswordStore to receive/send notifications about
-// Chrome sign-in/sign-out events.
+// Abstract class for notifying PasswordStore about Chrome sign-in events.
 // The logic of receiving sign-in events and notifying PasswordStore is split
 // in the base abstract class (this class, in components/) and an
 // implementation (in the chrome/browser/), because components/ doesn't know
-// anything about Chrome sign-in/sign-out.
+// anything about Chrome sign-in.
 class PasswordStoreSigninNotifier {
  public:
   PasswordStoreSigninNotifier();
@@ -32,8 +26,6 @@ class PasswordStoreSigninNotifier {
 
   virtual void SubscribeToSigninEvents(PasswordStore* store) = 0;
   virtual void UnsubscribeFromSigninEvents() = 0;
-  virtual void NotifyUISignoutWillDeleteCredentials(
-      const std::vector<autofill::PasswordForm>& unsynced_credentials) = 0;
 
  protected:
   void set_store(PasswordStore* store) { store_ = store; }

components/password_manager/core/browser/password_store_unittest.cc

@@ -6,7 +6,6 @@
 
 #include <memory>
 #include <utility>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -21,7 +20,6 @@
 #include "base/test/task_environment.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "components/autofill/core/common/password_form.h"
 #include "components/os_crypt/os_crypt_mocker.h"
 #include "components/password_manager/core/browser/android_affiliation/affiliated_match_helper.h"
 #include "components/password_manager/core/browser/android_affiliation/affiliation_service.h"
@@ -124,8 +122,6 @@ class MockPasswordStoreSigninNotifier : public PasswordStoreSigninNotifier {
  public:
   MOCK_METHOD1(SubscribeToSigninEvents, void(PasswordStore* store));
   MOCK_METHOD0(UnsubscribeFromSigninEvents, void());
-  MOCK_METHOD1(NotifyUISignoutWillDeleteCredentials,
-               void(const std::vector<autofill::PasswordForm>&));
 };
 
 }  // namespace