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 {
+  switch (button) {
+    case ui::DIALOG_BUTTON_OK:
+      return l10n_util::GetStringUTF16(IDS_DONE);
+    case ui::DIALOG_BUTTON_CANCEL:
+      return l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK);
+    case ui::DIALOG_BUTTON_NONE:
+      NOTREACHED();
+      break;
+  }
+  return base::string16();

[Peter Kasting, 2017/05/31 02:43:31]

Nit: Simpler:

  return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK)
                                       ? IDS_DONE
                                       : IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK);

Since DIALOG_BUTTON_NONE never reaches here anyway (and callers would not expect
it to), I think it's fine not to have extra code just to (effectively) verify
that and return string16().

[tapted, 2017/05/31 04:27:48]

Done.

 }
 
 // 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)
+  if (IsIOSPromotionEligible(
+          desktop_ios_promotion::PromotionEntryPoint::BOOKMARKS_BUBBLE)) {

[Peter Kasting, 2017/05/31 02:43:31]

Nit: Maybe "using desktop_ios_promotion::PromotionEntryPoint;" above this block
to reduce the qualifiers below, allowing you to avoid so much line-wrapping?

[tapted, 2017/05/31 04:27:49]

Done.

+    ShowIOSPromotion(
+        desktop_ios_promotion::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);

[Peter Kasting, 2017/05/31 02:43:31]

This block is basically a placeholder until we switch to using the correct title
machinery, right?

[tapted, 2017/05/31 04:27:48]

Nah - the title machinery is being used correctly already. This layout block now
holds the textfield and combobox only (with their corresponding labels).

+  views::Label* label = new views::Label(
+      l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_TITLE_TEXT));

[tapted, 2017/05/31 04:27:48]

I think this is the bit that makes it confusing IDS_BOOKMARK_BUBBLE_TITLE_TEXT
is actually "Name:". I'll fix it. (I renamed the textfield data member from
title -> name in the precursor since it tripped me up too.., this should be
consistent).

+  layout->AddView(label);
+

[Peter Kasting, 2017/05/31 02:43:31]

Don't we need to start a new row here or something?  And shouldn't there be a
padding row for the dialog content top inset?

[tapted, 2017/05/31 04:27:48]

The top and side padding is handled by |margins_|, at:

NonClientFrameView* BubbleDialogDelegateView::CreateNonClientFrameView(
    Widget* widget) {
  BubbleFrameView* frame = new BubbleFrameView(title_margins_, margins_);
...

  margins_ = provider->GetInsetsMetric(INSETS_BUBBLE_CONTENTS);

(which for harmony is kHarmonyLayoutUnit on all sides and
kPanel{Horiz,Vert}Margin otherwise)


those margins encompass contents AND buttons.  So, the annoying bit is the
padding between the contents and the button row. BubbleDialogDelegateView sets
it to zero with a hardcoded constant here:
ClientView* BubbleDialogDelegateView::CreateClientView(Widget* widget) {
  DialogClientView* client = new DialogClientView(widget, GetContentsView());
  client->SetButtonRowInsets(gfx::Insets());
..



... see later comments.

+  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_);
+
+  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));
+  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));
-
-  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->AddView(parent_combobox_);
 
   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));
+      kFixed,
+      provider->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS).bottom());

[Peter Kasting, 2017/05/31 02:43:31]

Can we just set the dialog contents insets as insets for the whole gridlayout
when we create it?  Seems like this would eliminate the need for this, address
my question above about whether we need a top inset padding row, and answer the
question I didn't ask about the dialog side insets I don't see anywhere.

[tapted, 2017/05/31 04:27:48]

Yeah - this still confounds me :/

Here, we only need the bottom margin; to set the distance between the bottom of
the combobox and the top of the button row. The old code used
views::DISTANCE_RELATED_CONTROL_VERTICAL for this. I don't think that's correct.
I think DISTANCE_UNRELATED_CONTROL_VERTICAL makes the most sense for now.

BubbleDialogDelegateView::margins_ handles the rest, but that only provides the
padding below the buttons, not above.

I think what we really want is for BubbleDialogDelegateView to still provide a
top() inset (only) for DialogClientView::button_row_insets_, and all the dialogs
(like this one) that put in their own padding row to remove the handcrafted one.
DialogClientView already has the logic to remove all button_row_insets_ when
there are no buttons, but it only works for webmodal dialogs since, for bubbles,
those insets are always zero anyway.

So, I've started the path towards this. It's awkward since the DialogClientView
is created after Init(), and we don't want the dialog to relayout after
Widget::Init determines the initial size.


(There was a views::INSETS_DIALOG_CONTENTS.top() previously, but that was being
used for the padding *between* the textfield and combo controls, so definitely
not an inset at all).

 
   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