bookmarks: Convert "Remove" link into a LabelButton.

chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h"
 
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
@@ skipping 26 matching lines @@
 
 namespace {
 
 // Padding between "Title:" and the actual title.
 const int kTitlePadding = 4;
 
 // Minimum width for the fields - they will push out the size of the bubble if
 // necessary. This should be big enough so that the field pushes the right side
 // of the bubble far enough so that the edit button's left edge is to the right
 // of the field's left edge.
 const int kMinimumFieldSize = 180;

[msw, 2013/06/01 01:18:48]

Make this kMinBubbleWidth = 336 (maybe 340) to match spec.

[tfarina, 2013/06/01 03:33:42]

Done.

 
 }  // namespace
 
 // Declared in browser_dialogs.h so callers don't have to depend on our header.
 
 namespace chrome {
 
 void ShowBookmarkBubbleView(views::View* anchor_view,
                             BookmarkBubbleViewObserver* observer,
                             Profile* profile,
@@ skipping 63 matching lines @@
 
 void BookmarkBubbleView::WindowClosing() {
   // We have to reset |bubble_| here, not in our destructor, because we'll be
   // destroyed asynchronously and the shown state will be checked before then.
   DCHECK_EQ(bookmark_bubble_, this);
   bookmark_bubble_ = NULL;
 
   if (observer_)
     observer_->OnBookmarkBubbleHidden();
  }
 

[msw, 2013/06/01 01:18:48]

Override GetPreferredSize() or GetMinimumSize to return:
view::Size size(views::BubbleDelegateView::GetPreferredSize());
size.SetToMax((gfx::Size(kMinBubbleWidth, 0));
return size;

 bool BookmarkBubbleView::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   if (accelerator.key_code() == ui::VKEY_RETURN) {
      if (edit_button_->HasFocus())
        HandleButtonPressed(edit_button_);
      else
        HandleButtonPressed(close_button_);
      return true;
   } else if (accelerator.key_code() == ui::VKEY_ESCAPE) {
     remove_bookmark_ = newly_bookmarked_;
     apply_edits_ = false;
   }
 
   return BubbleDelegateView::AcceleratorPressed(accelerator);
 }
 
 void BookmarkBubbleView::Init() {
-  remove_link_ = new views::Link(l10n_util::GetStringUTF16(
+  remove_button_ = new views::LabelButton(this, l10n_util::GetStringUTF16(
       IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK));
-  remove_link_->set_listener(this);
+  remove_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
 
   edit_button_ = new views::LabelButton(
       this, l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_OPTIONS));
   edit_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
 
   close_button_ = new views::LabelButton(
       this, l10n_util::GetStringUTF16(IDS_DONE));
   close_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
   close_button_->SetIsDefault(true);
 
   views::Label* combobox_label = new views::Label(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_FOLDER_TEXT));
 
   parent_combobox_ = new views::Combobox(&parent_model_);
   parent_combobox_->set_listener(this);
   parent_combobox_->SetAccessibleName(combobox_label->text());
 
   views::Label* title_label = new views::Label(

[msw, 2013/06/01 01:07:28]

Try GetBubbleFrameView()->SetTitle(...); remove this, its column set, and row.

[tfarina, 2013/06/01 03:33:42]

Not yet, looking into ViewHierarchyChanged() approach.

       l10n_util::GetStringUTF16(
           newly_bookmarked_ ? IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARKED :
                               IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARK));
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   title_label->SetFont(rb.GetFont(ui::ResourceBundle::MediumFont));
 
   GridLayout* layout = new GridLayout(this);
   SetLayoutManager(layout);
 
   ColumnSet* cs = layout->AddColumnSet(0);
 
   // Top (title) row.
   cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0, GridLayout::USE_PREF,
                 0, 0);
   cs->AddPaddingColumn(1, views::kUnrelatedControlHorizontalSpacing);
   cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0, GridLayout::USE_PREF,
                 0, 0);
 
-  // Middle (input field) rows.
   cs = layout->AddColumnSet(2);
   cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 0,
                 GridLayout::USE_PREF, 0, 0);
   cs->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);

[msw, 2013/06/01 01:07:28]

This value (8) needs to be increased to match 15 in the spec.
The closest constant is kUnrelatedControlHorizontalSpacing (12), use that.

[tfarina, 2013/06/01 03:33:42]

Done.

-  cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
+
+  cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 0,
                 GridLayout::USE_PREF, 0, kMinimumFieldSize);

[msw, 2013/06/01 01:18:48]

Remove kMinimumFieldSize here.

[tfarina, 2013/06/01 03:33:42]

Done.

+  cs->AddPaddingColumn(1, views::kUnrelatedControlLargeHorizontalSpacing);
 
-  // Bottom (buttons) row.
-  cs = layout->AddColumnSet(3);
-  cs->AddPaddingColumn(1, views::kRelatedControlHorizontalSpacing);
   cs->AddColumn(GridLayout::LEADING, GridLayout::TRAILING, 0,
                 GridLayout::USE_PREF, 0, 0);
   // We subtract 2 to account for the natural button padding, and
   // to bring the separation visually in line with the row separation
   // height.
   cs->AddPaddingColumn(0, views::kRelatedButtonHSpacing - 2);
