Implement policy for disabling auto fill.

Implement policy for disabling auto fill.

This introduces a new policy setting named AutoFillEnabled that administrators
can use to switch off the auto fill feature. The UI adjustments disable the
autofill button and dialog in the preferences dialog appropriate.

Preferences.xib change: Bind enabled state of the auto fill button to
autoFillSettingsButtonEnabled @property.
AutoFillDialog.xib change: Bind enabled state of the auto fill enabled checkbox and save button
to the autoFillEnabledAvailable and saveEnabled @properties, respectively.

BUG=49611
TEST=autofill_dialog_controller_mac_unittest.mm, manual testing

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57302

[Mattias Nissler (ping if slow), 2010-08-23 15:50:38]

Hi folks, please review!

I thought finnur could do the general review and dhollowa and georgey the mac
and windows specific changes to the autofill dialog.

Thanks.

[dhollowa, 2010-08-23 16:26:07]

http://codereview.chromium.org/3140024/diff/1/4
File chrome/app/policy/mac/app-Manifest.plist (right):

http://codereview.chromium.org/3140024/diff/1/4#newcode182
chrome/app/policy/mac/app-Manifest.plist:182: <string>This policy controls the
auto fill feature. If you disable this setting, auto fill will not be available
to the user. If you enable it, auto fill will be always enabled, but the user
still needs to fill in their auto fill profile.</string>
s/auto fill/AutoFill/

Wondering if first-person "you..." should be replaced with more passive voice. 
I.e. "When this policy control is disabled, the AutoFill feature becomes
unavailable to the user.  Conversely, when enabled, the AutoFill feature becomes
available."

http://codereview.chromium.org/3140024/diff/1/5
File chrome/app/policy/windows/adm/en-US/chrome.adm (right):

http://codereview.chromium.org/3140024/diff/1/5#newcode271
chrome/app/policy/windows/adm/en-US/chrome.adm:271: AutoFillEnabled="Enable auto
fill"
s/auto fill/AutoFill (and below).

http://codereview.chromium.org/3140024/diff/1/5#newcode274
chrome/app/policy/windows/adm/en-US/chrome.adm:274: If you disable this setting,
the auto fill feature will not be available to users.\n\n\
Again, wondering about the first person "you...".

http://codereview.chromium.org/3140024/diff/1/7
File chrome/app/policy/windows/admx/en-US/chrome.adml (right):

http://codereview.chromium.org/3140024/diff/1/7#newcode103
chrome/app/policy/windows/admx/en-US/chrome.adml:103: <string
id="AutoFillEnabled">Enable auto fill</string>
s/auto fill/AutoFill/

http://codereview.chromium.org/3140024/diff/1/8
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3140024/diff/1/8#newcode67
chrome/browser/autofill/autofill_dialog_controller_mac.h:67: // Tracks the
autofill enabled preference.
s/autofill/AutoFill/

http://codereview.chromium.org/3140024/diff/1/8#newcode73
chrome/browser/autofill/autofill_dialog_controller_mac.h:73: // Whether the
autofill enabled checkbox is enabled for editing by the user.
s/autofill/AutoFill/

http://codereview.chromium.org/3140024/diff/1/8#newcode102
chrome/browser/autofill/autofill_dialog_controller_mac.h:102: // Watches for
changes to the autofill enabled preference.
s/autofill/AutoFill/

http://codereview.chromium.org/3140024/diff/1/9
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3140024/diff/1/9#newcode90
chrome/browser/autofill/autofill_dialog_controller_mac.mm:90: // Called upon
changes to sync preferences that should be reflected in the UI.
s/sync/AutoFill/

