views: remove DialogDelegate::ShouldUseCustomFrame

This is now a property of an instance rather than a delegate method.

This change introduces one minor behavior change: on Windows,
HungRendererDialogView used to use a custom frame when Aero Glass was
enabled; it will now use a custom frame when Aero Glass is enabled *or*
the dialog is parented. I think the old behavior was a bug - it is not
supported to use a custom frame with no parent window.

Some reworking of the DialogTest suite was required, because this suite
used to override ShouldUseCustomFrame to return true despite not having
a parent widget for the test dialogs. This change introduces a parent
widget so that these tests continue to get the custom frame they desire.

Bug: 1011446
Change-Id: Ib3a8521368e1d7d779c2346fc74c97633fd13f16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1846317Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704614}

chrome/browser/ui/views/extensions/extension_dialog.cc

@@ -39,6 +39,8 @@ ExtensionDialog::ExtensionDialog(
     std::unique_ptr<extensions::ExtensionViewHost> host,
     ExtensionDialogObserver* observer)
     : host_(std::move(host)), observer_(observer) {
+  DialogDelegate::set_use_custom_frame(false);
+
   AddRef();  // Balanced in DeleteDelegate();
 
   registrar_.Add(this,
@@ -220,10 +222,6 @@ views::View* ExtensionDialog::GetContentsView() {
   return GetExtensionView();
 }
 
-bool ExtensionDialog::ShouldUseCustomFrame() const {
-  return false;
-}
-
 /////////////////////////////////////////////////////////////////////////////
 // content::NotificationObserver overrides.
 

chrome/browser/ui/views/extensions/extension_dialog.h

@@ -79,7 +79,6 @@ class ExtensionDialog : public views::DialogDelegate,
   views::Widget* GetWidget() override;
   const views::Widget* GetWidget() const override;
   views::View* GetContentsView() override;
-  bool ShouldUseCustomFrame() const override;
 
   // content::NotificationObserver overrides.
   void Observe(int type,

chrome/browser/ui/views/hung_renderer_view.cc

@@ -270,6 +270,11 @@ bool HungRendererDialogView::IsFrameActive(WebContents* contents) {
 
 HungRendererDialogView::HungRendererDialogView()
     : info_label_(nullptr), hung_pages_table_(nullptr), initialized_(false) {
+#if defined(OS_WIN)
+  // Never use the custom frame when Aero Glass is disabled. See
+  // https://crbug.com/323278
+  DialogDelegate::set_use_custom_frame(ui::win::IsAeroGlassEnabled());
+#endif
   set_margins(ChromeLayoutProvider::Get()->GetDialogInsetsForContentType(
       views::TEXT, views::CONTROL));
   chrome::RecordDialogCreation(chrome::DialogIdentifier::HUNG_RENDERER);
@@ -410,16 +415,6 @@ bool HungRendererDialogView::Close() {
   return Accept();
 }
 
-bool HungRendererDialogView::ShouldUseCustomFrame() const {
-#if defined(OS_WIN)
-  // Use the old dialog style without Aero glass, otherwise the dialog will be
-  // visually constrained to browser window bounds. See http://crbug.com/323278
-  return ui::win::IsAeroGlassEnabled();
-#else
-  return views::DialogDelegateView::ShouldUseCustomFrame();
-#endif
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // HungRendererDialogView, HungPagesTableModel::Delegate overrides:
 

chrome/browser/ui/views/hung_renderer_view.h

@@ -171,7 +171,6 @@ class HungRendererDialogView : public views::DialogDelegateView,
   bool Cancel() override;
   bool Accept() override;
   bool Close() override;
-  bool ShouldUseCustomFrame() const override;
 
   // HungPagesTableModel::Delegate overrides:
   void TabUpdated() override;

chrome/browser/ui/views/profiles/user_manager_view.cc

@@ -68,6 +68,8 @@ UserManagerProfileDialogDelegate::UserManagerProfileDialogDelegate(
     const std::string& email_address,
     const GURL& url)
     : parent_(parent), web_view_(web_view), email_address_(email_address) {
+  DialogDelegate::set_use_custom_frame(false);
+
   AddChildView(web_view_);
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
@@ -99,10 +101,6 @@ bool UserManagerProfileDialogDelegate::CanMinimize() const {
   return true;
 }
 
-bool UserManagerProfileDialogDelegate::ShouldUseCustomFrame() const {
-  return false;
-}
-
 ui::ModalType UserManagerProfileDialogDelegate::GetModalType() const {
   return ui::MODAL_TYPE_WINDOW;
 }
@@ -300,6 +298,7 @@ UserManagerView::UserManagerView()
     : web_view_(nullptr),
       delegate_(nullptr),
       user_manager_started_showing_(base::Time()) {
+  DialogDelegate::set_use_custom_frame(false);
   keep_alive_ = std::make_unique<ScopedKeepAlive>(
       KeepAliveOrigin::USER_MANAGER_VIEW, KeepAliveRestartOption::DISABLED);
   chrome::RecordDialogCreation(chrome::DialogIdentifier::USER_MANAGER);
@@ -473,10 +472,6 @@ void UserManagerView::WindowClosing() {
     g_user_manager_view = nullptr;
 }
 
-bool UserManagerView::ShouldUseCustomFrame() const {
-  return false;
-}
-
 void UserManagerView::DisplayErrorMessage() {
   if (delegate_)
     delegate_->DisplayErrorMessage();

chrome/browser/ui/views/profiles/user_manager_view.h

@@ -48,7 +48,6 @@ class UserManagerProfileDialogDelegate
   bool CanResize() const override;
   bool CanMaximize() const override;
   bool CanMinimize() const override;
-  bool ShouldUseCustomFrame() const override;
   ui::ModalType GetModalType() const override;
   void DeleteDelegate() override;
   base::string16 GetWindowTitle() const override;
@@ -128,7 +127,6 @@ class UserManagerView : public views::DialogDelegateView {
   base::string16 GetWindowTitle() const override;
   int GetDialogButtons() const override;
   void WindowClosing() override;
-  bool ShouldUseCustomFrame() const override;
 
   views::WebView* web_view_;
 

chrome/browser/ui/views/task_manager_view.cc

@@ -241,10 +241,6 @@ void TaskManagerView::WindowClosing() {
   table_model_->StoreColumnsSettings();
 }
 
-bool TaskManagerView::ShouldUseCustomFrame() const {
-  return false;
-}
-
 void TaskManagerView::GetGroupRange(int model_index, views::GroupRange* range) {
   table_model_->GetRowsGroupRange(model_index, &range->start, &range->length);
 }
@@ -301,6 +297,7 @@ TaskManagerView::TaskManagerView()
     : tab_table_(nullptr),
       tab_table_parent_(nullptr),
       is_always_on_top_(false) {
+  DialogDelegate::set_use_custom_frame(false);
   Init();
   chrome::RecordDialogCreation(chrome::DialogIdentifier::TASK_MANAGER);
 }

chrome/browser/ui/views/task_manager_view.h

@@ -68,7 +68,6 @@ class TaskManagerView : public TableViewDelegate,
   base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
   bool IsDialogButtonEnabled(ui::DialogButton button) const override;
   void WindowClosing() override;
-  bool ShouldUseCustomFrame() const override;
 
   // views::TableGrouper:
   void GetGroupRange(int model_index, views::GroupRange* range) override;

components/constrained_window/constrained_window_views.cc

@@ -227,7 +227,7 @@ views::Widget* CreateBrowserModalDialogViews(views::DialogDelegate* dialog,
   views::Widget* widget =
       views::DialogDelegate::CreateDialogWidget(dialog, nullptr, parent_view);
 
-  bool requires_positioning = dialog->ShouldUseCustomFrame();
+  bool requires_positioning = dialog->use_custom_frame();
 
 #if defined(OS_MACOSX)
   // On Mac, window modal dialogs are displayed as sheets, so their position is

ui/views/window/dialog_client_view.cc

@@ -225,7 +225,7 @@ void DialogClientView::OnThemeChanged() {
   // dialog style simply inherits the bubble's frame view color.
   const DialogDelegate* dialog = GetDialogDelegate();
 
-  if (dialog && !dialog->ShouldUseCustomFrame()) {
+  if (dialog && !dialog->use_custom_frame()) {
     SetBackground(views::CreateSolidBackground(GetNativeTheme()->GetSystemColor(
         ui::NativeTheme::kColorId_DialogBackground)));
   }

ui/views/window/dialog_client_view_unittest.cc

@@ -39,6 +39,8 @@ class DialogClientViewTest : public test::WidgetTest,
   void SetUp() override {
     WidgetTest::SetUp();
 
+    DialogDelegate::set_use_custom_frame(false);
+
     // Note: not using DialogDelegate::CreateDialogWidget(..), since that can
     // alter the frame type according to the platform.
     widget_ = new views::Widget;
@@ -62,8 +64,6 @@ class DialogClientViewTest : public test::WidgetTest,
     return client_view_;
   }
 
-  bool ShouldUseCustomFrame() const override { return false; }
-
   void DeleteDelegate() override {
     // DialogDelegateView would delete this, but |this| is owned by the test.
   }

ui/views/window/dialog_delegate.cc

@@ -81,9 +81,9 @@ Widget::InitParams DialogDelegate::GetDialogWidgetInitParams(
   DialogDelegate* dialog = delegate->AsDialogDelegate();
 
   if (dialog)
-    dialog->supports_custom_frame_ &= CanSupportCustomFrame(parent);
+    dialog->params_.custom_frame &= CanSupportCustomFrame(parent);
 
-  if (!dialog || dialog->ShouldUseCustomFrame()) {
+  if (!dialog || dialog->use_custom_frame()) {
     params.opacity = Widget::InitParams::TRANSLUCENT_WINDOW;
     params.remove_standard_frame = true;
 #if !defined(OS_MACOSX)
@@ -211,7 +211,7 @@ ClientView* DialogDelegate::CreateClientView(Widget* widget) {
 }
 
 NonClientFrameView* DialogDelegate::CreateNonClientFrameView(Widget* widget) {
-  if (ShouldUseCustomFrame())
+  if (use_custom_frame())
     return CreateDialogFrameView(widget);
   return WidgetDelegate::CreateNonClientFrameView(widget);
 }
@@ -241,10 +241,6 @@ NonClientFrameView* DialogDelegate::CreateDialogFrameView(Widget* widget) {
   return frame;
 }
 
-bool DialogDelegate::ShouldUseCustomFrame() const {
-  return supports_custom_frame_;
-}
-
 const DialogClientView* DialogDelegate::GetDialogClientView() const {
   if (!GetWidget())
     return nullptr;

ui/views/window/dialog_delegate.h

@@ -41,6 +41,12 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
     base::Optional<int> default_button = base::nullopt;
     bool round_corners = true;
     bool draggable = false;
+
+    // Whether to use the Views-styled frame (if true) or a platform-native
+    // frame if false. In general, dialogs that look like fully separate windows
+    // should use the platform-native frame, and all other dialogs should use
+    // the Views-styled one.
+    bool custom_frame = true;
   };
 
   DialogDelegate();
@@ -131,11 +137,6 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
 
   static NonClientFrameView* CreateDialogFrameView(Widget* widget);
 
-  // Returns true if this particular dialog should use a Chrome-styled frame
-  // like the one used for bubbles. The alternative is a more platform-native
-  // frame.
-  virtual bool ShouldUseCustomFrame() const;
-
   const gfx::Insets& margins() const { return margins_; }
   void set_margins(const gfx::Insets& margins) { margins_ = margins; }
 
@@ -155,6 +156,8 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   void set_use_round_corners(bool round) { params_.round_corners = round; }
   void set_draggable(bool draggable) { params_.draggable = draggable; }
   bool draggable() const { return params_.draggable; }
+  void set_use_custom_frame(bool use) { params_.custom_frame = use; }
+  bool use_custom_frame() const { return params_.custom_frame; }
 
  protected:
   ~DialogDelegate() override;
@@ -165,10 +168,6 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   const Params& GetParams() const { return params_; }
 
  private:
-  // A flag indicating whether this dialog is able to use the custom frame
-  // style for dialogs.
-  bool supports_custom_frame_ = true;
-
   // The margins between the content and the inside of the border.
   // TODO(crbug.com/733040): Most subclasses assume they must set their own
   // margins explicitly, so we set them to 0 here for now to avoid doubled

ui/views/window/dialog_delegate_unittest.cc

@@ -69,7 +69,6 @@ class TestDialog : public DialogDelegateView {
   }
   base::string16 GetWindowTitle() const override { return title_; }
   View* GetInitiallyFocusedView() override { return input_; }
-  bool ShouldUseCustomFrame() const override { return true; }
   int GetDialogButtons() const override { return dialog_buttons_; }
 
   void CheckAndResetStates(bool canceled,
@@ -123,12 +122,24 @@ class DialogTest : public ViewsTestBase {
 
   void SetUp() override {
     ViewsTestBase::SetUp();
+
+    // These tests all expect to use a custom frame on the dialog so they can
+    // control hit-testing and other behavior. Custom frames are only supported
+    // with a parent widget, so create the parent widget here.
+    views::Widget::InitParams params =
+        CreateParams(views::Widget::InitParams::TYPE_WINDOW);
+    params.bounds = gfx::Rect(10, 11, 200, 200);
+    params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    parent_widget_.Init(std::move(params));
+    parent_widget_.Show();
+
     InitializeDialog();
     ShowDialog();
   }
 
   void TearDown() override {
     dialog_->TearDown();
+    parent_widget_.Close();
     ViewsTestBase::TearDown();
   }
 
@@ -140,10 +151,14 @@ class DialogTest : public ViewsTestBase {
     dialog_->Init();
   }
 
-  void ShowDialog() {
-    DialogDelegate::CreateDialogWidget(dialog_, GetContext(), nullptr)->Show();
+  views::Widget* CreateDialogWidget(DialogDelegate* dialog) {
+    views::Widget* widget = DialogDelegate::CreateDialogWidget(
+        dialog, GetContext(), parent_widget_.GetNativeView());
+    return widget;
   }
 
+  void ShowDialog() { CreateDialogWidget(dialog_)->Show(); }
+
   void SimulateKeyEvent(const ui::KeyEvent& event) {
     ui::KeyEvent event_copy = event;
     if (dialog()->GetFocusManager()->OnKeyEvent(event_copy))
@@ -151,8 +166,10 @@ class DialogTest : public ViewsTestBase {
   }
 
   TestDialog* dialog() const { return dialog_; }
+  views::Widget* parent_widget() { return &parent_widget_; }
 
  private:
+  views::Widget parent_widget_;
   TestDialog* dialog_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(DialogTest);
@@ -301,7 +318,7 @@ TEST_F(DialogTest, HitTest_CloseButton) {
 TEST_F(DialogTest, BoundsAccommodateTitle) {
   TestDialog* dialog2(new TestDialog());
   dialog2->set_title(base::ASCIIToUTF16("Title"));
-  DialogDelegate::CreateDialogWidget(dialog2, GetContext(), nullptr);
+  CreateDialogWidget(dialog2);
 
   // Remove the close button so it doesn't influence the bounds if it's taller
   // than the title.
@@ -369,8 +386,7 @@ class InitialFocusTestDialog : public DialogDelegateView {
 // focus, test it is still able to receive focus once the Widget is activated.
 TEST_F(DialogTest, InitialFocusWithDeactivatedWidget) {
   InitialFocusTestDialog* dialog = new InitialFocusTestDialog();
-  Widget* dialog_widget =
-      DialogDelegate::CreateDialogWidget(dialog, GetContext(), nullptr);
+  Widget* dialog_widget = CreateDialogWidget(dialog);
   // Set the initial focus while the Widget is unactivated to prevent the
   // initially focused View from receiving focus. Use a minimised state here to
   // prevent the Widget from being activated while this happens.
@@ -402,8 +418,7 @@ TEST_F(DialogTest, UnfocusableInitialFocus) {
   DialogDelegateView* dialog = new DialogDelegateView();
   Textfield* textfield = new Textfield();
   dialog->AddChildView(textfield);
-  Widget* dialog_widget =
-      DialogDelegate::CreateDialogWidget(dialog, GetContext(), nullptr);
+  Widget* dialog_widget = CreateDialogWidget(dialog);
 
 #if !defined(OS_MACOSX)
   // For non-Mac, turn off focusability on all the dialog's buttons manually.