Implement logging of the Credential Manager API calls to chrome://password-manager-internals.

Implement logging of the Credential Manager API calls to chrome://password-manager-internals.

BUG=400674
Committed: https://crrev.com/5ac4a0fe33d587cbce11fb6b4a5b9e5c74ab9855
Cr-Commit-Position: refs/heads/master@{#415268}

[vasilii, 2016-08-29 12:37:52]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 12:38:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-08-29 12:38:42]

vasilii@chromium.org changed reviewers:
+ vabr@chromium.org

[vasilii, 2016-08-29 12:38:42]

Hi Vaclav,

please review.

[vabr (Chromium), 2016-08-29 13:10:44]

Hi Vasilii,

I have some comments below.

Cheers,
Vaclav

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_impl.cc:56:
CredentialManagerLogger(client_->GetLogManager())
For all the current logging the code checks LogManager::IsLoggingActive() as
soon as possible, so that the performance impact on the code is minimised in
cases when the internals page is not open. Please follow that pattern here as
well, do not construct the logger before you know you need it.

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:14: GURL
ScrubURL(const GURL& url) {
I'd prefer to have this function consistent among the loggers. Could you make
CredentialManagerLogger a subclass of SavePasswordProgressLogger and use
SavePasswordProgressLogger::ScrubURL?

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:29: if
(!log_manager_->IsLoggingActive())
Please move this check out to the logging code (see my comment in
credential_manager_impl.cc).
Also below.

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:31:
std::stringstream ss;
Streams are not completely forbidden [1], but a lot of Chromium code is written
with the assumption that << is only meant for logging. For example, the << for
GURL, which is used below, has this comment: "Stream operator so GURL can be
used in assertion statements."

The similar class BrowserSavePasswordProgressLogger uses just string and
operator + for this reason. I would suggest to follow that pattern.


[1] https://google.github.io/styleguide/cppguide.html#Streams

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/core/browser/credential_manager_logger.h
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.h:39: }  //
namespace password_manager
nit: Should there be a blank line between 39 and 40?

[commit-bot: I haz the power, 2016-08-29 13:46:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 13:46:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vasilii, 2016-08-29 15:12:38]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[vasilii, 2016-08-29 15:12:39]

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_impl.cc:56:
CredentialManagerLogger(client_->GetLogManager())
On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> For all the current logging the code checks LogManager::IsLoggingActive() as
> soon as possible, so that the performance impact on the code is minimised in
> cases when the internals page is not open. Please follow that pattern here as
> well, do not construct the logger before you know you need it.

Done.

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:14: GURL
ScrubURL(const GURL& url) {
On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> I'd prefer to have this function consistent among the loggers. Could you make
> CredentialManagerLogger a subclass of SavePasswordProgressLogger and use
> SavePasswordProgressLogger::ScrubURL?

SavePasswordProgressLogger is abstract and heavy. But I merged the
implementations.

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:29: if
(!log_manager_->IsLoggingActive())
On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> Please move this check out to the logging code (see my comment in
> credential_manager_impl.cc).
> Also below.

Done.

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:31:
std::stringstream ss;
On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> Streams are not completely forbidden [1], but a lot of Chromium code is
written
> with the assumption that << is only meant for logging. For example, the << for
> GURL, which is used below, has this comment: "Stream operator so GURL can be
> used in assertion statements."
> 
> The similar class BrowserSavePasswordProgressLogger uses just string and
> operator + for this reason. I would suggest to follow that pattern.
> 
> 
> [1] https://google.github.io/styleguide/cppguide.html#Streams

You mention that << is used for logging while I am implementing a logger.
What to do with CredentialType::operator<< used here?

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/core/browser/credential_manager_logger.h
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.h:39: }  //
namespace password_manager
On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> nit: Should there be a blank line between 39 and 40?

Done.

[vabr (Chromium), 2016-08-29 15:27:38]

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:14: GURL
ScrubURL(const GURL& url) {
On 2016/08/29 15:12:39, vasilii wrote:
> On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> > I'd prefer to have this function consistent among the loggers. Could you
make
> > CredentialManagerLogger a subclass of SavePasswordProgressLogger and use
> > SavePasswordProgressLogger::ScrubURL?
> 
> SavePasswordProgressLogger is abstract and heavy. But I merged the
> implementations.

Acknowledged.

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:31:
std::stringstream ss;
On 2016/08/29 15:12:39, vasilii wrote:
> On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> > Streams are not completely forbidden [1], but a lot of Chromium code is
> written
> > with the assumption that << is only meant for logging. For example, the <<
for
> > GURL, which is used below, has this comment: "Stream operator so GURL can be
> > used in assertion statements."
> > 
> > The similar class BrowserSavePasswordProgressLogger uses just string and
> > operator + for this reason. I would suggest to follow that pattern.
> > 
> > 
> > [1] https://google.github.io/styleguide/cppguide.html#Streams
> 
> You mention that << is used for logging while I am implementing a logger.
Sorry for my poor choice of words :). I meant console logging as opposed to the
internals page. The style guide says: "Avoid using streams for I/O that faces
external users," and the internals page seems more user-facing than console
logs, which are off by default.


