Fixed behavior of the bookmark bar visibility.

Fixed behavior of the bookmark bar visibility.

- Removed the kEnableBookmarkBar preference. This was only used from the policy
  code, and duplicated what the kShowBookmarkBar preference already does.
  Removed custom policy code that handle this.
- Fixed the pref UI for the kShowBookmarkBar preference, which wasn't working
  when set to true by the associated policy. Removed custom UI code for this.
- Fire NOTIFICATION_BOOKMARK_BAR_VISIBILITY_CHANGED from a single place, and
  every time that the preference changes.

- Made ChromeWebUI::force_bookmark_bar_visible a virtual method instead, so that
  subclasses can recompute the state dynamically. The previous implementation
  wouldn't react to pref changes on existing NTPs.

Current behavior of the bookmark bar:
- If kShowBookmarkBar is true, it is shown as part of the tab;
- If kShowBookmarkBar is false, it is shown inline IF the policy is not set, AND
  - using the old NTP, OR
  - using the new NTP without bookmark features.

The policy forces the bar to be always visible in all tabs, or to never appear
at all.

BUG=98918
TEST=The BookmarkBarEnabled policy does what it's supposed to do in the old NTP, new NTP, and new NTP with bookmarks. The show-bookmark-bar shortcut also does the right thing in all versions.


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

[Joao da Silva, 2011-10-04 14:29:23]

Please review, thanks!

@sky: main review; mostly bookmarks/ and ui/ changes
@jhawkins: options UI changes
@pastarmovj: policy changes
@mirandac: importer/ changes
@mnissler: prefs/ changes

[pastarmovj, 2011-10-04 14:40:21]

Policy part: LGTM.

[Mattias Nissler (ping if slow), 2011-10-04 14:49:34]

LGTM, thanks for cleaning this up!

[Miranda Callahan, 2011-10-04 15:17:30]

On 2011/10/04 14:49:34, Mattias Nissler wrote:
> LGTM, thanks for cleaning this up!

importer LGTM!

[James Hawkins, 2011-10-04 20:50:33]

WebUI lgtm

[Evan Stade, 2011-10-04 21:10:38]

lgtm

http://codereview.chromium.org/8135001/diff/1/chrome/browser/ui/webui/ntp/new...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/8135001/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:295:
!prefs->IsManagedPreference(prefs::kShowBookmarkBar) &&
only 4 spaces

http://codereview.chromium.org/8135001/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:316: if (IsBookmarkBarVisible()) {
don't add curlies

[Joao da Silva, 2011-10-05 14:31:25]

Thanks for reviewing! Waiting for @sky's review.

http://codereview.chromium.org/8135001/diff/1/chrome/browser/ui/webui/ntp/new...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/8135001/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:295:
!prefs->IsManagedPreference(prefs::kShowBookmarkBar) &&
On 2011/10/04 21:10:38, Evan Stade wrote:
> only 4 spaces

Done.

http://codereview.chromium.org/8135001/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:316: if (IsBookmarkBarVisible()) {
On 2011/10/04 21:10:38, Evan Stade wrote:
> don't add curlies

Done.

