PasswordManager - update "password_manager_enabled_" pref name

PasswordManager - update "password_manager_enabled_" pref name

This patch updates password_manager_enabled_ to saving_passwords_enabled_
to avoid confusion. This setting controls whether to offer to save password
or not.

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

[Nikhil, 2014-07-10 06:29:57]

Clean-up patch, PTAL. Thanks!

[vabr (Chromium), 2014-07-10 08:36:54]

LGTM with 2 comments.
Also please make sure to get the OWNERS approval for the //chrome/browser/*
part.

Thanks for doing this!

Vaclav

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

https://codereview.chromium.org/380713003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.h:183:
BooleanPrefMember saving_password_enabled_;
nit: saving_passwords_enabled_ (plural, as this applies to all passwords, not
just one)

https://codereview.chromium.org/380713003/diff/1/components/password_manager/...
File components/password_manager/core/common/password_manager_pref_names.cc
(right):

https://codereview.chromium.org/380713003/diff/1/components/password_manager/...
components/password_manager/core/common/password_manager_pref_names.cc:29: //
See http://crbug.com/392387
nit: Please also explain the difference in the constant name and value.
Something like:
"The constant value (=setting name) differs from its name for historical
reasons; making the setting name match the constant name was not deemed
important enough to justify adding migration code for that."

[Nikhil, 2014-07-10 09:56:36]

Made the changes as per review comments. PTAL. Thanks!

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

https://codereview.chromium.org/380713003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.h:183:
BooleanPrefMember saving_password_enabled_;
On 2014/07/10 08:36:54, vabr (Chromium) wrote:
> nit: saving_passwords_enabled_ (plural, as this applies to all passwords, not
> just one)

Done.

https://codereview.chromium.org/380713003/diff/1/components/password_manager/...
File components/password_manager/core/common/password_manager_pref_names.cc
(right):

https://codereview.chromium.org/380713003/diff/1/components/password_manager/...
components/password_manager/core/common/password_manager_pref_names.cc:29: //
See http://crbug.com/392387
On 2014/07/10 08:36:54, vabr (Chromium) wrote:
> nit: Please also explain the difference in the constant name and value.
> Something like:
> "The constant value (=setting name) differs from its name for historical
> reasons; making the setting name match the constant name was not deemed
> important enough to justify adding migration code for that."

Done.

[vabr (Chromium), 2014-07-10 10:00:28]

Thanks, LGTM.

Please don't forget the OWNERS approvals for
chrome/browser/extensions/api/preference/preference_api.cc
chrome/browser/extensions/api/preference/preference_apitest.cc
chrome/browser/policy/configuration_policy_handler_list_factory.cc
chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
before landing this.

Cheers,
Vaclav

[Nikhil, 2014-07-10 10:10:25]

Yes, I'm just going through OWNERS file to identify reviewers. Thanks for
reviewing this quickly.

Meanwhile, I received a lot of trybot failure emails. Do I need to do something
for this?

[Nikhil, 2014-07-10 10:23:54]

Adding more reviewers for OWNERS review. PTAL. Thanks!

@kalman - OWNERS review for c/b/extensions/api/preference/* please.

@bartfab - OWNERS review for
c/b/policy/configuration_policy_handler_list_factory.cc please.

@zea - OWNERS review for
c/b/sync/test/integration/two_client_preferences_sync_test.cc please.

[bartfab (slow), 2014-07-10 10:50:31]

c/b/policy/configuration_policy_handler_list_factory.cc LGTM

[vabr (Chromium), 2014-07-10 10:51:56]

On 2014/07/10 10:10:25, Nikhil wrote:
> Yes, I'm just going through OWNERS file to identify reviewers. Thanks for
> reviewing this quickly.
> 
> Meanwhile, I received a lot of trybot failure emails. Do I need to do
something
> for this?

The trybots do not seem to be able to apply your patch (see
http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...).

Are you sure you did not base your branch on something else than Chromium's tip
of the tree?

[Nikhil, 2014-07-10 14:18:16]

Rebase was required because of recent changes. PTAL. Thanks!

[Nikhil, 2014-07-10 14:20:57]

On 2014/07/10 10:51:56, vabr (Chromium) wrote:
> On 2014/07/10 10:10:25, Nikhil wrote:
> > Yes, I'm just going through OWNERS file to identify reviewers. Thanks for
> > reviewing this quickly.
> > 
> > Meanwhile, I received a lot of trybot failure emails. Do I need to do
> something
> > for this?
> 
> The trybots do not seem to be able to apply your patch (see
>
http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...).
> 
> Are you sure you did not base your branch on something else than Chromium's
tip
> of the tree?

Rebase was required. I've done so, and uploaded new patch. Let's see if trybots
are happy now. Thanks!

[not at google - send to devlin, 2014-07-10 15:24:01]

extensions lgtm

[Nikhil, 2014-07-10 15:25:56]

c/b/sync/test/integration/two_client_preferences_sync_test.cc isn't part of the
patch anymore after the rebase. So I'm going to try to submit this now.

[Nikhil, 2014-07-10 15:27:06]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-07-10 15:27:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/380713003/60001

[commit-bot: I haz the power, 2014-07-10 17:21:22]

Change committed as 282367