> What to do with CredentialType::operator<< used here?
One alternative would be to have a "ToString" function for CredentialType and
use it both in the implementation of operator<< and for the internals page.

One of the drawbacks mentioned by the style guide is that: "Resolving the many
overloads of << is extremely costly for the compiler," so it sounds worth
keeping it out of the codebase, especially given it is still rather rare in
//components (except for console logging).

[vasilii, 2016-08-29 17:19:52]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[vasilii, 2016-08-29 17:20:12]

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2293443002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_logger.cc:31:
std::stringstream ss;
On 2016/08/29 15:27:38, vabr (Chromium) wrote:
> On 2016/08/29 15:12:39, vasilii wrote:
> > On 2016/08/29 13:10:44, vabr (Chromium) wrote:
> > > Streams are not completely forbidden [1], but a lot of Chromium code is
> > written
> > > with the assumption that << is only meant for logging. For example, the <<
> for
> > > GURL, which is used below, has this comment: "Stream operator so GURL can
be
> > > used in assertion statements."
> > > 
> > > The similar class BrowserSavePasswordProgressLogger uses just string and
> > > operator + for this reason. I would suggest to follow that pattern.
> > > 
> > > 
> > > [1] https://google.github.io/styleguide/cppguide.html#Streams
> > 
> > You mention that << is used for logging while I am implementing a logger.
> Sorry for my poor choice of words :). I meant console logging as opposed to
the
> internals page. The style guide says: "Avoid using streams for I/O that faces
> external users," and the internals page seems more user-facing than console
> logs, which are off by default.
> 
> 
> > What to do with CredentialType::operator<< used here?
> One alternative would be to have a "ToString" function for CredentialType and
> use it both in the implementation of operator<< and for the internals page.
> 
> One of the drawbacks mentioned by the style guide is that: "Resolving the many
> overloads of << is extremely costly for the compiler," so it sounds worth
> keeping it out of the codebase, especially given it is still rather rare in
> //components (except for console logging).

Done.

[commit-bot: I haz the power, 2016-08-29 17:29:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-29 17:46:18]

Thanks, LGTM with minor comments.

Cheers,
Vaclav

https://codereview.chromium.org/2293443002/diff/40001/components/password_man...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2293443002/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_logger.cc:29: s +=
(", zero_click_only=" + base::IntToString(zero_click_only));
nit: The outer parentheses here are not needed.

Also on line 32.

https://codereview.chromium.org/2293443002/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_logger.cc:34:
log_manager_->LogSavePasswordProgress(std::move(s));
The std::move does not help, because LogSavePasswordProgress takes a const ref.

(Also below.)

[commit-bot: I haz the power, 2016-08-30 03:43:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 03:43:17]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-08-30 08:46:07]

https://codereview.chromium.org/2293443002/diff/40001/components/password_man...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2293443002/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_logger.cc:29: s +=
(", zero_click_only=" + base::IntToString(zero_click_only));
On 2016/08/29 17:46:17, vabr (Chromium) wrote:
> nit: The outer parentheses here are not needed.
> 
> Also on line 32.

Done.

https://codereview.chromium.org/2293443002/diff/40001/components/password_man...
components/password_manager/core/browser/credential_manager_logger.cc:34:
log_manager_->LogSavePasswordProgress(std::move(s));
On 2016/08/29 17:46:17, vabr (Chromium) wrote:
> The std::move does not help, because LogSavePasswordProgress takes a const
ref.
> 
> (Also below.)

Done.

[vasilii, 2016-08-30 08:46:25]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-08-30 08:46:26]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2293443002/#ps60001
(title: "nits")

[vasilii, 2016-08-30 08:46:35]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2016-08-30 08:46:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 10:52:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-30 10:54:18]

Description was changed from

==========
Implement logging of the Credential Manager API calls to
chrome://password-manager-internals.

BUG=400674
==========

to

==========
Implement logging of the Credential Manager API calls to
chrome://password-manager-internals.

BUG=400674

Committed: https://crrev.com/5ac4a0fe33d587cbce11fb6b4a5b9e5c74ab9855
Cr-Commit-Position: refs/heads/master@{#415268}
==========

[commit-bot: I haz the power, 2016-08-30 10:54:19]

Patchset 4 (id:??) landed as
https://crrev.com/5ac4a0fe33d587cbce11fb6b4a5b9e5c74ab9855
Cr-Commit-Position: refs/heads/master@{#415268}