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

components/web_modal/popup_manager.h

+// Copyright 2014 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.
+
+#ifndef COMPONENTS_WEB_MODAL_POPUP_MANAGER_H_
+#define COMPONENTS_WEB_MODAL_POPUP_MANAGER_H_
+
+#include <deque>
+
+#include "base/memory/scoped_ptr.h"
+#include "components/web_modal/single_popup_manager.h"
+#include "content/public/browser/web_contents_user_data.h"
+#include "ui/gfx/native_widget_types.h"
+
+namespace content {
+class FrameNavigateParams;
+struct LoadCommittedDetails;
+class WebContents;
+}
+
+namespace web_modal {
+
+// Per-Browser class to manage popups (bubbles, web-modal dialogs).
+class PopupManager : public SinglePopupManagerDelegate {

[Finnur, 2014/05/16 12:38:14]

What is the scope/lifetime of this object?

[Greg Billock, 2014/05/16 15:49:56]

Good question. I'm not sure how to manage creation, so I left that more-or-less
undefined for now. My sense is that it would be browser-scoped -- probably owned
and accessed through the Browser object, which the tab-specific stuff can access
using FindBrowserWithWebContents. I think most bubbles have access to the
browser. There may be some system-level stuff this doesn't cover, but my guess
is that we probably don't want to manage those anyway.

+ public:
+  virtual ~PopupManager();
+
+  // Schedules a popup governed by the |manager| to be shown. The popup
+  // may not be shown inline with this call, at a later asynchronous time,

[Finnur, 2014/05/16 12:38:14]

s/not// ?
s/asynchronous// ?

[Greg Billock, 2014/05/20 19:27:55]

Done.

+  // or not at all.
+  void ShowPopup(scoped_ptr<SinglePopupManager> manager);
+
+  // Returns true if any popups are active and not closed in the
+  // currently-visible WebContents.
+  bool IsPopupActiveInCurrentWebContents() const;

[Finnur, 2014/05/16 12:38:14]

The WebContents association here I found a bit surprising given that this
manages more than popups associated with a specific WebContents. For example: If
a browser-scoped bubble (managed by this) is currently showing, will this
function return true?

What *active* means should also be spelled out. I presume it is just the
currently visible popup view (bubble or otherwise), right? (not something in the
queue)

This function also seems to be used only in a test. Will that continue to be the
case? If so, should we append ForTesting to the name? 

I guess it would be beneficial, at least for my understanding, to see how this
would be used in "production code", preferably in bubble showing code. :)

[Greg Billock, 2014/05/16 15:49:56]

I don't know that this method survives. The intention is that a caller can use
this to know ahead of time in a predictable way whether something they do will
show up immediately. I think that may end up being useful to know. Of course, if
you've got a user-initiated popup, you don't care, but if not, you may care and
do something different, like turn on a notification or something. I don't know
exactly. But that's the philosophy.

[msw, 2014/05/16 17:10:57]

This part of the API could be more agnostic about the conditions under which a
bubble might show, and instead offer callers the information you describe
"whether something they do will show up immediately" -> CanShowPopup(popup), and
perhaps WillPopupCloseOtherUI(popup), or similar.

[Greg Billock, 2014/05/16 23:28:39]

That sounds good to me. We could also just wait to put those in.

The immediate reason for having this is that if this manager takes on the role
of WCMDM, that class has what amounts to this method being called in the views
code. We may be able to factor it away, however.

[msw, 2014/05/22 04:08:45]

Even if it doesn't make sense to replace this with CanShowPopup or
WillPopupCloseOtherUI right now, I think it should be renamed to IsPopupActive
now, since (as Finnur noted) the association of any popup with the current web
contents is tenuous at best. The comment should also be updated.

[Greg Billock, 2014/05/22 18:25:47]

reworked this substantially

+
+  // Focus the topmost popup.  IsPopupActiveInCurrentWebContents() must be true
+  // when calling this function.

[Finnur, 2014/05/16 12:38:14]

From purely an API standpoint (not looking at the implementation), this last
sentence is a bit weird. Are you saying callers should not call this unless
first calling IsPopupActiveInCurrentWebContents()? Should this be enforced via
DCHECK or something in the implementation instead (or simply just ignore the
call if no popup is active)?

[Greg Billock, 2014/05/16 15:49:56]

This is in the per-WC API for use by the WC view delegate in views, but I'm
suspicious it maybe should not survive. If we end up doing per-WC management
here, we may need it to support that call site, but may want to change the
signature.

[Mike Wittman, 2014/05/16 20:35:40]

The purpose of this function on the WCMD side is to move focus to the dialog
currently being displayed when the web contents view receives focus (e.g. on tab
switch). I imagine this would only be needed for WCMD management.

[Greg Billock, 2014/05/16 23:28:39]

Overall, do you think it should be a design goal to take over WCMDM functions?
Or just have this class perhaps delegate to WCMDMs internally but not take over
their function?

[Mike Wittman, 2014/05/17 00:24:06]

Yes, I'd prefer to see this encompass the WCMDM functions. I think the
similarities in managing WCMDs and popups are likely to be more significant than
the differences.

[msw, 2014/05/20 16:51:32]

I'd also like this class to replace WCMDM, but perhaps not in the first CL.

[Greg Billock, 2014/05/20 19:27:55]

No. I'm planning on this CL basically being a placeholder that gets the API
close, but doesn't take over any functionality yet. I still need to flesh out
who owns it and how you get a hold of it.

+  void FocusTopmostPopup();

[Finnur, 2014/05/16 12:38:14]

This function is not called at the moment. Who do you envision calling this when
this is fully implemented?

[Greg Billock, 2014/05/22 18:25:47]

added call site to cl

+
+  // Called when a NativePopup we own is about to be closed.
+  virtual void WillClose(NativePopup popup) OVERRIDE;

[Finnur, 2014/05/16 12:38:14]

At the moment only called by the test, which is already a friend. I presume it
won't be limited to the test in the future?

[Greg Billock, 2014/05/16 15:49:56]

Right. This is the main entry point for SinglePopupManager to notify the manager
it's closing a popup (probably through user interaction on it, although it could
be because of internal cancellation effects).