[sky, 2011-10-05 16:59:11]

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:798: case
chrome::NOTIFICATION_PREF_CHANGED: {
In theory there need not be a bookmarkmodel created when this pref changes.

It seems weird to listen for a pref change then turn around and fire another
notification. Can the observers listen directly for kShowBookmarkBar and we get
rid of NOTIFICATION_BOOKMARK_BAR_VISIBILITY_PREF_CHANGED?

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui.h (right):

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui.h:21: // Returns true if the bookmark bar
should be visible, inlined in the webui.
What does 'inlined in webui' mean here? Is that part of the sentence needed?

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:298: !NTP4BookmarkFeaturesEnabled();
I don't think this should check kShowBookmarkBar. One case that triggers
detached bookmarks even if kShowBookmarkBar is true is in fullscreen mode and on
the NTP.

[Joao da Silva, 2011-10-06 19:45:37]

@sky: thanks for the review, see replies inline. Please have another look.

@estade: thanks for the review too. Can you have a look at the new ui/gtk
changes?

@mark: adding to review the ui/cocoa changes. Please review, thanks!

A short summary of the new changes:

NOTIFICATION_BOOKMARK_BAR_VISIBILITY_PREF_CHANGED was doing the same as the pref
change notification for kShowBookmarBar, so it has been removed. This allows
removing a lot of duplicate code. The checks for the incognito profiles aren't
necessary anymore either, because the main and incognito profiles share the
underlying pref store, and the kShowBookmarkBar pref is shared.

Besides the original bug, this cleanup also fixes other glitches found when
switching the preference in fullscreen and using the old NTP or the new NTP with
bookmarks.

The "managed-by-your-admin" banner will also show up when using the associated
policy.

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:798: case
chrome::NOTIFICATION_PREF_CHANGED: {
On 2011/10/05 16:59:12, sky wrote:
> In theory there need not be a bookmarkmodel created when this pref changes.
> 
> It seems weird to listen for a pref change then turn around and fire another
> notification. Can the observers listen directly for kShowBookmarkBar and we
get
> rid of NOTIFICATION_BOOKMARK_BAR_VISIBILITY_PREF_CHANGED?

I thought of that but felt lazy :-) It's been done now.

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chrome_web_ui.h (right):

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chrome_web_ui.h:21: // Returns true if the bookmark bar
should be visible, inlined in the webui.
On 2011/10/05 16:59:12, sky wrote:
> What does 'inlined in webui' mean here? Is that part of the sentence needed?

It refers to the detached state. Made it clearer in the comment.

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/8135001/diff/6002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:298: !NTP4BookmarkFeaturesEnabled();
On 2011/10/05 16:59:12, sky wrote:
> I don't think this should check kShowBookmarkBar. One case that triggers
> detached bookmarks even if kShowBookmarkBar is true is in fullscreen mode and
on
> the NTP.

Yikes, totally forgot to check behavior with fullscreen. It's now working as
expected; in this place it just checks if it is disabled by a policy.

[Mark Mentovai, 2011-10-06 20:04:07]

If Miranda’s already familiar with this change, she’s competent to review the
bits in chrome/browser/ui/cocoa.

[Miranda Callahan, 2011-10-06 21:10:21]

importer and browser/ui/cocoa stuff LGTM with DCHECK change.

http://codereview.chromium.org/8135001/diff/15004/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/8135001/diff/15004/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:562: if (pref_name ==
prefs::kShowBookmarkBar)
Maybe change this if statement to just a DCHECK(pref_name ==
prefs::kShowBookmarkBar) before updating the bookmark bar visibility? This seems
like a DCHECK kind of situation, not needed for release code.

[sky, 2011-10-06 22:27:18]

LGTM
Thanks!

[Joao da Silva, 2011-10-07 09:12:31]

Addressed Miranda's comment. Thanks all for reviewing!

Waiting for an owner approval to the changes in ui/gtk before landing.

@estade: can you stamp those changes? Thanks! :-)

http://codereview.chromium.org/8135001/diff/15004/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/8135001/diff/15004/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:562: if (pref_name ==
prefs::kShowBookmarkBar)
On 2011/10/06 21:10:21, Miranda Callahan wrote:
> Maybe change this if statement to just a DCHECK(pref_name ==
> prefs::kShowBookmarkBar) before updating the bookmark bar visibility? This
seems
> like a DCHECK kind of situation, not needed for release code.

Done.

[Evan Stade, 2011-10-10 21:33:15]

http://codereview.chromium.org/8135001/diff/27001/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/8135001/diff/27001/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.cc:314: if (type ==
chrome::NOTIFICATION_PREF_CHANGED) {
I prefer DCHECKs to if/NOTREACHED. Easier to read. than tracing through all the
if/else clauses.

[Joao da Silva, 2011-10-10 21:44:27]

http://codereview.chromium.org/8135001/diff/27001/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/8135001/diff/27001/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.cc:314: if (type ==
chrome::NOTIFICATION_PREF_CHANGED) {
On 2011/10/10 21:33:15, Evan Stade wrote:
> I prefer DCHECKs to if/NOTREACHED. Easier to read. than tracing through all
the
> if/else clauses.

Done.

[Evan Stade, 2011-10-10 21:47:27]

lgtm with dcheck_eq change

http://codereview.chromium.org/8135001/diff/30002/chrome/browser/ui/gtk/globa...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/8135001/diff/30002/chrome/browser/ui/gtk/globa...
chrome/browser/ui/gtk/global_menu_bar.cc:314: DCHECK(type ==
chrome::NOTIFICATION_PREF_CHANGED);
DCHECK_EQ

[commit-bot: I haz the power, 2011-10-10 22:16:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8135001/30003

[commit-bot: I haz the power, 2011-10-11 00:35:02]

Change committed as 104827