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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by ttr314
Modified:
1 year, 5 months ago
Reviewers:
Ilya Sherman
CC:
chromium-reviews_chromium.org
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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -16 lines) Lint Patch
M AUTHORS View 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/browser/password_manager/password_manager.h View 1 2 2 chunks +2 lines, -5 lines 0 comments ? errors Download
M chrome/browser/password_manager/password_manager.cc View 3 chunks +2 lines, -8 lines 0 comments 0 errors Download
M chrome/browser/password_manager/password_manager_unittest.cc View 1 2 4 chunks +38 lines, -3 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 8
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. ...
1 year, 5 months ago #1
Ilya Sherman
Thanks, generally looks good -- just a couple of small nits below :) Can you ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #3
ttr314
Updated patch set is here for re-reviewal. I also added short test descriptions to the ...
1 year, 5 months ago #4
ttr314
I also confirm that I have successfully signed the Individual Contributor License Agreement.
1 year, 5 months ago #5
Ilya Sherman
LGTM -- thanks for contributing this patch!
1 year, 5 months ago #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
1 year, 5 months ago #7
I haz the power (commit-bot)
1 year, 5 months ago #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 1280:2d3e6564b7b6