Remove model_field_id() from DialogModelField

The DialogModelField pointer is instead used to identify the field.

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

ui/base/models/dialog_model.cc

@@ -108,18 +108,18 @@ void DialogModel::AddTextfield(base::string16 label,
                                base::string16 text,
                                const DialogModelTextfield::Params& params) {
   fields_.push_back(std::make_unique<DialogModelTextfield>(
-      ReserveField(), std::move(label), std::move(text), params));
+      GetPassKey(), this, std::move(label), std::move(text), params));
   if (host_)
-    host_->OnModelChanged(this);
+    host_->OnFieldAdded(fields_.back().get());
 }
 
 void DialogModel::AddCombobox(base::string16 label,
                               std::unique_ptr<ui::ComboboxModel> combobox_model,
                               const DialogModelCombobox::Params& params) {
   fields_.push_back(std::make_unique<DialogModelCombobox>(
-      ReserveField(), std::move(label), std::move(combobox_model), params));
+      GetPassKey(), this, std::move(label), std::move(combobox_model), params));
   if (host_)
-    host_->OnModelChanged(this);
+    host_->OnFieldAdded(fields_.back().get());
 }
 
 DialogModelField* DialogModel::GetFieldByUniqueId(int unique_id) {
@@ -150,20 +150,10 @@ DialogModelTextfield* DialogModel::GetTextfieldByUniqueId(int unique_id) {
   return static_cast<DialogModelTextfield*>(field);
 }
 
-DialogModelButton* DialogModel::GetDialogButton(DialogButton button) {
-  return GetButtonFromModelFieldId(button);
-}
-
-DialogModelButton* DialogModel::GetExtraButton() {
-  return GetButtonFromModelFieldId(kExtraButtonId);
-}
-
 void DialogModel::OnButtonPressed(util::PassKey<DialogModelHost>,
-                                  int id,
+                                  DialogModelButton* button,
                                   const Event& event) {
-  DCHECK_GT(id, DIALOG_BUTTON_LAST);
-
-  auto* button = GetButtonFromModelFieldId(id);
+  DCHECK_EQ(button->model_, this);
   if (button->callback_)
     button->callback_.Run(event);
 }
@@ -184,22 +174,24 @@ void DialogModel::OnDialogClosed(util::PassKey<DialogModelHost>) {
 }
 
 void DialogModel::OnComboboxSelectedIndexChanged(util::PassKey<DialogModelHost>,
-                                                 int id,
+                                                 DialogModelCombobox* combobox,
                                                  int index) {
-  GetComboboxFromModelFieldId(id)->selected_index_ = index;
+  DCHECK_EQ(combobox->model_, this);
+  combobox->selected_index_ = index;
 }
 
 void DialogModel::OnComboboxPerformAction(util::PassKey<DialogModelHost>,
-                                          int id) {
-  auto* model = GetComboboxFromModelFieldId(id);
-  if (model->callback_)
-    model->callback_.Run();
+                                          DialogModelCombobox* combobox) {
+  DCHECK_EQ(combobox->model_, this);
+  if (combobox->callback_)
+    combobox->callback_.Run();
 }
 
 void DialogModel::OnTextfieldTextChanged(util::PassKey<DialogModelHost>,
-                                         int id,
+                                         DialogModelTextfield* textfield,
                                          base::string16 text) {
-  GetTextfieldFromModelFieldId(id)->text_ = text;
+  DCHECK_EQ(textfield->model_, this);
+  textfield->text_ = std::move(text);
 }
 
 void DialogModel::OnWindowClosing(util::PassKey<DialogModelHost>) {
@@ -207,52 +199,26 @@ void DialogModel::OnWindowClosing(util::PassKey<DialogModelHost>) {
     std::move(window_closing_callback_).Run();
 }
 
-void DialogModel::AddDialogButton(int button,
+void DialogModel::AddDialogButton(int button_id,
                                   base::string16 label,
                                   const DialogModelButton::Params& params) {
-  DCHECK_LE(button, kExtraButtonId);
-  if (button != kExtraButtonId)  // Dialog buttons should use dialog callbacks.
-    DCHECK(!params.has_callback());
   DCHECK(!host_);  // Dialog buttons should be added before adding to host.
-  DCHECK(!GetFieldFromModelFieldId(button));
-  fields_.push_back(std::make_unique<DialogModelButton>(
-      DialogModelField::Reservation(this, button), std::move(label), params));
-}
-
-DialogModelField* DialogModel::GetFieldFromModelFieldId(int id) {
-  for (const auto& field : fields_) {
-    if (id == field->model_field_id_)
-      return field.get();
+  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();
   }
-  return nullptr;
-}
-
-DialogModelButton* DialogModel::GetButtonFromModelFieldId(int id) {
-  auto* field = GetFieldFromModelFieldId(id);
-  DCHECK(field);
-  DCHECK_EQ(field->type_, DialogModelField::kButton);
-  return static_cast<DialogModelButton*>(field);
-}
-
-DialogModelCombobox* DialogModel::GetComboboxFromModelFieldId(int id) {
-  auto* field = GetFieldFromModelFieldId(id);
-  DCHECK(field);
-  DCHECK_EQ(field->type_, DialogModelField::kCombobox);
-  return static_cast<DialogModelCombobox*>(field);
-}
-
-DialogModelTextfield* DialogModel::GetTextfieldFromModelFieldId(int id) {
-  auto* field = GetFieldFromModelFieldId(id);
-  DCHECK(field);
-  DCHECK_EQ(field->type_, DialogModelField::kTextfield);
-  return static_cast<DialogModelTextfield*>(field);
-}
-
-DialogModelField::Reservation DialogModel::ReserveField() {
-  const int id = next_field_id_++;
-  DCHECK(!GetFieldFromModelFieldId(id));
-
-  return DialogModelField::Reservation(this, id);
+  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

@@ -167,24 +167,21 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
   DialogModelCombobox* GetComboboxByUniqueId(int unique_id);
   DialogModelTextfield* GetTextfieldByUniqueId(int unique_id);
 
-  // Get dialog buttons.
-  DialogModelButton* GetDialogButton(DialogButton button);
-  DialogModelButton* GetExtraButton();
-
   // Methods with util::PassKey<DialogModelHost> are for host implementations
   // only.
   void OnButtonPressed(util::PassKey<DialogModelHost>,
-                       int id,
+                       DialogModelButton* field,
                        const Event& event);
   void OnDialogAccepted(util::PassKey<DialogModelHost>);
   void OnDialogCancelled(util::PassKey<DialogModelHost>);
   void OnDialogClosed(util::PassKey<DialogModelHost>);
-  void OnComboboxPerformAction(util::PassKey<DialogModelHost>, int id);
+  void OnComboboxPerformAction(util::PassKey<DialogModelHost>,
+                               DialogModelCombobox* combobox);
   void OnComboboxSelectedIndexChanged(util::PassKey<DialogModelHost>,
-                                      int id,
+                                      DialogModelCombobox* combobox,
                                       int index);
   void OnTextfieldTextChanged(util::PassKey<DialogModelHost>,
-                              int id,
+                              DialogModelTextfield* textfield,
                               base::string16 text);
   void OnWindowClosing(util::PassKey<DialogModelHost>);
 
@@ -206,6 +203,18 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
     return initially_focused_field_;
   }
 
+  DialogModelButton* ok_button(util::PassKey<DialogModelHost>) {
+    return ok_button_.has_value() ? &ok_button_.value() : nullptr;
+  }
+
+  DialogModelButton* cancel_button(util::PassKey<DialogModelHost>) {
+    return cancel_button_.has_value() ? &cancel_button_.value() : nullptr;
+  }
+
+  DialogModelButton* extra_button(util::PassKey<DialogModelHost>) {
+    return extra_button_.has_value() ? &extra_button_.value() : nullptr;
+  }
+
   // Accessor for ordered fields in the model. This includes DialogButtons even
   // though they should be handled separately (OK button has fixed position in
   // dialog).
@@ -215,19 +224,14 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
   }
 
  private:
+  util::PassKey<DialogModel> GetPassKey() {
+    return util::PassKey<DialogModel>();
+  }
+
   void AddDialogButton(int button,
                        base::string16 label,
                        const DialogModelButton::Params& params);
 
-  // TODO(pbos): See if the hosts can just return back the field pointer instead
-  // so we don't need to do lookup.
-  DialogModelField* GetFieldFromModelFieldId(int field_id);
-  DialogModelButton* GetButtonFromModelFieldId(int field_id);
-  DialogModelCombobox* GetComboboxFromModelFieldId(int field_id);
-  DialogModelTextfield* GetTextfieldFromModelFieldId(int field_id);
-
-  DialogModelField::Reservation ReserveField();
-
   std::unique_ptr<DialogModelDelegate> delegate_;
   DialogModelHost* host_ = nullptr;
 
@@ -235,12 +239,13 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
   base::string16 title_;
 
   static constexpr int kExtraButtonId = DIALOG_BUTTON_LAST + 1;
-  // kExtraButtonId is the last reserved id (ui::DialogButton are also reserved
-  // IDs).
-  int next_field_id_ = kExtraButtonId + 1;
   std::vector<std::unique_ptr<DialogModelField>> fields_;
   base::Optional<int> initially_focused_field_;
 
+  base::Optional<DialogModelButton> ok_button_;
+  base::Optional<DialogModelButton> cancel_button_;
+  base::Optional<DialogModelButton> extra_button_;
+
   base::OnceClosure accept_callback_;
   base::OnceClosure cancel_callback_;
   base::OnceClosure close_callback_;

ui/base/models/dialog_model_field.cc

@@ -7,13 +7,12 @@
 
 namespace ui {
 
-DialogModelField::DialogModelField(
-    const DialogModelField::Reservation& reservation,
-    DialogModelField::Type type,
+DialogModelField::DialogModelField(util::PassKey<DialogModel>,
+                                   DialogModel* model,
+                                   Type type,
                                    int unique_id,
                                    base::flat_set<Accelerator> accelerators)
-    : model_(reservation.model),
-      model_field_id_(reservation.model_field_id),
+    : model_(model),
       type_(type),
       unique_id_(unique_id),
       accelerators_(std::move(accelerators)) {
@@ -22,9 +21,6 @@ DialogModelField::DialogModelField(
 
 DialogModelField::~DialogModelField() = default;
 
-DialogModelField::Reservation::Reservation(DialogModel* model,
-                                           int model_field_id)
-    : model(model), model_field_id(model_field_id) {}
 
 DialogModelButton::Params::Params() = default;
 DialogModelButton::Params::~Params() = default;
@@ -41,11 +37,12 @@ DialogModelButton::Params& DialogModelButton::Params::SetCallback(
   return *this;
 }
 
-DialogModelButton::DialogModelButton(
-    const DialogModelField::Reservation& reservation,
+DialogModelButton::DialogModelButton(util::PassKey<DialogModel> pass_key,
+                                     DialogModel* model,
                                      base::string16 label,
                                      const DialogModelButton::Params& params)
-    : DialogModelField(reservation,
+    : DialogModelField(pass_key,
+                       model,
                        kButton,
                        params.unique_id_,
                        params.accelerators_),
@@ -83,11 +80,13 @@ DialogModelCombobox::Params& DialogModelCombobox::Params::SetAccessibleName(
 }
 
 DialogModelCombobox::DialogModelCombobox(
-    const DialogModelField::Reservation& reservation,
+    util::PassKey<DialogModel> pass_key,
+    DialogModel* model,
     base::string16 label,
     std::unique_ptr<ui::ComboboxModel> combobox_model,
     const DialogModelCombobox::Params& params)
-    : DialogModelField(reservation,
+    : DialogModelField(pass_key,
+                       model,
                        kCombobox,
                        params.unique_id_,
                        params.accelerators_),
@@ -122,11 +121,13 @@ DialogModelTextfield::Params& DialogModelTextfield::Params::SetAccessibleName(
 }
 
 DialogModelTextfield::DialogModelTextfield(
-    const DialogModelField::Reservation& reservation,
+    util::PassKey<DialogModel> pass_key,
+    DialogModel* model,
     base::string16 label,
     base::string16 text,
     const ui::DialogModelTextfield::Params& params)
-    : DialogModelField(reservation,
+    : DialogModelField(pass_key,
+                       model,
                        kTextfield,
                        params.unique_id_,
                        params.accelerators_),

ui/base/models/dialog_model_field.h

@@ -36,59 +36,26 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelField {
 
   // Accessors with util::PassKey<DialogModelHost> are only intended to be read
   // by the DialogModelHost implementation.
+  Type type(util::PassKey<DialogModelHost>) const { return type_; }
   const base::flat_set<Accelerator>& accelerators(
       util::PassKey<DialogModelHost>) const {
     return accelerators_;
   }
-  // Reserved ID generated by the model. This ID is generated for every field
-  // and used to map from DialogModelHost fields (like views::Textfield) to
-  // DialogModelFields (like DialogModelTextfield).
-  // WARNING: This should not be confused with the ID set by subclasses'
-  // Params::SetUniqueId() methods.
-  // TODO(pbos): See if this int-to-field mapping can be replaced with using
-  // DialogModelField pointers in DialogModelHost. This is currently only used
-  // because views has a convenient views::View::SetID() function.
-  int model_field_id(util::PassKey<DialogModelHost>) const {
-    return model_field_id_;
-  }
-  Type type(util::PassKey<DialogModelHost>) const { return type_; }
 
  protected:
-  // Struct that holds a reserved ID for the field in the model. This is
-  // protected to be able to be passed from DialogModelField children to the
-  // parent constructor. Its members are private because only DialogModel,
-  // DialogModelField and DialogModelHost should be able to read them.
-  // TODO(pbos): Reconsider whether this |model_field_id| can be avoided. This
-  // would take rewiring DialogModelHost to own a mapping between
-  // "DialogModelField*" and host classes (such as View) and not rely on things
-  // like View::SetID(model_field_id) to maintain that mapping. It would also
-  // require special handling of special buttons like DIALOG_BUTTON_OK.
-  struct Reservation {
-   private:
-    friend class DialogModel;
-    friend class DialogModelField;
-    // This is only to be constructed by DialogModel who makes the actual
-    // ID assignment.
-    Reservation(DialogModel* model, int model_field_id);
-
-    DialogModel* const model;
-    const int model_field_id;
-  };
-
-  DialogModelField(const Reservation& reservation,
+  // Children of this class need to be constructed through DialogModel to help
+  // enforce that they're added to the model.
+  DialogModelField(util::PassKey<DialogModel>,
+                   DialogModel* model,
                    Type type,
                    int unique_id,
                    base::flat_set<Accelerator> accelerators);
 
-  int model_field_id() const { return model_field_id_; }
-
  private:
   friend class DialogModel;
 
   DialogModel* const model_;
-  const int model_field_id_;
   const Type type_;
-
   const int unique_id_;
 
   const base::flat_set<Accelerator> accelerators_;
@@ -124,9 +91,10 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelButton : public DialogModelField {
     base::flat_set<Accelerator> accelerators_;
   };
 
-  // Note that this is constructed through a DialogModel (Reservation can't
-  // be created without one).
-  DialogModelButton(const Reservation& reservation,
+  // Note that this is constructed through a DialogModel which adds it to model
+  // fields.
+  DialogModelButton(util::PassKey<DialogModel> pass_key,
+                    DialogModel* model,
                     base::string16 label,
                     const Params& params);
   DialogModelButton(const DialogModelButton&) = delete;
@@ -178,9 +146,10 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelCombobox : public DialogModelField {
     base::flat_set<Accelerator> accelerators_;
   };
 
-  // Note that this is constructed through a DialogModel (Reservation can't
-  // be created without one).
-  DialogModelCombobox(const Reservation& reservation,
+  // Note that this is constructed through a DialogModel which adds it to model
+  // fields.
+  DialogModelCombobox(util::PassKey<DialogModel> pass_key,
+                      DialogModel* model,
                       base::string16 label,
                       std::unique_ptr<ui::ComboboxModel> combobox_model,
                       const Params& params);
@@ -230,9 +199,10 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelTextfield : public DialogModelField {
     base::flat_set<Accelerator> accelerators_;
   };
 
-  // Note that this is constructed through a DialogModel (Reservation can't
-  // be created without one).
-  DialogModelTextfield(const DialogModelField::Reservation& reservation,
+  // Note that this is constructed through a DialogModel which adds it to model
+  // fields.
+  DialogModelTextfield(util::PassKey<DialogModel> pass_key,
+                       DialogModel* model,
                        base::string16 label,
                        base::string16 text,
                        const Params& params);

ui/base/models/dialog_model_host.h

@@ -37,7 +37,7 @@ class COMPONENT_EXPORT(UI_BASE) DialogModelHost {
   // Called when various parts of the model changes.
   // TODO(pbos): Break this down to API that says what was added/removed/changed
   // to not have to reset everything.
-  virtual void OnModelChanged(DialogModel* model) = 0;
+  virtual void OnFieldAdded(DialogModelField* field) = 0;
 };
 
 }  // namespace ui

ui/views/bubble/bubble_dialog_model_host.h

@@ -20,7 +20,7 @@ class GridLayout;
 class Textfield;
 
 // BubbleDialogModelHost is a views implementation of ui::DialogModelHost which
-// hosts a ui::DialogModels as a BubbleDialogDelegateView. This exposes
+// hosts a ui::DialogModel as a BubbleDialogDelegateView. This exposes
 // views-specific methods such as SetAnchorView(), SetArrow() and
 // SetHighlightedButton(). For methods that are reflected in ui::DialogModelHost
 // (such as ::Close()), preferusing the ui::DialogModelHost to avoid
@@ -49,8 +49,7 @@ class VIEWS_EXPORT BubbleDialogModelHost : public BubbleDialogDelegateView,
   // ui::DialogModelHost:
   void Close() override;
   void SelectAllText(int unique_id) override;
-
-  void OnModelChanged(ui::DialogModel* model) override;
+  void OnFieldAdded(ui::DialogModelField* field) override;
 
   // ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
@@ -62,7 +61,8 @@ class VIEWS_EXPORT BubbleDialogModelHost : public BubbleDialogDelegateView,
   GridLayout* GetGridLayout();
   void ConfigureGridLayout();
 
-  Textfield* AddOrUpdateTextfield(const ui::DialogModelTextfield& field);
+  void AddInitialFields();
+  Textfield* AddOrUpdateTextfield(ui::DialogModelTextfield* field);
   Combobox* AddOrUpdateCombobox(ui::DialogModelCombobox* field);
   void AddLabelAndField(const base::string16& label_text,
                         std::unique_ptr<views::View> field,
@@ -70,10 +70,17 @@ class VIEWS_EXPORT BubbleDialogModelHost : public BubbleDialogDelegateView,
 
   void UpdateAccelerators();
 
-  void NotifyTextfieldTextChanged(int id, views::Textfield* textfield);
-  void NotifyComboboxSelectedIndexChanged(int id, views::Combobox* combobox);
+  void NotifyTextfieldTextChanged(views::Textfield* textfield);
+  void NotifyComboboxSelectedIndexChanged(views::Combobox* combobox);
+
+  View* FieldToView(ui::DialogModelField* field);
+
+  ui::DialogModelButton* FieldAsButton(ui::DialogModelField* field);
+  ui::DialogModelCombobox* FieldAsCombobox(ui::DialogModelField* field);
+  ui::DialogModelTextfield* FieldAsTextfield(ui::DialogModelField* field);
 
   std::unique_ptr<ui::DialogModel> model_;
+  base::flat_map<View*, ui::DialogModelField*> view_to_field_;
   std::vector<PropertyChangedSubscription> property_changed_subscriptions_;
 };