views: rework DialogDelegate button customization

There are a handful of dialogs that need to customize parts of the
DialogClientView or BubbleFrameView that DialogDelegate creates
internally.  Currently, that customization is achieved by overriding
DialogDelegate::UpdateButton(), which is called "whenever needed" by
DialogClientView. This change reworks that logic.

Specifically, this change:
1) Adds a virtual method DialogDelegate::OnDialogInitialized() which is
   called after the DialogDelegate's frame, widget, etc are all
   initialized but not yet shown. This will be the new customization
   point for DialogDelegate subclasses that need to customize these
   elements.
2) Adds a virtual method WidgetDelegate::OnWidgetInitialized() which is
   called after Widget::Init is complete. DialogDelegate uses this to
   call OnDialogInitialized at the proper point.
3) Adds some accessors to DialogDelegate to make getting at the buttons
   in the DialogClientView easier (and make users of DialogDelegate not
   need to know about DialogClientView, which should be an
   implementation detail).
4) Removes DialogDelegate::UpdateButton, which is now superfluous, and
   folds the body of the old base implementation into DialogClientView
   where it belongs.
5) Removes some of the GlobalErrorBubbleViewTest suite, which were
   "change detector" unit tests for the internals of DialogDelegate.

OnDialogInitialized is also a logical place to move some of the other
customizations of DialogDelegate that are currently done via other hooks
(e.g. CreateExtraView and friends); this will happen in a followup
change. This is why DialogDelegate has OnDialogInitialized rather than
encouraging subclasses to directly override
WidgetDelegate::OnWidgetInitialized: soon it will be necessary for
DialogDelegate to do its own setup in OnWidgetInitialized, and I want to
maintain the "contractless" behavior of OnDialogInitialized.

Bug: 1011446
Change-Id: I9a9a24aa077cd62fb219b8104267c5d6ab6edb40
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865410
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706754}

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

@@ -193,11 +193,10 @@ bool BookmarkBubbleView::Close() {
   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));
+void BookmarkBubbleView::OnDialogInitialized() {
+  views::Button* cancel = GetCancelButton();
+  if (cancel)
+    cancel->AddAccelerator(ui::Accelerator(ui::VKEY_R, ui::EF_ALT_DOWN));
 }
 
 // views::View -----------------------------------------------------------------

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

@@ -70,7 +70,7 @@ class BookmarkBubbleView : public LocationBarBubbleDelegateView,
   bool Cancel() override;
   bool Accept() override;
   bool Close() override;
-  void UpdateButton(views::LabelButton* button, ui::DialogButton type) override;
+  void OnDialogInitialized() override;
   const char* GetClassName() const override;
 
   // views::ButtonListener:

chrome/browser/ui/views/global_error_bubble_view.cc

@@ -151,21 +151,6 @@ void GlobalErrorBubbleView::Init() {
   set_close_on_deactivate(error_->ShouldCloseOnDeactivate());
 }
 
-void GlobalErrorBubbleView::UpdateButton(views::LabelButton* button,
-                                         ui::DialogButton type) {
-  if (error_) {
-    // UpdateButton can result in calls back in to GlobalErrorBubbleView,
-    // possibly accessing |error_|.
-    BubbleDialogDelegateView::UpdateButton(button, type);
-    if (type == ui::DIALOG_BUTTON_OK &&
-        error_->ShouldAddElevationIconToAcceptButton()) {
-      elevation_icon_setter_ = std::make_unique<ElevationIconSetter>(
-          button, base::BindOnce(&GlobalErrorBubbleView::SizeToContents,
-                                 base::Unretained(this)));
-    }
-  }
-}
-
 bool GlobalErrorBubbleView::ShouldShowCloseButton() const {
   return error_ && error_->ShouldShowCloseButton();
 }
@@ -198,6 +183,15 @@ std::unique_ptr<views::View> GlobalErrorBubbleView::CreateExtraView() {
   return view;
 }
 
