Use buttons from DialogClientView in BookmakBubbleView.

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/metrics/user_metrics.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
@@ skipping 13 matching lines @@
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/combobox/combobox.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/widget/widget.h"
+#include "ui/views/window/dialog_client_view.h"
 
 #if defined(OS_WIN)
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.h"
 #include "chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_footnote_view.h"
 #include "components/browser_sync/profile_sync_service.h"
 #endif
 
 using base::UserMetricsAction;
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
-using views::ColumnSet;
-using views::GridLayout;
 
 namespace {
 
 // This combobox prevents any lengthy content from stretching the bubble view.
 class UnsizedCombobox : public views::Combobox {
  public:
   explicit UnsizedCombobox(ui::ComboboxModel* model) : views::Combobox(model) {}
   ~UnsizedCombobox() override {}
 
   // views::Combobox:
@@ skipping 64 matching lines @@
       model->Remove(node);
   }
   // |parent_combobox_| needs to be destroyed before |parent_model_| as it
   // uses |parent_model_| in its destructor.
   delete parent_combobox_;
 }
 
 // ui::DialogModel -------------------------------------------------------------
 
 int BookmarkBubbleView::GetDialogButtons() const {
-  // TODO(tapted): DialogClientView should manage the buttons.
-  return ui::DIALOG_BUTTON_NONE;
+  // TODO(tapted): DialogClientView should manage the ios promo buttons too.
+  return is_showing_ios_promotion_
+             ? ui::DIALOG_BUTTON_NONE
+             : (ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL);
+}
+
+base::string16 BookmarkBubbleView::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK)
+                                       ? IDS_DONE
+                                       : IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK);
 }
 
 // views::WidgetDelegate -------------------------------------------------------
 
 views::View* BookmarkBubbleView::GetInitiallyFocusedView() {
   return name_field_;
 }
 
 base::string16 BookmarkBubbleView::GetWindowTitle() const {
 #if defined(OS_WIN)
@@ skipping 28 matching lines @@
   DCHECK_EQ(bookmark_bubble_, this);
   bookmark_bubble_ = NULL;
   is_showing_ios_promotion_ = false;
 
   if (observer_)
     observer_->OnBookmarkBubbleHidden();
 }
 
 // views::DialogDelegate -------------------------------------------------------
 
+views::View* BookmarkBubbleView::CreateExtraView() {
+  edit_button_ = views::MdTextButton::CreateSecondaryUiButton(
+      this, l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_OPTIONS));
+  edit_button_->AddAccelerator(ui::Accelerator(ui::VKEY_E, ui::EF_ALT_DOWN));
+  return edit_button_;
+}
+
+bool BookmarkBubbleView::GetExtraViewPadding(int* padding) {
+  *padding = ChromeLayoutProvider::Get()->GetDistanceMetric(
+      DISTANCE_UNRELATED_CONTROL_HORIZONTAL_LARGE);
+  return true;
+}
+
 views::View* BookmarkBubbleView::CreateFootnoteView() {
 #if defined(OS_WIN)
   if (!is_showing_ios_promotion_ &&
       IsIOSPromotionEligible(
           desktop_ios_promotion::PromotionEntryPoint::BOOKMARKS_FOOTNOTE)) {
     footnote_view_ = new DesktopIOSPromotionFootnoteView(profile_, this);
     return footnote_view_;
   }
 #endif
   if (!SyncPromoUI::ShouldShowSyncPromo(profile_))
     return nullptr;
 
   base::RecordAction(UserMetricsAction("Signin_Impression_FromBookmarkBubble"));
 
   footnote_view_ =
       new BubbleSyncPromoView(delegate_.get(), IDS_BOOKMARK_SYNC_PROMO_LINK,
                               IDS_BOOKMARK_SYNC_PROMO_MESSAGE);
   return footnote_view_;
 }
 
+bool BookmarkBubbleView::Cancel() {
+  base::RecordAction(UserMetricsAction("BookmarkBubble_Unstar"));
+  // Set this so we remove the bookmark after the window closes.
+  remove_bookmark_ = true;
+  apply_edits_ = false;
+  return true;
+}
+
+bool BookmarkBubbleView::Accept() {
+#if defined(OS_WIN)
+  using desktop_ios_promotion::PromotionEntryPoint;
+  if (IsIOSPromotionEligible(PromotionEntryPoint::BOOKMARKS_BUBBLE)) {
+    ShowIOSPromotion(PromotionEntryPoint::BOOKMARKS_BUBBLE);
+    return false;
+  }
+#endif
+  return true;
+}
+
+bool BookmarkBubbleView::Close() {
+  // Allow closing when activation lost. Default would call Accept().
+  return true;
+}
+
+void BookmarkBubbleView::UpdateButton(views::LabelButton* button,
+                                      ui::DialogButton type) {
+  LocationBarBubbleDelegateView::UpdateButton(button, type);
+  if (type == ui::DIALOG_BUTTON_CANCEL)
+    button->AddAccelerator(ui::Accelerator(ui::VKEY_R, ui::EF_ALT_DOWN));
+}
+
 // views::View -----------------------------------------------------------------
 
 const char* BookmarkBubbleView::GetClassName() const {
   return "BookmarkBubbleView";
 }
 
