Call back directly from DialogModelHost to fields

Replaces methods like DialogModel::OnButtonPressed() with
DialogModelButton::OnPressed(), so that those calls from DialogModelHost
don't have to go through DialogModel.

This also makes the callback for DialogModelButton mandatory and removes
it from Params. The dialog buttons OK and Cancel are the exceptions that
don't need one and use a NOTREACHED() lambda.

The change also adds util::PassKey<DialogModelHost> to additional
DialogModelField methods to narrow the public API from methods that are
unnecessary for public consumption.

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

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

@@ -193,12 +193,11 @@ void BookmarkBubbleView::ShowBubble(
               ui::DialogModelButton::Params().AddAccelerator(
                   ui::Accelerator(ui::VKEY_R, ui::EF_ALT_DOWN)))
           .AddDialogExtraButton(
+              base::BindRepeating(&BookmarkBubbleDelegate::OnEditButton,
+                                  base::Unretained(bubble_delegate)),
               l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_OPTIONS),
-              ui::DialogModelButton::Params()
-                  .SetCallback(
-                      base::BindRepeating(&BookmarkBubbleDelegate::OnEditButton,
-                                          base::Unretained(bubble_delegate)))
-                  .AddAccelerator(ui::Accelerator(ui::VKEY_E, ui::EF_ALT_DOWN)))
+              ui::DialogModelButton::Params().AddAccelerator(
+                  ui::Accelerator(ui::VKEY_E, ui::EF_ALT_DOWN)))
           .AddTextfield(
               l10n_util::GetStringUTF16(IDS_BOOKMARK_BUBBLE_NAME_LABEL),
               bookmark_node->GetTitle(),

ui/base/models/dialog_model.cc

@@ -4,6 +4,8 @@
 
 #include "ui/base/models/dialog_model.h"
 
+#include "base/bind_helpers.h"
+
 namespace ui {
 
 DialogModel::Builder::Builder(std::unique_ptr<DialogModelDelegate> delegate)
@@ -45,10 +47,15 @@ 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);
+  // NOTREACHED() is used below to make sure this callback isn't used.
+  // DialogModelHost should be using OnDialogAccepted() instead.
+  model_->ok_button_.emplace(
+      model_->GetPassKey(), model_.get(),
+      base::BindRepeating([](const Event&) { NOTREACHED(); }), std::move(label),
+      params);
+
   return *this;
 }
 
@@ -56,17 +63,24 @@ DialogModel::Builder& DialogModel::Builder::AddCancelButton(
     base::OnceClosure callback,
     base::string16 label,
     const DialogModelButton::Params& 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);
+  // NOTREACHED() is used below to make sure this callback isn't used.
+  // DialogModelHost should be using OnDialogCanceled() instead.
+  model_->cancel_button_.emplace(
+      model_->GetPassKey(), model_.get(),
+      base::BindRepeating([](const Event&) { NOTREACHED(); }), std::move(label),
+      params);
+
   return *this;
 }
 
 DialogModel::Builder& DialogModel::Builder::AddDialogExtraButton(
+    base::RepeatingCallback<void(const Event&)> callback,
     base::string16 label,
     const DialogModelButton::Params& params) {
-  model_->AddDialogButton(kExtraButtonId, std::move(label), params);
+  model_->extra_button_.emplace(model_->GetPassKey(), model_.get(),
+                                std::move(callback), std::move(label), params);
   return *this;
 }
 
@@ -150,14 +164,6 @@ DialogModelTextfield* DialogModel::GetTextfieldByUniqueId(int unique_id) {
   return static_cast<DialogModelTextfield*>(field);
 }
 