+void GlobalErrorBubbleView::OnDialogInitialized() {
+  views::LabelButton* ok_button = GetOkButton();
+  if (ok_button && error_ && error_->ShouldAddElevationIconToAcceptButton()) {
+    elevation_icon_setter_ = std::make_unique<ElevationIconSetter>(
+        ok_button, base::BindOnce(&GlobalErrorBubbleView::SizeToContents,
+                                  base::Unretained(this)));
+  }
+}
+
 bool GlobalErrorBubbleView::Cancel() {
   if (error_)
     error_->BubbleViewCancelButtonPressed(browser_);

chrome/browser/ui/views/global_error_bubble_view.h

@@ -36,10 +36,10 @@ class GlobalErrorBubbleView : public views::BubbleDialogDelegateView,
   // views::BubbleDialogDelegateView implementation.
   void Init() override;
   bool ShouldShowCloseButton() const override;
-  void UpdateButton(views::LabelButton* button, ui::DialogButton type) override;
   base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
   int GetDialogButtons() const override;
   std::unique_ptr<views::View> CreateExtraView() override;
+  void OnDialogInitialized() override;
   bool Cancel() override;
   bool Accept() override;
   bool Close() override;

chrome/browser/ui/views/global_error_bubble_view_unittest.cc

@@ -106,13 +106,6 @@ TEST_F(GlobalErrorBubbleViewTest, Basic) {
               BubbleViewDidClose(nullptr));
   view_->WindowClosing();
 
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_,
-              GetBubbleViewAcceptButtonLabel());
-  EXPECT_CALL(*mock_global_error_with_standard_bubble_,
-              ShouldAddElevationIconToAcceptButton())
-      .WillOnce(Return(false));
-  view_->UpdateButton(&button_, ui::DIALOG_BUTTON_OK);
-
   EXPECT_CALL(*mock_global_error_with_standard_bubble_,
               ShouldShowCloseButton());
   view_->ShouldShowCloseButton();
