Improve logging in password manager internals page

Improve logging in password manager internals page

This CL changes the return value of
PasswordManagerClient::PromptUserToSavePassword to bool, indicating if
the prompt was indeed displayed, and logs that appropriately through |logger|

BUG=393138
Committed: https://crrev.com/535e290f8d25b2fb9a8eea0ca1b53d1d6c76f2a6
Cr-Commit-Position: refs/heads/master@{#296985}

[Sunil Ratnu, 2014-09-26 14:19:54]

sunil.ratnu@samsung.com changed reviewers:
+ vabr@chromium.org

[Sunil Ratnu, 2014-09-26 14:19:54]

Hi Vaclav,

Please review this.

Thanks!
Sunil

[vabr (Chromium), 2014-09-26 15:39:46]

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

[vabr (Chromium), 2014-09-26 15:39:47]

Thanks, Sunil.
LGTM with comments.

Ilya,
Could you please review/rubberstamp
components/autofill/core/common/save_password_progress_logger.* for Sunil?

Thanks,
Vaclav

https://codereview.chromium.org/604293002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/604293002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:166: return
true;
Just keep one "return true;" after the whole if-else block.

If you kept this "return true;", then remove the "else" below (see
http://dev.chromium.org/developers/coding-style#Code_Formatting), but I would
prefer the single "return true;" at the end.

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

https://codereview.chromium.org/604293002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager_unittest.cc:70:
virtual bool PromptUserToSavePassword(
Actually, PromptUserToSavePassword is already defined in
StubPasswordManagerClient, so please delete the identical re-definition here.
(I'm not sure why it has been repeated here before, but I'm probably responsible
for it. :))

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

https://codereview.chromium.org/604293002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:54: // that
this form doesn't need to be saved.
Please comment on the meaning of the return value.

[Sunil Ratnu, 2014-09-26 16:01:20]

Hi Vaclav,

Thanks for the review. I've made the suggested changes.

Thanks!
Sunil

https://codereview.chromium.org/604293002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/604293002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:166: return
true;
On 2014/09/26 15:39:47, vabr (Chromium) wrote:
> Just keep one "return true;" after the whole if-else block.
> 
> If you kept this "return true;", then remove the "else" below (see
> http://dev.chromium.org/developers/coding-style#Code_Formatting), but I would
> prefer the single "return true;" at the end.

Done.

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

https://codereview.chromium.org/604293002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager_unittest.cc:70:
virtual bool PromptUserToSavePassword(
On 2014/09/26 15:39:47, vabr (Chromium) wrote:
> Actually, PromptUserToSavePassword is already defined in
> StubPasswordManagerClient, so please delete the identical re-definition here.
> (I'm not sure why it has been repeated here before, but I'm probably
responsible
> for it. :))

Done.

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

https://codereview.chromium.org/604293002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:54: // that
this form doesn't need to be saved.
On 2014/09/26 15:39:47, vabr (Chromium) wrote:
> Please comment on the meaning of the return value.

Done.

[vabr (Chromium), 2014-09-26 16:25:15]

LGTM, Thanks!

Over to Ilya for components/autofill/core/common/save_password_progress_logger.*

Cheers,
Vaclav

[Ilya Sherman, 2014-09-26 17:30:45]

//components/autofill lgtm

[Ilya Sherman, 2014-09-26 17:31:00]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-26 17:31:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604293002/40001

[commit-bot: I haz the power, 2014-09-26 18:33:28]

Committed patchset #3 (id:40001) as 190fd8f7b979a7a70db950509ddb05f74b5fa190

[commit-bot: I haz the power, 2014-09-26 18:34:14]

Patchset 3 (id:??) landed as
https://crrev.com/535e290f8d25b2fb9a8eea0ca1b53d1d6c76f2a6
Cr-Commit-Position: refs/heads/master@{#296985}