Chromium Code Reviews
Help | Chromium Project | Sign in
(51)

Issue 11361142: Fill passwords if password manager is disabled. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by ttr314
Modified:
2 years, 5 months ago
Reviewers:
Ilya Sherman
CC:
chromium-reviews
Visibility:
Public.

Description

Fill passwords if password manager is disabled. As of now, filling passwords is directly dependent on the manager being enabled in the settings. However, the password manager is supposed to fill previously stored passwords even when disabled. Changes introduced by the patch are as following: - Remove IsFillingEnabled() and update code relying on method accordingly. - Add test to ensure that password saving depends on manager enabled preference. - Add test to ensure that passwords are filled on a disabled password manager. - Add myself to AUTHORS file. Contributed by Timo Reimann <ttr314@googlemail.com>; R=isherman@chromium.org BUG=158296 Review URL: https://chromiumcodereview.appspot.com/11361142/ Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166878

Patch Set 1 #

Patch Set 2 : Fix lint warnings. #

Total comments: 4

Patch Set 3 : Make unit tests look consistent; add/extend in-code documentation. #

Messages

Total messages: 8 (0 generated)
ttr314
Hi, here's the CL for http://code.google.com/p/chromium/issues/detail?id=158296 to review. Let me know if I missed anything. ...
2 years, 5 months ago (2012-11-07 02:56:37 UTC) #1
Ilya Sherman
Thanks, generally looks good -- just a couple of small nits below :) Can you ...
2 years, 5 months ago (2012-11-08 06:11:26 UTC) #2
ttr314
https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/password_manager/password_manager.h File chrome/browser/password_manager/password_manager.h (right): https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/password_manager/password_manager.h#newcode119 chrome/browser/password_manager/password_manager.h:119: // want to save passwords but continue to fill ...
2 years, 5 months ago (2012-11-08 13:36:06 UTC) #3
ttr314
Updated patch set is here for re-reviewal. I also added short test descriptions to the ...
2 years, 5 months ago (2012-11-08 13:44:30 UTC) #4
ttr314
I also confirm that I have successfully signed the Individual Contributor License Agreement.
2 years, 5 months ago (2012-11-08 13:45:12 UTC) #5
Ilya Sherman
LGTM -- thanks for contributing this patch!
2 years, 5 months ago (2012-11-09 08:05:13 UTC) #6
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/11361142/2002
2 years, 5 months ago (2012-11-09 08:05:31 UTC) #7
I haz the power (commit-bot)
2 years, 5 months ago (2012-11-09 09:57:24 UTC) #8
Change committed as 166878
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 700cc9d