Password manager internals page: Introduce logger in renderer

components/password_manager/core/browser/password_manager_client.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_PASSWORD_MANAGER_CLIENT_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_CLIENT_H_
 
 #include "base/metrics/field_trial.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/password_form_fill_data.h"
@@ skipping 49 matching lines @@
   // Returns true if password sync is enabled in the embedder. The default
   // implementation returns false.
   virtual bool IsPasswordSyncEnabled();
 
   // Attach or detach (setting NULL) a logger for this client.
   virtual void SetLogger(PasswordManagerLogger* logger);
 
   // 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);
 };
 
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_CLIENT_H_