[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 {
+struct FrameNavigateParams;
+struct LoadCommittedDetails;
+class WebContents;
+}
+
+namespace web_modal {
+
+// Per-Browser class to manage popups (bubbles, web-modal dialogs).
+class PopupManager : public SinglePopupManagerDelegate {
+ public:
+  PopupManager();
+
+  virtual ~PopupManager();
+
+  // Schedules a popup governed by the |manager| to be shown. The popup
+  // may be shown inline with this call, at a later time, 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;

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

As per the older comment thread, consider IsPopupActive with a comment update.

In fact, directly porting WCMDM notions into PopupManager API will cause more
problems than it alleviates. For example, WCMDM knows that only one dialog may
be showing at a time, whereas this new manager might have multiple
dialogs/bubbles shown simultaneously, and be tracking some dialogs that are not
being shown for the current tab (like print preview in a non-active tab). I
think changing this to something like: IsPopupActive at least lets us return
true if one or more popups are being shown, regardless of their relation to the
active web contents or simply the browser window.

Even further, we should be thinking in terms of "what is the set of popups that
are currently being shown?", as that question relates to the next function. ie.
What if the only entries in |child_popups_| are for non-active web contents?
(all popups are hidden, because they're not associated with the active web
contents, but this returns true).

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

I've brought call sites into the CL now, and there are quite a few with varying
needs. I'm now not sure this method will actually perform the function needed,
and depending on the migration we may want just a direct
"IsWebModalDialogActive()" signature if we want to make progress faster. There's
some spots where we probably shouldn't call at all (i.e. print). Others where we
kind of want other information (block tab close?), and others where it's not
clear to me what we really need (mac extension/web-modal interaction).

[msw, 2014/05/22 21:40:02]

That's totally reasonable, at least for now. If no callers care about other
popups (ie. bubbles) for now, and the only concern is for WCMDs on the active
tab, then IsWebModalDialogActive() sounds perfect, and we can add more API and
consolidate as appropriate later.

[Greg Billock, 2014/05/27 22:42:02]

ok, let's head that direction. Added this API with a note that it's deprecated.

+
+  // Focus the topmost popup.  IsPopupActiveInCurrentWebContents() must be true

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

Perhaps this should just be a no-op if IsPopupActiveInCurrentWebContents is
false, rather than requiring the user of the API to do extra legwork. The same
could be said (and easily fixed) for the corresponding WCMDM functions. I think
some simple cleanup like that on WCMDM would be beneficial before copying all of
its detritus over to this class.

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

Agreed.

+  // when calling this function.
+  void FocusTopmostPopup();

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

Can you describe what topmost means in the comment? ie. showing(?), z-order(?)
sole popup(?). In fact, the placeholder implementation seems wrong given that a
*set* of popups may be showing at the moment, none of which are the front of the
|child_popups_| list (because the first element in the list happens to be a
print preview dialog for a tab that is not active).

This sort of complexity should at least be mentioned in the function comment, if
not addressed through a more nuanced API, like |PopupList GetVisiblePopups()|
and |bool FocusPopup(Popup* popup)|, or similar. Again, I think the
WCMDM::FocusTopmostDialog notion that you've copied here doesn't directly
translate to this new PopupManager.

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

I've changed this significantly, updated the comment, and left a note in the
call site about what it should do. I agree the implementation here is tricky and
a bit underspecified at present. For instance, it isn't at all clear to me what
we should do if there's a sticky popup that would ordinarily stick on tab switch
but that would conflict. Can this even happen?

[msw, 2014/05/22 21:40:02]

Oh jeez, you mean (1) a wcmd is shown for Tab A (2) Use switches to Tab B (3) A
sticky 'blocking' bubble is shown on Tab B (4) User switches back to Tab A (5)
What do we do? Yikes, I dunno, probably show the wcmd under the 'blocking'
bubble, or maybe delay re-showing the wcmd until the bubble is gone? Yeah, the
emergent behavior of interactions between loosely planned UI additions will
result in insane problems. Replace wcmd with a 'tab-modal' permission bubble
that wants to reshow on tab switch, and you've got a similar problem... Luckily
we can defer resolution of this behavior to followup CLs.

I still think the notion of a Topmost popup here doesn't make sense, unless we
decide (and clarify) that the most recently shown popup is "Topmost" or similar.
Since the callers are only concerned with WCMDs and this doesn't necessarily
make sense for bubbles, perhaps this could just deal with WCMDs for now (like we
are considering for IsPopupActive above). aka, for now, this could be:
FocusWebModalDialog(WebContents*). (IfActive could probably be implicit or just
noted in the function comment)

[Finnur, 2014/05/23 15:29:52]

This is anecdotal, but I think I've only used bubble sticky-ness as a way to
prevent them from going away during the startup sequence (avoid getting lost in
the "focus storm"). Maybe they could simply be closed when switching to another
tab that has an active WCMD popup?

[msw, 2014/05/23 17:22:42]

I'm very much in favor of any effort to reduce the stickiness of bubbles, they
were never meant to be so persistent. That said, I think it depends on a
case-by-case basis.

[Greg Billock, 2014/05/27 22:42:02]

Do we have enough info in the SinglePopupManager to do the right thing? We can
always add more there...

There's a single call site to this, and if it where in chrome/ I'd make it
private and friend the views caller. I'm suspicious we should be doing what gets
done there within the bubble manager itself -- that is, showing the tab-scoped
web modals upon re-activation -- based on the WasShown/WasHidden signals from
the WebcontentsObserver, and that the views code should move in-class. How does
mac do this, though, or put another way, why does views even need to do it? Is
focusing not part of re-activation? Or not reliably on views but on mac it is?

+
+  // Called when a NativePopup we own is about to be closed.
+  virtual void WillClose(NativePopup popup) OVERRIDE;
+
+ private:
+  friend class PopupManagerTest;
+
+  struct PopupState {

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

How does this relate to SinglePopupManager itself? It seems like they are
redundant, and the PopupList could simply be a list of SinglePopupManager
instances that can identify their NativePopup objects via popup(). The same
could be said and perhaps fixed for WCMDM before we bring the same pattern over
to this new manager.

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

Yeah, that was kind of a long-term plan for WCMDM. I'll add a
close_on_navigation accessor and just manage the SPMs directly.

+    PopupState(NativePopup popup, scoped_ptr<SinglePopupManager> manager);
+    ~PopupState();
+
+    NativePopup popup;
+    scoped_ptr<SinglePopupManager> manager;
+    bool close_on_interstitial_webui;

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

nit: I removed the corresponding flag from WCMDM's DialogState in r271250. This
was always true for WCMDs (except on Mac, where it's always false). I'll
hopefully make it always true on Mac soon.

That said, this won't always be true for bubbles (especially 'sticky' bubbles).
Perhaps this would be better named |close_on_navigation|, where the attachment
or detachment of an interstitial to the underlying web contents would be treated
here as a 'navigation' and would cause the bubble or dialog to close.

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

Sensible. I added this as a field of the SinglePopupManager. That should cover
things from the popup manager perspective.

+  };
+
+  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();
+
+  // 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_popups_;
+
+  // True while closing all popups.
+  bool closing_all_popups_;
+
+  DISALLOW_COPY_AND_ASSIGN(PopupManager);
+};
+
+}  // namespace web_modal
+
+#endif  // COMPONENTS_WEB_MODAL_POPUP_MANAGER_H_