Password manager internals page service: wiring it in

chrome/browser/password_manager/chrome_password_manager_client_unittest.cc

Index: chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
diff --git a/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc b/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
index 304975dcc946f166af83ffa38a6ccedaecb699be..f5a03deb50413ef95de2f9f8f792d2f6870d45ed 100644
--- a/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
+++ b/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
@@ -8,7 +8,10 @@
 
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/autofill/content/common/autofill_messages.h"
+#include "components/password_manager/content/browser/password_manager_internals_service_factory.h"
+#include "components/password_manager/core/browser/password_manager_internals_service.h"
 #include "components/password_manager/core/browser/password_manager_logger.h"
 #include "components/password_manager/core/common/password_manager_switches.h"
 #include "content/public/browser/browser_context.h"
@@ -24,10 +27,9 @@ namespace {
 
 const char kTestText[] = "abcd1234";
 
-class MockPasswordManagerLogger
-    : public password_manager::PasswordManagerLogger {
+class MockLogReceiver : public password_manager::PasswordManagerLogger {
  public:
-  MockPasswordManagerLogger() {}
+  MockLogReceiver() {}

[Ilya Sherman, 2014/05/13 04:32:18]

nit: Can this line be omitted?

[vabr (Chromium), 2014/05/13 09:27:11]

Done.

 
   MOCK_METHOD1(LogSavePasswordProgress, void(const std::string&));
 };
@@ -36,6 +38,8 @@ class MockPasswordManagerLogger
 
 class ChromePasswordManagerClientTest : public ChromeRenderViewHostTestHarness {
  public:
+  ChromePasswordManagerClientTest();
+
   virtual void SetUp() OVERRIDE;
 
  protected:
@@ -46,13 +50,22 @@ class ChromePasswordManagerClientTest : public ChromeRenderViewHostTestHarness {
   // returns false.
   bool WasLoggingActivationMessageSent(bool* activation_flag);
 
-  testing::StrictMock<MockPasswordManagerLogger> logger_;
+  password_manager::PasswordManagerInternalsService* service_;
+
+  testing::StrictMock<MockLogReceiver> receiver_;
 };
 
+ChromePasswordManagerClientTest::ChromePasswordManagerClientTest()
+    : service_(NULL) {
+}
+
 void ChromePasswordManagerClientTest::SetUp() {
   ChromeRenderViewHostTestHarness::SetUp();
   ChromePasswordManagerClient::CreateForWebContentsWithAutofillManagerDelegate(
       web_contents(), NULL);
+  service_ = password_manager::PasswordManagerInternalsServiceFactory::
+      GetForBrowserContext(profile());
+  ASSERT_TRUE(service_);
 }

[Ilya Sherman, 2014/05/13 04:32:18]

Optional nit: It's fine to inline these implementations in test files, if you
like.

[vabr (Chromium), 2014/05/13 09:27:11]

I actually prefer them separated: the declarations are easier to read for me,
and also I don't have to read test code differently from the prod code.
But I recognise this is subjective, and I don't care too strongly about it.
Since you marked this "optional", I left the code as is. If you want to drop the
"optional", I'm happy to inline it.

 
 ChromePasswordManagerClient* ChromePasswordManagerClientTest::GetClient() {
@@ -73,34 +86,39 @@ bool ChromePasswordManagerClientTest::WasLoggingActivationMessageSent(
   return true;
 }
 
-TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressNoLogger) {
+TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressNoReceiver) {
   ChromePasswordManagerClient* client = GetClient();
 
-  EXPECT_CALL(logger_, LogSavePasswordProgress(kTestText)).Times(0);
-  // Before attaching the logger, no text should be passed.
+  EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(0);
+  // Before attaching the receiver, no text should be passed.
   client->LogSavePasswordProgress(kTestText);
   EXPECT_FALSE(client->IsLoggingActive());
 }
 
-TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressAttachLogger) {
+TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressAttachReceiver) {
   ChromePasswordManagerClient* client = GetClient();
+  EXPECT_FALSE(client->IsLoggingActive());
 
   // After attaching the logger, text should be passed.
-  client->SetLogger(&logger_);
-  EXPECT_CALL(logger_, LogSavePasswordProgress(kTestText)).Times(1);
-  client->LogSavePasswordProgress(kTestText);
+  EXPECT_EQ(std::string(), service_->RegisterReceiver(&receiver_));
   EXPECT_TRUE(client->IsLoggingActive());
+  EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(1);
+  client->LogSavePasswordProgress(kTestText);
+  service_->UnregisterReceiver(&receiver_);
+  EXPECT_FALSE(client->IsLoggingActive());
 }
 
-TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressDetachLogger) {
+TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressDetachReceiver) {
   ChromePasswordManagerClient* client = GetClient();
 
-  client->SetLogger(&logger_);
+  EXPECT_EQ(std::string(), service_->RegisterReceiver(&receiver_));
+  EXPECT_TRUE(client->IsLoggingActive());
+  service_->UnregisterReceiver(&receiver_);
+  EXPECT_FALSE(client->IsLoggingActive());
+
   // After detaching the logger, no text should be passed.
-  client->SetLogger(NULL);
-  EXPECT_CALL(logger_, LogSavePasswordProgress(kTestText)).Times(0);
+  EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(0);
   client->LogSavePasswordProgress(kTestText);
-  EXPECT_FALSE(client->IsLoggingActive());
 }
 
 TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressNotifyRenderer) {
@@ -110,11 +128,13 @@ TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressNotifyRenderer) {
   // Initially, the logging should be off, so no IPC messages.
   EXPECT_FALSE(WasLoggingActivationMessageSent(&logging_active));
 
-  client->SetLogger(&logger_);
+  EXPECT_EQ(std::string(), service_->RegisterReceiver(&receiver_));
+  EXPECT_TRUE(client->IsLoggingActive());
   EXPECT_TRUE(WasLoggingActivationMessageSent(&logging_active));
   EXPECT_TRUE(logging_active);
 
-  client->SetLogger(NULL);
+  service_->UnregisterReceiver(&receiver_);
+  EXPECT_FALSE(client->IsLoggingActive());
   EXPECT_TRUE(WasLoggingActivationMessageSent(&logging_active));
   EXPECT_FALSE(logging_active);
 }