-bool BookmarkBubbleView::AcceleratorPressed(
-    const ui::Accelerator& accelerator) {
-  ui::KeyboardCode key_code = accelerator.key_code();
-  if (key_code == ui::VKEY_RETURN) {
-    HandleButtonPressed(save_button_);
-    return true;
-  }
-  if (key_code == ui::VKEY_E && accelerator.IsAltDown()) {
-    HandleButtonPressed(edit_button_);
-    return true;
-  }
-  if (key_code == ui::VKEY_R && accelerator.IsAltDown()) {
-    HandleButtonPressed(remove_button_);
-    return true;
-  }
-
-  return LocationBarBubbleDelegateView::AcceleratorPressed(accelerator);
-}
-
 void BookmarkBubbleView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   LocationBarBubbleDelegateView::GetAccessibleNodeData(node_data);
   node_data->SetName(l10n_util::GetStringUTF8(
       newly_bookmarked_ ? IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARKED
                         : IDS_BOOKMARK_AX_BUBBLE_PAGE_BOOKMARK));
 }
 
 // views::ButtonListener -------------------------------------------------------
 
 void BookmarkBubbleView::ButtonPressed(views::Button* sender,
                                        const ui::Event& event) {
-  HandleButtonPressed(sender);
+  base::RecordAction(UserMetricsAction("BookmarkBubble_Edit"));
+  ShowEditor();
 }
 
 // views::ComboboxListener -----------------------------------------------------
 
 void BookmarkBubbleView::OnPerformAction(views::Combobox* combobox) {
   if (combobox->selected_index() + 1 == parent_model_.GetItemCount()) {
     base::RecordAction(UserMetricsAction("BookmarkBubble_EditFromCombobox"));
     ShowEditor();
   }
 }
 
 // DesktopIOSPromotionFootnoteDelegate -----------------------------------------
 
 void BookmarkBubbleView::OnIOSPromotionFootnoteLinkClicked() {
 #if defined(OS_WIN)
   ShowIOSPromotion(
       desktop_ios_promotion::PromotionEntryPoint::FOOTNOTE_FOLLOWUP_BUBBLE);
 #endif
 }
 
 // views::BubbleDialogDelegateView ---------------------------------------------
 
 void BookmarkBubbleView::Init() {
-  remove_button_ = views::MdTextButton::CreateSecondaryUiButton(
-      this, l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK));
+  using views::GridLayout;
 
-  edit_button_ = views::MdTextButton::CreateSecondaryUiButton(
-      this, l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_OPTIONS));
+  SetLayoutManager(new views::FillLayout());
+  bookmark_contents_view_ = new views::View();
+  GridLayout* layout = new GridLayout(bookmark_contents_view_);
+  bookmark_contents_view_->SetLayoutManager(layout);
 
-  save_button_ = views::MdTextButton::CreateSecondaryUiButton(
-      this, l10n_util::GetStringUTF16(IDS_DONE));
-  save_button_->SetIsDefault(true);
+  // This column set is used for the labels and textfields.
+  constexpr int kColumnId = 0;
+  constexpr float kFixed = 0.f;
+  constexpr float kStretchy = 1.f;
+  views::ColumnSet* cs = layout->AddColumnSet(kColumnId);
+  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
 
+  cs->AddColumn(provider->GetControlLabelGridAlignment(), GridLayout::CENTER,
+                kFixed, GridLayout::USE_PREF, 0, 0);
+  cs->AddPaddingColumn(kFixed, provider->GetDistanceMetric(
+                                   DISTANCE_UNRELATED_CONTROL_HORIZONTAL));
+  cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, kStretchy,
+                GridLayout::USE_PREF, 0, 0);
+
+  layout->StartRow(kFixed, kColumnId);
+  views::Label* label = new views::Label(
+      l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_NAME_LABEL));
+  layout->AddView(label);
+
+  name_field_ = new views::Textfield();
+  name_field_->SetText(GetBookmarkName());
+  name_field_->SetAccessibleName(
+      l10n_util::GetStringUTF16(IDS_BOOKMARK_AX_BUBBLE_NAME_LABEL));
+  layout->AddView(name_field_);
+
+  layout->StartRowWithPadding(
+      kFixed, kColumnId, kFixed,
+      provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL));
   views::Label* combobox_label = new views::Label(
-      l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_FOLDER_TEXT));
+      l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_FOLDER_LABEL));
+  layout->AddView(combobox_label);
 
   parent_combobox_ = new UnsizedCombobox(&parent_model_);
   parent_combobox_->set_listener(this);
   parent_combobox_->SetAccessibleName(
-      l10n_util::GetStringUTF16(IDS_BOOKMARK_AX_BUBBLE_FOLDER_TEXT));
+      l10n_util::GetStringUTF16(IDS_BOOKMARK_AX_BUBBLE_FOLDER_LABEL));
+  layout->AddView(parent_combobox_);
 
