MacViews: Unify web contents modal dialog types

chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm

Index: chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
diff --git a/chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm b/chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
index 3c9dc5916e63b0fd7e4038649bdb239a165140a3..e412c96d509d4a633dfda7294588be1e6c8c42af 100644
--- a/chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
+++ b/chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
@@ -5,15 +5,9 @@
 #include "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h"
 
 #include "base/logging.h"
-#include "chrome/browser/ui/browser_finder.h"
-#include "chrome/browser/ui/browser_window.h"
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h"
-#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet_controller.h"
-#import "chrome/browser/ui/cocoa/tabs/tab_strip_controller.h"
-#include "components/web_modal/popup_manager.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/web_contents.h"
 #include "extensions/browser/guest_view/guest_view_base.h"
 
 using web_modal::WebContentsModalDialogManager;
@@ -25,8 +19,7 @@ ConstrainedWindowMac::ConstrainedWindowMac(
     id<ConstrainedWindowSheet> sheet)
     : delegate_(delegate),
       web_contents_(NULL),
-      sheet_([sheet retain]),
-      shown_(false) {
+      sheet_([sheet retain]) {
   DCHECK(web_contents);
   extensions::GuestViewBase* guest_view =
       extensions::GuestViewBase::FromWebContents(web_contents);
@@ -35,58 +28,27 @@ ConstrainedWindowMac::ConstrainedWindowMac(
   web_contents_ = guest_view && guest_view->embedder_web_contents() ?
                       guest_view->embedder_web_contents() : web_contents;
   DCHECK(sheet_.get());
-  web_modal::PopupManager* popup_manager =
-      web_modal::PopupManager::FromWebContents(web_contents_);
-  if (popup_manager)
-    popup_manager->ShowModalDialog(this, web_contents_);
+
+  auto manager = WebContentsModalDialogManager::FromWebContents(web_contents_);
+  native_manager_ = new WebContentsModalDialogManagerCocoa(sheet_, manager);
+  native_manager_->AddObserver(this);
+  manager->ShowDialogWithManager([sheet_ sheetWindow],
+                                 make_scoped_ptr(native_manager_));

[tapted, 2015/02/26 23:34:49]

This feels a bit like "cheating" to me :). i.e. passing a scoped_ptr while
holding on to a reference.

I think the observer-interface could be formalized more. That is, instead of
being just an observer, inherit from a `CocoaDialogOwner` (something like
ui::LayerOwner I guess), or perhaps `CocoaModalDialogClient` (or delegate..).
This way the WebContentsModalDialogManagerCocoa can be responsible for
clobbering the reference, rather than relying on client code to add observers
and correctly nulling-out references.

I think there is currently only be one observer, so observer-style isn't really
needed.

The CocoaModalDialogClient base is initialized first in the constructor
initializer list with the webcontents. (the data member probably isn't needed on
ConstrainedWindowMa as well)

The code here would be just `super::ShowSheet(sheet)`, being

CocoaModalDialogClient::ShowSheet(id<ConstrainedWindowSheet> sheet) {
  auto manager = WebContentsModalDialogManager::FromWebContents(web_contents_);
  scoped_ptr<SingleWebContentsModalDialogManagerCocoa> native_manager(
      new SingleWebContentsModalDialogManagerCocoa(this, sheet, manager));
  WebContentsModalDialogManager::FromWebContents(web_contents_)
      ->ShowDialogWithManager([sheet_ sheetWindow], native_manager.Pass());
}

[maybe you can even pass web_contents to ShowSheet as well, and not have a data
member]

And the constructor,

SingleWebContentsModalDialogManagerCocoa(CocoaModalDialogClient* client, sheet)
    : client_(client) {
  /* other stuff */
  client->SetManager(this);
}

and then SingleWebContentsModalDialogManagerCocoa just does
client_->SetManager(nullptr); whenever it needs to.

[Andre, 2015/03/02 02:55:56]

Done, PTAL.
But the separation between ModalDialogClient and ConstrainedWindowMac does not
feel that
useful now. Maybe we should just merge them?

[tapted, 2015/03/02 04:28:50]

I'm not opposed to merging them, but there might be a curly lifetime issue that
makes it not-quite-fit.. (i guess we'd have to try it :). But this seems
reasonably neat to me.

[Andre, 2015/03/02 18:47:31]

I merged them in the latest patch, it looks good.

[tapted, 2015/03/02 22:25:46]

still lgtm after the merge.

 }
 
 ConstrainedWindowMac::~ConstrainedWindowMac() {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-}
-
-void ConstrainedWindowMac::ShowWebContentsModalDialog() {
-  if (shown_)
-    return;
-
-  NSWindow* parent_window = web_contents_->GetTopLevelNativeWindow();
-  NSView* parent_view = GetSheetParentViewForWebContents(web_contents_);
-  if (!parent_window || !parent_view)
-    return;
-
-  shown_ = true;
-  ConstrainedWindowSheetController* controller =
-      [ConstrainedWindowSheetController
-          controllerForParentWindow:parent_window];
-  [controller showSheet:sheet_ forParentView:parent_view];
+  if (native_manager_)

[tapted, 2015/02/26 23:34:49]

I *think* this should be DCHECK(!GetManager());  [is there a codepath where that
fails?]

i.e. the destructor is usually too late for these things -- in my experience, if
the window hasn't been closed by this point then something is leaking.

Maybe see WindowedInstallDialogController in
chrome/browser/ui/cocoa/extensions/windowed_install_dialog_controller.h for
ideas

[Andre, 2015/03/02 02:55:55]

Done. Added the DCHECK in ~ModalDialogClientCocoa.

+    native_manager_->RemoveObserver(this);
 }
 
 void ConstrainedWindowMac::CloseWebContentsModalDialog() {
-  [[ConstrainedWindowSheetController controllerForSheet:sheet_]
-      closeSheet:sheet_];
-  // TODO(gbillock): get this object in config, not from a global.
-  WebContentsModalDialogManager* web_contents_modal_dialog_manager =
-      WebContentsModalDialogManager::FromWebContents(web_contents_);
+  if (native_manager_)

[tapted, 2015/02/26 23:34:49]

in a `CocoaModalDialgClient` world this would be

if (GetManager())
  GetManager()->Close();

[Andre, 2015/03/02 02:55:56]

Done.
I went with calling super's Close() to match how Show() is done.

+    native_manager_->Close();
+}
 
-  // Will result in the delegate being deleted.
+void ConstrainedWindowMac::OnDialogClosing() {
+  native_manager_ = nullptr;
   if (delegate_)
     delegate_->OnConstrainedWindowClosed(this);
-
-  // Will cause this object to be deleted.
-  web_contents_modal_dialog_manager->WillClose(this);
-}
-
-void ConstrainedWindowMac::FocusWebContentsModalDialog() {
-}
-
-void ConstrainedWindowMac::PulseWebContentsModalDialog() {
-  [[ConstrainedWindowSheetController controllerForSheet:sheet_]
-      pulseSheet:sheet_];
-}
-
-NativeWebContentsModalDialog ConstrainedWindowMac::GetNativeDialog() {
-  // TODO(wittman): Ultimately this should be changed to the
-  // ConstrainedWindowSheet pointer, in conjunction with the corresponding
-  // changes to NativeWebContentsModalDialogManagerCocoa.
-  return this;
 }