-void DialogModel::OnButtonPressed(util::PassKey<DialogModelHost>,
-                                  DialogModelButton* button,
-                                  const Event& event) {
-  DCHECK_EQ(button->model_, this);
-  if (button->callback_)
-    button->callback_.Run(event);
-}
-
 void DialogModel::OnDialogAccepted(util::PassKey<DialogModelHost>) {
   if (accept_callback_)
     std::move(accept_callback_).Run();
@@ -173,52 +179,9 @@ void DialogModel::OnDialogClosed(util::PassKey<DialogModelHost>) {
     std::move(close_callback_).Run();
 }
 
-void DialogModel::OnComboboxSelectedIndexChanged(util::PassKey<DialogModelHost>,
-                                                 DialogModelCombobox* combobox,
-                                                 int index) {
-  DCHECK_EQ(combobox->model_, this);
-  combobox->selected_index_ = index;
-}
-
-void DialogModel::OnComboboxPerformAction(util::PassKey<DialogModelHost>,
-                                          DialogModelCombobox* combobox) {
-  DCHECK_EQ(combobox->model_, this);
-  if (combobox->callback_)
-    combobox->callback_.Run();
-}
-
-void DialogModel::OnTextfieldTextChanged(util::PassKey<DialogModelHost>,
-                                         DialogModelTextfield* textfield,
-                                         base::string16 text) {
-  DCHECK_EQ(textfield->model_, this);
-  textfield->text_ = std::move(text);
-}
-
 void DialogModel::OnWindowClosing(util::PassKey<DialogModelHost>) {
   if (window_closing_callback_)
     std::move(window_closing_callback_).Run();
 }
 
-void DialogModel::AddDialogButton(int button_id,
-                                  base::string16 label,
-                                  const DialogModelButton::Params& params) {
-  DCHECK(!host_);  // Dialog buttons should be added before adding to host.
-  base::Optional<DialogModelButton>* button = nullptr;
-  switch (button_id) {
-    case ui::DIALOG_BUTTON_OK:
-      button = &ok_button_;
-      break;
-    case ui::DIALOG_BUTTON_CANCEL:
-      button = &cancel_button_;
-      break;
-    case kExtraButtonId:
-      button = &extra_button_;
-      break;
-    default:
-      NOTREACHED();
-  }
-  DCHECK(!button->has_value());
-  button->emplace(GetPassKey(), this, std::move(label), params);
-}
-
 }  // namespace ui
\ No newline at end of file

ui/base/models/dialog_model.h

@@ -121,8 +121,10 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
     // Use of the extra button in new dialogs are discouraged. If this is deemed
     // necessary please double-check with UX before adding any new dialogs with
     // them.
-    Builder& AddDialogExtraButton(base::string16 label,
-                                  const DialogModelButton::Params& params);
+    Builder& AddDialogExtraButton(
+        base::RepeatingCallback<void(const Event&)> callback,
+        base::string16 label,
+        const DialogModelButton::Params& params);
 
     // Adds a textfield. See DialogModel::AddTextfield().
     Builder& AddTextfield(base::string16 label,
@@ -167,22 +169,11 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
   DialogModelCombobox* GetComboboxByUniqueId(int unique_id);
   DialogModelTextfield* GetTextfieldByUniqueId(int unique_id);
 
-  // Methods with util::PassKey<DialogModelHost> are for host implementations
-  // only.
-  void OnButtonPressed(util::PassKey<DialogModelHost>,
-                       DialogModelButton* field,
-                       const Event& event);
+  // Methods with util::PassKey<DialogModelHost> are only intended to be called
+  // by the DialogModelHost implementation.
   void OnDialogAccepted(util::PassKey<DialogModelHost>);
   void OnDialogCancelled(util::PassKey<DialogModelHost>);
   void OnDialogClosed(util::PassKey<DialogModelHost>);
-  void OnComboboxPerformAction(util::PassKey<DialogModelHost>,
-                               DialogModelCombobox* combobox);
-  void OnComboboxSelectedIndexChanged(util::PassKey<DialogModelHost>,
-                                      DialogModelCombobox* combobox,
-                                      int index);
-  void OnTextfieldTextChanged(util::PassKey<DialogModelHost>,
-                              DialogModelTextfield* textfield,
-                              base::string16 text);
   void OnWindowClosing(util::PassKey<DialogModelHost>);
 
   // Called when added to a DialogModelHost.
@@ -228,10 +219,6 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
     return util::PassKey<DialogModel>();
   }
 
-  void AddDialogButton(int button,
-                       base::string16 label,
-                       const DialogModelButton::Params& params);
-
   std::unique_ptr<DialogModelDelegate> delegate_;
   DialogModelHost* host_ = nullptr;
 

ui/base/models/dialog_model_field.cc

