Password manager: introduce logging for the internals page

Password manager: introduce logging for the internals page

Follow-up CL of https://codereview.chromium.org/228263004/

This CL adds instances of SavePasswordProgressLogger into crucial parts of the password manager code. After this CL, the logging should already work, i.e., when the command line flag is enabled, the internals page should show debugging info about saving a password on a website.

More context in the design doc: https://docs.google.com/document/d/1ArDhTo0w-8tOPiTwqM1gG6ZGqODo8UTpXlJjjnCNK4s/edit?usp=sharing

BUG=347927
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266481

[vabr (Chromium), 2014-04-10 14:15:31]

Hi Ilya,

This CL actually uses the loggers introduced previously, PTAL.

The internals page itself seems to have a couple of bugs so far (still logs
itself, once it even crashed in Debug), but that's a different part of code, and
I'll fix it in separate CLs. Given that the whole thing is behind a flag, I hope
it's acceptable, but let me know if not.

Thanks,
Vaclav

[vabr (Chromium), 2014-04-10 14:27:56]

On 2014/04/10 14:15:31, vabr (Chromium) wrote:
> Hi Ilya,
> 
> This CL actually uses the loggers introduced previously, PTAL.
> 
> The internals page itself seems to have a couple of bugs so far (still logs
> itself, once it even crashed in Debug), but that's a different part of code,
and
> I'll fix it in separate CLs. Given that the whole thing is behind a flag, I
hope
> it's acceptable, but let me know if not.
> 
> Thanks,
> Vaclav

P.S. Never mind the trybots, I forgot this depends on
https://codereview.chromium.org/228263004/, which has not landed yet.

[vabr (Chromium), 2014-04-10 20:28:31]

Ilya, given the recent security discussions about this feature, I'll have to do
some changes on this CL as well.
Please ignore this CL until I notify you that it's ready again.

Thanks,
Vaclav

[vabr (Chromium), 2014-04-22 13:09:19]

Hi Ilya,

I updated this CL with the recent changes to the SavePasswordProgressLogger
interface. PTAL.

Issues I'd like your opinion on:

1) It looks like I should also test that the PasswordAutofillAgent starts with
the logging turned on, and reacts to the AutofillMsg_ChangeLoggingState
accordingly. But there seem to be no unit-tests for PasswordAutofillAgent -- do
I need to add one, or are tests done in another way for that class?

2) Could all the
  if (logger) {...}
scoped_ptr-is-NULL tests slow down the code significantly when logging is not
active (i.e., logger is NULL)?
The pointer value is not changed during the function calls, so I was wondering
if I should cache the value in a const pointer to allow some compiler
optimization, like:
  scoped_ptr<SavePasswordProgressLogger> logger(...);
  const SavePasswordProgressLogger* cached_logger(logger.get());
  if (cached_logger) {...}

Thanks,
Vaclav

[Ilya Sherman, 2014-04-23 05:19:55]

On 2014/04/22 13:09:19, vabr (Chromium) wrote:
> Hi Ilya,
> 
> I updated this CL with the recent changes to the SavePasswordProgressLogger
> interface. PTAL.
> 
> Issues I'd like your opinion on:
> 
> 1) It looks like I should also test that the PasswordAutofillAgent starts with
> the logging turned on, and reacts to the AutofillMsg_ChangeLoggingState
> accordingly. But there seem to be no unit-tests for PasswordAutofillAgent --
do
> I need to add one, or are tests done in another way for that class?

There is currently only a browser test [1].  If you feel up for adding a
lighter-weight set of unit tests, that would be great!

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au...

> 2) Could all the
>   if (logger) {...}
> scoped_ptr-is-NULL tests slow down the code significantly when logging is not
> active (i.e., logger is NULL)?

They shouldn't, no.  NULL pointer checks are ridiculously cheap, as is the
.get() accessor.

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:187: //
logging.
nit: "Also inform the renderer process to start or stop logging."

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:145:
logger->LogMessage(SavePasswordProgressLogger::STRING_SET_AUTH_METHOD);
nit: Please move the construction of the logger, as well as this first message,
to be above where |already_handled| is defined.  That helps make this logic
easier to follow.

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:150: if (already_handled )
nit: Spurious space.

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:158: if (logger)
nit: Needs curly braces.

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

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:206: void
LogHTMLForm(SavePasswordProgressLogger* logger,
nit: Docs, please.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:209:
DCHECK(logger);
nit: No need for this DCHECK, since you immediately use the logger below.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:413: if
(only_visible && logging_state_active_) {
nit: Why does the value of |only_visible| matter w.r.t. logging?  Worth
documenting, if this logic is accurate.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:416:
SavePasswordProgressLogger::STRING_SEND_PASSWORD_FORMS_METHOD);
The "SavePasswordProgressLogger::" is unfortunately long enough to cause most
logging lines to wrap.  What do you think of aliasing this name to something
shorter via a typedef at the top of the file?  Normally I'm not much of a fan of
that, but in this case, I think the string provides very little semantic context
relative to the code that already has to be present around it.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:421: if (logger)
nit: Needs curly braces.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:423:
GURL(origin.toString().utf8()));
The WebUrl class defines a conversion operator to a GURL.  Can you convert the
origin to a WebURL, rather than to a GURL directly?  That would feel more
natural in this code.  This applies also to the form.action() conversion that
you have above.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:467: if (logger)
nit: Needs curly braces.

