Password manager internals page service: introduction

components/password_manager/core/browser/log_router.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/password_manager/core/browser/log_router.h"
+
+#include "base/stl_util.h"
+#include "components/password_manager/core/browser/password_manager_client.h"
+#include "components/password_manager/core/browser/password_manager_logger.h"
+
+namespace password_manager {
+
+LogRouter::LogRouter() {
+}
+
+LogRouter::~LogRouter() {
+}
+
+void LogRouter::ProcessLog(const std::string& text) {
+  // This may not be called when there are no receivers (i.e., the router is
+  // inactive), because in that case the logs cannot be displayed.
+  CHECK(receivers_.might_have_observers());

[Ilya Sherman, 2014/05/07 00:28:39]

nit: DCHECK?

[vabr (Chromium), 2014/05/07 13:42:59]

I was thinking about a DCHECK, but there's a catch to that:
If I allow release builds to ProcessLog when there's no receiver, I still need
to not add the logs to accumulated_logs_ (line below).

That's something I need to test, and such tests would need to be Release-only
(in Debug they would crash).

I understood that it is a poor practice to have Release and Debug have different
behaviour up to the point of needing different tests for them. That's why I
chose CHECK.

As long as the LogRouter only lives on a single thread (added ThreadChecker to
guard and document that -- note that guarding only works in Debug), a
cooperating client should be able to 100% avoid calling ProcessLog when there's
no receiver.

If you prefer Release-build-only tests to the danger of release build crashes,
please let me know. Alternatively we can switch back to being permissive and
make ProcessLog a harmless no-op when there is no receiver (with adding the
tests mentioned above, but for both Release and Debug).

[Ilya Sherman, 2014/05/08 03:38:52]

A DCHECK doesn't mean that something is disallowed in Debug builds and allowed
in Release builds.  It's an assertion that a certain condition should always
hold.  However, that assertion is only checked in Debug builds, for a variety of
contentious reasons like performance and binary bloat.  Essentially, none of the
reasons apply to an individual assertion; but they're thought to add up across
the whole codebase.

CHECK is generally reserved for exceptional cases: For example to prevent
potential security exploits due to subtle or easy-to-misuse APIs, or to generate
crash dumps to investigate a hard-to-reproduce issue.

[vabr (Chromium), 2014/05/08 17:41:54]

Thanks Ilya, for explaining the use-cases for CHECK vs. DCHECK. I should have
read
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
more carefully.

Since the style guide also says not to handle situations after a DCHECK, I did
not try to avoid adding stuff to accumulated_logs_ when there are no receivers.
Instead I put one more accumulated_logs_.clear() at the place when the first
receiver is registered (and would otherwise get the accumulated_logs_). That
seems like a reasonable thing to have anyway. I did not add any new tests,
because there is nothing new to test in Debug. If you want me to add a
Release-only test for not preserving logs passed through ProcessLog without
receivers, just let me know.

+  accumulated_logs_.append(text);
+  FOR_EACH_OBSERVER(
+      PasswordManagerLogger, receivers_, LogSavePasswordProgress(text));
+}
+
+bool LogRouter::RegisterClient(PasswordManagerClient* client) {
+  DCHECK(client);
+  clients_.AddObserver(client);
+  return receivers_.might_have_observers();
+}
+
+void LogRouter::UnregisterClient(PasswordManagerClient* client) {
+  DCHECK(client);
+  clients_.RemoveObserver(client);
+}
+
+std::string LogRouter::RegisterReceiver(PasswordManagerLogger* receiver) {
+  DCHECK(receiver);
+  DCHECK(accumulated_logs_.empty() || receivers_.might_have_observers());
+
+  // TODO(vabr): Once the clients provide API for that, notify them if the
+  // number of receivers went from 0 to 1.
+  receivers_.AddObserver(receiver);
+  return accumulated_logs_;
+}
+
+void LogRouter::UnregisterReceiver(PasswordManagerLogger* receiver) {
+  DCHECK(receiver);

[Ilya Sherman, 2014/05/07 00:28:39]

nit: IMO DCHECK(receivers_.HasObserver(receiver)) would be more meaningful.

[vabr (Chromium), 2014/05/07 13:42:59]

Agreed, also for the UnregisterClient.
Done.

+  receivers_.RemoveObserver(receiver);
+  if (!receivers_.might_have_observers()) {
+    accumulated_logs_.clear();
+    // TODO(vabr): Once the clients provide API for that, notify them that the
+    // number of receivers went from 1 to 0.
+  }
+}
+
+}  // namespace password_manager