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

components/web_modal/popup_manager.cc

Index: components/web_modal/popup_manager.cc
diff --git a/components/web_modal/popup_manager.cc b/components/web_modal/popup_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cfa9d1fc5ebdcda78396064903c505bb24499e87
--- /dev/null
+++ b/components/web_modal/popup_manager.cc
@@ -0,0 +1,126 @@
+// Copyright (c) 2012 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/web_modal/popup_manager.h"
+
+#include "components/web_modal/web_contents_modal_dialog_host.h"
+#include "components/web_modal/web_contents_modal_dialog_manager.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_user_data.h"
+#include "ui/gfx/geometry/size.h"
+
+using content::WebContents;
+
+namespace web_modal {
+
+namespace {
+
+const char kPopupManagerUserDataKey[] = "PopupManager";
+
+class PopupManagerRelay : public content::WebContentsUserData<PopupManager> {

[Finnur, 2014/06/30 13:53:36]

nit: Document purpose.

[Greg Billock, 2014/07/01 00:53:04]

Done.

+ public:
+  explicit PopupManagerRelay(base::WeakPtr<PopupManager> manager)
+      : manager_(manager) {}
+
+  virtual ~PopupManagerRelay() {}

[Finnur, 2014/06/30 13:53:36]

Does this need to be virtual?

[Greg Billock, 2014/07/01 00:53:04]

I think so, for UserData.

+
+  base::WeakPtr<PopupManager> manager_;

[Finnur, 2014/06/30 13:53:36]

Make this private plus add a public accessor, instead of exposing it directly.

[Greg Billock, 2014/07/01 00:53:04]

I think this is clear enough. There's no real encapsulation danger.

+};

[Finnur, 2014/06/30 13:53:36]

Nit: Missing DISALLOW_etc?

[Greg Billock, 2014/07/01 00:53:04]

This class is pretty single-purpose, so putting a bunch of boilerplate in it
just obscures its use.

+
+}  // namespace
+
+PopupManager::PopupManager(WebContentsModalDialogHost* host)
+    : host_(host),
+      weak_factory_(this) {}
+
+PopupManager::~PopupManager() {
+}
+
+void PopupManager::ShowPopup(scoped_ptr<SinglePopupManager> manager) {
+  content::WebContents* web_contents = manager->GetBoundWebContents();
+  // TODO(gbillock): get rid of this when we handle bubbles
+  DCHECK(web_contents);
+
+  // TODO(gbillock): remove when we port the popup management logic to this
+  // class.
+  NativeWebContentsModalDialog dialog =
+      static_cast<NativeWebContentsModalDialog>(manager->popup());
+
+  WebContentsModalDialogManager* wm_manager =
+      WebContentsModalDialogManager::FromWebContents(web_contents);
+  DCHECK(wm_manager);
+  wm_manager->ShowModalDialog(dialog);
+}
+
+void PopupManager::ShowModalDialog(NativePopup popup,
+                                   content::WebContents* web_contents) {
+  // TODO make a new native popup manager and call ShowPopup.
+  // For now just lay off to WCMDM.
+  WebContentsModalDialogManager* manager =
+      WebContentsModalDialogManager::FromWebContents(web_contents);
+  manager->ShowModalDialog(popup);
+}
+
+bool PopupManager::IsWebModalDialogActive(
+    const content::WebContents* web_contents) const {
+  if (web_contents == NULL)
+    return false;
+
+  const WebContentsModalDialogManager* manager =
+      WebContentsModalDialogManager::FromWebContents(web_contents);
+  return manager && manager->IsDialogActive();
+}
+
+void PopupManager::WasFocused(const content::WebContents* web_contents) {
+  if (!IsWebModalDialogActive(web_contents))
+    return;
+
+  const WebContentsModalDialogManager* manager =
+      WebContentsModalDialogManager::FromWebContents(web_contents);
+  if (manager)
+    manager->FocusTopmostDialog();

[Finnur, 2014/06/30 13:53:36]

Just looking at this function (without any other context), it is a bit weird
that a function called WasFocused does not return if something has focus, but
instead sets focus. Should it be called OnFocus instead?

[Greg Billock, 2014/07/01 00:53:04]

I feel like OnFocus raises the doubt that the popup manager gets focus, but I
agree WasFocused isn't that clear either.

+}
+
+void PopupManager::WillClose(NativePopup popup) {
+}
+
+void PopupManager::RegisterWith(content::WebContents* web_contents) {
+  web_contents->SetUserData(kPopupManagerUserDataKey,
+                            new PopupManagerRelay(weak_factory_.GetWeakPtr()));

[groby-ooo-7-16, 2014/07/01 18:12:38]

Going back to my earlier question, I guess this is the part I don't understand:
Why do the web contents get a relay with a weak ptr to the manager? What exactly
is the life cycle of the involved objects?

[Greg Billock, 2014/07/01 19:33:19]

PopupManagerRelay is scoped to the WebContents. That's the only way to do
WebContentsUserData. PopupManager is scoped to the browser. The component
architecture barrier makes this the way to attach tab A to slot B. You can't
just do it directly because of DEPS boundaries.

[groby-ooo-7-16, 2014/07/02 20:13:55]

Ah, you're trying to reverse the dependecy. Got you there - but to me, that
means the specific association between popup manager and WebContents is a Chrome
concept, not a component one. And the popup manager should be retrieved via
WebContentsDelegate, similar to the JavaScriptDialogManager.

[Greg Billock, 2014/07/02 21:17:26]

Not exactly. We're sitting in a completely different module, and can have no
chrome dependencies. The delegate has no UserData construct on it, and the
WebContentsDelegate interface can't refer to a PopupManager. The problem to be
solved is "give me the right PopupManager for this WebContents" given those
constraints. I agree this is a bit convoluted, but I think it's the right
solution to that problem.

If you think that's the wrong problem, and callers should just know the Browser
object and get the PopupManager directly from there, that's an argument we had
up-thread and I think picked the right path forward, at least for now -- it is
remarkably hard to get the Browser associated with a WebContents, and that isn't
even what you want -- not all WC have a Browser. So you want the
"PopupManagerOwner" -- but that's pretty much exactly what WCUD is for, so it's
a good tool to use.

[groby-ooo-7-16, 2014/07/03 01:11:25]

Completely agreed - this is the problem we want to solve.
It bugs me that this means the PopupManager "knows" there's an intermediate
Browser object - you'd never have the Relay without that. This essentially means
we have internals leaking out from chrome/ to components/

The "standard" solution seems to be something like
* Add a PopupManagerHelper object which does nothing except store a
PopupManagerDelegate
* Browser derives from PopupManagerDelegate. One method: GetPopupManager().
* Browser::SetAsDelegate calls
PopupManagerHelper::FromWebContents()->set_delegate(this). 
* PopupManager::FromWebContents calls
PopupManagerHelper::FromWebContents()->GetPopupManager();

just like WebContentsModalDialogManager does now. It's rather similar, and also
a bit messy, but at least it's consistent with what we do in other such cases,
it has only a single entry point on the component
(PopupManagerHelper::set_delegate), and a single point of control in chrome -
Browser::SetDelegate.

I don't know if that justifies shuffling things around, but it feels _slightly_
better to me. 

[Greg Billock, 2014/07/03 01:33:15]

I'm not sure what you mean -- there's no knowledge of Browser here (there can't
be).
 
There will probably be a PopupHost (nee PopupManagerDelegate in the end, but
that object will end up swalling up the WebModalDialogHost and use a separate
API than what PopupManager clients would use. If you want to talk to Mike and
sort out a different approach to this migration, that's fine with me.
The WCMDM is a per-WC-scoped object, so its in the sweet spot of the WCUD API.
This class is basically just using that API without really committing to the
usual convention. That's done a couple of other places, but I don't think it's
particularly awesome.
Maybe. To me that seems more double jointed, and with more public surface area,
than this solution. If you and Mike want to do it that way, though, that's fine.

[groby-ooo-7-16, 2014/07/03 02:23:44]

There is, and there isn't. There's no _reference_ to Browser, but the existence
of the relay is caused by knowing about the Browser object. The content layer
doesn't expose the idea of intermediary groupings. 
There both double-jointed and awkward. I wish I had a better suggestion. I'm not
too happy about this one either, so unless it makes Mike jump up and down with
delight, I'm fine with tabling this for now. Until we have a better idea for the
underlying problem of WebContent groups, at least.

[Mike Wittman, 2014/07/03 03:21:06]

It seems to me the PopupManager has to know that there may be a group of
multiple WebContents' associated with a single host object, since it's intended
to manage display among a pool of dialogs scoped to different WebContents and
dialogs scoped to the Browser. 
 
I'm slightly leaning towards Greg's approach based on the API surface area
argument, but I think it's worth keeping this approach in the back pocket.

I suspect that the determining factor for what's best will be the implementation
of transferring WebContents between PopupManagers in the case of dragging tabs
outside of the Browser. I'm not entirely sure how this will work at the moment,
but I suspect with the PopupManagerHelper approach it would involve a bunch of
work in PopupManagerHelper::SetDelegate(), which also seems fairly non-standard.

+  // TODO(gbillock): Need to do something more extreme here to manage changing
+  // popup managers with popups in-flight?
+}
+
+void PopupManager::UnregisterWith(content::WebContents* web_contents) {
+  web_contents->RemoveUserData(kPopupManagerUserDataKey);
+  // TODO(gbillock): Need to do something more extreme here to manage changing
+  // popup managers with popups in-flight?
+}
+
+PopupManager* PopupManager::FromWebContents(
+    content::WebContents* web_contents) {
+  PopupManagerRelay* relay = static_cast<PopupManagerRelay*>(
+      web_contents->GetUserData(kPopupManagerUserDataKey));
+  if (!relay)
+    return NULL;
+
+  return relay->manager_.get();

[Finnur, 2014/06/30 13:53:36]

Maybe there's something I'm missing, but why do you need the relay? Can you do
away with it and store the weak_ptr in there directly (without the wrapper)?

[Greg Billock, 2014/07/01 00:53:04]

No, not really. The object is in the wrong component scope, so it can't hold a
reference to this object directly. The UserData mechanism is the way to deal
with that.

+}
+
+gfx::NativeView PopupManager::GetHostView() const {
+  // TODO(gbillock): replace this with a PopupManagerHost or something.
+  DCHECK(host_);
+  return host_->GetHostView();
+}
+
+void PopupManager::CloseAllDialogsForTesting(
+    content::WebContents* web_contents) {
+  // TODO: re-implement, probably in terms of something in the host_,
+  // or of owned WCMDMs.

[Finnur, 2014/06/30 13:53:36]

nit: This comment is hard to parse -- what does "re-implement in terms of" mean?

[Greg Billock, 2014/07/01 00:53:04]

This class will end up either consuming or owning the modal dialog managers.
This method probably shouldn't even exist, but if it does, it needs to use them
directly.

+  WebContentsModalDialogManager* manager =
+      WebContentsModalDialogManager::FromWebContents(web_contents);
+  manager->CloseAllDialogs();
+}
+
+}  // namespace web_modal