Password manager internals page service: wiring it in

chrome/browser/password_manager/chrome_password_manager_client_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 
 #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"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserContext;
 using content::WebContents;
 
 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&));
 };
 
 }  // namespace
 
 class ChromePasswordManagerClientTest : public ChromeRenderViewHostTestHarness {
  public:
+  ChromePasswordManagerClientTest();
+
   virtual void SetUp() OVERRIDE;
 
  protected:
   ChromePasswordManagerClient* GetClient();
 
   // If the test IPC sink contains an AutofillMsg_ChangeLoggingState message,
   // then copies its argument into |activation_flag| and returns true. Otherwise
   // 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() {
   return ChromePasswordManagerClient::FromWebContents(web_contents());
 }
 
 bool ChromePasswordManagerClientTest::WasLoggingActivationMessageSent(
     bool* activation_flag) {
   const uint32 kMsgID = AutofillMsg_ChangeLoggingState::ID;
   const IPC::Message* message =
       process()->sink().GetFirstMessageMatching(kMsgID);
   if (!message)
     return false;
   Tuple1<bool> param;
   AutofillMsg_ChangeLoggingState::Read(message, &param);
   *activation_flag = param.a;
   process()->sink().ClearMessages();
   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);
+  EXPECT_EQ(std::string(), service_->RegisterReceiver(&receiver_));
+  EXPECT_TRUE(client->IsLoggingActive());
+  EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(1);
   client->LogSavePasswordProgress(kTestText);
-  EXPECT_TRUE(client->IsLoggingActive());
-}
-
-TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressDetachLogger) {
-  ChromePasswordManagerClient* client = GetClient();
-
-  client->SetLogger(&logger_);
-  // After detaching the logger, no text should be passed.
-  client->SetLogger(NULL);
-  EXPECT_CALL(logger_, LogSavePasswordProgress(kTestText)).Times(0);
-  client->LogSavePasswordProgress(kTestText);
+  service_->UnregisterReceiver(&receiver_);
   EXPECT_FALSE(client->IsLoggingActive());
 }
 
+TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressDetachReceiver) {
+  ChromePasswordManagerClient* client = GetClient();
+
+  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.
+  EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(0);
+  client->LogSavePasswordProgress(kTestText);
+}
+
 TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressNotifyRenderer) {
   ChromePasswordManagerClient* client = GetClient();
   bool logging_active = false;
 
   // 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);
 }
 
 TEST_F(ChromePasswordManagerClientTest,
        IsAutomaticPasswordSavingEnabledDefaultBehaviourTest) {
   EXPECT_FALSE(GetClient()->IsAutomaticPasswordSavingEnabled());
 }
 
 TEST_F(ChromePasswordManagerClientTest,
        IsAutomaticPasswordSavingEnabledWhenFlagIsSetTest) {
   CommandLine::ForCurrentProcess()->AppendSwitch(
       password_manager::switches::kEnableAutomaticPasswordSaving);
   if (chrome::VersionInfo::GetChannel() == chrome::VersionInfo::CHANNEL_UNKNOWN)
     EXPECT_TRUE(GetClient()->IsAutomaticPasswordSavingEnabled());
   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());
+}