[Ilya Sherman, 2014-04-23 05:19:56]

On 2014/04/22 13:09:19, vabr (Chromium) wrote:
> Hi Ilya,
> 
> I updated this CL with the recent changes to the SavePasswordProgressLogger
> interface. PTAL.
> 
> Issues I'd like your opinion on:
> 
> 1) It looks like I should also test that the PasswordAutofillAgent starts with
> the logging turned on, and reacts to the AutofillMsg_ChangeLoggingState
> accordingly. But there seem to be no unit-tests for PasswordAutofillAgent --
do
> I need to add one, or are tests done in another way for that class?

There is currently only a browser test [1].  If you feel up for adding a
lighter-weight set of unit tests, that would be great!

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au...

> 2) Could all the
>   if (logger) {...}
> scoped_ptr-is-NULL tests slow down the code significantly when logging is not
> active (i.e., logger is NULL)?

They shouldn't, no.  NULL pointer checks are ridiculously cheap, as is the
.get() accessor.

[vabr (Chromium), 2014-04-23 18:15:34]

Hi Ilya,

Thanks for your comments, PTAL at how I addressed them.

I'm working on the unit test for the P.A.Agent, but that will require some more
work and probably adding a couple of files, so will keep it for a separate CL,
if it's OK with you.

Also, to make typedefing the Save...Loger to just Logger I ran clang-format over
the password_autofill_agent.cc and password_manager.cc files. I'd say the
changes are not making it worse, but feel free to tell me to remove all
unnecessary ones.

Thanks,
Vaclav

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:187: //
logging.
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: "Also inform the renderer process to start or stop logging."

Done.

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:145:
logger->LogMessage(SavePasswordProgressLogger::STRING_SET_AUTH_METHOD);
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: Please move the construction of the logger, as well as this first
message,
> to be above where |already_handled| is defined.  That helps make this logic
> easier to follow.

Done.

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:150: if (already_handled )
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: Spurious space.

Done.

https://codereview.chromium.org/231283003/diff/130001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:158: if (logger)
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: Needs curly braces.

Done.

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

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:206: void
LogHTMLForm(SavePasswordProgressLogger* logger,
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: Docs, please.

Done.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:209:
DCHECK(logger);
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: No need for this DCHECK, since you immediately use the logger below.

Done.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:413: if
(only_visible && logging_state_active_) {
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: Why does the value of |only_visible| matter w.r.t. logging?  Worth
> documenting, if this logic is accurate.

Done.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:416:
SavePasswordProgressLogger::STRING_SEND_PASSWORD_FORMS_METHOD);
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> The "SavePasswordProgressLogger::" is unfortunately long enough to cause most
> logging lines to wrap.  What do you think of aliasing this name to something
> shorter via a typedef at the top of the file?  Normally I'm not much of a fan
of
> that, but in this case, I think the string provides very little semantic
context
> relative to the code that already has to be present around it.

I like the idea, super-long lines bothered me as I wrote this up.
I also considered shortening RendererSavePasswordProgressLogger to
RendererLogger, but that is not justified by un-breaking lines, so I did not do
that. Happy to do so if you thinks it improves consistency.

I also did the typedef in the PasswordManager code. Let me know if that's not
OK. The login_prompt.cc file contains only two mentions, so I left the typename
long there.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:421: if (logger)
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: Needs curly braces.

Done.

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:423:
GURL(origin.toString().utf8()));
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> The WebUrl class defines a conversion operator to a GURL.  Can you convert the
> origin to a WebURL, rather than to a GURL directly?  That would feel more
> natural in this code.
I'm afraid I found no way to get a WebURL from a WebSecurityOrigin or a
WebString. Please let me know if I missed something, but the current way is the
only I know to work.

> This applies also to the form.action() conversion that
> you have above.
form.action() is a WebString, I don't think there is a direct conversion to GURL
from that. Am I wrong?

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:467: if (logger)
On 2014/04/23 05:19:56, Ilya Sherman wrote:
> nit: Needs curly braces.

Done.
Sorry about these. I now went through the code looking for more and fixed those
places as well.

