MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window.

components/constrained_window/constrained_window_views_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/constrained_window/constrained_window_views.h"
 
 #include <memory>
 
 #include "base/macros.h"
+#include "components/constrained_window/constrained_window_views_client.h"
 #include "components/web_modal/test_web_contents_modal_dialog_host.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/border.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 
 using views::Widget;
 
 namespace constrained_window {
 namespace {
 
 class DialogContents : public views::DialogDelegateView {
  public:
   DialogContents() {}
-  ~DialogContents() override {}
+  ~DialogContents() override {
+    if (destroy_watcher_)
+      *destroy_watcher_ = true;
+  }
 
   void set_preferred_size(const gfx::Size& preferred_size) {
     preferred_size_ = preferred_size;
   }
 
-  // Overriden from DialogDelegateView:
+  void set_modal_type(ui::ModalType modal_type) { modal_type_ = modal_type; }
+  void set_destroy_watcher(bool* destroy_watcher) {
+    destroy_watcher_ = destroy_watcher;
+  }
+
+  // DialogDelegateView:
   views::View* GetContentsView() override { return this; }
   gfx::Size GetPreferredSize() const override { return preferred_size_; }
   gfx::Size GetMinimumSize() const override { return gfx::Size(); }
 
+  // WidgetDelegate:
+  ui::ModalType GetModalType() const override { return modal_type_; }
+
  private:
   gfx::Size preferred_size_;
+  ui::ModalType modal_type_ = ui::MODAL_TYPE_NONE;
+  bool* destroy_watcher_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(DialogContents);
 };
 
+class TestConstrainedWindowViewsClient

[msw, 2016/10/18 23:47:15]

nit: add a simple class comment, event something like: "Dummy client that
returns a null modal dialog host and host view."

[tapted, 2016/10/19 08:13:48]

Done.

+    : public constrained_window::ConstrainedWindowViewsClient {
+ public:
+  TestConstrainedWindowViewsClient() {}
+
+  // ConstrainedWindowViewsClient:
+  web_modal::ModalDialogHost* GetModalDialogHost(
+      gfx::NativeWindow parent) override {
+    return nullptr;
+  }
+  gfx::NativeView GetDialogHostView(gfx::NativeWindow parent) override {
+    return nullptr;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestConstrainedWindowViewsClient);
+};
+
+// ViewsDelegate to provide context to dialog creation functions such as
+// CreateBrowserModalDialogViews() which do not allow InitParams to be set, and
+// pass a null |context| argument to DialogDelegate::CreateDialogWidget().
+class TestViewsDelegateWithContext : public views::TestViewsDelegate {
+ public:
+  TestViewsDelegateWithContext() {}
+
+  void set_context(gfx::NativeWindow context) { context_ = context; }
+
+  // ViewsDelegate:
+  void OnBeforeWidgetInit(
+      views::Widget::InitParams* params,
+      views::internal::NativeWidgetDelegate* delegate) override {
+    if (!params->context)

[msw, 2016/10/18 23:47:15]

It's a bummer that we need this, but IIRC a null context (and a null parent) is
not okay on any platform... Should we just set the ViewsTestBase::GetContext()
as a backup/default in TestViewsDelegate::OnBeforeWidgetInit? Perhaps that's too
heavy of a hammer? WDYT?

[tapted, 2016/10/19 08:13:48]

It was certainly true that Weird Things would happen with a null context|parent
when Metro mode was involved. Now things are mostly sane:
 - On ChromeOS, ChromeViewsDelegate::OnBeforeWidgetInit will DCHECK only if
InitParams::child is set (in combination with a null parent and null context).
But fall back to ash::Shell::GetPrimaryRootWindow() in all cases when parent and
context are both null.
 - On other Aura platforms, ChromeViewsDelegate doesn't DCHECK and just uses a
DesktopNativeWidgetAura when context is null
 - On Mac, there's just the one NativeWidget type - so widget.cc's
CreateNativeWidget(..) will just invoke
internal::NativeWidgetPrivate::CreateNativeWidget(delegate) most of the time

I guess going forward (and, e.g., for Mus), the logic in OnBeforeWidgetInit()
might move to native_widget_factory(), but currently ChromeViewsDelegate seems
OK with the null-context+parent arrangement.

But it's only *desktop* aura widgets that are OK with a null context. I had to
add this because of the DCHECK in NativeWidgetAura::InitNativeWidget (I could
set TestViewsDelegate::use_desktop_widgets, but then I couldn't run the test on
ChromeOS). Hm. I guess this test has more appropriate coverage with
use_desktop_widgets anyway, so I should do that. [Done.]


So.. I suppose it's OK for TestViewsDelegate::OnBeforeWidgetInit() to fallback
to GetContext(), since that's what ChromeOS does. However, maybe there are
implications for Mus down the track? i.e. if we are OK with the GetContext()
fallback, we should be able to scrap InitParams::context altogether and instead
always get it from ViewsDelegate. Which is a much scarier change..

[msw, 2016/10/19 19:02:07]

This is certainly fine as-is, it's a much more directed change.

+      params->context = context_;
+    TestViewsDelegate::OnBeforeWidgetInit(params, delegate);
+  }
+
+ private:
+  gfx::NativeWindow context_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(TestViewsDelegateWithContext);
+};
+
 class ConstrainedWindowViewsTest : public views::ViewsTestBase {
  public:
   ConstrainedWindowViewsTest() : contents_(nullptr), dialog_(nullptr) {}
   ~ConstrainedWindowViewsTest() override {}
 
   void SetUp() override {
+    std::unique_ptr<TestViewsDelegateWithContext> views_delegate(
+        new TestViewsDelegateWithContext);
+
+    // set_views_delegate() must be called before SetUp(), and GetContext() is
+    // null before that, so take a reference.
+    TestViewsDelegateWithContext* views_delegate_weak = views_delegate.get();
+    set_views_delegate(std::move(views_delegate));
     views::ViewsTestBase::SetUp();
+    views_delegate_weak->set_context(GetContext());
+
     contents_ = new DialogContents;
     dialog_ = views::DialogDelegate::CreateDialogWidget(
         contents_, GetContext(), nullptr);
     dialog_host_.reset(new web_modal::TestWebContentsModalDialogHost(
         dialog_->GetNativeView()));
     dialog_host_->set_max_dialog_size(gfx::Size(5000, 5000));
 
     // Make sure the dialog size is dominated by the preferred size of the
     // contents.
     gfx::Size preferred_size = dialog()->GetRootView()->GetPreferredSize();
@@ skipping 85 matching lines @@
   EXPECT_EQ(expected_size.ToString(), GetDialogSize().ToString());
 
   // Increasing the maximum dialog size should bring the dialog back to its
   // original size.
   max_dialog_size.Enlarge(100, 100);
   dialog_host()->set_max_dialog_size(max_dialog_size);
   UpdateWebContentsModalDialogPosition(dialog(), dialog_host());
   EXPECT_EQ(full_dialog_size.ToString(), GetDialogSize().ToString());
 }
 
+// Ensure CreateBrowserModalDialogViews() works correctly with a null parent.
+TEST_F(ConstrainedWindowViewsTest, NullModalParent) {
+  bool destroyed = false;
+  SetConstrainedWindowViewsClient(
+      base::MakeUnique<TestConstrainedWindowViewsClient>());
+  DialogContents* contents = new DialogContents;
+  contents->set_destroy_watcher(&destroyed);
+  contents->set_modal_type(ui::MODAL_TYPE_WINDOW);
+  views::Widget* widget = CreateBrowserModalDialogViews(contents, nullptr);
+  widget->Show();
+  EXPECT_TRUE(widget->IsVisible());
+  widget->CloseNow();
+  EXPECT_TRUE(destroyed);

[msw, 2016/10/18 23:47:15]

nit: this (and the supporting code) is probably unnecessary.

[tapted, 2016/10/19 08:13:48]

Done. (I think I added this to satisfy myself that `CloseNow` was working
correctly, since Window-Modal "sheets" on Mac currently don't like being closed
synchronously due to the way it interacts with OS window animations, but other
DCHECKs should be hit if that goes awry).

+}
+
 }  // namespace constrained_window