@@ -31,26 +31,29 @@ DialogModelButton::Params& DialogModelButton::Params::AddAccelerator(
   return *this;
 }
 
-DialogModelButton::Params& DialogModelButton::Params::SetCallback(
-    base::RepeatingCallback<void(const Event&)> callback) {
-  callback_ = std::move(callback);
-  return *this;
-}
-
-DialogModelButton::DialogModelButton(util::PassKey<DialogModel> pass_key,
-                                     DialogModel* model,
-                                     base::string16 label,
-                                     const DialogModelButton::Params& params)
+DialogModelButton::DialogModelButton(
+    util::PassKey<DialogModel> pass_key,
+    DialogModel* model,
+    base::RepeatingCallback<void(const Event&)> callback,
+    base::string16 label,
+    const DialogModelButton::Params& params)
     : DialogModelField(pass_key,
                        model,
                        kButton,
                        params.unique_id_,
                        params.accelerators_),
       label_(std::move(label)),
-      callback_(params.callback_) {}
+      callback_(std::move(callback)) {
+  DCHECK(callback_);
+}
 
 DialogModelButton::~DialogModelButton() = default;
 
+void DialogModelButton::OnPressed(util::PassKey<DialogModelHost>,
+                                  const Event& event) {
+  callback_.Run(event);
+}
+
 DialogModelCombobox::Params::Params() = default;
 DialogModelCombobox::Params::~Params() = default;
 
