Implement logging of the Credential Manager API calls to chrome://password-manager-internals.

components/password_manager/core/browser/credential_manager_logger.cc

Index: components/password_manager/core/browser/credential_manager_logger.cc
diff --git a/components/password_manager/core/browser/credential_manager_logger.cc b/components/password_manager/core/browser/credential_manager_logger.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a370f85fcdc0574b004d23c73fed1ebec7cd1b86
--- /dev/null
+++ b/components/password_manager/core/browser/credential_manager_logger.cc
@@ -0,0 +1,68 @@
+// Copyright 2016 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/password_manager/core/browser/credential_manager_logger.h"
+
+#include <sstream>
+
+#include "components/password_manager/core/browser/log_manager.h"
+
+namespace password_manager {
+namespace {
+
+GURL ScrubURL(const GURL& url) {

[vabr (Chromium), 2016/08/29 13:10:44]

I'd prefer to have this function consistent among the loggers. Could you make
CredentialManagerLogger a subclass of SavePasswordProgressLogger and use
SavePasswordProgressLogger::ScrubURL?

[vasilii, 2016/08/29 15:12:39]

SavePasswordProgressLogger is abstract and heavy. But I merged the
implementations.

[vabr (Chromium), 2016/08/29 15:27:37]

Acknowledged.

+  return url.GetWithEmptyPath();
+}
+
+}  // namespace
+
+CredentialManagerLogger::CredentialManagerLogger(const LogManager* log_manager)
+    : log_manager_(log_manager) {}
+
+CredentialManagerLogger::~CredentialManagerLogger() = default;
+
+void CredentialManagerLogger::LogRequestCredential(
+    const GURL& url,
+    bool zero_click_only,
+    const std::vector<GURL>& federations) {
+  if (!log_manager_->IsLoggingActive())

[vabr (Chromium), 2016/08/29 13:10:44]

Please move this check out to the logging code (see my comment in
credential_manager_impl.cc).
Also below.

[vasilii, 2016/08/29 15:12:39]

Done.

+    return;
+  std::stringstream ss;

[vabr (Chromium), 2016/08/29 13:10:44]

Streams are not completely forbidden [1], but a lot of Chromium code is written
with the assumption that << is only meant for logging. For example, the << for
GURL, which is used below, has this comment: "Stream operator so GURL can be
used in assertion statements."

The similar class BrowserSavePasswordProgressLogger uses just string and
operator + for this reason. I would suggest to follow that pattern.


[1] https://google.github.io/styleguide/cppguide.html#Streams

[vasilii, 2016/08/29 15:12:39]

You mention that << is used for logging while I am implementing a logger.
What to do with CredentialType::operator<< used here?

[vabr (Chromium), 2016/08/29 15:27:38]

Sorry for my poor choice of words :). I meant console logging as opposed to the
internals page. The style guide says: "Avoid using streams for I/O that faces
external users," and the internals page seems more user-facing than console
logs, which are off by default.
One alternative would be to have a "ToString" function for CredentialType and
use it both in the implementation of operator<< and for the internals page.

One of the drawbacks mentioned by the style guide is that: "Resolving the many
overloads of << is extremely costly for the compiler," so it sounds worth
keeping it out of the codebase, especially given it is still rather rare in
//components (except for console logging).

[vasilii, 2016/08/29 17:20:11]

Done.

+  ss << "CM API get credentials: origin=" << ScrubURL(url)
+     << ", zero_click_only=" << zero_click_only << ", federations=";
+  for (const GURL& federation_provider : federations)
+    ss << ScrubURL(federation_provider) << ", ";
+
+  log_manager_->LogSavePasswordProgress(ss.str());
+}
+
+void CredentialManagerLogger::LogSendCredential(const GURL& url,
+                                                CredentialType type) {
+  if (!log_manager_->IsLoggingActive())
+    return;
+  std::stringstream ss;
+  ss << "CM API send a credential: origin=" << ScrubURL(url)
+     << ", CredentialType=" << type;
+  log_manager_->LogSavePasswordProgress(ss.str());
+}
+
+void CredentialManagerLogger::LogStoreCredential(const GURL& url,
+                                                 CredentialType type) {
+  if (!log_manager_->IsLoggingActive())
+    return;
+  std::stringstream ss;
+  ss << "CM API save a credential: origin=" << ScrubURL(url)
+     << ", CredentialType=" << type;
+  log_manager_->LogSavePasswordProgress(ss.str());
+}
+
+void CredentialManagerLogger::LogRequireUserMediation(const GURL& url) {
+  if (!log_manager_->IsLoggingActive())
+    return;
+  std::stringstream ss;
+  ss << "CM API sign out: origin=" << ScrubURL(url);
+  log_manager_->LogSavePasswordProgress(ss.str());
+}
+
+}  // namespace password_manager