[Ilya Sherman, 2014-04-23 20:21:43]

LGTM with the remaining comments addressed.  Thanks, Václav!

On 2014/04/23 18:15:34, vabr (Chromium) wrote:
> I'm working on the unit test for the P.A.Agent, but that will require some
more
> work and probably adding a couple of files, so will keep it for a separate CL,
> if it's OK with you.

Works for me.  Thanks!

> Also, to make typedefing the Save...Loger to just Logger I ran clang-format
over
> the password_autofill_agent.cc and password_manager.cc files. I'd say the
> changes are not making it worse, but feel free to tell me to remove all
> unnecessary ones.

Would you mind doing this as a separate CL that you land first?  That way, this
CL just contains the diffs that are relevant to the introduced logging.

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

https://codereview.chromium.org/231283003/diff/130001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:423:
GURL(origin.toString().utf8()));
On 2014/04/23 18:15:35, vabr (Chromium) wrote:
> On 2014/04/23 05:19:56, Ilya Sherman wrote:
> > The WebUrl class defines a conversion operator to a GURL.  Can you convert
the
> > origin to a WebURL, rather than to a GURL directly?  That would feel more
> > natural in this code.
> I'm afraid I found no way to get a WebURL from a WebSecurityOrigin or a
> WebString. Please let me know if I missed something, but the current way is
the
> only I know to work.
> 
> > This applies also to the form.action() conversion that
> > you have above.
> form.action() is a WebString, I don't think there is a direct conversion to
GURL
> from that. Am I wrong?

Hmm, seems that you're right.  Ok, nevermind then.

https://codereview.chromium.org/231283003/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/231283003/diff/200001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:59:
const uint32 kMsgID = AutofillMsg_ChangeLoggingState::ID;
nit: De-indent this line and all of the other lines in the method body by two
spaces.

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

https://codereview.chromium.org/231283003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:210: void
LogHTMLForm(Logger* logger,
Optional nit: I'd prefer to still use the full class name for instances of the
SavePasswordProgressLogger class.  I'd only use the briefer name for referencing
members of the enum.  However, I don't feel strongly about this.

https://codereview.chromium.org/231283003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:280: const
PasswordFormFillData& fill_data = iter->second.fill_data;
The clang-format changes look fine, but I'd prefer if we could split them off
into a separate CL, so that they don't clutter this one.  Would you mind doing
so?  Feel free to TBR me so that you're not delayed any extra.

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:161: // match.
The previous alignment for this comment was better.  Alternately, we can move
this comment into the else-stmt, or somewhere else reasonable.  But it's not
really within the if-stmt.

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:166: }
nit: No need for curly braces

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:202:
logger->LogMessage(Logger::STRING_DECISION_DROP);
nit: Maybe it makes sense to move the logging into RecordFailure?

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:418: 31,
nit: This seems worse, as it's less obviously a date when spread across three
lines.

[vabr (Chromium), 2014-04-24 10:59:27]

Thanks, Ilya!
I addressed your comments, and also removed some "password_manager::" where
already inside password_manager namespace.

Peter,
Please review chrome/browser/ui/login/login_prompt.cc
The added "logger" reports values to a coming password manager internals page.

Tom,
Please review the added IPC message in
components/autofill/content/common/autofill_messages.h

Thanks all!
Vaclav

https://codereview.chromium.org/231283003/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/231283003/diff/200001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:59:
const uint32 kMsgID = AutofillMsg_ChangeLoggingState::ID;
On 2014/04/23 20:21:44, Ilya Sherman wrote:
> nit: De-indent this line and all of the other lines in the method body by two
> spaces.

Done.

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

https://codereview.chromium.org/231283003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:210: void
LogHTMLForm(Logger* logger,
On 2014/04/23 20:21:44, Ilya Sherman wrote:
> Optional nit: I'd prefer to still use the full class name for instances of the
> SavePasswordProgressLogger class.  I'd only use the briefer name for
referencing
> members of the enum.  However, I don't feel strongly about this.

Sounds reasonable. I changed this and checked that on all other places it's
really just for the enums, and also updated the comment at the typedef to
clarify that.

https://codereview.chromium.org/231283003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:280: const
PasswordFormFillData& fill_data = iter->second.fill_data;
On 2014/04/23 20:21:44, Ilya Sherman wrote:
> The clang-format changes look fine, but I'd prefer if we could split them off
> into a separate CL, so that they don't clutter this one.  Would you mind doing
> so?  Feel free to TBR me so that you're not delayed any extra.

Done as https://codereview.chromium.org/254573005/.
I appreciate the TBR offer from you! :)
I'll rebase this CL on the above, so the unrelated style changes should not
appear here since the next patch set, independent of whether the other CL has
already landed or not.

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:161: // match.
On 2014/04/23 20:21:44, Ilya Sherman wrote:
> The previous alignment for this comment was better.  Alternately, we can move
> this comment into the else-stmt, or somewhere else reasonable.  But it's not
> really within the if-stmt.