-  SetLayoutManager(new views::FillLayout);
-  bookmark_contents_view_ = new views::View();
-  GridLayout* layout = new GridLayout(bookmark_contents_view_);
-  bookmark_contents_view_->SetLayoutManager(layout);
-
-  // This column set is used for the labels and textfields as well as the
-  // buttons at the bottom.
-  const int cs_id = 0;
-  ColumnSet* cs = layout->AddColumnSet(cs_id);
-  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
-
-  cs->AddColumn(provider->GetControlLabelGridAlignment(), GridLayout::CENTER, 0,
-                GridLayout::USE_PREF, 0, 0);
-  cs->AddPaddingColumn(
-      0, provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL));
-
-  cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 0, GridLayout::USE_PREF,
-                0, 0);
-  cs->AddPaddingColumn(1, provider->GetDistanceMetric(
-                              DISTANCE_UNRELATED_CONTROL_HORIZONTAL_LARGE));
-
-  cs->AddColumn(GridLayout::LEADING, GridLayout::TRAILING, 0,
-                GridLayout::USE_PREF, 0, 0);
-  cs->AddPaddingColumn(0, provider->GetDistanceMetric(
-                              views::DISTANCE_RELATED_BUTTON_HORIZONTAL));
-  cs->AddColumn(GridLayout::LEADING, GridLayout::TRAILING, 0,
-                GridLayout::USE_PREF, 0, 0);
-
-  layout->StartRow(0, cs_id);
-  views::Label* label = new views::Label(
-      l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_TITLE_TEXT));
-  layout->AddView(label);
-  name_field_ = new views::Textfield();
-  name_field_->SetText(GetBookmarkName());
-  name_field_->SetAccessibleName(
-      l10n_util::GetStringUTF16(IDS_BOOKMARK_AX_BUBBLE_TITLE_TEXT));
-
-  layout->AddView(name_field_, 5, 1);
-
-  layout->AddPaddingRow(
-      0, provider->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS).top());
-
-  layout->StartRow(0, cs_id);
-  layout->AddView(combobox_label);
-  layout->AddView(parent_combobox_, 5, 1);
-
-  layout->AddPaddingRow(
-      0, provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL));
-
-  layout->StartRow(0, cs_id);
-  layout->SkipColumns(2);
-  layout->AddView(remove_button_);
-  layout->AddView(edit_button_);
-  layout->AddView(save_button_);
-
-  AddAccelerator(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE));
-  AddAccelerator(ui::Accelerator(ui::VKEY_E, ui::EF_ALT_DOWN));
-  AddAccelerator(ui::Accelerator(ui::VKEY_R, ui::EF_ALT_DOWN));
+  // TODO(tapted): BubbleDialogDelegateView should do this by default.
+  set_button_row_insets(gfx::Insets(
+      provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_VERTICAL), 0, 0,

[Peter Kasting, 2017/05/31 05:45:46]

I don't think we want to do this, as it makes bubbles and dialogs do opposite
things with their padding.  Bret is making dialogs use INSETS_DIALOG_BUTON_ROW,
which has a 0 top inset, with the expectation that dialog content area (above
the button row) uses INSETS_DIALOG_CONTENTS, and the nonzero bottom inset there
spaces things out correctly in total.  This would basically put the 0 on the
other side of the dividing line between contents and button row (which is more
how I wanted to do it, but whatever).  I think we should be consistent.

If BubbleDialogDelegateView::margins_ is being drawn around the bubble content
and buttons (but not title), then it seems like what we really want is for it to
be around content (but not buttons or title), and then we can set the title,
content, and button row insets with the three INSETS_DIALOG_XXX values like we
already do for other dialogs.  In an ideal world this would mean all the
BubbleDialogDelegateView stuff would just go away and parent class
implementations would do all this.

If you not up to speed on what Bret is doing, you might want to coordinate with
him here.  Otherwise, if you want to get something landed for now so larger
changes can be done separately, I'd basically go back to what you were doing in
your previous patch set.

[tapted, 2017/05/31 05:54:27]

Done.

+      0));
 
   AddChildView(bookmark_contents_view_);
 }
 
 // Private methods -------------------------------------------------------------
 
 BookmarkBubbleView::BookmarkBubbleView(
     views::View* anchor_view,
     bookmarks::BookmarkBubbleObserver* observer,
     std::unique_ptr<BubbleSyncPromoDelegate> delegate,
     Profile* profile,
     const GURL& url,
     bool newly_bookmarked)
     : LocationBarBubbleDelegateView(anchor_view, nullptr),
       observer_(observer),
       delegate_(std::move(delegate)),
       profile_(profile),
       url_(url),
       newly_bookmarked_(newly_bookmarked),
       parent_model_(BookmarkModelFactory::GetForBrowserContext(profile_),
                     BookmarkModelFactory::GetForBrowserContext(profile_)
-                        ->GetMostRecentlyAddedUserNodeForURL(url)),
-      remove_button_(nullptr),
-      edit_button_(nullptr),
-      save_button_(nullptr),
-      name_field_(nullptr),
-      parent_combobox_(nullptr),
-      ios_promo_view_(nullptr),
-      footnote_view_(nullptr),
-      remove_bookmark_(false),
-      apply_edits_(true),
-      is_showing_ios_promotion_(false) {
+                        ->GetMostRecentlyAddedUserNodeForURL(url)) {
   chrome::RecordDialogCreation(chrome::DialogIdentifier::BOOKMARK);
 }
 
 base::string16 BookmarkBubbleView::GetBookmarkName() {
   BookmarkModel* bookmark_model =
       BookmarkModelFactory::GetForBrowserContext(profile_);
   const BookmarkNode* node =
       bookmark_model->GetMostRecentlyAddedUserNodeForURL(url_);
   if (node)
     return node->GetTitle();
   else
     NOTREACHED();
   return base::string16();
 }
 
