Fix password manager internals renderer reporting

components/password_manager/core/browser/log_manager.cc

Index: components/password_manager/core/browser/log_manager.cc
diff --git a/components/password_manager/core/browser/log_manager.cc b/components/password_manager/core/browser/log_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..33f9f70445004455a090849acf5075eac5e7fcb9
--- /dev/null
+++ b/components/password_manager/core/browser/log_manager.cc
@@ -0,0 +1,79 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.

[vabr (Chromium), 2015/11/12 21:55:44]

In response to:
Yes, good idea! I separated the LogManager implementation into a LogManagerImpl
(keeping the interface for easy testing).

[vasilii, 2015/11/13 12:17:31]

I think the CL got better after this change :-)

+// 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/log_manager.h"
+
+#include "base/macros.h"
+#include "components/password_manager/core/browser/log_router.h"
+
+namespace password_manager {
+
+namespace {
+
+class LogManagerImpl : public LogManager {
+ public:
+  LogManagerImpl(LogRouter* log_router,
+                 base::Callback<void(bool)> notification_callback);
+
+  ~LogManagerImpl() override;
+
+  // LogManager
+  void OnLogRouterAvailabilityChanged(bool router_can_be_used) override;
+  void LogSavePasswordProgress(const std::string& text) const override;
+  bool IsLoggingActive() const override;
+
+ private:
+  // A LogRouter instance obtained on construction. May be null.
+  LogRouter* const log_router_;
+
+  // True if |this| is registered with some LogRouter which can accept logs.
+  bool can_use_log_router_;
+
+  // Called every time the logging activity status changes, passing the status
+  // as the argument.
+  base::Callback<void(bool)> notification_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(LogManagerImpl);
+};
+
+LogManagerImpl::LogManagerImpl(LogRouter* log_router,
+                               base::Callback<void(bool)> notification_callback)
+    : log_router_(log_router),
+      can_use_log_router_(log_router_ && log_router_->RegisterManager(this)),
+      notification_callback_(notification_callback) {}
+
+LogManagerImpl::~LogManagerImpl() {
+  if (log_router_)
+    log_router_->UnregisterManager(this);
+}
+
+void LogManagerImpl::OnLogRouterAvailabilityChanged(bool router_can_be_used) {
+  DCHECK(log_router_);  // |log_router_| should be calling this method.
+  if (can_use_log_router_ == router_can_be_used)
+    return;
+  can_use_log_router_ = router_can_be_used;
+
+  notification_callback_.Run(can_use_log_router_);

[vasilii, 2015/11/13 12:17:31]

Can the callback be NULL?

[vabr (Chromium), 2015/11/13 20:48:18]

Good question! I think it makes sense to allow it being null, because then there
is still IsLoggingActive for some usecases. I made it work with null callbacks
now.

+}
+
+void LogManagerImpl::LogSavePasswordProgress(const std::string& text) const {
+  if (!IsLoggingActive())
+    return;
+  log_router_->ProcessLog(text);
+}
+
+bool LogManagerImpl::IsLoggingActive() const {
+  return can_use_log_router_;
+}
+
+}  // namespace
+
+//  static

[vasilii, 2015/11/13 12:17:31]

one space

[vabr (Chromium), 2015/11/13 20:48:18]

Done.

+scoped_ptr<LogManager> LogManager::Create(
+    LogRouter* log_router,
+    base::Callback<void(bool)> notification_callback) {
+  return make_scoped_ptr(new LogManagerImpl(log_router, notification_callback));
+}
+
+}  // namespace password_manager