Don't rely on a real instance of ChromePasswordManagerClient in tests

The overhead of creating a proper ChromePasswordManagerClient instance
is too big for unit tests and may lead to unexpected side-effects.

Bug: 1007218
Change-Id: I98f02d64a513befa1cf22346ca94847fa7b1f75c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1912717Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714976}

chrome/browser/password_manager/password_generation_controller_impl.cc

@@ -19,6 +19,7 @@
 #include "components/autofill/core/common/signatures_util.h"
 #include "components/password_manager/core/browser/password_generation_frame_helper.h"
 #include "components/password_manager/core/browser/password_manager.h"
+#include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 
@@ -137,8 +138,7 @@ void PasswordGenerationControllerImpl::OnGenerationRequested(
     PasswordGenerationType type) {
   if (type == PasswordGenerationType::kManual) {
     manual_generation_requested_ = true;
-    ChromePasswordManagerClient::FromWebContents(web_contents_)
-        ->GeneratePassword();
+    client_->GeneratePassword();
   } else {
     ShowDialog(PasswordGenerationType::kAutomatic);
   }
@@ -173,31 +173,36 @@ gfx::NativeWindow PasswordGenerationControllerImpl::top_level_native_window()
 // static
 void PasswordGenerationControllerImpl::CreateForWebContentsForTesting(
     content::WebContents* web_contents,
-    base::WeakPtr<ManualFillingController> manual_filling_controller_,
+    password_manager::PasswordManagerClient* client,
+    base::WeakPtr<ManualFillingController> manual_filling_controller,
     CreateDialogFactory create_dialog_factory) {
   DCHECK(web_contents) << "Need valid WebContents to attach controller to!";
   DCHECK(!FromWebContents(web_contents)) << "Controller already attached!";
-  DCHECK(manual_filling_controller_);
+  DCHECK(manual_filling_controller);
 
   web_contents->SetUserData(
-      UserDataKey(), base::WrapUnique(new PasswordGenerationControllerImpl(
-                         web_contents, std::move(manual_filling_controller_),
-                         create_dialog_factory)));
+      UserDataKey(),
+      base::WrapUnique(new PasswordGenerationControllerImpl(
+          web_contents, client, std::move(manual_filling_controller),
+          create_dialog_factory)));
 }
 
 PasswordGenerationControllerImpl::PasswordGenerationControllerImpl(
     content::WebContents* web_contents)
     : web_contents_(web_contents),
+      client_(ChromePasswordManagerClient::FromWebContents(web_contents)),
       create_dialog_factory_(
           base::BindRepeating(&PasswordGenerationDialogViewInterface::Create)) {
 }
 
 PasswordGenerationControllerImpl::PasswordGenerationControllerImpl(
     content::WebContents* web_contents,
-    base::WeakPtr<ManualFillingController> manual_filling_controller_,
+    password_manager::PasswordManagerClient* client,
+    base::WeakPtr<ManualFillingController> manual_filling_controller,
     CreateDialogFactory create_dialog_factory)
     : web_contents_(web_contents),