http://codereview.chromium.org/3140024/diff/1/9#newcode790
chrome/browser/autofill/autofill_dialog_controller_mac.mm:790: if (prefName ==
prefs::kAutoFillEnabled) {
Add DCHECK(prefName == prefs::kAutoFillEnabled)

http://codereview.chromium.org/3140024/diff/1/10
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/3140024/diff/1/10#newcode776
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:776: //
Disable autofill through configuration policy.
s/autofill/AutoFill/

http://codereview.chromium.org/3140024/diff/1/10#newcode788
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:788:
[controller_ cancel:nil];
Please add test to confirm that a call to [controller_ save:nil]; will save data
if policy is true, and won't save data if policy is false.

http://codereview.chromium.org/3140024/diff/1/15
File chrome/browser/gtk/options/content_page_gtk.h (right):

http://codereview.chromium.org/3140024/diff/1/15#newcode72
chrome/browser/gtk/options/content_page_gtk.h:72: // Widgets for the Autofill
group.
s/Autofill/AutoFill/

http://codereview.chromium.org/3140024/diff/1/21
File chrome/browser/views/autofill_profiles_view_win.h (right):

http://codereview.chromium.org/3140024/diff/1/21#newcode151
chrome/browser/views/autofill_profiles_view_win.h:151: 
nit: one newline not two.

[Mattias Nissler (ping if slow), 2010-08-23 20:06:44]

Thanks for the feedback! I've just uploaded an updated version with fixes,
please take a look.

http://codereview.chromium.org/3140024/diff/1/4
File chrome/app/policy/mac/app-Manifest.plist (right):

http://codereview.chromium.org/3140024/diff/1/4#newcode182
chrome/app/policy/mac/app-Manifest.plist:182: <string>This policy controls the
auto fill feature. If you disable this setting, auto fill will not be available
to the user. If you enable it, auto fill will be always enabled, but the user
still needs to fill in their auto fill profile.</string>
On 2010/08/23 16:26:08, dhollowa wrote:
> s/auto fill/AutoFill/
> 
> Wondering if first-person "you..." should be replaced with more passive voice.

> I.e. "When this policy control is disabled, the AutoFill feature becomes
> unavailable to the user.  Conversely, when enabled, the AutoFill feature
becomes
> available."

I was matching the windows ADM style here, where it is common to address "the
administrator" in first person, I've change the description to a more passive
style that matches the rest of the existing descriptions better. Note that the
policy definitions in this file as well as the windows templates are soon to be
replaced by generated version from a master policy definitions file, so they
won't stay for too long.
Done.

http://codereview.chromium.org/3140024/diff/1/5
File chrome/app/policy/windows/adm/en-US/chrome.adm (right):

http://codereview.chromium.org/3140024/diff/1/5#newcode271
chrome/app/policy/windows/adm/en-US/chrome.adm:271: AutoFillEnabled="Enable auto
fill"
On 2010/08/23 16:26:08, dhollowa wrote:
> s/auto fill/AutoFill (and below).

Done.

http://codereview.chromium.org/3140024/diff/1/5#newcode274
chrome/app/policy/windows/adm/en-US/chrome.adm:274: If you disable this setting,
the auto fill feature will not be available to users.\n\n\
On 2010/08/23 16:26:08, dhollowa wrote:
> Again, wondering about the first person "you...".

This is actually common practice for windows policy templates and matches the
other policies, so I'd like to keep it.

http://codereview.chromium.org/3140024/diff/1/7
File chrome/app/policy/windows/admx/en-US/chrome.adml (right):

http://codereview.chromium.org/3140024/diff/1/7#newcode103
chrome/app/policy/windows/admx/en-US/chrome.adml:103: <string
id="AutoFillEnabled">Enable auto fill</string>
On 2010/08/23 16:26:08, dhollowa wrote:
> s/auto fill/AutoFill/

Done. The .adml and .admx files are actually generated from the .adm.

http://codereview.chromium.org/3140024/diff/1/8
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3140024/diff/1/8#newcode67
chrome/browser/autofill/autofill_dialog_controller_mac.h:67: // Tracks the
autofill enabled preference.
On 2010/08/23 16:26:08, dhollowa wrote:
> s/autofill/AutoFill/

Done.

http://codereview.chromium.org/3140024/diff/1/8#newcode73
chrome/browser/autofill/autofill_dialog_controller_mac.h:73: // Whether the
autofill enabled checkbox is enabled for editing by the user.
On 2010/08/23 16:26:08, dhollowa wrote:
> s/autofill/AutoFill/

Done.

http://codereview.chromium.org/3140024/diff/1/8#newcode102
chrome/browser/autofill/autofill_dialog_controller_mac.h:102: // Watches for
changes to the autofill enabled preference.
On 2010/08/23 16:26:08, dhollowa wrote:
> s/autofill/AutoFill/

Done.

http://codereview.chromium.org/3140024/diff/1/9
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3140024/diff/1/9#newcode90
chrome/browser/autofill/autofill_dialog_controller_mac.mm:90: // Called upon
changes to sync preferences that should be reflected in the UI.
On 2010/08/23 16:26:08, dhollowa wrote:
> s/sync/AutoFill/

Done.

http://codereview.chromium.org/3140024/diff/1/9#newcode790
chrome/browser/autofill/autofill_dialog_controller_mac.mm:790: if (prefName ==
prefs::kAutoFillEnabled) {
On 2010/08/23 16:26:08, dhollowa wrote:
> Add DCHECK(prefName == prefs::kAutoFillEnabled)

What's the rationale? That DCHECK would have to be removed anyway if we add more
prefs to observe. If you'd like to make sure notifications get only sent for
prefs that we have notifiers registered for, I'd rather add an else {
NOTREACHED(); }

http://codereview.chromium.org/3140024/diff/1/10
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/3140024/diff/1/10#newcode776
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:776: //
Disable autofill through configuration policy.
On 2010/08/23 16:26:08, dhollowa wrote:
> s/autofill/AutoFill/

Done.

http://codereview.chromium.org/3140024/diff/1/10#newcode788
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:788:
[controller_ cancel:nil];
On 2010/08/23 16:26:08, dhollowa wrote:
> Please add test to confirm that a call to [controller_ save:nil]; will save
data
> if policy is true, and won't save data if policy is false.

Done.

http://codereview.chromium.org/3140024/diff/1/15
File chrome/browser/gtk/options/content_page_gtk.h (right):

http://codereview.chromium.org/3140024/diff/1/15#newcode72
chrome/browser/gtk/options/content_page_gtk.h:72: // Widgets for the Autofill
group.
On 2010/08/23 16:26:08, dhollowa wrote:
> s/Autofill/AutoFill/

Done.

http://codereview.chromium.org/3140024/diff/1/21
File chrome/browser/views/autofill_profiles_view_win.h (right):

http://codereview.chromium.org/3140024/diff/1/21#newcode151
chrome/browser/views/autofill_profiles_view_win.h:151: 
On 2010/08/23 16:26:08, dhollowa wrote:
> nit: one newline not two.

Done.

[dhollowa, 2010-08-23 22:29:41]

http://codereview.chromium.org/3140024/diff/1/9
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3140024/diff/1/9#newcode790
chrome/browser/autofill/autofill_dialog_controller_mac.mm:790: if (prefName ==
prefs::kAutoFillEnabled) {
On 2010/08/23 20:06:45, mnissler wrote:
> On 2010/08/23 16:26:08, dhollowa wrote:
> > Add DCHECK(prefName == prefs::kAutoFillEnabled)
> 
> What's the rationale? That DCHECK would have to be removed anyway if we add
more
> prefs to observe. If you'd like to make sure notifications get only sent for
> prefs that we have notifiers registered for, I'd rather add an else {
> NOTREACHED(); }

Yes, else { NOTREACHED(); } is better.

http://codereview.chromium.org/3140024/diff/12001/13007
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3140024/diff/12001/13007#newcode88
chrome/browser/autofill/autofill_dialog_controller_mac.h:88: BOOL saveEnabled_;
In general the logic looks good.  Regarding variable naming I'd prefer to move
to something more in the MVC spirit.  These BOOLs are, what I think of as
quasi-model objects.  Because of this I try to stay away from "view" terminology
like "enabled" as it applies to the state of the view.

I'd suggest:
 autoFillEnabledAvailable_ -> autoFillIsManaged_
 saveEnabled_ -> autoFillIsManagedAndDisabled_

And then change the binding of the save button use the NSNegateBoolean modifier
in IB.  Similarly for the enabled state of the checkbox bound to
autoFillIsManaged_.

http://codereview.chromium.org/3140024/diff/12001/13008
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3140024/diff/12001/13008#newcode282
chrome/browser/autofill/autofill_dialog_controller_mac.mm:282: bool
disabledByPolicy =
Maybe |autoFillIsManagedAndDisabled| for variable name here too for consistency.

http://codereview.chromium.org/3140024/diff/12001/13008#newcode798
chrome/browser/autofill/autofill_dialog_controller_mac.mm:798: bool
disabledByPolicy =
Could remove temporary variable here.  And then:
[setAutoFillIsManagedAndDisabled:prefAutoFillEnabled_.IsManaged() &&
!prefAutoFillEnabled_.GetValue()];

[Mattias Nissler (ping if slow), 2010-08-24 09:34:37]

David, here's an updated version.

http://codereview.chromium.org/3140024/diff/12001/13007
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3140024/diff/12001/13007#newcode88
chrome/browser/autofill/autofill_dialog_controller_mac.h:88: BOOL saveEnabled_;
On 2010/08/23 22:29:41, dhollowa wrote:
> In general the logic looks good.  Regarding variable naming I'd prefer to move
> to something more in the MVC spirit.  These BOOLs are, what I think of as
> quasi-model objects.  Because of this I try to stay away from "view"
terminology
> like "enabled" as it applies to the state of the view.
> 
> I'd suggest:
>  autoFillEnabledAvailable_ -> autoFillIsManaged_
>  saveEnabled_ -> autoFillIsManagedAndDisabled_
> 
> And then change the binding of the save button use the NSNegateBoolean
modifier
> in IB.  Similarly for the enabled state of the checkbox bound to
> autoFillIsManaged_.
> 

Good points. Done.

http://codereview.chromium.org/3140024/diff/12001/13008
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3140024/diff/12001/13008#newcode282
chrome/browser/autofill/autofill_dialog_controller_mac.mm:282: bool
disabledByPolicy =
On 2010/08/23 22:29:41, dhollowa wrote:
> Maybe |autoFillIsManagedAndDisabled| for variable name here too for
consistency.

I switch the check to use the autoFillManagedAndDisabled_ member, it has the
right value.

http://codereview.chromium.org/3140024/diff/12001/13008#newcode798
chrome/browser/autofill/autofill_dialog_controller_mac.mm:798: bool
disabledByPolicy =
On 2010/08/23 22:29:41, dhollowa wrote:
> Could remove temporary variable here.  And then:
> [setAutoFillIsManagedAndDisabled:prefAutoFillEnabled_.IsManaged() &&
> !prefAutoFillEnabled_.GetValue()];

Done.

[Finnur, 2010-08-24 10:27:02]

Didn't look at the Mac parts, but overall looks good. Three comments.

http://codereview.chromium.org/3140024/diff/22001/23004
File chrome/app/policy/windows/adm/en-US/chrome.adm (right):

http://codereview.chromium.org/3140024/diff/22001/23004#newcode273
chrome/app/policy/windows/adm/en-US/chrome.adm:273: If you enable this setting,
the AutoFill feature will always be enabled. Users still need to fill in their
AutoFill profile.\n\n\
This seems a little vague to me. Should we say:
By enabling this setting, the AutoFill feature will be active and users need to
fill in their AutoFill profile.

http://codereview.chromium.org/3140024/diff/22001/23019
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/3140024/diff/22001/23019#newcode197
chrome/browser/views/autofill_profiles_view_win.cc:197:
DCHECK(enable_auto_fill_button_);
This is not the right usage of the DCHECK macro.

These DCHECKs are all redundant and only add cruft to the debug builds. If you
are going to dereference the pointer immediately after the DCHECK you might as
well skip the DCHECK since it doesn't tell you anything new than crashing at
run-time will in a debug build.

It would be different if you assign a member variable in one function (say ctor)
and use it in another. Then it would make sense to add the DCHECK at the point
of assignment, so that you catch the caller setting it to NULL. Here, however,
you don't need this.

http://codereview.chromium.org/3140024/diff/22001/23019#newcode204
chrome/browser/views/autofill_profiles_view_win.cc:204:
enable_auto_fill_button_->SetChecked(autofill_enabled);
Why not just combine these two lines into one and skip the autofill_enabled
variable?

http://codereview.chromium.org/3140024/diff/22001/23019#newcode421
chrome/browser/views/autofill_profiles_view_win.cc:421: void 
AutoFillProfilesView::Observe(NotificationType type,
nit: Extra space after void (and don't forget to realign the next two lines) :)

[Mattias Nissler (ping if slow), 2010-08-24 12:34:58]

Finnur, here's an updated version with your comments addressed.

http://codereview.chromium.org/3140024/diff/22001/23004
File chrome/app/policy/windows/adm/en-US/chrome.adm (right):

http://codereview.chromium.org/3140024/diff/22001/23004#newcode273
chrome/app/policy/windows/adm/en-US/chrome.adm:273: If you enable this setting,
the AutoFill feature will always be enabled. Users still need to fill in their
AutoFill profile.\n\n\
On 2010/08/24 10:27:02, Finnur wrote:
> This seems a little vague to me. Should we say:
> By enabling this setting, the AutoFill feature will be active and users need
to
> fill in their AutoFill profile.

The point is that this flips the "enable" switch forcibly, but autofill won't be
functional until users fill in their profile data. I rephrased to make it
clearer.

http://codereview.chromium.org/3140024/diff/22001/23019
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/3140024/diff/22001/23019#newcode197
chrome/browser/views/autofill_profiles_view_win.cc:197:
DCHECK(enable_auto_fill_button_);
On 2010/08/24 10:27:02, Finnur wrote:
> This is not the right usage of the DCHECK macro.
> 
> These DCHECKs are all redundant and only add cruft to the debug builds. If you
> are going to dereference the pointer immediately after the DCHECK you might as
> well skip the DCHECK since it doesn't tell you anything new than crashing at
> run-time will in a debug build.
> 
> It would be different if you assign a member variable in one function (say
ctor)
> and use it in another. Then it would make sense to add the DCHECK at the point
> of assignment, so that you catch the caller setting it to NULL. Here, however,
> you don't need this.

Agreed. I guess I could have used some more questioning when writing that
instead of just copying the pattern :) I've removed them, along with more
instances of this pattern in the file.

http://codereview.chromium.org/3140024/diff/22001/23019#newcode204
chrome/browser/views/autofill_profiles_view_win.cc:204:
enable_auto_fill_button_->SetChecked(autofill_enabled);
On 2010/08/24 10:27:02, Finnur wrote:
> Why not just combine these two lines into one and skip the autofill_enabled
> variable?

Looking up preference values is a nontrivial operation after the introduction of
all the new pref stores, so I'd rather use a local temp variable. And note that
we access the value 5 times in this function, so readability would suffer as
well.

http://codereview.chromium.org/3140024/diff/22001/23019#newcode421
chrome/browser/views/autofill_profiles_view_win.cc:421: void 
AutoFillProfilesView::Observe(NotificationType type,
On 2010/08/24 10:27:02, Finnur wrote:
> nit: Extra space after void (and don't forget to realign the next two lines)
:)

Done.

[Finnur, 2010-08-24 13:13:03]

LG with those changes, one more nit.

http://codereview.chromium.org/3140024/diff/22001/23004
File chrome/app/policy/windows/adm/en-US/chrome.adm (right):

http://codereview.chromium.org/3140024/diff/22001/23004#newcode273
chrome/app/policy/windows/adm/en-US/chrome.adm:273: If you enable this setting,
the AutoFill feature will always be enabled. Users still need to fill in their
AutoFill profile.\n\n\
It still is a little vague for the admin, I think... Especially what happens if
the users don't fill out the info.

Will something break? Will they be prompted to fill it out on first use of
Chrome? Will it act exactly as if it is disabled until the fill it out?

On 2010/08/24 12:34:59, mnissler wrote:
> On 2010/08/24 10:27:02, Finnur wrote:
> > This seems a little vague to me. Should we say:
> > By enabling this setting, the AutoFill feature will be active and users need
> to
> > fill in their AutoFill profile.
> 
> The point is that this flips the "enable" switch forcibly, but autofill won't
be
> functional until users fill in their profile data. I rephrased to make it
> clearer.
> 
>

http://codereview.chromium.org/3140024/diff/22001/23019
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/3140024/diff/22001/23019#newcode204
chrome/browser/views/autofill_profiles_view_win.cc:204:
enable_auto_fill_button_->SetChecked(autofill_enabled);
Ah, right. I missed that.

[Mattias Nissler (ping if slow), 2010-08-24 13:47:25]

On 2010/08/24 13:13:03, Finnur wrote:
> LG with those changes, one more nit.
> 
> http://codereview.chromium.org/3140024/diff/22001/23004
> File chrome/app/policy/windows/adm/en-US/chrome.adm (right):
> 
> http://codereview.chromium.org/3140024/diff/22001/23004#newcode273
> chrome/app/policy/windows/adm/en-US/chrome.adm:273: If you enable this
setting,
> the AutoFill feature will always be enabled. Users still need to fill in their
> AutoFill profile.\n\n\
> It still is a little vague for the admin, I think... Especially what happens
if
> the users don't fill out the info.
> 
> Will something break? Will they be prompted to fill it out on first use of
> Chrome? Will it act exactly as if it is disabled until the fill it out?

Nothing will break and it will act as if disabled until the profile is filled
in. I don't think checking on first run makes sense, since the policy can also
be flipped after the first run, and we don't want to prompt then. The long term
option would be to somehow configure the profile also through policy. Maybe it's
better to just ignore the policy if it's set to true? Since the user can disable
autofill effectively by not entering profile information, there's no point of
forcing the enabled switch. WDYT?

> 
> On 2010/08/24 12:34:59, mnissler wrote:
> > On 2010/08/24 10:27:02, Finnur wrote:
> > > This seems a little vague to me. Should we say:
> > > By enabling this setting, the AutoFill feature will be active and users
need
> > to
> > > fill in their AutoFill profile.
> > 
> > The point is that this flips the "enable" switch forcibly, but autofill
won't
> be
> > functional until users fill in their profile data. I rephrased to make it
> > clearer.
> > 
> >
> 
> http://codereview.chromium.org/3140024/diff/22001/23019
> File chrome/browser/views/autofill_profiles_view_win.cc (right):
> 
> http://codereview.chromium.org/3140024/diff/22001/23019#newcode204
> chrome/browser/views/autofill_profiles_view_win.cc:204:
> enable_auto_fill_button_->SetChecked(autofill_enabled);
> Ah, right. I missed that.

[Finnur, 2010-08-24 15:17:33]

Don't get me wrong. I'm not advocating we change behavior here, I'm just saying
we need to be clear as to what happens. How about:

"If you enable this setting, users can choose to enable AutoFill by filling out
their AutoFill profile. For others, AutoFill will remain inactive until they
fill out their profile."

Does this sound OK and match the implementation well?

[Mattias Nissler (ping if slow), 2010-08-24 15:27:48]

On 2010/08/24 15:17:33, Finnur wrote:
> Don't get me wrong. I'm not advocating we change behavior here, I'm just
saying
> we need to be clear as to what happens. How about:
> 
> "If you enable this setting, users can choose to enable AutoFill by filling
out
> their AutoFill profile. For others, AutoFill will remain inactive until they
> fill out their profile."
> 
> Does this sound OK and match the implementation well?

Oh, I do understand your point, but it got me thinking ;) What you propose
sounds OK and matches the implementation. But there is really no difference in
behavior to the non-managed case (other than a checkbox in the UI being
disabled). I think changing the implementation is not hard. I'll leave the UI
bits as they are, because we shouldn't make any assumptions on consistency
invariants on preferences values. I'll upload an update shortly.

[Finnur, 2010-08-24 15:38:30]

OK, that makes sense.

[Mattias Nissler (ping if slow), 2010-08-24 16:09:00]

Finnur, here is an update that only sets the preference if the admin actually
set the autofill policy bit to false. The only code change is in
configuration_policy_pref_store.cc

[Finnur, 2010-08-24 16:36:08]

LG, one nit (feel free to ignore).

http://codereview.chromium.org/3140024/diff/46001/5005
File chrome/app/policy/windows/adm/en-US/chrome.adm (right):

http://codereview.chromium.org/3140024/diff/46001/5005#newcode285
chrome/app/policy/windows/adm/en-US/chrome.adm:285: If you enable or do not
configure this setting, users can turn on and configure AutoFill according to
their preference."
nit: "according to their preference" is redundant.

[Mattias Nissler (ping if slow), 2010-08-24 17:32:01]

On 2010/08/24 16:36:08, Finnur wrote:
> LG, one nit (feel free to ignore).
> 
> http://codereview.chromium.org/3140024/diff/46001/5005
> File chrome/app/policy/windows/adm/en-US/chrome.adm (right):
> 
> http://codereview.chromium.org/3140024/diff/46001/5005#newcode285
> chrome/app/policy/windows/adm/en-US/chrome.adm:285: If you enable or do not
> configure this setting, users can turn on and configure AutoFill according to
> their preference."
> nit: "according to their preference" is redundant.

Ok, I'll fix that before submitting.

Now waiting for LGTM from dhollowa for the mac UI bits.

[dhollowa, 2010-08-24 17:58:54]

LGTM.  Thanks!

[Mattias Nissler (ping if slow), 2010-08-24 19:34:19]

Uploaded the final version that I'll commit tomorrow (European time) if nobody
objects.

[James Hawkins, 2010-08-24 21:13:34]

http://codereview.chromium.org/3140024/diff/37002/48021
File chrome/browser/views/autofill_profiles_view_win.cc (left):

http://codereview.chromium.org/3140024/diff/37002/48021#oldcode336
chrome/browser/views/autofill_profiles_view_win.cc:336: DCHECK(observer_);
Why did you take out all of these DCHECKs?

[Mattias Nissler (ping if slow), 2010-08-24 21:29:53]

On 2010/08/24 21:13:34, James Hawkins wrote:
> http://codereview.chromium.org/3140024/diff/37002/48021
> File chrome/browser/views/autofill_profiles_view_win.cc (left):
> 
> http://codereview.chromium.org/3140024/diff/37002/48021#oldcode336
> chrome/browser/views/autofill_profiles_view_win.cc:336: DCHECK(observer_);
> Why did you take out all of these DCHECKs?

See Finnur's comment at
http://codereview.chromium.org/3140024/diff/22001/23019#newcode197 for the
reasoning.

[James Hawkins, 2010-08-24 21:31:03]

LGTM then.