[IdleTimeout] Make IdleDialog modal to the browser

Previously IdleDialog would appear in the middle of the screen, with
no parent and no modality. UX isn't happy with this, and would rather
do the same thing as RelaunchRequiredDialogView.

- Show IdleDialog in the currently-active browser, instead of no parent.
- Make it modal to that browser.
- If there is no active browser, skip the dialog and run actions
  immediately.

Bug: 4544766
Change-Id: I9cb1a289bbcc257a568b61b61815b9c3c54700bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4544766
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1154034}

chrome/browser/enterprise/idle/action.cc

@@ -63,10 +63,17 @@ class ShowDialogAction : public Action {
     continuation_ = std::move(continuation);
     // Action object's lifetime extends until it calls `continuation_`, so
     // passing `this` as a raw pointer is safe.
-    subscription_ = DialogManager::GetInstance()->ShowDialog(
-        timeout, action_types_,
-        base::BindOnce(&ShowDialogAction::OnCloseFinished,
-                       base::Unretained(this)));
+    base::CallbackListSubscription subscription =
+        DialogManager::GetInstance()->MaybeShowDialog(
+            profile, timeout, action_types_,
+            base::BindOnce(&ShowDialogAction::OnDialogFinished,
+                           base::Unretained(this)));
+    if (subscription) {
+      // If there is no dialog to show, MaybeShowDialog() resolves immediately
+      // and we destroy this object via OnCloseFinished(). This if guards
+      // against a use-after-free.
+      subscription_ = std::move(subscription);
+    }
   }
 
   bool ShouldNotifyUserOfPendingDestructiveAction(Profile* profile) override {
@@ -75,7 +82,7 @@ class ShowDialogAction : public Action {
   }
 
  private:
-  void OnCloseFinished(bool expired) {
+  void OnDialogFinished(bool expired) {
     std::move(continuation_).Run(/*success=*/expired);
   }
 

chrome/browser/enterprise/idle/dialog_manager.cc

@@ -10,8 +10,14 @@
 #include "base/check_is_test.h"
 #include "base/command_line.h"
 #include "base/functional/bind.h"
+#include "base/ranges/algorithm.h"
 #include "base/task/single_thread_task_runner.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "components/prefs/pref_service.h"
 
 namespace enterprise_idle {
 
@@ -20,6 +26,16 @@ namespace {
 constexpr base::TimeDelta kTestDialogTimeout = base::Seconds(5);
 constexpr base::TimeDelta kDialogTimeout = base::Seconds(30);
 
+IdleDialog::ActionSet GetActionSet(PrefService* prefs) {
+  std::vector<ActionType> actions;
+  base::ranges::transform(prefs->GetList(prefs::kIdleTimeoutActions),
+                          std::back_inserter(actions),
+                          [](const base::Value& action) {
+                            return static_cast<ActionType>(action.GetInt());
+                          });
+  return ActionsToActionSet(base::flat_set<ActionType>(std::move(actions)));
+}
+
 }  // namespace
 
 DialogManager::DialogManager() = default;
@@ -31,32 +47,38 @@ DialogManager* DialogManager::GetInstance() {
   return instance.get();
 }
 
-base::CallbackListSubscription DialogManager::ShowDialog(
+base::CallbackListSubscription DialogManager::MaybeShowDialog(
+    Profile* profile,
     base::TimeDelta threshold,
     const base::flat_set<ActionType>& action_types,
     FinishedCallback on_finished) {
-  // Passed the guards: we're really going to show the dialog and close
-  // browsers.
-  base::CallbackListSubscription subscription =
-      callbacks_.Add(std::move(on_finished));
-
   if (dialog_) {
     // The dialog is already visible, re-use it.
-    return subscription;
+    return callbacks_.Add(std::move(on_finished));
   }
 
+  Browser* active_browser = chrome::FindBrowserWithActiveWindow();
+  if (!active_browser || !active_browser->is_type_normal()) {
+    // User is in another app, or in a window that shouldn't show the dialog
+    // (e.g. DevTools). Skip the dialog, and run actions immediately.
+    std::move(on_finished).Run(/*expired=*/true);
+    return base::CallbackListSubscription();
+  }
+
+  // Create a new dialog, modal to `active_browser`.
   base::TimeDelta timeout = base::CommandLine::ForCurrentProcess()->HasSwitch(
                                 switches::kSimulateIdleTimeout)
                                 ? kTestDialogTimeout
                                 : kDialogTimeout;
-  dialog_ =
-      IdleDialog::Show(timeout, threshold, ActionsToActionSet(action_types),
-                       base::BindOnce(&DialogManager::OnDialogDismissedByUser,
-                                      base::Unretained(this)));
   dialog_timer_.Start(
       FROM_HERE, timeout,
       base::BindOnce(&DialogManager::OnDialogExpired, base::Unretained(this)));
-  return subscription;
+  dialog_ = IdleDialog::Show(
+      active_browser, timeout, threshold, GetActionSet(profile->GetPrefs()),
+      base::BindOnce(&DialogManager::OnDialogDismissedByUser,
+                     base::Unretained(this)));
+
+  return callbacks_.Add(std::move(on_finished));
 }
 
 void DialogManager::DismissDialogForTesting() {

chrome/browser/enterprise/idle/dialog_manager.h

@@ -16,10 +16,16 @@
 #include "chrome/browser/ui/idle_dialog.h"
 #include "ui/views/widget/widget.h"
 
+class Profile;
+
 namespace enterprise_idle {
 
 // A singleton that manages IdleDialog's state. Shows the dialog, manages its
 // expiration, and runs a callback when it closes.
+//
+// The dialog is anchored to the currently-active browser window. If there is no
+// active browser window (e.g. user is in another app), the timer is completely
+// skipped and other actions resolve immediately.
 class DialogManager {
  public:
   using FinishedCallback = base::OnceCallback<void(bool expired)>;
@@ -28,7 +34,11 @@ class DialogManager {
 
   // Show a 30s dialog--or if it's already visible, re-use the existing one.
   // Run `on_finished` after 30s, or if the user dismisses the dialog.
-  base::CallbackListSubscription ShowDialog(
+  //
+  // If the user is in another app, skip the dialog and call
+  // `on_finished(true)`.
+  base::CallbackListSubscription MaybeShowDialog(
+      Profile* profile,
       base::TimeDelta threshold,
       const base::flat_set<ActionType>& action_types,
       FinishedCallback on_finished);

chrome/browser/enterprise/idle/idle_service.cc

@@ -20,6 +20,7 @@
 
 #if !BUILDFLAG(IS_ANDROID)
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/idle_bubble.h"
@@ -52,7 +53,7 @@ class IdleService::BrowserObserver : public BrowserListObserver {
   void OnBrowserSetLastActive(Browser* browser) override {
     Profile* profile = browser->profile();
     auto* prefs = profile->GetPrefs();
-    if (profile == profile_ &&
+    if (browser->is_type_normal() && profile == profile_ &&
         prefs->GetBoolean(prefs::kIdleTimeoutShowBubbleOnStartup)) {
       ShowIdleBubble(browser, base::Minutes(5), GetActionSet(prefs));
       prefs->SetBoolean(prefs::kIdleTimeoutShowBubbleOnStartup, false);

chrome/browser/enterprise/idle/idle_service_interactive_uitest.cc

@@ -47,6 +47,10 @@
 #include "ui/base/idle/idle_time_provider.h"
 #include "ui/base/test/idle_test_utils.h"
 
+#if BUILDFLAG(IS_LINUX)
+#include "ui/ozone/buildflags.h"
+#endif  // BUILDFLAG(IS_LINUX)
+
 using base::TestMockTimeTaskRunner;
 using testing::_;
 using testing::ElementsAre;
@@ -188,6 +192,19 @@ class IdleServiceTest : public InProcessBrowserTest {
   }
 
   void ActivateBrowser(Browser* browser) {
+#if BUILDFLAG(IS_LINUX)
+#if BUILDFLAG(OZONE_PLATFORM_WAYLAND)
+    // TODO(nicolaso): BrowserActivationWaiter times out on Wayland. Figure out
+    // why.
+#else
+    ActivateBrowserImpl(browser);
+#endif
+#else
+    ActivateBrowserImpl(browser);
+#endif
+  }
+
+  void ActivateBrowserImpl(Browser* browser) {
     if (IsIdleBubbleOpenForTesting(browser)) {
       return;
     }
@@ -354,6 +371,7 @@ IN_PROC_BROWSER_TEST_F(IdleServiceTest, MAYBE_MultiProfile) {
             "Profile 3"));
   }
   std::ignore = CreateBrowser(profile3);
+  ActivateBrowser(browser());
 
   EXPECT_EQ(2, GetBrowserCount(profile));
   EXPECT_EQ(1, GetBrowserCount(profile2));
@@ -374,17 +392,17 @@ IN_PROC_BROWSER_TEST_F(IdleServiceTest, MAYBE_MultiProfile) {
   EXPECT_FALSE(ProfilePicker::IsOpen());
 
   // 300s, threshold is reached. Close browsers, then show the Profile Picker.
-  EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
-      .WillOnce(Return(base::Seconds(300)));
-  task_runner()->FastForwardBy(base::Seconds(1));
-  EXPECT_TRUE(IsDialogOpen());
-  EXPECT_FALSE(ProfilePicker::IsOpen());
+  {
+    BrowserCloseWaiter waiter({browser(), browser2, browser3});
+    EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
+        .WillOnce(Return(base::Seconds(300)));
+    task_runner()->FastForwardBy(base::Seconds(1));
 
-  EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
-      .WillRepeatedly(Return(base::Seconds(315)));
-  BrowserCloseWaiter waiter({browser(), browser2, browser3});
-  task_runner()->FastForwardBy(base::Seconds(30));
-  waiter.Wait();
+    EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
+        .WillRepeatedly(Return(base::Seconds(315)));
+    task_runner()->FastForwardBy(base::Seconds(30));
+    waiter.Wait();
+  }
   EXPECT_EQ(0, GetBrowserCount(profile));
   EXPECT_EQ(0, GetBrowserCount(profile2));
   EXPECT_EQ(1, GetBrowserCount(profile3));
@@ -439,13 +457,13 @@ IN_PROC_BROWSER_TEST_F(IdleServiceTest,
 
   // 300s, threshold is reached for `profile`. Close its browsers, then show the
   // Profile Picker.
-  EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
-      .WillOnce(Return(base::Seconds(300)));
-  task_runner()->FastForwardBy(base::Seconds(1));
-  EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
-      .WillRepeatedly(Return(base::Seconds(315)));
   {
     BrowserCloseWaiter waiter({browser(), browser2});
+    EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
+        .WillOnce(Return(base::Seconds(300)));
+    task_runner()->FastForwardBy(base::Seconds(1));
+    EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
+        .WillRepeatedly(Return(base::Seconds(315)));
     task_runner()->FastForwardBy(base::Seconds(30));
     waiter.Wait();
   }
@@ -455,13 +473,13 @@ IN_PROC_BROWSER_TEST_F(IdleServiceTest,
   EXPECT_TRUE(ProfilePicker::IsOpen());
 
   // 360s, threshold is reached for `profile2`. Close its browsers.
-  EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
-      .WillOnce(Return(base::Seconds(360)));
-  task_runner()->FastForwardBy(base::Seconds(1));
-  EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
-      .WillRepeatedly(Return(base::Seconds(375)));
   {
     BrowserCloseWaiter waiter({browser3});
+    EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
+        .WillOnce(Return(base::Seconds(360)));
+    task_runner()->FastForwardBy(base::Seconds(1));
+    EXPECT_CALL(idle_time_provider(), CalculateIdleTime())
+        .WillRepeatedly(Return(base::Seconds(375)));
     task_runner()->FastForwardBy(base::Seconds(30));
     waiter.Wait();
   }

chrome/browser/ui/idle_dialog.h

@@ -10,6 +10,8 @@
 #include "base/time/time.h"
 #include "ui/views/widget/widget.h"
 
+class Browser;
+
 // Idle timeout dialog. This is shown to users to inform them that Chrome will
 // be closed by the IdleService, as dictated by the IdleProfileCloseTimeout
 // policy.
@@ -27,7 +29,8 @@ class IdleDialog {
   };
 
   // Implemented in //chrome/browser/ui/views/idle_dialog_view.cc
-  static base::WeakPtr<views::Widget> Show(base::TimeDelta dialog_duration,
+  static base::WeakPtr<views::Widget> Show(Browser* browser,
+                                           base::TimeDelta dialog_duration,
                                            base::TimeDelta idle_threshold,
                                            ActionSet actions,
                                            base::OnceClosure on_close_by_user);

chrome/browser/ui/views/policy/idle_dialog_view.cc

@@ -53,27 +53,29 @@ std::unique_ptr<views::Label> CreateLabel() {
 
 // static
 base::WeakPtr<views::Widget> IdleDialog::Show(
+    Browser* browser,
     base::TimeDelta dialog_duration,
     base::TimeDelta idle_threshold,
     IdleDialog::ActionSet actions,
     base::OnceClosure on_close_by_user) {
-  return policy::IdleDialogView::Show(dialog_duration, idle_threshold, actions,
-                                      std::move(on_close_by_user));
+  return policy::IdleDialogView::Show(browser, dialog_duration, idle_threshold,
+                                      actions, std::move(on_close_by_user));
 }
 
 namespace policy {
 
 // static
 base::WeakPtr<views::Widget> IdleDialogView::Show(
+    Browser* browser,
     base::TimeDelta dialog_duration,
     base::TimeDelta idle_threshold,
     IdleDialog::ActionSet actions,
     base::OnceClosure on_close_by_user) {
-  auto view = std::make_unique<IdleDialogView>(
-      dialog_duration, idle_threshold, actions, std::move(on_close_by_user));
-  auto* widget = CreateBubble(std::move(view));
+  views::Widget* widget = constrained_window::CreateBrowserModalDialogViews(
+      std::make_unique<IdleDialogView>(dialog_duration, idle_threshold, actions,
+                                       std::move(on_close_by_user)),
+      browser->window()->GetNativeWindow());
   widget->Show();
-  widget->CenterWindow(widget->non_client_view()->GetPreferredSize());
   return widget->GetWeakPtr();
 }
 
@@ -117,10 +119,8 @@ IdleDialogView::IdleDialogView(base::TimeDelta dialog_duration,
   SetAcceptCallback(std::move(callback1));
   SetCancelCallback(std::move(callback2));
 
-  set_draggable(true);
-  set_has_parent(false);
-  set_close_on_deactivate(false);
-  SetModalType(ui::MODAL_TYPE_NONE);
+  SetModalType(ui::MODAL_TYPE_WINDOW);
+  SetShowCloseButton(false);
   set_fixed_width(views::LayoutProvider::Get()->GetDistanceMetric(
       views::DISTANCE_MODAL_DIALOG_PREFERRED_WIDTH));
 
@@ -129,6 +129,10 @@ IdleDialogView::IdleDialogView(base::TimeDelta dialog_duration,
   layout->set_cross_axis_alignment(
       views::BoxLayout::CrossAxisAlignment::kStretch);
 
+  const ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
+  set_margins(provider->GetDialogInsetsForContentType(
+      views::DialogContentType::kText, views::DialogContentType::kText));
+
   main_label_ = AddChildView(CreateLabel());
   incognito_label_ = AddChildView(CreateLabel());
   countdown_label_ = AddChildView(CreateLabel());

chrome/browser/ui/views/policy/idle_dialog_view.h

@@ -21,7 +21,7 @@ namespace policy {
 // A View for the idle timeout dialog. This is shown to users to inform them
 // that Chrome will be closed by the IdleService, as dictated by the
 // IdleProfileCloseTimeout policy.
-class IdleDialogView : public views::BubbleDialogDelegateView {
+class IdleDialogView : public views::DialogDelegateView {
  public:
   IdleDialogView(base::TimeDelta dialog_duration,
                  base::TimeDelta idle_threshold,
@@ -33,7 +33,8 @@ class IdleDialogView : public views::BubbleDialogDelegateView {
   // IdleProfileCloseTimeout policy, for displaying to the user.
   // |on_close_by_user| is run if the user clicks on "Continue", or presses
   // Escape to close the dialog.
-  static base::WeakPtr<views::Widget> Show(base::TimeDelta dialog_duration,
+  static base::WeakPtr<views::Widget> Show(Browser* browser,
+                                           base::TimeDelta dialog_duration,
                                            base::TimeDelta idle_threshold,
                                            IdleDialog::ActionSet actions,
                                            base::OnceClosure on_close_by_user);