-      manual_filling_controller_(std::move(manual_filling_controller_)),
+      client_(client),
+      manual_filling_controller_(std::move(manual_filling_controller)),
       create_dialog_factory_(create_dialog_factory) {}
 
 void PasswordGenerationControllerImpl::ShowDialog(PasswordGenerationType type) {

chrome/browser/password_manager/password_generation_controller_impl.h

@@ -18,6 +18,7 @@ class PasswordGenerationDialogViewInterface;
 
 namespace password_manager {
 class PasswordManagerDriver;
+class PasswordManagerClient;
 }  // namespace password_manager
 
 // In its current state, this class is not involved in the editing flow for
@@ -63,6 +64,7 @@ class PasswordGenerationControllerImpl
   // testing.
   static void CreateForWebContentsForTesting(
       content::WebContents* web_contents,
+      password_manager::PasswordManagerClient* client,
       base::WeakPtr<ManualFillingController> manual_filling_controller,
       CreateDialogFactory create_dialog_callback);
 
@@ -77,9 +79,10 @@ class PasswordGenerationControllerImpl
 
   friend class content::WebContentsUserData<PasswordGenerationControllerImpl>;
 
-  // Constructor that allows to inject a mock or fake view.
+  // Constructor that allows to inject a mock or fake dependencies
   PasswordGenerationControllerImpl(
       content::WebContents* web_contents,
+      password_manager::PasswordManagerClient* client,
       base::WeakPtr<ManualFillingController> manual_filling_controller,
       CreateDialogFactory create_dialog_callback);
 
@@ -102,6 +105,10 @@ class PasswordGenerationControllerImpl
   // The tab for which this class is scoped.
   content::WebContents* web_contents_;
 
+  // The PasswordManagerClient associated with the current |web_contents_|.
+  // Used to tell the renderer that manual generation was requested.
+  password_manager::PasswordManagerClient* client_ = nullptr;
+
   // Data for the generation element used to generate the password.
   std::unique_ptr<GenerationElementData> generation_element_data_;
 

chrome/browser/password_manager/password_generation_controller_impl_unittest.cc

@@ -47,42 +47,19 @@ using testing::NiceMock;
 using testing::Return;
 using testing::StrictMock;
 
-class TestPasswordManagerClient : public ChromePasswordManagerClient {
+class TestPasswordManagerClient
+    : public password_manager::StubPasswordManagerClient {
  public:
-  static TestPasswordManagerClient* CreateForWebContents(
-      content::WebContents* web_contents,
-      autofill::AutofillClient* autofill_client);
-
-  TestPasswordManagerClient(content::WebContents* web_contents,
-                            autofill::AutofillClient* autofill_client);
+  TestPasswordManagerClient();
   ~TestPasswordManagerClient() override;
 
-  bool IsSavingAndFillingEnabled(const GURL& url) const override;
-  void GeneratePassword() override;
   password_manager::PasswordStore* GetProfilePasswordStore() const override;
 
  private:
   scoped_refptr<MockPasswordStore> mock_password_store_;
 };
 
-// static
-TestPasswordManagerClient* TestPasswordManagerClient::CreateForWebContents(
-    content::WebContents* web_contents,
-    autofill::AutofillClient* autofill_client) {
-  if (FromWebContents(web_contents))
-    ADD_FAILURE() << "Can't attatch TestPasswordManagerClient to WebContents. "
-                     "A ChromePasswordManagerClient is already attatched to "
-                     "the given WebContents.";
-  TestPasswordManagerClient* test_client =
-      new TestPasswordManagerClient(web_contents, autofill_client);
-  web_contents->SetUserData(UserDataKey(), base::WrapUnique(test_client));
-  return test_client;
-}
-
-TestPasswordManagerClient::TestPasswordManagerClient(
-    content::WebContents* web_contents,
-    autofill::AutofillClient* autofill_client)
-    : ChromePasswordManagerClient(web_contents, autofill_client) {
+TestPasswordManagerClient::TestPasswordManagerClient() {
   mock_password_store_ = new MockPasswordStore();
 }
 
@@ -90,13 +67,6 @@ TestPasswordManagerClient::~TestPasswordManagerClient() {
   mock_password_store_->ShutdownOnUIThread();
 }
 
-bool TestPasswordManagerClient::IsSavingAndFillingEnabled(
-    const GURL& url) const {
-  return true;
-}
-
-void TestPasswordManagerClient::GeneratePassword() {}
-
 password_manager::PasswordStore*
 TestPasswordManagerClient::GetProfilePasswordStore() const {
   return mock_password_store_.get();
@@ -193,14 +163,14 @@ class PasswordGenerationControllerTest
  public:
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
+    test_pwd_manager_client_ = std::make_unique<TestPasswordManagerClient>();
     PasswordGenerationControllerImpl::CreateForWebContentsForTesting(
-        web_contents(), mock_manual_filling_controller_.AsWeakPtr(),
+        web_contents(), test_pwd_manager_client_.get(),
+        mock_manual_filling_controller_.AsWeakPtr(),
         mock_dialog_factory_.Get());
-    test_pwd_manager_client_ = TestPasswordManagerClient::CreateForWebContents(
-        web_contents(), &test_autofill_client_);
 
     password_manager_ = std::make_unique<password_manager::PasswordManager>(
-        test_pwd_manager_client_);
+        test_pwd_manager_client_.get());
     mock_password_manager_driver_ =
         std::make_unique<NiceMock<MockPasswordManagerDriver>>();
 
@@ -209,7 +179,7 @@ class PasswordGenerationControllerTest
     password_autofill_manager_ =
         std::make_unique<password_manager::PasswordAutofillManager>(
             mock_password_manager_driver_.get(), &test_autofill_client_,
-            test_pwd_manager_client_);
+            test_pwd_manager_client_.get());
 
     ON_CALL(*mock_password_manager_driver_, GetPasswordManager())
         .WillByDefault(Return(password_manager_.get()));
@@ -262,7 +232,7 @@ class PasswordGenerationControllerTest
   std::unique_ptr<password_manager::PasswordManager> password_manager_;
   std::unique_ptr<password_manager::PasswordAutofillManager>
       password_autofill_manager_;
-  TestPasswordManagerClient* test_pwd_manager_client_ = nullptr;
+  std::unique_ptr<TestPasswordManagerClient> test_pwd_manager_client_;
   autofill::TestAutofillClient test_autofill_client_;
 };