+
   cs->AddColumn(GridLayout::LEADING, GridLayout::TRAILING, 0,
                 GridLayout::USE_PREF, 0, 0);
 
   layout->StartRow(0, 0);
   layout->AddView(title_label);
-  layout->AddView(remove_link_);
 
   layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing);
+
   layout->StartRow(0, 2);
   views::Label* label = new views::Label(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_TITLE_TEXT));
   layout->AddView(label);
   title_tf_ = new views::Textfield();
   title_tf_->SetText(GetTitle());
-  layout->AddView(title_tf_);
+  layout->AddView(title_tf_, 5, 1);
 
   layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing);

[msw, 2013/06/01 01:07:28]

This value (4) needs to be increased to match 10 in the spec.
The closest constant is kRelatedControlVerticalSpacing (8), use that.

[tfarina, 2013/06/01 03:33:42]

Done.

 
   layout->StartRow(0, 2);
   layout->AddView(combobox_label);
-  layout->AddView(parent_combobox_);
+  layout->AddView(parent_combobox_, 5, 1);
+
   layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing);

[msw, 2013/06/01 01:07:28]

ditto, use kRelatedControlVerticalSpacing.

[tfarina, 2013/06/01 03:33:42]

Done.

 
-  layout->StartRow(0, 3);
+  layout->StartRow(0, 2);
+  layout->SkipColumns(2);
+  layout->AddView(remove_button_);
   layout->AddView(edit_button_);
   layout->AddView(close_button_);
 
   AddAccelerator(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE));
 }
 
 BookmarkBubbleView::BookmarkBubbleView(views::View* anchor_view,

[msw, 2013/06/01 01:07:28]

The left/right/bottom insets are 7 and should be 20, like your other CL:
https://codereview.chromium.org/15808006/diff/1/chrome/browser/ui/views/bookm...
set_margins(gfx::Insets(20, 20, 20, 20));
You may use kUnrelatedControlLargeHorizontalSpacing and
kUnrelatedControlVerticalSpacing.
(top may be wrong, and depends on whether you use SetTitle() or not)

[tfarina, 2013/06/01 03:33:42]

Done.

                                        BookmarkBubbleViewObserver* observer,
                                        Profile* profile,
                                        const GURL& url,
                                        bool newly_bookmarked)
     : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
       observer_(observer),
       profile_(profile),
       url_(url),
       newly_bookmarked_(newly_bookmarked),
       parent_model_(
           BookmarkModelFactory::GetForProfile(profile_),
           BookmarkModelFactory::GetForProfile(profile_)->
               GetMostRecentlyAddedNodeForURL(url)),
-      remove_link_(NULL),
+      remove_button_(NULL),
       edit_button_(NULL),
       close_button_(NULL),
       title_tf_(NULL),
       parent_combobox_(NULL),
       remove_bookmark_(false),
       apply_edits_(true) {
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(5, 0, 5, 0));
 }
 
 string16 BookmarkBubbleView::GetTitle() {
   BookmarkModel* bookmark_model =
       BookmarkModelFactory::GetForProfile(profile_);
   const BookmarkNode* node =
       bookmark_model->GetMostRecentlyAddedNodeForURL(url_);
   if (node)
     return node->GetTitle();
   else
     NOTREACHED();
   return string16();
 }
 
 void BookmarkBubbleView::ButtonPressed(views::Button* sender,
                                        const ui::Event& event) {
   HandleButtonPressed(sender);
 }
 
-void BookmarkBubbleView::LinkClicked(views::Link* source, int event_flags) {
-  DCHECK_EQ(remove_link_, source);
-  content::RecordAction(UserMetricsAction("BookmarkBubble_Unstar"));
-
-  // Set this so we remove the bookmark after the window closes.
-  remove_bookmark_ = true;
-  apply_edits_ = false;
-  StartFade(false);
-}
-
 void BookmarkBubbleView::OnSelectedIndexChanged(views::Combobox* combobox) {
   if (combobox->selected_index() + 1 == parent_model_.GetItemCount()) {
     content::RecordAction(UserMetricsAction("BookmarkBubble_EditFromCombobox"));
     ShowEditor();
   }
 }
 
 void BookmarkBubbleView::HandleButtonPressed(views::Button* sender) {
-  if (sender == edit_button_) {
+  if (sender == remove_button_) {
+    content::RecordAction(UserMetricsAction("BookmarkBubble_Unstar"));
+    // Set this so we remove the bookmark after the window closes.
+    remove_bookmark_ = true;
+    apply_edits_ = false;
+    StartFade(false);
+  } else if (sender == edit_button_) {
     content::RecordAction(UserMetricsAction("BookmarkBubble_Edit"));
     ShowEditor();
   } else {
     DCHECK_EQ(close_button_, sender);
     StartFade(false);
   }
 }
 
 void BookmarkBubbleView::ShowEditor() {
   const BookmarkNode* node = BookmarkModelFactory::GetForProfile(
@@ skipping 20 matching lines @@
   if (node) {
     const string16 new_title = title_tf_->text();
     if (new_title != node->GetTitle()) {
       model->SetTitle(node, new_title);
       content::RecordAction(
           UserMetricsAction("BookmarkBubble_ChangeTitleInBubble"));
     }
     parent_model_.MaybeChangeParent(node, parent_combobox_->selected_index());
   }
 }