Password manager internals page service: introduction

components/password_manager/core/browser/log_router.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_PASSWORD_MANAGER_CORE_BROWSER_LOG_ROUTER_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_LOG_ROUTER_H_
+
+#include <set>
+#include <string>
+
+#include "base/macros.h"
+
+namespace password_manager {
+
+class PasswordManagerClient;
+class PasswordManagerLogger;
+
+// The router stands between PasswordManagerClient instances and log receivers.
+// During the process of saving a password, the password manager code generates
+// the log strings, and passes them to the router. The receivers are capable of
+// actually displaying them to the user; the receivers can register with and
+// unregister from the router. The router updates the registered receivers with
+// new logs. It also stores all logs until the moment when there are no
+// registered receivers. On registration, a receiver gets all the currently
+// stored logs.

[Ilya Sherman, 2014/05/02 23:38:38]

Is this complexity worth it, vs. just making the internals page be a singleton?

[vabr (Chromium), 2014/05/06 13:16:30]

Having the internals page a singleton is only a half of the problem -- another
one is how to discover it from the other tabs. There is an established and easy
way to get hold of a service, but not a one for a UI controller.

Also having the internals page as a singleton would require some non-trivial
code arrangement, IMO. I did not see an easy way to handle things like the user
trying to open multiple tabs with the internals page while keeping the UI
controller a singleton.

+// The clients must also register, to be notified about the router toggling its
+// activity state.
+// TODO(vabr): The receivers are objects of type PasswordManagerLogger. That
+// type should be renamed to LogReceiver instead.
+class LogRouter {
+ public:
+  // Depending on whether the router has any receivers registered or not, it is
+  // active or not. Logs passed to the router are forwarded during the active
+  // state, and dropped otherwise. The clients are notified about the state on
+  // its change, and on registration.
+  enum RouterActivityState { ROUTER_ACTIVE, ROUTER_NOT_ACTIVE };

[Ilya Sherman, 2014/05/02 23:38:38]

nit: "NOT_ACTIVE" -> "INACTIVE"

[vabr (Chromium), 2014/05/06 13:16:30]

Actually, I dropped the enum. Instead of introducing the notion of
"active"/"inactive" I tried to directly speak about having 0 or more receivers.
In the follow-up CL, where I will introduce the API for state change
notification (switching between 0 and more receivers), if having the enum proves
more readable than sending a bool, I'll reintroduce it with the word "inactive"
for the opposite of active.

+
+  LogRouter();
+  virtual ~LogRouter();
+
+  // Call this to pass logs to the router. The router may display them or drop
+  // them, based on its activity state.
+  void ProcessLog(const std::string& text);

[Ilya Sherman, 2014/05/02 23:38:38]

Perhaps it should be an error to call this method when the router is inactive?

[vabr (Chromium), 2014/05/06 13:16:30]

I'll give it a try, since I could not actually think of a situation when the
user would close the internals page just at the right time so the client would
still think it's active and call ProcessLog, while the state would already have
changed.

If there turns out to be a situation like this, then I'd like to go back to
dropping the logs silently, because at the time the internals page is being
closed, nobody is interested in them any more.

Done.

+
+  // Registration methods for the PasswordManagerClient and for the receivers to
+  // call, both return true on success. Register*() fails for already registered
+  // objects. Unregister*() fails for not registered objects. Never pass a NULL
+  // pointer to these methods. Every registered client and receiver must
+  // unregister before being destroyed.
+  bool RegisterClient(PasswordManagerClient* client);

[vabr (Chromium), 2014/05/02 15:33:10]

TODO(vabr): Remove the failure reporting via return value (turned out that in my
subsequent CL I was always ignoring the value). Make RegisterClient return the
current logging activity state instead. (That will spare some headaches in the
next CL about RegisterClient calling directly back to the client to set the
state via a virtual method, in combination with RegisterClient itself being
called from the client's constructor.)

+  bool UnregisterClient(PasswordManagerClient* client);
+  bool RegisterReceiver(PasswordManagerLogger* receiver);
+  bool UnregisterReceiver(PasswordManagerLogger* receiver);
+
+ private:
+  typedef std::set<PasswordManagerClient*> ClientSet;
+  typedef std::set<PasswordManagerLogger*> ReceiverSet;

[Ilya Sherman, 2014/05/02 23:38:38]

nit: Did you consider using an ObserverList?

[vabr (Chromium), 2014/05/06 13:16:30]

Thanks for pointing me to that.
While I'm not sure I need the guarantee that iterators through the list are not
invalidated during list modification, I appreciate that using the ObserverList
will make the code easier to understand, and also reduce the amount of new code.
Done.

+
+  // Goes through all of the clients and sends them the current state.
+  void NotifyClients(RouterActivityState state);
+
+  // Clients send log strings to the router, and are notified by the router when
+  // logging activity changes.
+  ClientSet clients_;
+
+  // Receivers receive strings from the router.
+  ReceiverSet receivers_;
+
+  // Logs seen so far since the last time IsActive() changed from false to true.
+  std::string accumulated_logs_;
+
+  DISALLOW_COPY_AND_ASSIGN(LogRouter);
+};
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_LOG_ROUTER_H_