Hook up the prefs for the optional home buttons and page/wrench buttons. Move default registration of this pref out of platform code and into shared code to avoid having to do it in at least 3 places. Fix gradient buttons to not draw their borders unless asked.

BUG=13151
TEST=showing/hiding home button and page/wrench buttons should work. Menus for page/wrench should work except for a few straggler items that aren't yet implemented even in the main menubar.
Review URL: http://codereview.chromium.org/155151

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20052 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 222 insertions, 19 deletions

diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 6de60dec..5205114 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -226,9 +226,6 @@
 
   bookmarkMenuBridge_.reset(new BookmarkMenuBridge());
 
-  // Register any Mac-specific preferences.
-  PrefService* prefs = [self defaultProfile]->GetPrefs();
-  prefs->RegisterBooleanPref(prefs::kShowPageOptionsButtons, false);
   [self setUpdateCheckInterval];
 
   // Build up the encoding menu, the order of the items differs based on the
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index f835795..f2fe26e 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1185,6 +1185,17 @@ void Browser::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterIntegerPref(prefs::kCookieBehavior,
                              net::CookiePolicy::ALLOW_ALL_COOKIES);
   prefs->RegisterBooleanPref(prefs::kShowHomeButton, false);
+#if defined(OS_MACOSX)
+  // This really belongs in platform code, but there's no good place to
+  // initialize it between the time when the AppController is created
+  // (where there's no profile) and the time the controller gets another
+  // crack at the start of the main event loop. By that time, BrowserInit
+  // has already created the browser window, and it's too late: we need the
+  // pref to be already initialized. Doing it here also saves us from having
+  // to hard-code pref registration in the several unit tests that use
+  // this preference.
+  prefs->RegisterBooleanPref(prefs::kShowPageOptionsButtons, false);
+#endif
   prefs->RegisterStringPref(prefs::kRecentlySelectedEncoding, L"");
   prefs->RegisterBooleanPref(prefs::kDeleteBrowsingHistory, true);
   prefs->RegisterBooleanPref(prefs::kDeleteDownloadHistory, true);
diff --git a/chrome/browser/cocoa/gradient_button_cell.mm b/chrome/browser/cocoa/gradient_button_cell.mm
index 91cec0d..f8f6173 100644
--- a/chrome/browser/cocoa/gradient_button_cell.mm
+++ b/chrome/browser/cocoa/gradient_button_cell.mm
@@ -58,11 +58,18 @@
                                               endingColor:end] autorelease];
   }
 
+  // Stroke the borders and appropriate fill gradient. If we're borderless,
+  // the only time we want to draw the inner gradient is if we're highlighted.
   [[NSColor colorWithCalibratedWhite:1.0 alpha:0.25] set];
