Fill passwords if password manager is disabled.

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

[Timo Reimann, 2012-11-07 02:56:37]

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.

And please bear with me -- this is my first CL. :)


Regards,

--Timo

[Ilya Sherman, 2012-11-08 06:11:26]

Thanks, generally looks good -- just a couple of small nits below :)

Can you confirm that you've signed the Individual Contributor License Agreement,
as described at [ http://www.chromium.org/developers/contributing-code ]?

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
File chrome/browser/password_manager/password_manager.h (right):

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
chrome/browser/password_manager/password_manager.h:119: // want to save
passwords but continue to fill passwords).
nit: I think this would be slightly clearer if the second clause were "but will
continue to fill passwords" (i.e. adding an extra "will")

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
File chrome/browser/password_manager/password_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
chrome/browser/password_manager/password_manager_unittest.cc:357:
EXPECT_CALL(*store_, GetLogins(_, _));
nit: It looks like the other tests also include "EXPECT_CALL(delegate_,
FillPasswordForm(_));" -- perhaps include that here as well?

[Timo Reimann, 2012-11-08 13:36:06]

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
File chrome/browser/password_manager/password_manager.h (right):

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
chrome/browser/password_manager/password_manager.h:119: // want to save
passwords but continue to fill passwords).
On 2012/11/08 06:11:26, Ilya Sherman wrote:
> nit: I think this would be slightly clearer if the second clause were "but
will
> continue to fill passwords" (i.e. adding an extra "will")

Done.

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
File chrome/browser/password_manager/password_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/11361142/diff/4002/chrome/browser/pass...
chrome/browser/password_manager/password_manager_unittest.cc:357:
EXPECT_CALL(*store_, GetLogins(_, _));
On 2012/11/08 06:11:26, Ilya Sherman wrote:
> nit: It looks like the other tests also include "EXPECT_CALL(delegate_,
> FillPasswordForm(_));" -- perhaps include that here as well?

Agreed -- it should make the unit tests look more consistent.

I'll need to extend the unit test a bit then because the expected outcome of
FillPasswordForm() depends on the password store's content. Should be fairly
simple though.

[Timo Reimann, 2012-11-08 13:44:30]

Updated patch set is here for re-reviewal.

I also added short test descriptions to the new unit tests because the other
tests had descriptions as well and I felt they could help with understanding the
test purposes more easily.

[Timo Reimann, 2012-11-08 13:45:12]

I also confirm that I have successfully signed the Individual Contributor
License Agreement.

[Ilya Sherman, 2012-11-09 08:05:13]

LGTM -- thanks for contributing this patch!

[commit-bot: I haz the power, 2012-11-09 08:05:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttr314@googlemail.com/11361142/2002

[commit-bot: I haz the power, 2012-11-09 09:57:24]

Change committed as 166878