Password manager internals page: Introduce logger in renderer

A follow-up to https://codereview.chromium.org/216183008/

Password manager internals page serves as a debugging output from the process of observing submitted password forms and offering the user to save the password. The user will be able to grab the debugging info and pass it onto the Chrome developers to help investigate issues.

This CL introduces:
1) RendererSavePasswordProgressLogger, a specialization of the SavePasswordProgressLogger for the renderer part of the password management code.

2) Additional support in the PasswordManagementClient to use the Logger in the renderer code.

More context in the design doc: https://docs.google.com/document/d/1ArDhTo0w-8tOPiTwqM1gG6ZGqODo8UTpXlJjjnCNK4s/edit?usp=sharing

A follow-up CL will introduce actual logging calls.

TBR=inferno@chromium.org
BUG=347927

Review URL: https://codereview.chromium.org/228263004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@263002 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -179,10 +179,14 @@ void ChromePasswordManagerClient::SetLogger(
 
 void ChromePasswordManagerClient::LogSavePasswordProgress(
     const std::string& text) {
-  if (logger_)
+  if (IsLoggingActive())
     logger_->LogSavePasswordProgress(text);
 }
 
+bool ChromePasswordManagerClient::IsLoggingActive() const {
+  return logger_ != NULL;
+}
+
 // static
 password_manager::PasswordGenerationManager*
 ChromePasswordManagerClient::GetGenerationManagerFromWebContents(

chrome/browser/password_manager/chrome_password_manager_client.h

@@ -54,6 +54,7 @@ class ChromePasswordManagerClient
   virtual void SetLogger(password_manager::PasswordManagerLogger* logger)
       OVERRIDE;
   virtual void LogSavePasswordProgress(const std::string& text) OVERRIDE;
+  virtual bool IsLoggingActive() const OVERRIDE;
 
   // Hides any visible generation UI.
   void HidePasswordGenerationPopup();

chrome/browser/password_manager/chrome_password_manager_client_unittest.cc

@@ -16,6 +16,8 @@ using content::WebContents;
 
 namespace {
 
+const char kTestText[] = "abcd1234";
+
 class MockPasswordManagerLogger
     : public password_manager::PasswordManagerLogger {
  public:
@@ -32,6 +34,8 @@ class ChromePasswordManagerClientTest : public ChromeRenderViewHostTestHarness {
 
  protected:
   ChromePasswordManagerClient* GetClient();
+
+  testing::StrictMock<MockPasswordManagerLogger> logger;
 };
 
 void ChromePasswordManagerClientTest::SetUp() {
@@ -43,20 +47,32 @@ ChromePasswordManagerClient* ChromePasswordManagerClientTest::GetClient() {
   return ChromePasswordManagerClient::FromWebContents(web_contents());
 }
 
-TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgress) {
+TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressNoLogger) {
   ChromePasswordManagerClient* client = GetClient();
-  testing::StrictMock<MockPasswordManagerLogger> logger;
-  const std::string text("abcd1234");
 
+  EXPECT_CALL(logger, LogSavePasswordProgress(kTestText)).Times(0);
   // Before attaching the logger, no text should be passed.
-  client->LogSavePasswordProgress(text);
+  client->LogSavePasswordProgress(kTestText);
+  EXPECT_FALSE(client->IsLoggingActive());
+}
+
+TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressAttachLogger) {
+  ChromePasswordManagerClient* client = GetClient();
 
   // After attaching the logger, text should be passed.
   client->SetLogger(&logger);
-  EXPECT_CALL(logger, LogSavePasswordProgress(text)).Times(1);
-  client->LogSavePasswordProgress(text);
+  EXPECT_CALL(logger, LogSavePasswordProgress(kTestText)).Times(1);
+  client->LogSavePasswordProgress(kTestText);
+  EXPECT_TRUE(client->IsLoggingActive());
+}
 
-  // After detaching the logger, no text should be passed again.
+TEST_F(ChromePasswordManagerClientTest, LogSavePasswordProgressDetachLogger) {
+  ChromePasswordManagerClient* client = GetClient();
+
+  client->SetLogger(&logger);
+  // After detaching the logger, no text should be passed.
   client->SetLogger(NULL);
-  client->LogSavePasswordProgress(text);
+  EXPECT_CALL(logger, LogSavePasswordProgress(kTestText)).Times(0);
+  client->LogSavePasswordProgress(kTestText);
+  EXPECT_FALSE(client->IsLoggingActive());
 }

components/autofill.gypi

@@ -429,6 +429,8 @@
             'autofill/content/renderer/password_form_conversion_utils.h',
             'autofill/content/renderer/password_generation_agent.cc',
             'autofill/content/renderer/password_generation_agent.h',
+            'autofill/content/renderer/renderer_save_password_progress_logger.cc',
+            'autofill/content/renderer/renderer_save_password_progress_logger.h',
           ],
           # TODO(jschuh): crbug.com/167187 fix size_t to int truncations.
           'msvs_disabled_warnings': [4267, ],

components/autofill/content/common/autofill_messages.h

@@ -185,6 +185,10 @@ IPC_MESSAGE_ROUTED1(AutofillHostMsg_PasswordFormsRendered,
 IPC_MESSAGE_ROUTED1(AutofillHostMsg_PasswordFormSubmitted,
                     autofill::PasswordForm /* form */)
 
+// Notification that logs during saving the password have been gathered.
+IPC_MESSAGE_ROUTED1(AutofillHostMsg_RecordSavePasswordProgress,
+                    std::string /* log */)
+
 // Notification that a form has been submitted.  The user hit the button.
 IPC_MESSAGE_ROUTED2(AutofillHostMsg_FormSubmitted,
                     autofill::FormData /* form */,

components/autofill/content/renderer/renderer_save_password_progress_logger.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 "components/autofill/content/renderer/renderer_save_password_progress_logger.h"
+
+#include "components/autofill/content/common/autofill_messages.h"
+#include "ipc/ipc_sender.h"
+
+namespace autofill {
+
+RendererSavePasswordProgressLogger::RendererSavePasswordProgressLogger(
+    IPC::Sender* sender,
+    int routing_id)
+    : sender_(sender), routing_id_(routing_id) {
+  DCHECK(sender_);
+}
+
+RendererSavePasswordProgressLogger::~RendererSavePasswordProgressLogger() {}
+
+void RendererSavePasswordProgressLogger::SendLog(const std::string& log) {
+  sender_->Send(
+      new AutofillHostMsg_RecordSavePasswordProgress(routing_id_, log));
+}
+
+}  // namespace autofill

components/autofill/content/renderer/renderer_save_password_progress_logger.h

+// 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.
+
+#ifndef COMPONENTS_AUTOFILL_CONTENT_RENDERER_RENDERER_BROWSER_SAVE_PASSWORD_PROGRESS_LOGGER_H_
+#define COMPONENTS_AUTOFILL_CONTENT_RENDERER_RENDERER_BROWSER_SAVE_PASSWORD_PROGRESS_LOGGER_H_
+
+#include <string>
+
+#include "components/autofill/core/common/save_password_progress_logger.h"
+
+class PasswordManagerClient;
+
+namespace IPC {
+class Sender;
+}
+
+namespace autofill {
+
+// This is the SavePasswordProgressLogger specialization for the renderer code,
+// which sends logs to the browser process over IPC.
+class RendererSavePasswordProgressLogger
+    : public autofill::SavePasswordProgressLogger {
+ public:
+  // The logger will use |sender| and |routing_id| to send a
+  // AutofillHostMsg_RecordSavePasswordProgress message with the logs to the
+  // browser.
+  RendererSavePasswordProgressLogger(IPC::Sender* sender, int routing_id);
+  virtual ~RendererSavePasswordProgressLogger();
+
+ protected:
+  // SavePasswordProgressLogger:
+  virtual void SendLog(const std::string& log) OVERRIDE;
+
+ private:
+  // Used by SendLog to send the IPC message with logs. |sender_| needs to
+  // outlive the logger.
+  IPC::Sender* const sender_;
+  const int routing_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(RendererSavePasswordProgressLogger);
+};
+
+}  // namespace autofill
+
+#endif  // COMPONENTS_AUTOFILL_CONTENT_RENDERER_RENDERER_BROWSER_SAVE_PASSWORD_PROGRESS_LOGGER_H_

components/autofill/content/renderer/renderer_save_password_progress_logger_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 "components/autofill/content/renderer/renderer_save_password_progress_logger.h"
+
+#include "components/autofill/content/common/autofill_messages.h"
+#include "ipc/ipc_test_sink.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace autofill {
+
+namespace {
+
+const char kTestText[] = "test";
+
+class TestLogger : public RendererSavePasswordProgressLogger {
+ public:
+  TestLogger() : RendererSavePasswordProgressLogger(&sink_, 0) {}
+
+  using RendererSavePasswordProgressLogger::SendLog;
+
+  // Searches for an |AutofillHostMsg_RecordSavePasswordProgress| message in the
+  // queue of sent IPC messages. If none is present, returns false. Otherwise,
+  // extracts the first |AutofillHostMsg_RecordSavePasswordProgress| message,
+  // fills the output parameter with the value of the message's parameter, and
+  // clears the queue of sent messages.
+  bool GetLogMessage(std::string* log) {
+    const uint32 kMsgID = AutofillHostMsg_RecordSavePasswordProgress::ID;
+    const IPC::Message* message = sink_.GetFirstMessageMatching(kMsgID);
+    if (!message)
+      return false;
+    Tuple1<std::string> param;
+    AutofillHostMsg_RecordSavePasswordProgress::Read(message, &param);
+    *log = param.a;
+    sink_.ClearMessages();
+    return true;
+  }
+
+ private:
+  IPC::TestSink sink_;
+};
+
+}  // namespace
+
+TEST(RendererSavePasswordProgressLoggerTest, SendLog) {
+  TestLogger logger;
+  logger.SendLog(kTestText);
+  std::string sent_log;
+  EXPECT_TRUE(logger.GetLogMessage(&sent_log));
+  EXPECT_EQ(kTestText, sent_log);
+}
+
+}  // namespace autofill

components/components_tests.gyp

@@ -232,11 +232,13 @@
           'conditions': [
             ['OS != "ios"', {
               'sources': [
+                'autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc',
                 'dom_distiller/content/dom_distiller_viewer_source_unittest.cc',
               ],
               'dependencies': [
                 # Dependencies of autofill
                 'components.gyp:autofill_content_browser',
+                'components.gyp:autofill_content_renderer',
                 'components.gyp:autofill_content_test_support',
 
                 # Dependencies of dom_distiller

components/password_manager/content/browser/content_password_manager_driver.cc

@@ -8,6 +8,7 @@
 #include "components/autofill/content/common/autofill_messages.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/password_manager_client.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
@@ -96,6 +97,9 @@ bool ContentPasswordManagerDriver::OnMessageReceived(
   IPC_MESSAGE_FORWARD(AutofillHostMsg_PasswordFormSubmitted,
                       &password_manager_,
                       PasswordManager::OnPasswordFormSubmitted)
+  IPC_MESSAGE_FORWARD(AutofillHostMsg_RecordSavePasswordProgress,
+                      password_manager_.client(),
+                      PasswordManagerClient::LogSavePasswordProgress)
   IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -14,9 +14,9 @@
 #include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/password_manager/core/browser/password_form_manager.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_store.h"
+#include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "components/password_manager/core/browser/test_password_store.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -60,7 +60,7 @@ class MockPasswordManagerDriver : public PasswordManagerDriver {
                void(const std::vector<autofill::FormData>&));
 };
 
-class TestPasswordManagerClient : public PasswordManagerClient {
+class TestPasswordManagerClient : public StubPasswordManagerClient {
  public:
   explicit TestPasswordManagerClient(PasswordStore* password_store)
       : password_store_(password_store) {

components/password_manager/core/browser/password_generation_manager_unittest.cc

@@ -15,7 +15,7 @@
 #include "components/autofill/core/common/form_field_data.h"
 #include "components/password_manager/core/browser/password_generation_manager.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/stub_password_manager_client.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -70,7 +70,7 @@ class TestPasswordManagerDriver : public PasswordManagerDriver {
   bool is_off_the_record_;
 };
 
-class TestPasswordManagerClient : public PasswordManagerClient {
+class TestPasswordManagerClient : public StubPasswordManagerClient {
  public:
   TestPasswordManagerClient(scoped_ptr<PrefService> prefs)
       : prefs_(prefs.Pass()), driver_(this), is_sync_enabled_(false) {}

components/password_manager/core/browser/password_manager.h

@@ -99,6 +99,8 @@ class PasswordManager : public LoginModel {
   virtual void OnPasswordFormSubmitted(
       const autofill::PasswordForm& password_form);
 
+  PasswordManagerClient* client() { return client_; }
+
  private:
   enum ProvisionalSaveFailure {
     SAVING_DISABLED,

components/password_manager/core/browser/password_manager_client.cc

@@ -14,9 +14,4 @@ PasswordManagerClient::GetProbabilityForExperiment(
 
 bool PasswordManagerClient::IsPasswordSyncEnabled() { return false; }
 
-void PasswordManagerClient::SetLogger(PasswordManagerLogger* /*logger*/) {}
-
-void PasswordManagerClient::LogSavePasswordProgress(
-    const std::string& /*text*/) {}
-
 }  // namespace password_manager

components/password_manager/core/browser/password_manager_client.h

@@ -62,10 +62,14 @@ class PasswordManagerClient {
   virtual bool IsPasswordSyncEnabled();
 
   // Attach or detach (setting NULL) a logger for this client.
-  virtual void SetLogger(PasswordManagerLogger* logger);
+  virtual void SetLogger(PasswordManagerLogger* logger) = 0;
 
   // Send |text| to the logger.
-  virtual void LogSavePasswordProgress(const std::string& text);
+  virtual void LogSavePasswordProgress(const std::string& text) = 0;
+
+  // Returns true if logs recorded via LogSavePasswordProgress will be
+  // displayed, and false otherwise.
+  virtual bool IsLoggingActive() const = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PasswordManagerClient);

components/password_manager/core/browser/stub_password_manager_client.cc

@@ -22,4 +22,11 @@ PasswordStore* StubPasswordManagerClient::GetPasswordStore() { return NULL; }
 
 PasswordManagerDriver* StubPasswordManagerClient::GetDriver() { return NULL; }
 
+void StubPasswordManagerClient::SetLogger(PasswordManagerLogger* logger) {}
+
+void StubPasswordManagerClient::LogSavePasswordProgress(
+    const std::string& text) {}
+
+bool StubPasswordManagerClient::IsLoggingActive() const { return false; }
+
 }  // namespace password_manager

components/password_manager/core/browser/stub_password_manager_client.h

@@ -25,6 +25,9 @@ class StubPasswordManagerClient : public PasswordManagerClient {
   virtual PrefService* GetPrefs() OVERRIDE;
   virtual PasswordStore* GetPasswordStore() OVERRIDE;
   virtual PasswordManagerDriver* GetDriver() OVERRIDE;
+  virtual void SetLogger(PasswordManagerLogger* logger) OVERRIDE;
+  virtual void LogSavePasswordProgress(const std::string& text) OVERRIDE;
+  virtual bool IsLoggingActive() const OVERRIDE;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(StubPasswordManagerClient);