[msw, 2014/05/16 17:10:57]

Can't notifications of closure be handled through widget/window observation?
Perhaps this is just a short-term way of integrating WCMDM?

[Mike Wittman, 2014/05/16 20:35:40]

That's how it's currently done for WCMDs: the Views
SingleWebContentsModalDialogManager is a views::WidgetObserver that invokes this
function on WidgetClosing. I suspect this will need to be retained to support
Cocoa since there's no cross-platform mechanism for getting notifications on
widget close.

[Greg Billock, 2014/05/16 23:28:39]

That's correct. If we had a cross-platform "WidgetObserver-alike" we wouldn't
need this. As it is we ask the SinglePopupManager to perform that job.

+
+ private:
+  friend class PopupManagerTest;
+
+  explicit PopupManager();

[Finnur, 2014/05/16 12:38:14]

nit: explicit?

[Greg Billock, 2014/05/20 19:27:55]

Done.

+
+  struct PopupState {
+    PopupState(NativePopup popup, scoped_ptr<SinglePopupManager> manager);
+    ~PopupState();
+
+    NativePopup popup;
+    scoped_ptr<SinglePopupManager> manager;
+    bool close_on_interstitial_webui;
+  };
+
+  typedef std::deque<PopupState*> PopupList;
+
+  // Utility function to get the state for a popup.
+  PopupList::iterator FindPopupState(NativePopup popup);
+
+  // Blocks/unblocks interaction with renderer process.
+  void BlockWebContentsInteraction(bool blocked);
+
+  void CloseAllPopups();

[Finnur, 2014/05/16 12:38:14]

This is currently private. Won't this need to be public?

[Greg Billock, 2014/05/16 15:49:56]

Shouldn't need to be. It isn't in the current WCMDM.

I want to try out the idea that this class absorbs the WCMDM, whether by
reference or in function, and to do so, it needs to support the current calls on
that object. I mangled them a bit, for example changing the WebContentsObserver
methods so we'd do multiple-tab observing as popups are attached to them.

Ultimately we may just want to delegate that to the existing WCMDM and handle it
all there, though. This is where knowing what policy we needed would be a great
help. :-/

+
+  // Notified from content::WebContentsObserver:
+  void DidNavigateMainFrame(
+      content::WebContents* web_contents,
+      const content::LoadCommittedDetails& details,
+      const content::FrameNavigateParams& params);
+  void DidGetIgnoredUIEvent(content::WebContents* web_contents);
+  void WasShown(content::WebContents* web_contents);
+  void WasHidden(content::WebContents* web_contents);
+  void WebContentsDestroyed(content::WebContents* web_contents);
+  void DidAttachInterstitialPage(content::WebContents* web_contents);
+
+  // All active popups.
+  PopupList child_dialogs_;

[Finnur, 2014/05/16 12:38:14]

We seem to be conflating dialogs and popups (bubbles aren't dialogs, per se, at
least not in the traditional sense). child_popups_ ?

[Greg Billock, 2014/05/16 15:49:56]

Yeah, I thought I got them all in the .h file, but I see I missed this one.

[Greg Billock, 2014/05/20 19:27:55]

Done.

+
+  // True while closing all popups.
+  bool closing_all_popups_;
+
+  DISALLOW_COPY_AND_ASSIGN(PopupManager);
+};
+
+}  // namespace web_modal
+
+#endif  // COMPONENTS_WEB_MODAL_POPUP_MANAGER_H_