@@ -142,7 +135,6 @@ TEST_F(GlobalErrorBubbleViewTest, ErrorIsNull) {
   view_->GetWindowTitle();
   view_->WindowClosing();
 
-  view_->UpdateButton(&button_, ui::DIALOG_BUTTON_OK);
   view_->ShouldShowCloseButton();
 
   EXPECT_EQ(base::string16(),

ui/views/widget/widget.cc

@@ -399,6 +399,9 @@ void Widget::Init(InitParams params) {
 #endif
   native_widget_initialized_ = true;
   native_widget_->OnWidgetInitDone();
+
+  if (delegate)
+    delegate->OnWidgetInitialized();
 }
 
 void Widget::ShowEmojiPanel() {

ui/views/widget/widget_delegate.h

@@ -44,6 +44,9 @@ class VIEWS_EXPORT WidgetDelegate {
   // menu bars, etc.) changes in size.
   virtual void OnWorkAreaChanged();
 
+  // Called when the widget's initialization is complete.
+  virtual void OnWidgetInitialized() {}
+
   // Called when the window has been requested to close, after all other checks
   // have run. Returns whether the window should be allowed to close (default is
   // true).

ui/views/window/dialog_client_view.cc

@@ -286,32 +286,33 @@ void DialogClientView::UpdateDialogButton(LabelButton** member,
     return;
   }
 
-  if (!*member) {
-    // In theory, this should only need to assign a newly constructed Button to
-    // |*member|. DialogDelegate::UpdateButton(), and any overrides of that,
-    // should be responsible for the rest. TODO(tapted): When there is only
-    // MdTextButton, make it so. Note that some overrides may not always update
-    // the title (they should). See http://crbug.com/697303 .
-    const base::string16 title = delegate->GetDialogButtonLabel(type);
-    std::unique_ptr<LabelButton> button;
-
-    const bool is_default = delegate->GetDefaultDialogButton() == type &&
-                            (type != ui::DIALOG_BUTTON_CANCEL ||
-                             PlatformStyle::kDialogDefaultButtonCanBeCancel);
+  const bool is_default = delegate->GetDefaultDialogButton() == type &&
+                          (type != ui::DIALOG_BUTTON_CANCEL ||
+                           PlatformStyle::kDialogDefaultButtonCanBeCancel);
+  const base::string16 title = delegate->GetDialogButtonLabel(type);
+
+  if (*member) {
+    LabelButton* button = *member;
+    button->SetEnabled(delegate->IsDialogButtonEnabled(type));
+    button->SetIsDefault(is_default);
+    button->SetText(title);
+    return;
+  }
 
-    button = is_default ? MdTextButton::CreateSecondaryUiBlueButton(this, title)
-                        : MdTextButton::CreateSecondaryUiButton(this, title);
+  std::unique_ptr<LabelButton> button =
+      is_default ? MdTextButton::CreateSecondaryUiBlueButton(this, title)
+                 : MdTextButton::CreateSecondaryUiButton(this, title);
 
-    const int minimum_width = LayoutProvider::Get()->GetDistanceMetric(
-        views::DISTANCE_DIALOG_BUTTON_MINIMUM_WIDTH);
-    button->SetMinSize(gfx::Size(minimum_width, 0));
+  button->SetIsDefault(is_default);
+  button->SetEnabled(delegate->IsDialogButtonEnabled(type));
 
-    button->SetGroup(kButtonGroup);
+  const int minimum_width = LayoutProvider::Get()->GetDistanceMetric(
+      views::DISTANCE_DIALOG_BUTTON_MINIMUM_WIDTH);
+  button->SetMinSize(gfx::Size(minimum_width, 0));
 
-    *member = button_row_container_->AddChildView(std::move(button));
-  }
+  button->SetGroup(kButtonGroup);
 
-  delegate->UpdateButton(*member, type);
+  *member = button_row_container_->AddChildView(std::move(button));
 }
 
 int DialogClientView::GetExtraViewSpacing() const {

ui/views/window/dialog_delegate.cc

@@ -164,17 +164,6 @@ bool DialogDelegate::Close() {
   return Accept();
 }
 
-void DialogDelegate::UpdateButton(LabelButton* button, ui::DialogButton type) {
-  button->SetText(GetDialogButtonLabel(type));
-  button->SetEnabled(IsDialogButtonEnabled(type));
-  bool is_default = type == GetDefaultDialogButton();
-  if (!PlatformStyle::kDialogDefaultButtonCanBeCancel &&
-      type == ui::DIALOG_BUTTON_CANCEL) {
-    is_default = false;
-  }
-  button->SetIsDefault(is_default);
-}
-
 View* DialogDelegate::GetInitiallyFocusedView() {
   // Focus the default button if any.
   const DialogClientView* dcv = GetDialogClientView();
@@ -248,6 +237,18 @@ DialogClientView* DialogDelegate::GetDialogClientView() {
   return GetWidget()->client_view()->AsDialogClientView();
 }
 
+views::LabelButton* DialogDelegate::GetOkButton() {
+  DCHECK(GetWidget()) << "Don't call this before OnDialogInitialized";
+  auto* client = GetDialogClientView();
+  return client ? client->ok_button() : nullptr;
+}
+
+views::LabelButton* DialogDelegate::GetCancelButton() {
+  DCHECK(GetWidget()) << "Don't call this before OnDialogInitialized";
+  auto* client = GetDialogClientView();
+  return client ? client->cancel_button() : nullptr;
+}
+
 void DialogDelegate::AddObserver(DialogObserver* observer) {
   observer_list_.AddObserver(observer);
 }
@@ -270,6 +271,10 @@ ax::mojom::Role DialogDelegate::GetAccessibleWindowRole() {
   return ax::mojom::Role::kDialog;
 }
 
+void DialogDelegate::OnWidgetInitialized() {
+  OnDialogInitialized();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // DialogDelegateView:
 

ui/views/window/dialog_delegate.h

@@ -73,6 +73,16 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
                                                       gfx::NativeView parent,
                                                       const gfx::Rect& bounds);
 
+  // Called when the DialogDelegate and its frame have finished initializing but
+  // not been shown yet. Override this to perform customizations to the dialog
+  // that need to happen after the dialog's widget, border, buttons, and so on
+  // are ready.
+  //
+  // Overrides of this method should be quite rare - prefer to do dialog
+  // customization before the frame/widget/etc are ready if at all possible, via
+  // other setters on this class.
+  virtual void OnDialogInitialized() {}
+
   // Returns a mask specifying which of the available DialogButtons are visible
   // for the dialog. Note: Dialogs with just an OK button are frowned upon.
   virtual int GetDialogButtons() const;
@@ -120,11 +130,6 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   // must remain open.
   virtual bool Close();
 
-  // Updates the properties and appearance of |button| which has been created
-  // for type |type|. Override to do special initialization above and beyond
-  // the typical.
-  virtual void UpdateButton(LabelButton* button, ui::DialogButton type);
-
   // Overridden from WidgetDelegate:
   View* GetInitiallyFocusedView() override;
   DialogDelegate* AsDialogDelegate() override;
@@ -141,6 +146,11 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   const DialogClientView* GetDialogClientView() const;
   DialogClientView* GetDialogClientView();
 
+  // Helpers for accessing parts of the DialogClientView without needing to know
+  // about DialogClientView. Do not call these before OnDialogInitialized.
+  views::LabelButton* GetOkButton();
+  views::LabelButton* GetCancelButton();
+
   // Add or remove an observer notified by calls to DialogModelChanged().
   void AddObserver(DialogObserver* observer);
   void RemoveObserver(DialogObserver* observer);
@@ -168,6 +178,10 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   const Params& GetParams() const { return params_; }
 
  private:
+  // Overridden from WidgetDelegate. If you need to hook after widget
+  // initialization, use OnDialogInitialized above.
+  void OnWidgetInitialized() final;
+
   // The margins between the content and the inside of the border.
   // TODO(crbug.com/733040): Most subclasses assume they must set their own
   // margins explicitly, so we set them to 0 here for now to avoid doubled