Cleanup BookmarkBubbleView, remove LocationBarBubbleDelegateView::GetDialogButtons()

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

Index: chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
index 94b7b428ac54438d88f901df8b64c066a9becd97..c29e3c84d7aec081b4ecc01a9c29b1a1e5d2445f 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
@@ -4,14 +4,9 @@
 
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h"
 
-#include <utility>
-
-#include "base/macros.h"
 #include "base/metrics/user_metrics.h"
-#include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -29,11 +24,9 @@
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/events/keycodes/keyboard_codes.h"
-#include "ui/views/bubble/bubble_frame_view.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/link.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/grid_layout.h"
@@ -60,6 +53,7 @@ class UnsizedCombobox : public views::Combobox {
   explicit UnsizedCombobox(ui::ComboboxModel* model) : views::Combobox(model) {}
   ~UnsizedCombobox() override {}
 
+  // views::Combobox:
   gfx::Size GetPreferredSize() const override {
     return gfx::Size(0, views::Combobox::GetPreferredSize().height());
   }
@@ -70,7 +64,7 @@ class UnsizedCombobox : public views::Combobox {
 
 }  // namespace
 
-BookmarkBubbleView* BookmarkBubbleView::bookmark_bubble_ = NULL;
+BookmarkBubbleView* BookmarkBubbleView::bookmark_bubble_ = nullptr;
 
 // static
 views::Widget* BookmarkBubbleView::ShowBubble(
@@ -100,7 +94,7 @@ views::Widget* BookmarkBubbleView::ShowBubble(
       views::BubbleDialogDelegateView::CreateBubble(bookmark_bubble_);
   bubble_widget->Show();
   // Select the entire title textfield contents when the bubble is first shown.
-  bookmark_bubble_->title_tf_->SelectAll(true);
+  bookmark_bubble_->name_field_->SelectAll(true);
   bookmark_bubble_->SetArrowPaintType(views::BubbleBorder::PAINT_TRANSPARENT);
 
   if (bookmark_bubble_->observer_) {
@@ -111,6 +105,7 @@ views::Widget* BookmarkBubbleView::ShowBubble(
   return bubble_widget;
 }
 
+// static
 void BookmarkBubbleView::Hide() {
   if (bookmark_bubble_)
     bookmark_bubble_->GetWidget()->Close();
@@ -130,10 +125,17 @@ BookmarkBubbleView::~BookmarkBubbleView() {
   delete parent_combobox_;
 }
 
+// ui::DialogModel -------------------------------------------------------------
+
+int BookmarkBubbleView::GetDialogButtons() const {
+  // TODO(tapted): DialogClientView should manage the buttons.
+  return ui::DIALOG_BUTTON_NONE;
+}
+
 // views::WidgetDelegate -------------------------------------------------------
 
 views::View* BookmarkBubbleView::GetInitiallyFocusedView() {
-  return title_tf_;
+  return name_field_;
 }
 
 base::string16 BookmarkBubbleView::GetWindowTitle() const {
@@ -206,7 +208,7 @@ bool BookmarkBubbleView::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   ui::KeyboardCode key_code = accelerator.key_code();
   if (key_code == ui::VKEY_RETURN) {
-    HandleButtonPressed(close_button_);
+    HandleButtonPressed(save_button_);
     return true;
   }
   if (key_code == ui::VKEY_E && accelerator.IsAltDown()) {
@@ -262,9 +264,9 @@ void BookmarkBubbleView::Init() {
   edit_button_ = views::MdTextButton::CreateSecondaryUiButton(
       this, l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_OPTIONS));
 
-  close_button_ = views::MdTextButton::CreateSecondaryUiButton(
+  save_button_ = views::MdTextButton::CreateSecondaryUiButton(
       this, l10n_util::GetStringUTF16(IDS_DONE));
-  close_button_->SetIsDefault(true);
+  save_button_->SetIsDefault(true);
 
   views::Label* combobox_label = new views::Label(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_FOLDER_TEXT));
@@ -275,9 +277,9 @@ void BookmarkBubbleView::Init() {
       l10n_util::GetStringUTF16(IDS_BOOKMARK_AX_BUBBLE_FOLDER_TEXT));
 
   SetLayoutManager(new views::FillLayout);
-  bookmark_details_view_ = base::MakeUnique<View>();
-  GridLayout* layout = new GridLayout(bookmark_details_view_.get());
-  bookmark_details_view_->SetLayoutManager(layout);
+  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.
@@ -290,8 +292,8 @@ void BookmarkBubbleView::Init() {
   cs->AddPaddingColumn(
       0, provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL));
 
-  cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 0,
-                GridLayout::USE_PREF, 0, 0);
+  cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 0, GridLayout::USE_PREF,
+                0, 0);
   cs->AddPaddingColumn(1, provider->GetDistanceMetric(
                               DISTANCE_UNRELATED_CONTROL_HORIZONTAL_LARGE));
 
@@ -306,12 +308,12 @@ void BookmarkBubbleView::Init() {
   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());
-  title_tf_->SetAccessibleName(
+  name_field_ = new views::Textfield();
+  name_field_->SetText(GetBookmarkName());
+  name_field_->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_AX_BUBBLE_TITLE_TEXT));
 
-  layout->AddView(title_tf_, 5, 1);
+  layout->AddView(name_field_, 5, 1);
 
   layout->AddPaddingRow(
       0, provider->GetInsetsMetric(views::INSETS_DIALOG_CONTENTS).top());
@@ -327,13 +329,13 @@ void BookmarkBubbleView::Init() {
   layout->SkipColumns(2);
   layout->AddView(remove_button_);
   layout->AddView(edit_button_);
-  layout->AddView(close_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));
 
-  AddChildView(bookmark_details_view_.get());
+  AddChildView(bookmark_contents_view_);

[Peter Kasting, 2017/05/27 00:28:45]

Wow, did the old code double-free?  I don't see a set_owned_by_parent() call
anywhere.

[tapted, 2017/05/27 01:09:39]

I don't think so, but it's really subtle. Since the unique_ptr was a member of a
DialogDelegateView, instead of being deleted by View::DoRemoveChildView(), it
gets deleted by DialogDelegateView::DeleteDelegate(). This means the unique_ptr
destructor called at the end of ~BookmarkBubbleView() triggers a separate
removal of the contents, and by the time unwinding gets to the parent class,
~View() destructor, the child view will be removed.

So this isn't to fix a bug, but just to use more conventional patterns for
manipulating Views.

(effectively, it was leaning on the set_owned_by_parent() call invoked by the
DialogDelegateView constructor)

 }
 
 // Private methods -------------------------------------------------------------
@@ -356,8 +358,8 @@ BookmarkBubbleView::BookmarkBubbleView(
                         ->GetMostRecentlyAddedUserNodeForURL(url)),
       remove_button_(nullptr),
       edit_button_(nullptr),
-      close_button_(nullptr),
-      title_tf_(nullptr),
+      save_button_(nullptr),
+      name_field_(nullptr),
       parent_combobox_(nullptr),
       ios_promo_view_(nullptr),
       footnote_view_(nullptr),
@@ -367,7 +369,7 @@ BookmarkBubbleView::BookmarkBubbleView(
   chrome::RecordDialogCreation(chrome::DialogIdentifier::BOOKMARK);
 }
 
-base::string16 BookmarkBubbleView::GetTitle() {
+base::string16 BookmarkBubbleView::GetBookmarkName() {
   BookmarkModel* bookmark_model =
       BookmarkModelFactory::GetForBrowserContext(profile_);
   const BookmarkNode* node =
@@ -390,7 +392,7 @@ void BookmarkBubbleView::HandleButtonPressed(views::Button* sender) {
     base::RecordAction(UserMetricsAction("BookmarkBubble_Edit"));
     ShowEditor();
   } else {
-    DCHECK_EQ(close_button_, sender);
+    DCHECK_EQ(save_button_, sender);
 #if defined(OS_WIN)
     if (IsIOSPromotionEligible(
             desktop_ios_promotion::PromotionEntryPoint::BOOKMARKS_BUBBLE)) {
@@ -431,7 +433,7 @@ void BookmarkBubbleView::ApplyEdits() {
   BookmarkModel* model = BookmarkModelFactory::GetForBrowserContext(profile_);
   const BookmarkNode* node = model->GetMostRecentlyAddedUserNodeForURL(url_);
   if (node) {
-    const base::string16 new_title = title_tf_->text();
+    const base::string16 new_title = name_field_->text();
     if (new_title != node->GetTitle()) {
       model->SetTitle(node, new_title);
       base::RecordAction(
@@ -455,7 +457,8 @@ bool BookmarkBubbleView::IsIOSPromotionEligible(
 void BookmarkBubbleView::ShowIOSPromotion(
     desktop_ios_promotion::PromotionEntryPoint entry_point) {
   DCHECK(!is_showing_ios_promotion_);
-  RemoveChildView(bookmark_details_view_.get());
+  // Hide the contents, but don't delete. It's needed in the destructor.

[Peter Kasting, 2017/05/27 00:28:45]

Where?

[tapted, 2017/05/27 01:09:39]

updated comment

  // 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;