[WebModals] New API for browser-scoped popup management.

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index db19aee5d85607283b8c7f1bf17e173b15e94a7a..35a16dabf3b0501cd83ea7c90567edfc295a58bb 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -152,6 +152,7 @@
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/google/core/browser/google_url_tracker.h"
 #include "components/startup_metric_utils/startup_metric_utils.h"
+#include "components/web_modal/popup_manager.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/devtools_manager.h"
 #include "content/public/browser/download_item.h"
@@ -444,6 +445,12 @@ Browser::Browser(const CreateParams& params)
   }
 
   fullscreen_controller_.reset(new FullscreenController(this));
+
+  // Must be initialized after window_.
+  // This would DCHECK on presence of the dialog host but tests
+  // may not implement, and currently Cocoa does not either.

[Peter Kasting, 2014/07/08 19:13:54]

Nit: I completely fail to understand this second sentence.  Are you trying to
say something like this?:

Ideally we would DCHECK that GetWebContentsModalDialogHost() returned non-NULL
here, but on Mac or in certain tests that will fail.

(And if you are, _why_ would we ideally DCHECK this?  How can it be important if
Mac doesn't do it?)

[Greg Billock, 2014/07/09 19:28:12]

I can remove it if its puzzling. I found a DCHECK here to validate the ordering
to be a very inviting idea, and surprising that it won't work, hence the
comment.

[Peter Kasting, 2014/07/09 19:41:41]

I'm not sure you need to remove the comment, but it needs to spell things out
explicitly for readers like me who don't know why one might expect this function
to return non-NULL to begin with.  Part of why the current comment is confusing
is that it assumes the reader understands why one would want this DCHECK in the
first place.

[Greg Billock, 2014/07/10 18:32:03]

Done.

+  popup_manager_.reset(new web_modal::PopupManager(
+      GetWebContentsModalDialogHost()));
 }
 
 Browser::~Browser() {
@@ -892,6 +899,10 @@ void Browser::TabInsertedAt(WebContents* contents,
                             int index,
                             bool foreground) {
   SetAsDelegate(contents, true);
+
+  if (popup_manager_.get())

[Peter Kasting, 2014/07/08 19:13:54]

Nit: Remove get() (not necessary to NULL-check a scoped_ptr anymore) (3 places)

[Greg Billock, 2014/07/09 19:28:12]

Done.

+    popup_manager_->RegisterWith(contents);
+
   SessionTabHelper* session_tab_helper =
       SessionTabHelper::FromWebContents(contents);
   session_tab_helper->SetWindowID(session_id());
@@ -933,6 +944,9 @@ void Browser::TabClosingAt(TabStripModel* tab_strip_model,
       content::Source<NavigationController>(&contents->GetController()),
       content::NotificationService::NoDetails());
 
+  if (popup_manager_.get())
+    popup_manager_->UnregisterWith(contents);
+
   // Sever the WebContents' connection back to us.
   SetAsDelegate(contents, false);
 }
@@ -948,6 +962,10 @@ void Browser::TabDetachedAt(WebContents* contents, int index) {
       session_service->SetSelectedTabInWindow(session_id(),
                                               old_active_index - 1);
   }
+
+  if (popup_manager_.get())
+    popup_manager_->UnregisterWith(contents);
+
   TabDetachedAtImpl(contents, index, DETACH_TYPE_DETACH);
 }