A little bit of bookmark bar cleanup, a little bit of padding improvement.

The spacing between the toolbar buttons now matches windows pixel for pixel (tested empirically).

The bookmark bar item spacing is now a little better, but we need some hackery to improve it to the state of windows (we need to annex about 5 pixels from the toolbar when the bookmark bar is open).

The code for packing bookmark bar buttons is now all in one place.

Functional change: There will be no other bookmarks button until the model is loaded. If you care I can fix this, it just doesn't seem like a very important matter since it's unusable anyway when the model isn't loaded.o

I also removed a log warning from chrome_dll_main because with this change it started spamming a lot. We're already living with this warning, and have a bug filed for it (they fixed the root problem in newer versions of gtk so I think this bug will just never get resolved), so I don't think this is a big deal.

BUG=15870
TEST=things look marginally better, theming still works properly
Review URL: http://codereview.chromium.org/155342

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

chrome/app/chrome_dll_main.cc

@@ -175,7 +175,6 @@ static void GLibLogHandler(const gchar* log_domain,
                              "allocate widget with width") &&
              !GTK_CHECK_VERSION(2, 16, 1)) {
     // http://crbug.com/11133
-    LOG(ERROR) << "Bug 11133";
   } else if (strstr(message, "Theme file for default has no") ||
              strstr(message, "Theme directory")) {
     LOG(ERROR) << "GTK theme error: " << message;

chrome/browser/gtk/bookmark_bar_gtk.cc

@@ -85,8 +85,6 @@ void BookmarkBarGtk::SetProfile(Profile* profile) {
   if (model_)
     model_->RemoveObserver(this);
 
-  gtk_widget_set_sensitive(other_bookmarks_button_, false);
-
   // TODO(erg): Handle extensions
 
   model_ = profile_->GetBookmarkModel();
@@ -103,10 +101,6 @@ void BookmarkBarGtk::SetPageNavigator(PageNavigator* navigator) {
 }
 
 void BookmarkBarGtk::Init(Profile* profile) {
-  // Load the default images from the resource bundle.
-  ResourceBundle& rb = ResourceBundle::GetSharedInstance();
-  static GdkPixbuf* folder_icon = rb.GetPixbufNamed(IDR_BOOKMARK_BAR_FOLDER);
-
   bookmark_hbox_.Own(gtk_hbox_new(FALSE, 0));
 
   instructions_ = gtk_alignment_new(0.0, 0.0, 1.0, 1.0);
@@ -155,21 +149,6 @@ void BookmarkBarGtk::Init(Profile* profile) {
   // we can have finer control over its label.
   other_bookmarks_button_ = gtk_chrome_button_new();
   ConnectFolderButtonEvents(other_bookmarks_button_);
-  gtk_chrome_button_set_use_gtk_rendering(
-      GTK_CHROME_BUTTON(other_bookmarks_button_),
-      GtkThemeProvider::UseSystemThemeGraphics(profile));
-
-  GtkWidget* image = gtk_image_new_from_pixbuf(folder_icon);
-  other_bookmarks_label_ = gtk_label_new(
-      l10n_util::GetStringUTF8(IDS_BOOMARK_BAR_OTHER_BOOKMARKED).c_str());
-  GtkThemeProperties properties(profile);
-  bookmark_utils::SetButtonTextColors(other_bookmarks_label_, &properties);
-
-  GtkWidget* box = gtk_hbox_new(FALSE, bookmark_utils::kBarButtonPadding);
-  gtk_box_pack_start(GTK_BOX(box), image, FALSE, FALSE, 0);
-  gtk_box_pack_start(GTK_BOX(box), other_bookmarks_label_, FALSE, FALSE, 0);
-  gtk_container_add(GTK_CONTAINER(other_bookmarks_button_), box);
-
   gtk_box_pack_start(GTK_BOX(bookmark_hbox_.get()), other_bookmarks_button_,
                      FALSE, FALSE, 0);
 
@@ -230,13 +209,9 @@ void BookmarkBarGtk::Loaded(BookmarkModel* model) {
   // shutdown. Do nothing.
   if (!instructions_)
     return;
+
   RemoveAllBookmarkButtons();
-
-  const BookmarkNode* node = model_->GetBookmarkBarNode();
-  DCHECK(node && model_->other_node());
-  CreateAllBookmarkButtons(node);
-
-  gtk_widget_set_sensitive(other_bookmarks_button_, true);
+  CreateAllBookmarkButtons();
 }
 
 void BookmarkBarGtk::BookmarkModelBeingDeleted(BookmarkModel* model) {
@@ -271,7 +246,7 @@ void BookmarkBarGtk::BookmarkNodeAdded(BookmarkModel* model,
   gtk_toolbar_insert(GTK_TOOLBAR(bookmark_toolbar_.get()),
                      item, index);
 
-  SetInstructionState(parent);
+  SetInstructionState();
 }
 
 void BookmarkBarGtk::BookmarkNodeRemoved(BookmarkModel* model,
@@ -288,7 +263,7 @@ void BookmarkBarGtk::BookmarkNodeRemoved(BookmarkModel* model,
   gtk_container_remove(GTK_CONTAINER(bookmark_toolbar_.get()),
                        to_remove);
 
-  SetInstructionState(parent);
+  SetInstructionState();
 }
 
 void BookmarkBarGtk::BookmarkNodeChanged(BookmarkModel* model,
@@ -319,22 +294,28 @@ void BookmarkBarGtk::BookmarkNodeChildrenReordered(BookmarkModel* model,
 
   // Purge and rebuild the bar.
   RemoveAllBookmarkButtons();
-  CreateAllBookmarkButtons(node);
+  CreateAllBookmarkButtons();
 }
 
-void BookmarkBarGtk::CreateAllBookmarkButtons(const BookmarkNode* node) {
+void BookmarkBarGtk::CreateAllBookmarkButtons() {
+  const BookmarkNode* node = model_->GetBookmarkBarNode();
+  DCHECK(node && model_->other_node());
+
   // Create a button for each of the children on the bookmark bar.
   for (int i = 0; i < node->GetChildCount(); ++i) {
     GtkToolItem* item = CreateBookmarkToolItem(node->GetChild(i));
     gtk_toolbar_insert(GTK_TOOLBAR(bookmark_toolbar_.get()), item, -1);
   }
 
-  SetInstructionState(node);
+  GtkThemeProperties properties(profile_);
+  bookmark_utils::ConfigureButtonForNode(model_->other_node(),
+      model_, other_bookmarks_button_, &properties);
+
+  SetInstructionState();
 }
 
-void BookmarkBarGtk::SetInstructionState(
-    const BookmarkNode* boomarks_bar_node) {
-  show_instructions_ = (boomarks_bar_node->GetChildCount() == 0);
+void BookmarkBarGtk::SetInstructionState() {
+  show_instructions_ = (model_->GetBookmarkBarNode()->GetChildCount() == 0);
   if (show_instructions_) {
     gtk_widget_show_all(instructions_);
   } else {
@@ -359,19 +340,11 @@ bool BookmarkBarGtk::IsAlwaysShown() {
 }
 
 void BookmarkBarGtk::UserChangedTheme(GtkThemeProperties* properties) {
-  gtk_chrome_button_set_use_gtk_rendering(
-      GTK_CHROME_BUTTON(other_bookmarks_button_),
-      properties->use_gtk_rendering);
-  bookmark_utils::SetButtonTextColors(other_bookmarks_label_, properties);
-
   if (model_) {
     // Regenerate the bookmark bar with all new objects with their theme
     // properties set correctly for the new theme.
     RemoveAllBookmarkButtons();
-
-    const BookmarkNode* node = model_->GetBookmarkBarNode();
-    DCHECK(node && model_->other_node());
-    CreateAllBookmarkButtons(node);
+    CreateAllBookmarkButtons();
   } else {
     DLOG(ERROR) << "Received a theme change notification while we don't have a "
                 << "BookmarkModel. Taking no action.";

chrome/browser/gtk/bookmark_bar_gtk.h

@@ -77,11 +77,11 @@ class BookmarkBarGtk : public AnimationDelegate,
 
  private:
   // Helper function which generates GtkToolItems for |bookmark_toolbar_|.
-  void CreateAllBookmarkButtons(const BookmarkNode* node);
+  void CreateAllBookmarkButtons();
 
   // Sets the visibility of the instructional text based on whether there are
-  // any bookmarks in |node|. |node| is assumed to be the bookmarks bar node.
-  void SetInstructionState(const BookmarkNode* boomarks_bar_node);
+  // any bookmarks in the bookmark bar node.
+  void SetInstructionState();
 
   // Helper function which destroys all the bookmark buttons in the GtkToolbar.
   void RemoveAllBookmarkButtons();
@@ -93,7 +93,7 @@ class BookmarkBarGtk : public AnimationDelegate,
 
   // Overridden from BookmarkModelObserver:
 
-  // Invoked when the bookmark bar model has finished loading. Creates a button
+  // Invoked when the bookmark model has finished loading. Creates a button
   // for each of the children of the root node from the model.
   virtual void Loaded(BookmarkModel* model);
 
@@ -217,10 +217,6 @@ class BookmarkBarGtk : public AnimationDelegate,
   // The other bookmarks button.
   GtkWidget* other_bookmarks_button_;
 
-  // The label inside |other_bookmarks_button_|. We keep a reference so we can
-  // change the text color.
-  GtkWidget* other_bookmarks_label_;
-
   // Whether we should ignore the next button release event (because we were
   // dragging).
   bool ignore_button_release_;

chrome/browser/gtk/bookmark_utils_gtk.cc

@@ -30,6 +30,15 @@ const size_t kMaxCharsOnAButton = 15;
 // Only used for the background of the drag widget.
 const GdkColor kBackgroundColor = GDK_COLOR_RGB(0xe6, 0xed, 0xf4);
 
+// Padding between the chrome button highlight border and the contents (favicon,
+// text).
+// TODO(estade): we need to adjust the top and bottom padding, but first we need
+// to give the bookmark bar more space (at the expense of the toolbar).
+const int kButtonPaddingTop = 0;
+const int kButtonPaddingBottom = 0;
+const int kButtonPaddingLeft = 2;
+const int kButtonPaddingRight = 0;
+
 void* AsVoid(const BookmarkNode* node) {
   return const_cast<BookmarkNode*>(node);
 }
@@ -40,6 +49,7 @@ namespace bookmark_utils {
 
 const char kBookmarkNode[] = "bookmark-node";
 
+// Spacing between the buttons on the bar.
 const int kBarButtonPadding = 4;
 
 GdkPixbuf* GetFolderIcon() {
@@ -123,7 +133,13 @@ void ConfigureButtonForNode(const BookmarkNode* node, BookmarkModel* model,
   GtkWidget* box = gtk_hbox_new(FALSE, kBarButtonPadding);
   gtk_box_pack_start(GTK_BOX(box), image, FALSE, FALSE, 0);
   gtk_box_pack_start(GTK_BOX(box), label, FALSE, FALSE, 0);
-  gtk_container_add(GTK_CONTAINER(button), box);
+
+  GtkWidget* alignment = gtk_alignment_new(0.0, 0.0, 1.0, 1.0);
+  gtk_alignment_set_padding(GTK_ALIGNMENT(alignment),
+      kButtonPaddingTop, kButtonPaddingBottom,
+      kButtonPaddingLeft, kButtonPaddingRight);
+  gtk_container_add(GTK_CONTAINER(alignment), box);
+  gtk_container_add(GTK_CONTAINER(button), alignment);
 
   SetButtonTextColors(label, properties);
   g_object_set_data(G_OBJECT(button), bookmark_utils::kBookmarkNode,
@@ -132,7 +148,7 @@ void ConfigureButtonForNode(const BookmarkNode* node, BookmarkModel* model,
   gtk_chrome_button_set_use_gtk_rendering(GTK_CHROME_BUTTON(button),
                                           properties->use_gtk_rendering);
 
-  gtk_widget_show_all(box);
+  gtk_widget_show_all(alignment);
 }
 
 std::string BuildTooltipFor(const BookmarkNode* node) {

chrome/browser/gtk/browser_toolbar_gtk.cc

@@ -45,7 +45,7 @@ namespace {
 const int kToolbarHeight = 37;
 
 // Interior spacing between toolbar widgets.
-const int kToolbarWidgetSpacing = 6;
+const int kToolbarWidgetSpacing = 4;
 
 // The amount of space between the bottom of the star and the top of the
 // Omnibox results popup window.  We want a two pixel space between the bottom