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

components/constrained_window/constrained_window_views_unittest.cc

Index: components/constrained_window/constrained_window_views_unittest.cc
diff --git a/components/constrained_window/constrained_window_views_unittest.cc b/components/constrained_window/constrained_window_views_unittest.cc
index e929867eb8175a7a8d8ebd8e0206e879f0b8dd0e..5ff3034e3d3decc86b888c28b2797449c0fea61d 100644
--- a/components/constrained_window/constrained_window_views_unittest.cc
+++ b/components/constrained_window/constrained_window_views_unittest.cc
@@ -7,6 +7,7 @@
 #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"
@@ -25,30 +26,94 @@ 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);
@@ -154,4 +219,19 @@ TEST_F(ConstrainedWindowViewsTest, MaximumWebContentsDialogSize) {
   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