-  [outerPath stroke];
-  [gradient drawInBezierPath:path angle:90.0];
-  [[NSColor colorWithCalibratedWhite:0.0 alpha:0.15] set];
-  [path stroke];
+  if ([self isBordered]) {
+    [outerPath stroke];
+    [gradient drawInBezierPath:path angle:90.0];
+    [[NSColor colorWithCalibratedWhite:0.0 alpha:0.15] set];
+    [path stroke];
+  } else {
+    if (highlighted)
+      [gradient drawInBezierPath:path angle:90.0];
+  }
 
   if (type == kLeftButtonWithShadowType) {
     NSRect borderRect, contentRect;
diff --git a/chrome/browser/cocoa/preferences_window_controller_unittest.mm b/chrome/browser/cocoa/preferences_window_controller_unittest.mm
index 2a8898a..1241100 100644
--- a/chrome/browser/cocoa/preferences_window_controller_unittest.mm
+++ b/chrome/browser/cocoa/preferences_window_controller_unittest.mm
@@ -32,13 +32,10 @@ namespace {
 class PrefsControllerTest : public PlatformTest {
  public:
   PrefsControllerTest() {
-    // Since this is a platform-specific preference, it's not registered by
-    // any of the shared code. We have to register it ourselves.
-    PrefService* prefs = browser_helper_.profile()->GetPrefs();
-    prefs->RegisterBooleanPref(prefs::kShowPageOptionsButtons, false);
     // The metrics reporting pref is registerd on the local state object in
     // real builds, but we don't have one of those for unit tests. Register
     // it on prefs so we'll find it when we go looking.
+    PrefService* prefs = browser_helper_.profile()->GetPrefs();
     prefs->RegisterBooleanPref(prefs::kMetricsReportingEnabled, false);
 
     pref_controller_.reset([[PreferencesWindowController alloc]
diff --git a/chrome/browser/cocoa/toolbar_controller.h b/chrome/browser/cocoa/toolbar_controller.h
index 1aa38bb..236f777 100644
--- a/chrome/browser/cocoa/toolbar_controller.h
+++ b/chrome/browser/cocoa/toolbar_controller.h
@@ -10,10 +10,14 @@
 #include "base/scoped_ptr.h"
 #include "base/scoped_nsobject.h"
 #import "chrome/browser/cocoa/command_observer_bridge.h"
+#include "chrome/common/pref_member.h"
 
 class CommandUpdater;
 class LocationBar;
 class LocationBarViewMac;
+namespace ToolbarControllerInternal {
+class PrefObserverBridge;
+}
 class Profile;
 class TabContents;
 class ToolbarModel;
@@ -40,14 +44,25 @@ class ToolbarView;
   scoped_ptr<LocationBarViewMac> locationBarView_;
   scoped_nsobject<LocationBarFieldEditor> locationBarFieldEditor_;  // strong
 
+  // Used for monitoring the optional toolbar button prefs.
+  scoped_ptr<ToolbarControllerInternal::PrefObserverBridge> prefObserver_;
+  BooleanPrefMember showHomeButton_;
+  BooleanPrefMember showPageOptionButtons_;
+
+  IBOutlet NSMenu* pageMenu_;
+  IBOutlet NSMenu* wrenchMenu_;
+
   // The ordering is important for unit tests. If new items are added or the
   // ordering is changed, make sure to update |-toolbarViews| and the
   // corresponding enum in the unit tests.
   IBOutlet NSButton* backButton_;
   IBOutlet NSButton* forwardButton_;
   IBOutlet NSButton* reloadButton_;
+  IBOutlet NSButton* homeButton_;
   IBOutlet NSButton* starButton_;
   IBOutlet NSButton* goButton_;
+  IBOutlet NSButton* pageButton_;
+  IBOutlet NSButton* wrenchButton_;
   IBOutlet NSTextField* locationBar_;
 }
 
@@ -83,12 +98,19 @@ class ToolbarView;
 // state.
 - (void)setIsLoading:(BOOL)isLoading;
 
+// Actions for the optional menu buttons for the page and wrench menus. These
+// will show a menu while the mouse is down.
+- (IBAction)showPageMenu:(id)sender;
+- (IBAction)showWrenchMenu:(id)sender;
+
 @end
 
 // A set of private methods used by tests, in the absence of "friends" in ObjC.
 @interface ToolbarController(PrivateTestMethods)
 // Returns an array of views in the order of the outlets above.
 - (NSArray*)toolbarViews;
+- (void)showOptionalHomeButton;
+- (void)showOptionalPageWrenchButtons;
 @end
 
 #endif  // CHROME_BROWSER_COCOA_TOOLBAR_CONTROLLER_H_
diff --git a/chrome/browser/cocoa/toolbar_controller.mm b/chrome/browser/cocoa/toolbar_controller.mm
index 81fdf04..27466c4 100644
--- a/chrome/browser/cocoa/toolbar_controller.mm
+++ b/chrome/browser/cocoa/toolbar_controller.mm
@@ -8,7 +8,13 @@
 #include "base/sys_string_conversions.h"
 #include "chrome/app/chrome_dll_resource.h"
 #import "chrome/browser/cocoa/location_bar_view_mac.h"
+#include "chrome/browser/profile.h"
 #include "chrome/browser/toolbar_model.h"
+#include "chrome/common/notification_details.h"
+#include "chrome/common/notification_observer.h"
+#include "chrome/common/notification_type.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/pref_service.h"
 
 // Names of images in the bundle for the star icon (normal and 'starred').
 static NSString* const kStarImageName = @"star";
@@ -40,8 +46,30 @@ static NSString* const kStarredImageName = @"starred";
 
 @interface ToolbarController(Private)
 - (void)initCommandStatus:(CommandUpdater*)commands;
+- (void)prefChanged:(std::wstring*)prefName;
 @end
 
+namespace ToolbarControllerInternal {
+
+// A C++ class registered for changes in preferences. Bridges the
+// notification back to the ToolbarController.
+class PrefObserverBridge : public NotificationObserver {
+ public:
+  PrefObserverBridge(ToolbarController* controller)
+      : controller_(controller) { }
+  // Overridden from NotificationObserver:
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) {
+    if (type == NotificationType::PREF_CHANGED)
+      [controller_ prefChanged:Details<std::wstring>(details).ptr()];
+  }
+ private:
+  ToolbarController* controller_;  // weak, owns us
+};
+
+}  // namespace
+
 @implementation ToolbarController
 
 - (id)initWithModel:(ToolbarModel*)model
@@ -73,6 +101,16 @@ static NSString* const kStarredImageName = @"starred";
   locationBarView_.reset(new LocationBarViewMac(locationBar_, commands_,
                                                 toolbarModel_, profile_));
   [locationBar_ setFont:[NSFont systemFontOfSize:[NSFont systemFontSize]]];
+
+  // Register pref observers for the optional home and page/options buttons
+  // and then add them to the toolbar them based on those prefs.
+  prefObserver_.reset(new ToolbarControllerInternal::PrefObserverBridge(self));
+  PrefService* prefs = profile_->GetPrefs();
+  showHomeButton_.Init(prefs::kShowHomeButton, prefs, prefObserver_.get());
+  showPageOptionButtons_.Init(prefs::kShowPageOptionsButtons, prefs,
+                              prefObserver_.get());
+  [self showOptionalHomeButton];
+  [self showOptionalPageWrenchButtons];
 }
 
 - (LocationBar*)locationBar {
@@ -97,7 +135,7 @@ static NSString* const kStarredImageName = @"starred";
       button = forwardButton_;
       break;
     case IDC_HOME:
-      // TODO(pinkerton): add home button
+      button = homeButton_;
       break;
     case IDC_STAR:
       button = starButton_;
@@ -112,9 +150,8 @@ static NSString* const kStarredImageName = @"starred";
   [backButton_ setEnabled:commands->IsCommandEnabled(IDC_BACK) ? YES : NO];
   [forwardButton_
       setEnabled:commands->IsCommandEnabled(IDC_FORWARD) ? YES : NO];
-  [reloadButton_
-      setEnabled:commands->IsCommandEnabled(IDC_RELOAD) ? YES : NO];
-  // TODO(pinkerton): Add home button.
+  [reloadButton_ setEnabled:commands->IsCommandEnabled(IDC_RELOAD) ? YES : NO];
+  [homeButton_ setEnabled:commands->IsCommandEnabled(IDC_HOME) ? YES : NO];
   [starButton_ setEnabled:commands->IsCommandEnabled(IDC_STAR) ? YES : NO];
 }
 
@@ -161,7 +198,92 @@ static NSString* const kStarredImageName = @"starred";
 // Returns an array of views in the order of the outlets above.
 - (NSArray*)toolbarViews {
   return [NSArray arrayWithObjects:backButton_, forwardButton_, reloadButton_,
-            starButton_, goButton_, locationBar_, nil];
+            homeButton_, starButton_, goButton_, pageButton_, wrenchButton_,
+            locationBar_, nil];
+}
+
+// Moves |rect| to the right by |delta|, keeping the right side fixed by
+// shrinking the width to compensate. Passing a negative value for |deltaX|
+// moves to the left and increases the width.
+- (NSRect)adjustRect:(NSRect)rect byAmount:(float)deltaX {
+  NSRect frame = NSOffsetRect(rect, deltaX, 0);
+  frame.size.width -= deltaX;
+  return frame;
+}
+
+// Computes the padding between the buttons that should have a separation from
+// the positions in the nib. Since the forward and reload buttons are always
+// visible, we use those buttons as the canonical spacing.
+- (float)interButtonSpacing {
+  NSRect forwardFrame = [forwardButton_ frame];
+  NSRect reloadFrame = [reloadButton_ frame];
+  DCHECK(NSMinX(reloadFrame) > NSMaxX(forwardFrame));
+  return NSMinX(reloadFrame) - NSMaxX(forwardFrame);
+}
+
+// Show or hide the home button based on the pref.
+- (void)showOptionalHomeButton {
+  BOOL hide = showHomeButton_.GetValue() ? NO : YES;
+  if (hide == [homeButton_ isHidden])
+    return;  // Nothing to do, view state matches pref state.
+
+  // Always shift the star and text field by the width of the home button plus
+  // the appropriate gap width. If we're hiding the button, we have to
+  // reverse the direction of the movement (to the left).
+  float moveX = [self interButtonSpacing] + [homeButton_ frame].size.width;
+  if (hide)
+    moveX *= -1;  // Reverse the direction of the move.
+
+  [starButton_ setFrame:NSOffsetRect([starButton_ frame], moveX, 0)];
+  [locationBar_ setFrame:[self adjustRect:[locationBar_ frame]
+                                 byAmount:moveX]];
+  [homeButton_ setHidden:hide];
+}
+
+// Show or hide the page and wrench buttons based on the pref.
+- (void)showOptionalPageWrenchButtons {
+  DCHECK([pageButton_ isHidden] == [wrenchButton_ isHidden]);
+  BOOL hide = showPageOptionButtons_.GetValue() ? NO : YES;
+  if (hide == [pageButton_ isHidden])
+    return;  // Nothing to do, view state matches pref state.
+
+  // Shift the go button and resize the text field by the width of the
+  // page/wrench buttons plus two times the gap width. If we're showing the
+  // buttons, we have to reverse the direction of movement (to the left). Unlike
+  // the home button above, we only ever have to resize the text field, we don't
+  // have to move it.
+  float moveX = 2 * [self interButtonSpacing] + NSWidth([pageButton_ frame]) +
+                  NSWidth([wrenchButton_ frame]);
+  if (!hide)
+    moveX *= -1;  // Reverse the direction of the move.
+  [goButton_ setFrame:NSOffsetRect([goButton_ frame], moveX, 0)];
+  NSRect locationFrame = [locationBar_ frame];
+  locationFrame.size.width += moveX;
+  [locationBar_ setFrame:locationFrame];
+
+  [pageButton_ setHidden:hide];
+  [wrenchButton_ setHidden:hide];
+}
+
+- (void)prefChanged:(std::wstring*)prefName {
+  if (!prefName) return;
+  if (*prefName == prefs::kShowHomeButton) {
+    [self showOptionalHomeButton];
+  } else if (*prefName == prefs::kShowPageOptionsButtons) {
+    [self showOptionalPageWrenchButtons];
+  }
+}
+
+- (IBAction)showPageMenu:(id)sender {
+  [NSMenu popUpContextMenu:pageMenu_
+                 withEvent:[NSApp currentEvent]
+                   forView:pageButton_];
+}
+
+- (IBAction)showWrenchMenu:(id)sender {
+  [NSMenu popUpContextMenu:wrenchMenu_
+                 withEvent:[NSApp currentEvent]
+                   forView:wrenchButton_];
 }
 
 @end
diff --git a/chrome/browser/cocoa/toolbar_controller_unittest.mm b/chrome/browser/cocoa/toolbar_controller_unittest.mm
index 2afc327..6562cce 100644
--- a/chrome/browser/cocoa/toolbar_controller_unittest.mm
+++ b/chrome/browser/cocoa/toolbar_controller_unittest.mm
@@ -9,6 +9,8 @@
 #include "chrome/browser/cocoa/browser_test_helper.h"
 #import "chrome/browser/cocoa/cocoa_test_helper.h"
 #import "chrome/browser/cocoa/toolbar_controller.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -19,8 +21,8 @@ class ToolbarControllerTest : public testing::Test {
   // Indexes that match the ordering returned by the private ToolbarController
   // |-toolbarViews| method.
   enum {
-    kBackIndex, kForwardIndex, kReloadIndex, kStarIndex, kGoIndex,
-    kLocationIndex,
+    kBackIndex, kForwardIndex, kReloadIndex, kHomeIndex, kStarIndex, kGoIndex,
+    kPageIndex, kWrenchIndex, kLocationIndex,
   };
 
   ToolbarControllerTest() {
@@ -48,7 +50,8 @@ class ToolbarControllerTest : public testing::Test {
               [[views objectAtIndex:kForwardIndex] isEnabled] ? true : false);
     EXPECT_EQ(updater->IsCommandEnabled(IDC_RELOAD),
               [[views objectAtIndex:kReloadIndex] isEnabled] ? true : false);
-    // TODO(pinkerton): Add IDC_HOME when we get a view for it
+    EXPECT_EQ(updater->IsCommandEnabled(IDC_HOME),
+              [[views objectAtIndex:kHomeIndex] isEnabled] ? true : false);
     EXPECT_EQ(updater->IsCommandEnabled(IDC_STAR),
               [[views objectAtIndex:kStarIndex] isEnabled] ? true : false);
   }
@@ -111,4 +114,48 @@ TEST_F(ToolbarControllerTest, LoadingState) {
   EXPECT_EQ([go tag], IDC_GO);
 }
 
+// Check that toggling the state of the home button changes the visible
+// state of the home button and moves the other buttons accordingly.
+TEST_F(ToolbarControllerTest, ToggleHome) {
+  PrefService* prefs = helper_.profile()->GetPrefs();
+  bool showHome = prefs->GetBoolean(prefs::kShowHomeButton);
+  NSView* homeButton = [[bar_ toolbarViews] objectAtIndex:kHomeIndex];
+  EXPECT_EQ(showHome, ![homeButton isHidden]);
+
+  NSView* starButton = [[bar_ toolbarViews] objectAtIndex:kStarIndex];
+  NSView* locationBar = [[bar_ toolbarViews] objectAtIndex:kLocationIndex];
+  NSRect originalStarFrame = [starButton frame];
+  NSRect originalLocationBarFrame = [locationBar frame];
+
+  // Toggle the pref and make sure the button changed state and the other
+  // views moved.
+  prefs->SetBoolean(prefs::kShowHomeButton, !showHome);
+  EXPECT_EQ(showHome, [homeButton isHidden]);
+  EXPECT_NE(NSMinX(originalStarFrame), NSMinX([starButton frame]));
+  EXPECT_NE(NSMinX(originalLocationBarFrame), NSMinX([locationBar frame]));
+  EXPECT_NE(NSWidth(originalLocationBarFrame), NSWidth([locationBar frame]));
+}
+
+TEST_F(ToolbarControllerTest, TogglePageWrench) {
+  PrefService* prefs = helper_.profile()->GetPrefs();
+  bool showButtons = prefs->GetBoolean(prefs::kShowPageOptionsButtons);
+  NSView* pageButton = [[bar_ toolbarViews] objectAtIndex:kPageIndex];
+  NSView* wrenchButton = [[bar_ toolbarViews] objectAtIndex:kWrenchIndex];
+  EXPECT_EQ(showButtons, ![pageButton isHidden]);
+  EXPECT_EQ(showButtons, ![wrenchButton isHidden]);
+
+  NSView* goButton = [[bar_ toolbarViews] objectAtIndex:kGoIndex];
+  NSView* locationBar = [[bar_ toolbarViews] objectAtIndex:kLocationIndex];
+  NSRect originalGoFrame = [goButton frame];
+  NSRect originalLocationBarFrame = [locationBar frame];
+
+  // Toggle the pref and make sure the buttons changed state and the other
+  // views moved (or in the case of the location bar, it changed width).
+  prefs->SetBoolean(prefs::kShowPageOptionsButtons, !showButtons);
+  EXPECT_EQ(showButtons, [pageButton isHidden]);
+  EXPECT_EQ(showButtons, [wrenchButton isHidden]);
+  EXPECT_NE(NSMinX(originalGoFrame), NSMinX([goButton frame]));
+  EXPECT_NE(NSWidth(originalLocationBarFrame), NSWidth([locationBar frame]));
+}
+
 }  // namespace