-void BookmarkBubbleView::HandleButtonPressed(views::Button* sender) {
-  if (sender == remove_button_) {
-    base::RecordAction(UserMetricsAction("BookmarkBubble_Unstar"));
-    // Set this so we remove the bookmark after the window closes.
-    remove_bookmark_ = true;
-    apply_edits_ = false;
-    GetWidget()->Close();
-  } else if (sender == edit_button_) {
-    base::RecordAction(UserMetricsAction("BookmarkBubble_Edit"));
-    ShowEditor();
-  } else {
-    DCHECK_EQ(save_button_, sender);
-#if defined(OS_WIN)
-    if (IsIOSPromotionEligible(
-            desktop_ios_promotion::PromotionEntryPoint::BOOKMARKS_BUBBLE)) {
-      ShowIOSPromotion(
-          desktop_ios_promotion::PromotionEntryPoint::BOOKMARKS_BUBBLE);
-    } else {
-      GetWidget()->Close();
-    }
-#else
-    GetWidget()->Close();
-#endif
-  }
-}
-
 void BookmarkBubbleView::ShowEditor() {
   const BookmarkNode* node =
       BookmarkModelFactory::GetForBrowserContext(profile_)
           ->GetMostRecentlyAddedUserNodeForURL(url_);
   gfx::NativeWindow native_parent =
       anchor_widget() ? anchor_widget()->GetNativeWindow()
                       : platform_util::GetTopLevel(parent_window());
   DCHECK(native_parent);
 
   Profile* profile = profile_;
@@ skipping 30 matching lines @@
   PrefService* prefs = profile_->GetPrefs();
   const browser_sync::ProfileSyncService* sync_service =
       ProfileSyncServiceFactory::GetForProfile(profile_);
   return desktop_ios_promotion::IsEligibleForIOSPromotion(prefs, sync_service,
                                                           entry_point);
 }
 
 void BookmarkBubbleView::ShowIOSPromotion(
     desktop_ios_promotion::PromotionEntryPoint entry_point) {
   DCHECK(!is_showing_ios_promotion_);
+  edit_button_->SetVisible(false);
   // Hide the contents, but don't delete. Its child views are accessed in the
   // destructor if there are edits to apply.
   bookmark_contents_view_->SetVisible(false);
   delete footnote_view_;
   footnote_view_ = nullptr;
   is_showing_ios_promotion_ = true;
   ios_promo_view_ = new DesktopIOSPromotionBubbleView(profile_, entry_point);
   AddChildView(ios_promo_view_);
   GetWidget()->UpdateWindowIcon();
   GetWidget()->UpdateWindowTitle();
-  // Resize the bubble so it has the same width as the parent bubble.
-  ios_promo_view_->UpdateBubbleHeight();
+  GetDialogClientView()->UpdateDialogButtons();
+  SizeToContents();
 }
 #endif