Allow DialogModel without DialogModelDelegate

This lets dialogs that don't need to store state inside a DialogModel
avoid instantiating (and subclassing) a DialogModelDelegate.

DialogModel::Builder::model() is added to permit accessing a pointer to
the model before it's been built so that it can be bound to callbacks.

This change removes DialogModelDelegate use from WindowNamePrompt and
OutdatedUpgradeBubbleView.

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

chrome/browser/ui/views/outdated_upgrade_bubble_view.cc

@@ -46,84 +46,59 @@ constexpr int kMaxIgnored = 50;
 // The number of buckets we want the NumLaterPerReinstall histogram to use.
 constexpr int kNumIgnoredBuckets = 5;
 
-class OutdatedUpgradeBubbleDelegate;
-// The currently showing bubble.
-OutdatedUpgradeBubbleDelegate* g_upgrade_bubble = nullptr;
+bool g_upgrade_bubble_is_showing = false;
 
 // The number of times the user ignored the bubble before finally choosing to
 // reinstall.
 int g_num_ignored_bubbles = 0;
 
-class OutdatedUpgradeBubbleDelegate : public ui::DialogModelDelegate {
- public:
-  OutdatedUpgradeBubbleDelegate(content::PageNavigator* navigator,
-                                bool auto_update_enabled)
-      : auto_update_enabled_(auto_update_enabled), navigator_(navigator) {}
-
-  OutdatedUpgradeBubbleDelegate(const OutdatedUpgradeBubbleDelegate&) = delete;
-  OutdatedUpgradeBubbleDelegate& operator=(
-      const OutdatedUpgradeBubbleDelegate&) = delete;
-
-  ~OutdatedUpgradeBubbleDelegate() override {
-    // Increment the ignored bubble count (if this bubble wasn't ignored, this
-    // increment is offset by a decrement in OnDialogAccepted()).
-    if (g_num_ignored_bubbles < kMaxIgnored)
-      ++g_num_ignored_bubbles;
-  }
+void OnWindowClosing() {
+  g_upgrade_bubble_is_showing = false;
 
-  void OnWindowClosing() {
-    // Reset |g_upgrade_bubble| here, not in destructor, because destruction is
-    // asynchronous and ShowBubble may be called before full destruction and
-    // would attempt to show a bubble that is closing.
-    DCHECK_EQ(g_upgrade_bubble, this);
-    g_upgrade_bubble = nullptr;
-  }
+  // Increment the ignored bubble count (if this bubble wasn't ignored, this
+  // increment is offset by a decrement in OnDialogAccepted()).
+  if (g_num_ignored_bubbles < kMaxIgnored)
+    ++g_num_ignored_bubbles;
+}
 
-  void OnDialogAccepted() {
-    // Offset the +1 in the dtor.
-    --g_num_ignored_bubbles;
-    if (auto_update_enabled_) {
-      DCHECK(UpgradeDetector::GetInstance()->is_outdated_install());
-      UMA_HISTOGRAM_CUSTOM_COUNTS("OutdatedUpgradeBubble.NumLaterPerReinstall",
-                                  g_num_ignored_bubbles, 1, kMaxIgnored,
-                                  kNumIgnoredBuckets);
-      base::RecordAction(
-          base::UserMetricsAction("OutdatedUpgradeBubble.Reinstall"));
-      navigator_->OpenURL(
-          content::OpenURLParams(GURL(kDownloadChromeUrl), content::Referrer(),
-                                 WindowOpenDisposition::NEW_FOREGROUND_TAB,
-                                 ui::PAGE_TRANSITION_LINK, false));
+void OnDialogAccepted(content::PageNavigator* navigator,
+                      bool auto_update_enabled) {
+  // Offset the +1 in OnWindowClosing().
+  --g_num_ignored_bubbles;
+  if (auto_update_enabled) {
+    DCHECK(UpgradeDetector::GetInstance()->is_outdated_install());
+    UMA_HISTOGRAM_CUSTOM_COUNTS("OutdatedUpgradeBubble.NumLaterPerReinstall",
+                                g_num_ignored_bubbles, 1, kMaxIgnored,
+                                kNumIgnoredBuckets);
+    base::RecordAction(
+        base::UserMetricsAction("OutdatedUpgradeBubble.Reinstall"));
+    navigator->OpenURL(
+        content::OpenURLParams(GURL(kDownloadChromeUrl), content::Referrer(),
+                               WindowOpenDisposition::NEW_FOREGROUND_TAB,
+                               ui::PAGE_TRANSITION_LINK, false));
 #if defined(OS_WIN)
-    } else {
-      DCHECK(UpgradeDetector::GetInstance()->is_outdated_install_no_au());
-      UMA_HISTOGRAM_CUSTOM_COUNTS("OutdatedUpgradeBubble.NumLaterPerEnableAU",
-                                  g_num_ignored_bubbles, 1, kMaxIgnored,
-                                  kNumIgnoredBuckets);
-      base::RecordAction(
-          base::UserMetricsAction("OutdatedUpgradeBubble.EnableAU"));
-      // Record that the autoupdate flavour of the dialog has been shown.
-      if (g_browser_process->local_state()) {
-        g_browser_process->local_state()->SetBoolean(
-            prefs::kAttemptedToEnableAutoupdate, true);
-      }
-
-      // Re-enable updates by shelling out to setup.exe asynchronously.
-      base::ThreadPool::PostTask(
-          FROM_HERE,
-          {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-           base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
-          base::BindOnce(&google_update::ElevateIfNeededToReenableUpdates));
-#endif  // defined(OS_WIN)
+  } else {
+    DCHECK(UpgradeDetector::GetInstance()->is_outdated_install_no_au());
+    UMA_HISTOGRAM_CUSTOM_COUNTS("OutdatedUpgradeBubble.NumLaterPerEnableAU",
+                                g_num_ignored_bubbles, 1, kMaxIgnored,
+                                kNumIgnoredBuckets);
+    base::RecordAction(
+        base::UserMetricsAction("OutdatedUpgradeBubble.EnableAU"));
+    // Record that the autoupdate flavour of the dialog has been shown.
+    if (g_browser_process->local_state()) {
+      g_browser_process->local_state()->SetBoolean(
+          prefs::kAttemptedToEnableAutoupdate, true);
     }
-  }
 
- private:
-  // Identifies if auto-update is enabled or not.
-  const bool auto_update_enabled_;
-
-  // The PageNavigator to use for opening the Download Chrome URL.
-  content::PageNavigator* const navigator_;
-};
+    // Re-enable updates by shelling out to setup.exe asynchronously.
+    base::ThreadPool::PostTask(
+        FROM_HERE,
+        {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+         base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
+        base::BindOnce(&google_update::ElevateIfNeededToReenableUpdates));
+#endif  // defined(OS_WIN)
+  }
+}
 
 }  // namespace
 
@@ -133,28 +108,23 @@ class OutdatedUpgradeBubbleDelegate : public ui::DialogModelDelegate {
 void OutdatedUpgradeBubbleView::ShowBubble(views::View* anchor_view,
                                            content::PageNavigator* navigator,
                                            bool auto_update_enabled) {
-  if (g_upgrade_bubble)
+  if (g_upgrade_bubble_is_showing)
     return;
 
-  auto delegate = std::make_unique<OutdatedUpgradeBubbleDelegate>(
-      navigator, auto_update_enabled);
-  g_upgrade_bubble = delegate.get();
+  g_upgrade_bubble_is_showing = true;
 
   auto dialog_model =
-      ui::DialogModel::Builder(std::move(std::move(delegate)))
+      ui::DialogModel::Builder()
           .SetShowCloseButton(true)
           .SetTitle(l10n_util::GetStringUTF16(IDS_UPGRADE_BUBBLE_TITLE))
           .AddOkButton(
-              base::BindOnce(&OutdatedUpgradeBubbleDelegate::OnDialogAccepted,
-                             base::Unretained(g_upgrade_bubble)),
+              base::BindOnce(&OnDialogAccepted, navigator, auto_update_enabled),
               l10n_util::GetStringUTF16(auto_update_enabled
                                             ? IDS_REINSTALL_APP
                                             : IDS_REENABLE_UPDATES))
           .AddBodyText(
               ui::DialogModelLabel(IDS_UPGRADE_BUBBLE_TEXT).set_is_secondary())
-          .SetWindowClosingCallback(
-              base::BindOnce(&OutdatedUpgradeBubbleDelegate::OnWindowClosing,
-                             base::Unretained(g_upgrade_bubble)))
+          .SetWindowClosingCallback(base::BindOnce(&OnWindowClosing))
           .SetCloseCallback(base::BindOnce(
               &base::RecordAction,
               base::UserMetricsAction("OutdatedUpgradeBubble.Later")))

chrome/browser/ui/views/window_name_prompt.cc

@@ -18,25 +18,20 @@ namespace {
 
 constexpr int kWindowNameFieldId = 1;
 
-class WindowNamePromptDelegate : public ui::DialogModelDelegate {
- public:
-  void SetBrowserTitleFromTextField(Browser* browser) {
-    browser->SetWindowUserTitle(base::UTF16ToUTF8(
-        dialog_model()->GetTextfieldByUniqueId(kWindowNameFieldId)->text()));
-  }
-};
+void SetBrowserTitleFromTextfield(Browser* browser,
+                                  ui::DialogModel* dialog_model) {
+  browser->SetWindowUserTitle(base::UTF16ToUTF8(
+      dialog_model->GetTextfieldByUniqueId(kWindowNameFieldId)->text()));
+}
 
 std::unique_ptr<views::DialogDelegate> CreateWindowNamePrompt(
     Browser* browser) {
-  auto bubble_delegate_unique = std::make_unique<WindowNamePromptDelegate>();
-  WindowNamePromptDelegate* bubble_delegate = bubble_delegate_unique.get();
-
+  ui::DialogModel::Builder dialog_builder;
   auto dialog_model =
-      ui::DialogModel::Builder(std::move(bubble_delegate_unique))
+      dialog_builder
           .SetTitle(l10n_util::GetStringUTF16(IDS_NAME_WINDOW_PROMPT_TITLE))
-          .AddOkButton(base::BindOnce(
-              &WindowNamePromptDelegate::SetBrowserTitleFromTextField,
-              base::Unretained(bubble_delegate), browser))
+          .AddOkButton(base::BindOnce(&SetBrowserTitleFromTextfield, browser,
+                                      dialog_builder.model()))
           .AddCancelButton(base::DoNothing())
           .AddTextfield(
               l10n_util::GetStringUTF16(IDS_NAME_WINDOW_PROMPT_WINDOW_NAME),

ui/base/models/dialog_model.cc

@@ -12,6 +12,9 @@ namespace ui {
 DialogModel::Builder::Builder(std::unique_ptr<DialogModelDelegate> delegate)
     : model_(std::make_unique<DialogModel>(util::PassKey<Builder>(),
                                            std::move(delegate))) {}
+
+DialogModel::Builder::Builder() : Builder(nullptr) {}
+
 DialogModel::Builder::~Builder() {
   DCHECK(!model_) << "Model should've been built.";
 }
@@ -75,7 +78,8 @@ DialogModel::Builder& DialogModel::Builder::SetInitiallyFocusedField(
 DialogModel::DialogModel(util::PassKey<Builder>,
                          std::unique_ptr<DialogModelDelegate> delegate)
     : delegate_(std::move(delegate)) {
-  delegate_->set_dialog_model(this);
+  if (delegate_)
+    delegate_->set_dialog_model(this);
 }
 
 DialogModel::~DialogModel() = default;

ui/base/models/dialog_model.h

@@ -83,13 +83,34 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
  public:
   // Builder for DialogModel. Used for properties that are either only or
   // commonly const after construction.
-  class COMPONENT_EXPORT(UI_BASE) Builder {
+  class COMPONENT_EXPORT(UI_BASE) Builder final {
    public:
+    // Constructs a Builder for a DialogModel with a DialogModelDelegate whose
+    // lifetime (and storage) is tied to the lifetime of the DialogModel.
     explicit Builder(std::unique_ptr<DialogModelDelegate> delegate);
+
+    // Constructs a DialogModel without a DialogModelDelegate (that doesn't
+    // require storage tied to the DialogModel). For access to the DialogModel
+    // during construction (for use in callbacks), use model().
+    Builder();
+
+    Builder(const Builder&) = delete;
+    Builder& operator=(const Builder&) = delete;
+
     ~Builder();
 
     std::unique_ptr<DialogModel> Build() WARN_UNUSED_RESULT;
 
+    // Gets the DialogModel. Used for setting up callbacks that make use of the
+    // model later once it's fully constructed. This is useful for dialogs or
+    // callbacks that don't use DialogModelDelegate and don't have direct access
+    // to the model through DialogModelDelegate::dialog_model().
+    //
+    // Note that the DialogModel* returned here is only for registering
+    // callbacks with the DialogModel::Builder. These callbacks share lifetimes
+    // with the DialogModel so uses of it will not result in use-after-frees.
+    DialogModel* model() { return model_.get(); }
+
     Builder& SetShowCloseButton(bool show_close_button) {
       model_->show_close_button_ = show_close_button;
       return *this;
@@ -192,6 +213,10 @@ class COMPONENT_EXPORT(UI_BASE) DialogModel final {
 
   DialogModel(util::PassKey<DialogModel::Builder>,
               std::unique_ptr<DialogModelDelegate> delegate);
+
+  DialogModel(const DialogModel&) = delete;
+  DialogModel& operator=(const DialogModel&) = delete;
+
   ~DialogModel();
 
   // The host in which this model is hosted. Set by the Host implementation

ui/views/bubble/bubble_dialog_model_host_unittest.cc

@@ -19,48 +19,33 @@ using BubbleDialogModelHostTest = ViewsTestBase;
 // check its compliance.
 
 namespace {
-// TODO(pbos): Consider moving this to a non-views testutil location. This is
-// likely usable without/outside views (even if the test suite doesn't move).
-class TestModelDelegate : public ui::DialogModelDelegate {
+// WeakPtrs to this delegate is used to infer when DialogModel is destroyed.
+class WeakDialogModelDelegate : public ui::DialogModelDelegate {
  public:
-  struct Stats {
-    int window_closing_count = 0;
-  };
-
-  explicit TestModelDelegate(Stats* stats) : stats_(stats) {}
-
-  base::WeakPtr<TestModelDelegate> GetWeakPtr() {
+  base::WeakPtr<WeakDialogModelDelegate> GetWeakPtr() {
     return weak_ptr_factory_.GetWeakPtr();
   }
 
-  static std::unique_ptr<ui::DialogModel> BuildModel(
-      std::unique_ptr<TestModelDelegate> delegate) {
-    auto* delegate_ptr = delegate.get();
-    return ui::DialogModel::Builder(std::move(delegate))
-        .SetWindowClosingCallback(
-            base::BindOnce(&TestModelDelegate::OnWindowClosing,
-                           base::Unretained(delegate_ptr)))
-        .Build();
-  }
-
-  void OnWindowClosing() { ++stats_->window_closing_count; }
-
  private:
-  Stats* const stats_;
-  base::WeakPtrFactory<TestModelDelegate> weak_ptr_factory_{this};
+  base::WeakPtrFactory<WeakDialogModelDelegate> weak_ptr_factory_{this};
 };
+
 }  // namespace
 
 TEST_F(BubbleDialogModelHostTest, CloseIsSynchronousAndCallsWindowClosing) {
   std::unique_ptr<Widget> anchor_widget =
       CreateTestWidget(Widget::InitParams::TYPE_WINDOW);
 
-  TestModelDelegate::Stats stats;
-  auto delegate = std::make_unique<TestModelDelegate>(&stats);
+  auto delegate = std::make_unique<WeakDialogModelDelegate>();
   auto weak_delegate = delegate->GetWeakPtr();
 
+  int window_closing_count = 0;
   auto host = std::make_unique<BubbleDialogModelHost>(
-      TestModelDelegate::BuildModel(std::move(delegate)),
+      ui::DialogModel::Builder(std::move(delegate))
+          .SetWindowClosingCallback(base::BindOnce(base::BindOnce(
+              [](int* window_closing_count) { ++(*window_closing_count); },
+              &window_closing_count)))
+          .Build(),
       anchor_widget->GetContentsView(), BubbleBorder::Arrow::TOP_RIGHT);
   auto* host_ptr = host.get();
 
@@ -68,10 +53,10 @@ TEST_F(BubbleDialogModelHostTest, CloseIsSynchronousAndCallsWindowClosing) {
       BubbleDialogDelegateView::CreateBubble(host.release());
   test::WidgetDestroyedWaiter waiter(bubble_widget);
 
-  EXPECT_EQ(0, stats.window_closing_count);
+  EXPECT_EQ(0, window_closing_count);
   DCHECK_EQ(host_ptr, weak_delegate->dialog_model()->host());
   weak_delegate->dialog_model()->host()->Close();
-  EXPECT_EQ(1, stats.window_closing_count);
+  EXPECT_EQ(1, window_closing_count);
 
   // The model (and hence delegate) should destroy synchronously, so the
   // WeakPtr should disappear before waiting for the views Widget to close.