Extend logging for password manager internals page

Extend logging for password manager internals page

This CL adds logging for the internals page during filling passwords in the
renderer code. Those have been missing and would have been helpful for
investigating the associated bug.

BUG=606736
Committed: https://crrev.com/4b06ffc533f2c176a235caa7ef0e0975b3c6f4c3
Cr-Commit-Position: refs/heads/master@{#389810}

[vabr (Chromium), 2016-04-26 12:37:58]

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

[commit-bot: I haz the power, 2016-04-26 12:38:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1918823005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1918823005/1

[commit-bot: I haz the power, 2016-04-26 12:51:19]

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

[commit-bot: I haz the power, 2016-04-26 12:51:20]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[vabr (Chromium), 2016-04-26 12:56:06]

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

[commit-bot: I haz the power, 2016-04-26 12:56:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1918823005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1918823005/20001

[vabr (Chromium), 2016-04-26 13:25:12]

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

[vabr (Chromium), 2016-04-26 13:25:13]

Hi Vadym

Please take a look.

Thanks!
Vaclav

[commit-bot: I haz the power, 2016-04-26 13:43:22]

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

[commit-bot: I haz the power, 2016-04-26 13:43:23]

Dry run: This issue passed the CQ dry run.

[dvadym, 2016-04-26 14:47:35]

LGTM. Thanks, it will be helpful to have more logging. There are some
suggestions, feel free to ignore them if you think that it would be not very
helpful.

https://codereview.chromium.org/1918823005/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1918823005/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:506:
logger->LogMessage(Logger::STRING_USERNAME_FILLED);
It seems that it would be helpful to log also nameForAutofill() of a username
field, then we will know from log, whether we fill incorrectly. WDYT?

https://codereview.chromium.org/1918823005/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:526:
logger->LogMessage(Logger::STRING_PASSWORD_FILLED);
It seems that it would be helpful to log also nameForAutofill() of a password
field, then we will know from log, whether we fill incorrectly. WDYT?

[vabr (Chromium), 2016-04-26 16:55:41]

Thanks, Vadym!

I do think your suggestions are helpful, but as I explain below, my proposal is
to do them in a separate CL.

Since you allowed me the discretion of addressing the comments as I see fit,
I'll land this as it is and will do the follow-up.

Cheers,
Vaclav

https://codereview.chromium.org/1918823005/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1918823005/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:506:
logger->LogMessage(Logger::STRING_USERNAME_FILLED);
On 2016/04/26 14:47:34, dvadym wrote:
> It seems that it would be helpful to log also nameForAutofill() of a username
> field, then we will know from log, whether we fill incorrectly. WDYT?

While I agree, this would need some more changes to the
SavePasswordProgressLogger. Currently, no free strings are allowed, for security
reasons. The only exception is the element names as captured in a PasswordForm,
there the code does some processing to avoid exploits. So what we'd need is to
add a method similar to SavePasswordProgressLogger::LogPasswordForm, but just
logging element names. It's not a lot of work, but I wanted to keep this CL
small.

My suggestion is to do a follow-up CL for the logging of element names.

https://codereview.chromium.org/1918823005/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:526:
logger->LogMessage(Logger::STRING_PASSWORD_FILLED);
On 2016/04/26 14:47:34, dvadym wrote:
> It seems that it would be helpful to log also nameForAutofill() of a password
> field, then we will know from log, whether we fill incorrectly. WDYT?

Ditto.

[vabr (Chromium), 2016-04-26 16:55:48]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-04-26 16:56:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1918823005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1918823005/20001

[commit-bot: I haz the power, 2016-04-26 17:00:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 17:01:55]

Description was changed from

==========
Extend logging for password manager internals page

This CL adds logging for the internals page during filling passwords in the
renderer code. Those have been missing and would have been helpful for
investigating the associated bug.

BUG=606736
==========

to

==========
Extend logging for password manager internals page

This CL adds logging for the internals page during filling passwords in the
renderer code. Those have been missing and would have been helpful for
investigating the associated bug.

BUG=606736

Committed: https://crrev.com/4b06ffc533f2c176a235caa7ef0e0975b3c6f4c3
Cr-Commit-Position: refs/heads/master@{#389810}
==========

[commit-bot: I haz the power, 2016-04-26 17:01:56]

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