@@ -133,3 +153,15 @@ TEST_F(ChromePasswordManagerClientTest,
   else
     EXPECT_FALSE(GetClient()->IsAutomaticPasswordSavingEnabled());
 }
+
+TEST_F(ChromePasswordManagerClientTest, LogToAReceiver) {
+  ChromePasswordManagerClient* client = GetClient();
+  EXPECT_EQ(std::string(), service_->RegisterReceiver(&receiver_));

[Ilya Sherman, 2014/05/13 04:32:18]

I'm noticing that all of the tests in this file expect the return value from
RegisterReceiver to be the empty string.  Should there also be test coverage
here of the other case?  If not, maybe the EXPECT_EQ's are not needed at all,
since that doesn't seem to be an integral part of what's being tested in this
file.

(In general, there are lots of things that you can assert within a test.  It's
nice if the only assertions are the ones corresponding specifically to what the
test is aimed at testing.  That tends to make the test code easier to follow.)

[vabr (Chromium), 2014/05/13 09:27:11]

That's a fair point. Coverage for non-empty strings returned from
RegisterReceiver is in log_router_unittest.cc. I'll remove the EXPECT_EQ from
here to avoid distracting the reader.

+  EXPECT_TRUE(client->IsLoggingActive());
+
+  EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(1);
+  client->LogSavePasswordProgress(kTestText);
+
+  service_->UnregisterReceiver(&receiver_);
+  EXPECT_FALSE(client->IsLoggingActive());
+}