Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(353)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6264011: Makes the other bookmark button on the bookmark bar visible only if it (Closed)

Created:
9 years, 11 months ago by sky
Modified:
9 years, 7 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Makes the other bookmark button on the bookmark bar visible only if it contains something. BUG=64997 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72549

Patch Set 1 #

Patch Set 2 : fix comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -10 lines) Patch
M chrome/browser/ui/views/bookmark_bar_view.cc View 1 7 chunks +21 lines, -10 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
sky
9 years, 11 months ago (2011-01-22 23:31:49 UTC) #1
Ben Goodger (Google)
I don't have a problem with this in principle except that there's no way to ...
9 years, 11 months ago (2011-01-25 17:07:26 UTC) #2
Ben Goodger (Google)
9 years, 11 months ago (2011-01-25 20:02:50 UTC) #3 
LGTM.

On Sat, Jan 22, 2011 at 3:31 PM, <sky@chromium.org> wrote:

> Reviewers: Ben Goodger,
>
> Description:
> Makes the other bookmark button on the bookmark bar visible only if it
> contains something.
>
> BUG=64997
> TEST=none
>
> Please review this at http://codereview.chromium.org/6264011/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/ui/views/bookmark_bar_view.cc
>
>
> Index: chrome/browser/ui/views/bookmark_bar_view.cc
> diff --git a/chrome/browser/ui/views/bookmark_bar_view.cc
> b/chrome/browser/ui/views/bookmark_bar_view.cc
> index
>
456f92d1fc8f44e3033389d757e4fbe715cb6d8d..5e76410419d0b981aa6df699108237fdd82590ed
> 100644
> --- a/chrome/browser/ui/views/bookmark_bar_view.cc
> +++ b/chrome/browser/ui/views/bookmark_bar_view.cc
> @@ -974,6 +974,8 @@ void BookmarkBarView::Loaded(BookmarkModel* model) {
>   for (int i = 0, child_count = node->GetChildCount(); i < child_count;
> ++i)
>     AddChildView(i, CreateBookmarkButton(node->GetChild(i)));
>   UpdateColors();
> +  other_bookmarked_button_->SetVisible(
> +      model->other_node()->GetChildCount() > 0);
>   other_bookmarked_button_->SetEnabled(true);
>
>   Layout();
> @@ -1011,6 +1013,8 @@ void
> BookmarkBarView::BookmarkNodeAdded(BookmarkModel* model,
>  void BookmarkBarView::BookmarkNodeAddedImpl(BookmarkModel* model,
>                                             const BookmarkNode* parent,
>                                             int index) {
> +  other_bookmarked_button_->SetVisible(
> +      model->other_node()->GetChildCount() > 0);
>   NotifyModelChanged();
>   if (parent != model_->GetBookmarkBarNode()) {
>     // We only care about nodes on the bookmark bar.
> @@ -1037,6 +1041,9 @@ void
> BookmarkBarView::BookmarkNodeRemoved(BookmarkModel* model,
>  void BookmarkBarView::BookmarkNodeRemovedImpl(BookmarkModel* model,
>                                               const BookmarkNode* parent,
>                                               int index) {
> +  other_bookmarked_button_->SetVisible(
> +      model->other_node()->GetChildCount() > 0);
> +
>   StopThrobbing(true);
>   // No need to start throbbing again as the bookmark bubble can't be up at
>   // the same time as the user reorders.
> @@ -1560,9 +1567,9 @@ void BookmarkBarView::StartThrobbing(const
> BookmarkNode* node,
>  }
>
>  int BookmarkBarView::GetBookmarkButtonCount() {
> -  // We contain at least four non-bookmark button views: other bookmarks,
> -  // bookmarks separator, chevrons (for overflow), the instruction label
> and
> -  // the sync error button.
> +  // We contain five non-bookmark button views: other bookmarks, bookmarks
> +  // separator, chevrons (for overflow), the instruction label and the
> sync
> +  // error button.
>   return GetChildViewCount() - 5;
>  }
>
> @@ -1614,7 +1621,8 @@ gfx::Size BookmarkBarView::LayoutItems(bool
> compute_bounds_only) {
>   }
>
>   gfx::Size other_bookmarked_pref =
> -      other_bookmarked_button_->GetPreferredSize();
> +      other_bookmarked_button_->IsVisible() ?
> +      other_bookmarked_button_->GetPreferredSize() : gfx::Size();
>   gfx::Size overflow_pref = overflow_button_->GetPreferredSize();
>   gfx::Size bookmarks_separator_pref =
>       bookmarks_separator_view_->GetPreferredSize();
> @@ -1624,9 +1632,10 @@ gfx::Size BookmarkBarView::LayoutItems(bool
> compute_bounds_only) {
>   if (sync_ui_util::ShouldShowSyncErrorButton(sync_service_)) {
>     sync_error_total_width += kButtonPadding +
> sync_error_button_pref.width();
>   }
> -  const int max_x = width - other_bookmarked_pref.width() - kButtonPadding
> -
> -      overflow_pref.width() - kButtonPadding -
> +  int max_x = width - overflow_pref.width() - kButtonPadding -
>       bookmarks_separator_pref.width() - sync_error_total_width;
> +  if (other_bookmarked_button_->IsVisible())
> +    max_x -= other_bookmarked_pref.width() + kButtonPadding;
>
>   // Next, layout out the buttons. Any buttons that are placed beyond the
>   // visible region and made invisible.
> @@ -1686,11 +1695,13 @@ gfx::Size BookmarkBarView::LayoutItems(bool
> compute_bounds_only) {
>   x += bookmarks_separator_pref.width();
>
>   // The other bookmarks button.
> -  if (!compute_bounds_only) {
> -    other_bookmarked_button_->SetBounds(x, y,
> other_bookmarked_pref.width(),
> -                                        height);
> +  if (other_bookmarked_button_->IsVisible()) {
> +    if (!compute_bounds_only) {
> +      other_bookmarked_button_->SetBounds(x, y,
> other_bookmarked_pref.width(),
> +                                          height);
> +    }
> +    x += other_bookmarked_pref.width() + kButtonPadding;
>   }
> -  x += other_bookmarked_pref.width() + kButtonPadding;
>
>   // Set the real bounds of the sync error button only if it needs to
> appear on
>   // the bookmarks bar.
>
>
>

Powered by Google App Engine
This is Rietveld 408576698