Prevent accelerators from toggling the bookmark bar when disabled by policy.

Prevent accelerators from toggling the bookmark bar when disabled by policy.

BookmarkBarEnabled policy also sets the kShowBookmarkBar pref, so that the pref is disabled in the preferences UI.

BUG=90731
TEST=Set the BookmarkBarEnabled policy. When true, the bookmark bar is always shown on the NTP. When false, it is never shown anywhere. In either case, it can't be modified by the user.


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

[Joao da Silva, 2011-08-04 09:01:24]

Please review, thanks!

@pastarmovj: review overall patch, policy changes

@pkasting: owner review for ui/

@mark: owner review for ui/cocoa

@erg: owner review for ui/gtk

[pastarmovj, 2011-08-04 09:41:04]

LGTM. I like that the UI code has become simple again and all the checks needed
before are removed in favor of some more logic on policy loading.

[Mark Mentovai, 2011-08-04 12:59:00]

LGTM for OWNERS approval for chrome/browser/ui/cocoa. I didn’t review the rest
of this change.

[Elliot Glaysher, 2011-08-04 17:57:33]

gtk lgtm stamp

[Peter Kasting, 2011-08-04 18:44:43]

LGTM

http://codereview.chromium.org/7564011/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7564011/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:373: if
(ApplyBookmarksPolicy(policy, value))
Nit: Shorter would be

if (ApplyXYZPolicy(policy, value) ||
    ApplyABCPolicy(policy, value) ||
    ...
  return;

http://codereview.chromium.org/7564011/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:572: // the bookmarks
bar either is completely disabled or only shows on the NTP.
There's no policy configuration that corresponds to "show the bookmark bar all
the time", like normal users can do?  That seems very odd.

[Joao da Silva, 2011-08-05 09:24:09]

Will commit if the try bots are pleased. Thanks for reviewing!

http://codereview.chromium.org/7564011/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7564011/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:373: if
(ApplyBookmarksPolicy(policy, value))
On 2011/08/04 18:44:43, Peter Kasting wrote:
> Nit: Shorter would be
> 
> if (ApplyXYZPolicy(policy, value) ||
>     ApplyABCPolicy(policy, value) ||
>     ...
>   return;

Done.

http://codereview.chromium.org/7564011/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:572: // the bookmarks
bar either is completely disabled or only shows on the NTP.
On 2011/08/04 18:44:43, Peter Kasting wrote:
> There's no policy configuration that corresponds to "show the bookmark bar all
> the time", like normal users can do?  That seems very odd.

The "always show the bookmarks bar" preference just toggles where the bookmarks
bar is placed (as a toolbar, or in the NTP). I guess there wasn't any demand to
control that with a policy. This policy is meant to make the bar completely go
away, or just stay fixed in the NTP instead.

[commit-bot: I haz the power, 2011-08-05 13:32:23]

Change committed as 95608