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

chrome/browser/ui/browser_commands.cc

Index: chrome/browser/ui/browser_commands.cc
diff --git a/chrome/browser/ui/browser_commands.cc b/chrome/browser/ui/browser_commands.cc
index 8a593d55da12f375366a2edd253feb7f6b24ab29..455d72374fb8c3beaf747665ae765ce1eca8fc45 100644
--- a/chrome/browser/ui/browser_commands.cc
+++ b/chrome/browser/ui/browser_commands.cc
@@ -64,7 +64,7 @@
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/translate/core/browser/language_state.h"
-#include "components/web_modal/web_contents_modal_dialog_manager.h"
+#include "components/web_modal/popup_manager.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
@@ -109,7 +109,6 @@ using content::OpenURLParams;
 using content::Referrer;
 using content::SSLStatus;
 using content::WebContents;
-using web_modal::WebContentsModalDialogManager;
 
 namespace chrome {
 namespace {
@@ -251,9 +250,14 @@ bool IsShowingWebContentsModalDialog(const Browser* browser) {
   if (!web_contents)
     return false;
 
-  WebContentsModalDialogManager* web_contents_modal_dialog_manager =
-      WebContentsModalDialogManager::FromWebContents(web_contents);
-  return web_contents_modal_dialog_manager->IsDialogActive();
+  // TODO(gbillock): This gets called by the CanPrint method, and may be too
+  // restrictive if we allow print preview to overlap -- we should just queue
+  // print requests or attach a user gesture or whatever we know.

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

Nit: I don't know what "whatever we know" means here.  Also, if this check is
only for tests, does this TODO really matter?  Or are you saying this can affect
non-test code?

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

Me either. This is dependent on popup policy which is TBD.
The TODO is for the whole notion that this code stanza should exist in anything
resembling its current form, which it likely should not. On the bright side,
it's perfectly fine in the current state of things -- it's only bad in a
centrally-managed-popups world.

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

I might just rewrite the comment to be simpler then, like "TODO: When we move to
centrally-managed popups, this code likely doesn't make sense [optionally:
"because x"].  Instead think about <blah>."

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

Done.

+  // Note: check because tests may not have a popup manager.

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

Nit: Move this sentence above the TODO and rewrite: "In test code we may not
have a popup manager."

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

Done.

+  if (!browser->popup_manager())
+    return false;
+
+  return browser->popup_manager()->IsWebModalDialogActive(web_contents);
 }
 
 bool PrintPreviewShowing(const Browser* browser) {