Password manager internals page: Introduce logger in renderer

components/password_manager/core/browser/password_manager_client.h

Index: components/password_manager/core/browser/password_manager_client.h
diff --git a/components/password_manager/core/browser/password_manager_client.h b/components/password_manager/core/browser/password_manager_client.h
index 4990a18511ddd09561854ec6e8d60334ec3952b3..7a254918d76d5d233929760eb6f3cd8be999a83b 100644
--- a/components/password_manager/core/browser/password_manager_client.h
+++ b/components/password_manager/core/browser/password_manager_client.h
@@ -67,6 +67,10 @@ class PasswordManagerClient {
   // Send |text| to the logger.
   virtual void LogSavePasswordProgress(const std::string& text);
 
+  // Return value true means that logs sent to LogSavePasswordProgress will be
+  // displayed. Return value false means they will be dropped.

[Ilya Sherman, 2014/04/09 21:10:08]

nit: "Returns true if logs recorded via LogSavePasswordProgress will be
displayed, and false otherwise."

[vabr (Chromium), 2014/04/10 08:51:25]

Done.

+  virtual bool IsLoggingActive() const;

[Ilya Sherman, 2014/04/09 21:10:08]

Should this be pure virtual?  Ditto for the two methods above, actually.

Actually actually, are these methods needed on the base class at all?  Can they
perhaps be moved into the ContentPasswordManagerClient class instead?

[vabr (Chromium), 2014/04/10 08:51:25]

I'm not sure. My reason for providing default implementation (for all 3 of them)
was that a PasswordManagerClient works still reasonably if the logging feature
is off (whereas it cannot if it has no Prefs or PasswordStore, for example).
I'd like to keep them up here, because I expect we'll need this kind of logging
on the mobile platforms as well (note: there is no ContentPasswordManagerClient,
it's ChromePasswordManagerClient, so excluding also Android). Also, if I move it
to the embedder, I'd need to move the SavePasswordProgressLogger and children
there as well, as they use these methods. 

[Ilya Sherman, 2014/04/10 09:23:07]

Let's make them pure virtual for now.  We can always revisit the decision later
if need be; but I think it's nice to have PasswordManagerClient be a pure
interface if it can be.
Fair enough :)

[blundell, 2014/04/11 14:39:16]

PasswordClient is already not a pure interface FWIW (see e.g.
PasswordWasAutofilled). I think that there's value in having "optional" client
methods have default implementations, compared to "mandatory" client methods
that are pure virtual (e.g., GetPrefs()).

On 2014/04/10 09:23:07, Ilya Sherman wrote:

+
  private:
   DISALLOW_COPY_AND_ASSIGN(PasswordManagerClient);
 };