Replace AddDialogButton with Add{Ok,Cancel}Button

This is more readable to me (after using it twice) and you don't need to
remember how to spell ui::DIALOG_BUTTON_CANCEL.

It also removes Set{Accept,Cancel}Callback by making these callbacks
part of Add{Ok,Cancel}Button().

Bug: 1106422
Change-Id: I208b80c041616ac506e4b77913a7bb6150c200dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2351370
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797479}

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

@@ -180,18 +180,15 @@ void BookmarkBubbleView::ShowBubble(
           .SetTitle(l10n_util::GetStringUTF16(
               already_bookmarked ? IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARK
                                  : IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARKED))
-          .SetAcceptCallback(base::BindOnce(&BookmarkBubbleDelegate::ApplyEdits,
-                                            base::Unretained(bubble_delegate)))
-          .SetCancelCallback(
-              base::BindOnce(&BookmarkBubbleDelegate::RemoveBookmark,
-                             base::Unretained(bubble_delegate)))
           .SetWindowClosingCallback(
               base::BindOnce(&BookmarkBubbleDelegate::OnWindowClosing,
                              base::Unretained(bubble_delegate)))
-          .AddDialogButton(ui::DIALOG_BUTTON_OK,
-                           l10n_util::GetStringUTF16(IDS_DONE))
-          .AddDialogButton(
-              ui::DIALOG_BUTTON_CANCEL,
+          .AddOkButton(base::BindOnce(&BookmarkBubbleDelegate::ApplyEdits,
+                                      base::Unretained(bubble_delegate)),
+                       l10n_util::GetStringUTF16(IDS_DONE))
+          .AddCancelButton(
+              base::BindOnce(&BookmarkBubbleDelegate::RemoveBookmark,
+                             base::Unretained(bubble_delegate)),
               l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK),
               ui::DialogModelButton::Params().AddAccelerator(
                   ui::Accelerator(ui::VKEY_R, ui::EF_ALT_DOWN)))

ui/base/models/dialog_model.cc

@@ -29,18 +29,6 @@ DialogModel::Builder& DialogModel::Builder::SetTitle(base::string16 title) {
   return *this;
 }
 
-DialogModel::Builder& DialogModel::Builder::SetAcceptCallback(
-    base::OnceClosure callback) {
-  model_->accept_callback_ = std::move(callback);
-  return *this;
-}
-
-DialogModel::Builder& DialogModel::Builder::SetCancelCallback(
-    base::OnceClosure callback) {
-  model_->cancel_callback_ = std::move(callback);
-  return *this;
-}
-
 DialogModel::Builder& DialogModel::Builder::SetCloseCallback(
     base::OnceClosure callback) {
   model_->close_callback_ = std::move(callback);
@@ -53,11 +41,25 @@ DialogModel::Builder& DialogModel::Builder::SetWindowClosingCallback(
   return *this;
 }
 
-DialogModel::Builder& DialogModel::Builder::AddDialogButton(
-    DialogButton button,
+DialogModel::Builder& DialogModel::Builder::AddOkButton(
+    base::OnceClosure callback,
+    base::string16 label,
+    const DialogModelButton::Params& params) {
+  DCHECK(!params.has_callback()) << "Use |callback| only.";
+  DCHECK(!model_->accept_callback_);
+  model_->accept_callback_ = std::move(callback);
+  model_->AddDialogButton(ui::DIALOG_BUTTON_OK, std::move(label), params);
+  return *this;
+}
+
+DialogModel::Builder& DialogModel::Builder::AddCancelButton(
+    base::OnceClosure callback,
     base::string16 label,
     const DialogModelButton::Params& params) {
-  model_->AddDialogButton(button, std::move(label), params);
+  DCHECK(!params.has_callback()) << "Use |callback| only.";
+  DCHECK(!model_->cancel_callback_);
+  model_->cancel_callback_ = std::move(callback);
+  model_->AddDialogButton(ui::DIALOG_BUTTON_CANCEL, std::move(label), params);
   return *this;
 }
 

ui/base/models/dialog_model.h

@@ -64,14 +64,11 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelDelegate {
 // auto dialog_model =
 //     ui::DialogModel::Builder(std::move(model_delegate))
 //         .SetTitle(base::ASCIIToUTF16("Hello, world!"))
-//         .AddDialogButton(ui::DIALOG_BUTTON_OK,
-//                          l10n_util::GetStringUTF16(IDS_OK))
+//         .AddOkButton(base::BindOnce(&Delegate::OnDialogAccepted,
+//                                     base::Unretained(model_delegate_ptr)))
 //         .AddTextfield(
 //             base::ASCIIToUTF16("Name"), base::string16(),
 //             ui::DialogModelTextfield::Params().SetUniqueId(kNameTextfield))
-//         .SetAcceptCallback(
-//             base::BindOnce(&Delegate::OnDialogAccepted,
-//                            base::Unretained(model_delegate_ptr)))
 //         .Build();
 //
 // // DialogModelBase::Host specific. In this example, uses views-specific
@@ -96,12 +93,8 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
     Builder& SetShowCloseButton(bool show_close_button);
     Builder& SetTitle(base::string16 title);
 
-    // Called when the dialog is accepted, before it closes.
-    Builder& SetAcceptCallback(base::OnceClosure callback);
-    // Called when the dialog is cancelled.
-    Builder& SetCancelCallback(base::OnceClosure callback);
-    // Called when the dialog is explicitly closed (for instance, close-x). Not
-    // called during accept/cancel.
+    // Called when the dialog is explicitly closed (Esc, close-x). Not called
+    // during accept/cancel.
     Builder& SetCloseCallback(base::OnceClosure callback);
 
     // TODO(pbos): Clarify and enforce (through tests) that this is called after
@@ -110,12 +103,19 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
     // {accept,cancel,close} callbacks.
     Builder& SetWindowClosingCallback(base::OnceClosure callback);
 
-    // Adds a dialog button (ok, cancel) to the dialog. Note that the callbacks
-    // for these button actions should be set using SetAcceptCallback() and
-    // SetCancelCallback().
-    Builder& AddDialogButton(
-        DialogButton button,
-        base::string16 label,
+    // Adds a dialog button (ok, cancel) to the dialog. The |callback| is called
+    // when the dialog is accepted or cancelled, before it closes. Use
+    // base::DoNothing() as callback if you want nothing extra to happen as a
+    // result, besides the dialog closing.
+    // If no |label| is provided, default strings are chosen by the
+    // DialogModelHost implementation.
+    Builder& AddOkButton(
+        base::OnceClosure callback,
+        base::string16 label = base::string16(),
+        const DialogModelButton::Params& params = DialogModelButton::Params());
+    Builder& AddCancelButton(
+        base::OnceClosure callback,
+        base::string16 label = base::string16(),
         const DialogModelButton::Params& params = DialogModelButton::Params());
 
     // Use of the extra button in new dialogs are discouraged. If this is deemed

ui/views/bubble/bubble_dialog_model_host.cc

@@ -75,9 +75,11 @@ BubbleDialogModelHost::BubbleDialogModelHost(
 
     button_mask |= field->model_field_id(GetPassKey());
     SetButtons(button_mask);
-    SetButtonLabel(
-        static_cast<ui::DialogButton>(field->model_field_id(GetPassKey())),
-        button->label());
+    if (!button->label().empty()) {
+      SetButtonLabel(
+          static_cast<ui::DialogButton>(field->model_field_id(GetPassKey())),
+          button->label());
+    }
   }
 
   // Populate dialog using the observer functions to make sure they use the same