@@ -98,6 +101,16 @@ DialogModelCombobox::DialogModelCombobox(
 
 DialogModelCombobox::~DialogModelCombobox() = default;
 
+void DialogModelCombobox::OnSelectedIndexChanged(util::PassKey<DialogModelHost>,
+                                                 int selected_index) {
+  selected_index_ = selected_index;
+}
+
+void DialogModelCombobox::OnPerformAction(util::PassKey<DialogModelHost>) {
+  if (callback_)
+    callback_.Run();
+}
+
 DialogModelTextfield::Params::Params() = default;
 DialogModelTextfield::Params::~Params() = default;
 
@@ -137,4 +150,9 @@ DialogModelTextfield::DialogModelTextfield(
 
 DialogModelTextfield::~DialogModelTextfield() = default;
 
+void DialogModelTextfield::OnTextChanged(util::PassKey<DialogModelHost>,
+                                         base::string16 text) {
+  text_ = std::move(text);
+}
+
 }  // namespace ui
\ No newline at end of file

ui/base/models/dialog_model_field.h

@@ -34,7 +34,7 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelField {
   DialogModelField& operator=(const DialogModelField&) = delete;
   virtual ~DialogModelField();
 
-  // Accessors with util::PassKey<DialogModelHost> are only intended to be read
+  // Methods with util::PassKey<DialogModelHost> are only intended to be called
   // by the DialogModelHost implementation.
   Type type(util::PassKey<DialogModelHost>) const { return type_; }
   const base::flat_set<Accelerator>& accelerators(
@@ -73,21 +73,13 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelButton : public DialogModelField {
 
     Params& SetUniqueId(int unique_id);
 
-    // The button callback gets called when the button is activated. Whether
-    // that happens on key-press, release, etc. is implementation (and platform)
-    // dependent.
-    Params& SetCallback(base::RepeatingCallback<void(const Event&)> callback);
-
     Params& AddAccelerator(Accelerator accelerator);
     Params& SetAccessibleName(base::string16 accessible_name);
 
-    bool has_callback() const { return !!callback_; }
-
    private:
     friend class DialogModelButton;
 
     int unique_id_ = -1;
-    base::RepeatingCallback<void(const Event&)> callback_;
     base::flat_set<Accelerator> accelerators_;
   };
 
@@ -95,18 +87,27 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelButton : public DialogModelField {
   // fields.
   DialogModelButton(util::PassKey<DialogModel> pass_key,
                     DialogModel* model,
+                    base::RepeatingCallback<void(const Event&)> callback,
                     base::string16 label,
                     const Params& params);
   DialogModelButton(const DialogModelButton&) = delete;
   DialogModelButton& operator=(const DialogModelButton&) = delete;
   ~DialogModelButton() override;
 
-  const base::string16& label() const { return label_; }
+  // Methods with util::PassKey<DialogModelHost> are only intended to be called
+  // by the DialogModelHost implementation.
+  const base::string16& label(util::PassKey<DialogModelHost>) const {
+    return label_;
+  }
+  void OnPressed(util::PassKey<DialogModelHost>, const Event& event);
 
  private:
   friend class DialogModel;
 
   const base::string16 label_;
+  // The button callback gets called when the button is activated. Whether
+  // that happens on key-press, release, etc. is implementation (and platform)
+  // dependent.
   base::RepeatingCallback<void(const Event&)> callback_;
 };
 
@@ -157,11 +158,21 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelCombobox : public DialogModelField {
   DialogModelCombobox& operator=(const DialogModelCombobox&) = delete;
   ~DialogModelCombobox() override;
 
-  const base::string16& label() const { return label_; }
-  const base::string16& accessible_name() const { return accessible_name_; }
   int selected_index() const { return selected_index_; }
   ui::ComboboxModel* combobox_model() { return combobox_model_.get(); }
 
+  // Methods with util::PassKey<DialogModelHost> are only intended to be called
+  // by the DialogModelHost implementation.
+  const base::string16& label(util::PassKey<DialogModelHost>) const {
+    return label_;
+  }
+  const base::string16& accessible_name(util::PassKey<DialogModelHost>) const {
+    return accessible_name_;
+  }
+  void OnSelectedIndexChanged(util::PassKey<DialogModelHost>,
+                              int selected_index);
+  void OnPerformAction(util::PassKey<DialogModelHost>);
+
  private:
   friend class DialogModel;
 
@@ -210,10 +221,18 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelTextfield : public DialogModelField {
   DialogModelTextfield& operator=(const DialogModelTextfield&) = delete;
   ~DialogModelTextfield() override;
 
-  const base::string16& label() const { return label_; }
-  const base::string16& accessible_name() const { return accessible_name_; }
   const base::string16& text() const { return text_; }
 
+  // Methods with util::PassKey<DialogModelHost> are only intended to be called
+  // by the DialogModelHost implementation.
+  const base::string16& label(util::PassKey<DialogModelHost>) const {
+    return label_;
+  }
+  const base::string16& accessible_name(util::PassKey<DialogModelHost>) const {
+    return accessible_name_;
+  }
+  void OnTextChanged(util::PassKey<DialogModelHost>, base::string16 text);
+
  private:
   friend class DialogModel;
 

ui/views/bubble/bubble_dialog_model_host.cc

@@ -59,15 +59,16 @@ BubbleDialogModelHost::BubbleDialogModelHost(
   auto* ok_button = model_->ok_button(GetPassKey());
   if (ok_button) {
     button_mask |= ui::DIALOG_BUTTON_OK;
-    if (!ok_button->label().empty())
-      SetButtonLabel(ui::DIALOG_BUTTON_OK, ok_button->label());
+    if (!ok_button->label(GetPassKey()).empty())
+      SetButtonLabel(ui::DIALOG_BUTTON_OK, ok_button->label(GetPassKey()));
   }
 
   auto* cancel_button = model_->cancel_button(GetPassKey());
   if (cancel_button) {
     button_mask |= ui::DIALOG_BUTTON_CANCEL;
-    if (!cancel_button->label().empty())
-      SetButtonLabel(ui::DIALOG_BUTTON_CANCEL, cancel_button->label());
+    if (!cancel_button->label(GetPassKey()).empty())
+      SetButtonLabel(ui::DIALOG_BUTTON_CANCEL,
+                     cancel_button->label(GetPassKey()));
   }
 
   // TODO(pbos): Consider refactoring ::SetExtraView() so it can be called after
@@ -75,8 +76,8 @@ BubbleDialogModelHost::BubbleDialogModelHost(
   // OnDialogInitialized() will not work until then.
   auto* extra_button = model_->extra_button(GetPassKey());
   if (extra_button) {
-    OnViewCreatedForField(SetExtraView(std::make_unique<views::MdTextButton>(
-                              this, extra_button->label())),
+    OnViewCreatedForField(SetExtraView(std::make_unique<MdTextButton>(
+                              this, extra_button->label(GetPassKey()))),
                           extra_button);
   }
 
@@ -202,9 +203,9 @@ Textfield* BubbleDialogModelHost::AddOrUpdateTextfield(
   // TODO(pbos): Support updates to the existing model.
 
   auto textfield = std::make_unique<Textfield>();
-  textfield->SetAccessibleName(model->accessible_name().empty()
-                                   ? model->text()
-                                   : model->accessible_name());
+  textfield->SetAccessibleName(model->accessible_name(GetPassKey()).empty()
+                                   ? model->label(GetPassKey())
+                                   : model->accessible_name(GetPassKey()));
   textfield->SetText(model->text());
 
   property_changed_subscriptions_.push_back(textfield->AddTextChangedCallback(
@@ -212,7 +213,7 @@ Textfield* BubbleDialogModelHost::AddOrUpdateTextfield(
                           base::Unretained(this), textfield.get())));
 
   auto* textfield_ptr = textfield.get();
-  AddLabelAndField(model->label(), std::move(textfield),
+  AddLabelAndField(model->label(GetPassKey()), std::move(textfield),
                    textfield_ptr->GetFontList());
 
   return textfield_ptr;
@@ -223,14 +224,14 @@ Combobox* BubbleDialogModelHost::AddOrUpdateCombobox(
   // TODO(pbos): Handle updating existing field.
 
   auto combobox = std::make_unique<Combobox>(model->combobox_model());
-  combobox->SetAccessibleName(model->accessible_name().empty()
-                                  ? model->label()
-                                  : model->accessible_name());
+  combobox->SetAccessibleName(model->accessible_name(GetPassKey()).empty()
+                                  ? model->label(GetPassKey())
+                                  : model->accessible_name(GetPassKey()));
   combobox->set_listener(this);
   // TODO(pbos): Add subscription to combobox selected-index changes.
   combobox->SetSelectedIndex(model->selected_index());
   auto* combobox_ptr = combobox.get();
-  AddLabelAndField(model->label(), std::move(combobox),
+  AddLabelAndField(model->label(GetPassKey()), std::move(combobox),
                    combobox_ptr->GetFontList());
   return combobox_ptr;
 }
@@ -251,22 +252,19 @@ void BubbleDialogModelHost::AddLabelAndField(const base::string16& label_text,
 }
 
 void BubbleDialogModelHost::NotifyTextfieldTextChanged(Textfield* textfield) {
-  model_->OnTextfieldTextChanged(GetPassKey(),
-                                 FieldAsTextfield(view_to_field_[textfield]),
-                                 textfield->GetText());
+  FieldAsTextfield(view_to_field_[textfield])
+      ->OnTextChanged(GetPassKey(), textfield->GetText());
 }
 
 void BubbleDialogModelHost::NotifyComboboxSelectedIndexChanged(
     Combobox* combobox) {
-  model_->OnComboboxSelectedIndexChanged(
-      GetPassKey(), FieldAsCombobox(view_to_field_[combobox]),
-      combobox->GetSelectedIndex());
+  FieldAsCombobox(view_to_field_[combobox])
+      ->OnSelectedIndexChanged(GetPassKey(), combobox->GetSelectedIndex());
 }
 
 void BubbleDialogModelHost::ButtonPressed(Button* sender,
                                           const ui::Event& event) {
-  model_->OnButtonPressed(GetPassKey(), FieldAsButton(view_to_field_[sender]),
-                          event);
+  FieldAsButton(view_to_field_[sender])->OnPressed(GetPassKey(), event);
 }
 
 void BubbleDialogModelHost::OnPerformAction(Combobox* combobox) {
@@ -274,8 +272,7 @@ void BubbleDialogModelHost::OnPerformAction(Combobox* combobox) {
   // but Combobox right now doesn't support listening to selected-index changes.
   NotifyComboboxSelectedIndexChanged(combobox);
 
-  model_->OnComboboxPerformAction(GetPassKey(),
-                                  FieldAsCombobox(view_to_field_[combobox]));
+  FieldAsCombobox(view_to_field_[combobox])->OnPerformAction(GetPassKey());
 }
 
 void BubbleDialogModelHost::OnViewCreatedForField(View* view,