Thanks for catching this.
I moved this comment inside the else-if block, and similarly with the comment
preceding the if block -- that will ensure that running clang-format will not
break this in the future.

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:166: }
On 2014/04/23 20:21:44, Ilya Sherman wrote:
> nit: No need for curly braces

Done.

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:202:
logger->LogMessage(Logger::STRING_DECISION_DROP);
On 2014/04/23 20:21:44, Ilya Sherman wrote:
> nit: Maybe it makes sense to move the logging into RecordFailure?

That's a great idea! Done.

https://codereview.chromium.org/231283003/diff/200001/components/password_man...
components/password_manager/core/browser/password_manager.cc:418: 31,
On 2014/04/23 20:21:44, Ilya Sherman wrote:
> nit: This seems worse, as it's less obviously a date when spread across three
> lines.

Agreed. Also filed http://b/14285621 to ask if there could be a way to disable
clang-format for particular cases like this.

Also, looks like that trial is past its expiration date. I guess I should file a
bug to get it removed.

[vabr (Chromium), 2014-04-24 11:00:16]

Now adding Peter and Tom as reviewers for real.

Peter,
Please review chrome/browser/ui/login/login_prompt.cc
The added "logger" reports values to a coming password manager internals page.

Tom,
Please review the added IPC message in
components/autofill/content/common/autofill_messages.h

Thanks all!
Vaclav

[Tom Sepez, 2014-04-24 17:20:09]

lgtm

https://codereview.chromium.org/231283003/diff/220001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/231283003/diff/220001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:116: // Notification that
decisions about saving the password should start (|active|
nit: Notification to start (...) or stop (...) logging the decisions made about
saving the password.

[Peter Kasting, 2014-04-24 20:51:14]

LGTM

https://codereview.chromium.org/231283003/diff/220001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/231283003/diff/220001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:36: using
autofill::SavePasswordProgressLogger;
Nit: Prefer to avoid using directives unless they make the code noticeably more
readable (this really goes for the existing statements here too, but whatever)

[Ilya Sherman, 2014-04-25 00:48:45]

(Still LGTM)

https://codereview.chromium.org/231283003/diff/220001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/231283003/diff/220001/components/password_man...
components/password_manager/core/browser/password_manager.cc:264: default:
I'd prefer if you directly listed NO_MATCHING_FORM and MAX_FAILURE_VALUE as
explicit cases.  That way, if any new provisional save failure reasons are added
in the future, the compiler will remind the code author to update this code. 
(For MAX_FAILURE_VALUE, I would have a NOTREACHED().)

[vabr (Chromium), 2014-04-25 09:38:47]

Thanks all for helpful comments.
All comments addressed.

Once I land https://codereview.chromium.org/254573005/, then I will send this to
the CQ.

(Never mind the red trybots at the latest patch set, that's because this CL
depends on the one cited above.)

Cheers,
Vaclav

https://codereview.chromium.org/231283003/diff/220001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/231283003/diff/220001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:36: using
autofill::SavePasswordProgressLogger;
On 2014/04/24 20:51:15, Peter Kasting wrote:
> Nit: Prefer to avoid using directives unless they make the code noticeably
more
> readable (this really goes for the existing statements here too, but whatever)

Fair enough, I removed the added "using" directive, as prefixing autofill:: to
SavePasswordProgressLogger below did not cause considerable changes.

https://codereview.chromium.org/231283003/diff/220001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/231283003/diff/220001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:116: // Notification that
decisions about saving the password should start (|active|
On 2014/04/24 17:20:10, Tom Sepez wrote:
> nit: Notification to start (...) or stop (...) logging the decisions made
about
> saving the password.

Done.

https://codereview.chromium.org/231283003/diff/220001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/231283003/diff/220001/components/password_man...
components/password_manager/core/browser/password_manager.cc:264: default:
On 2014/04/25 00:48:45, Ilya Sherman wrote:
> I'd prefer if you directly listed NO_MATCHING_FORM and MAX_FAILURE_VALUE as
> explicit cases.  That way, if any new provisional save failure reasons are
added
> in the future, the compiler will remind the code author to update this code. 
> (For MAX_FAILURE_VALUE, I would have a NOTREACHED().)

Good idea.
Reminded me that I actually also need to log a message for NO_MATCHING_FORM,
thanks! :)
Done.

[vabr (Chromium), 2014-04-26 20:02:56]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-04-26 20:03:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/231283003/260001

[commit-bot: I haz the power, 2014-04-28 08:04:39]

Change committed as 266481