Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
287123002:
[WebModals] New API for browser-scoped popup management. (Closed)
|
Created:
6 years, 7 months ago by Greg Billock Modified:
6 years, 5 months ago Reviewers:
Mike Wittman, Finnur, Yoyo Zhou, benwells, Peter Kasting, bartfab (slow), tapted, groby-ooo-7-16, koz (OOO until 15th September), msw, Nikita (slow), asargent_no_longer_on_chrome, Jeffrey Yasskin, Marijn Kruisselbrink, satorux1, Ben Goodger (Google), blundell CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[WebModals] New API for browser-scoped popup management.
This interface follows the WebContents-scoped WebContentsModalDialogManager
closely, the main difference being the change in scope -- the popup
manager is intended to manage popups (bubbles and web-modals) scoped both
at the per-WebContents level as well as at the browser/system level.
The policy for the manager is not known yet, so this change is setting up
the API which will be used to register popups with the manager, and the
lifecycle which the manager uses to display them. The critical facts about
a popup are expected to be:
1. Whether it is user-initiated.
2. If it is scoped to a particular WebContents (and if so, which one),
3. If it is overlappable.
Other important management facts about popups may be added later.
The lifecycle operation is all driven through an interface called
SinglePopupManager which is a one-per-window scoped manager owned by
the PopupManager. These classes take charge of showing and hiding
their popups depending on the state of the browser window, as governed
by whatever state change observations made by the overall PopupManager
relate to its policy.
When a window is closed by direct user action, the manager notifies the
overall BubbleManager, which then removes the window from the management
queue and can show another window. A window may also be closed by
direct command from the overall BubbleManager, for example if it is
superceded by another user-initiated popup.
BUG=375393
TBR=yoz
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283099
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 68
Patch Set 4 : . #
Total comments: 20
Patch Set 5 : . #
Total comments: 6
Patch Set 6 : Rebase #Patch Set 7 : more impl notes #Patch Set 8 : Make PopupManager a thin API for WCMDM #
Total comments: 13
Patch Set 9 : Add per-window PM for app window #Patch Set 10 : Enlarge API, change callers #Patch Set 11 : Remove max size api #Patch Set 12 : Rebase #Patch Set 13 : Nearly complete impl #Patch Set 14 : Add PopupManager to EVH and login window #Patch Set 15 : Rebase #Patch Set 16 : Rebase #Patch Set 17 : Correct popup manager registration #Patch Set 18 : . #Patch Set 19 : rebase #Patch Set 20 : help out tests #Patch Set 21 : More test touch-ups #Patch Set 22 : More test fixes #
Total comments: 11
Patch Set 23 : Cocoa, more test fiddles, etc #
Total comments: 4
Patch Set 24 : . #Patch Set 25 : . #Patch Set 26 : . #Patch Set 27 : Don't require web modal host for popup manager creation #Patch Set 28 : . #
Total comments: 5
Patch Set 29 : tweaks #
Total comments: 40
Patch Set 30 : Touchups #Patch Set 31 : interactive ui test #Patch Set 32 : rebase #Patch Set 33 : . #
Total comments: 4
Patch Set 34 : Rebase #
Total comments: 28
Patch Set 35 : comments #Patch Set 36 : unconst #
Total comments: 2
Patch Set 37 : . #Messages
Total messages: 108 (0 generated)
Some thoughts as I browsed through this... https://codereview.chromium.org/287123002/diff/40001/components/web_modal/nat... File components/web_modal/native_web_contents_modal_dialog.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/nat... components/web_modal/native_web_contents_modal_dialog.h:28: #endif I probably would merge these two #if defs. Out of curiosity... Anyone know the reason why we didn't append Ptr to these names (as in NativeViewPtr and NativePopupPtr)? :) https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:32: void PopupManager::FocusTopmostPopup() { Topmost isn't clear. How about FocusNextPopup? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:38: PopupList::iterator dlg = FindPopupState(popup); nit: Conflating dialogs and popups again. :) https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:43: return; This not the first time I've seen code trying to deal with WillClose being sent twice. I would have thought that to be a bug, no? (Yes, I know it is not your fault and that it is out of scope for this, I'm kind of posing it as a question for the entire group, not you specifically). https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:47: child_dialogs_.erase(dlg); You don't erase in CloseAllPopups. Is that intended? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:62: manager(mgr.release()), Can this be .Pass() instead of .release()? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:89: // Clear out any dialogs since we are leaving this page entirely. Will this function only be called when leaving a page? Also, is it correct to close all popup types upon leaving the page? (not saying it is not, just thinking out loud). https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:92: } nit: No braces. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:101: // Close constrained windows if necessary. I need to brush up on terminology. :) Are all popup types considered constrained windows? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:105: CloseAllPopups(); Don't you need to check here to see whether this is the right WebContents? Similar question -- is it right to close them all at this point? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:111: } nit: No braces. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:127: // some of these to close. CloseAllDialogs is async, so it might get called CloseAllDialogs? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:24: class PopupManager : public SinglePopupManagerDelegate { What is the scope/lifetime of this object? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:29: // may not be shown inline with this call, at a later asynchronous time, s/not// ? s/asynchronous// ? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:35: bool IsPopupActiveInCurrentWebContents() const; 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. :) https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // when calling this function. 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)? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:39: void FocusTopmostPopup(); This function is not called at the moment. Who do you envision calling this when this is fully implemented? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:42: virtual void WillClose(NativePopup popup) OVERRIDE; 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? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:47: explicit PopupManager(); nit: explicit? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:66: void CloseAllPopups(); This is currently private. Won't this need to be public? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:80: PopupList child_dialogs_; We seem to be conflating dialogs and popups (bubbles aren't dialogs, per se, at least not in the traditional sense). child_popups_ ? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager_unittest.cc (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:19: enum DialogState { PopupState? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:34: private: ... and ... DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:48: if (tracker_) You seem guaranteed to have tracker_ until you stop tracking so why all the if checks? Are you getting Show/Hide/Close calls after tracking stops? Seems at least the if check here is not needed, no? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:63: } Nit: Spacing is a little inconsistent here (sometimes space between functions, sometimes not). https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:90: NativePopup dialog_; popup_? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:112: delete manager; The OCD part of me wants to see TearDown be last, since Setup is first above. :) https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:123: PopupManager* manager; nit: Missing underscore for members. Also, use scoped_ptr? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager_unittest.cc:125: DISALLOW_COPY_AND_ASSIGN(PopupManagerTest); nit: Should you not have private: above this? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:20: virtual ~SinglePopupManagerDelegate() {} nit: Move these impls to .cc https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:23: // manager will be deleted before the end of this call. The term native manager is a little lost in this context. What does it refer to? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:76: SinglePopupManager() {} nit: Move to .cc
https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:89: // Clear out any dialogs since we are leaving this page entirely. On 2014/05/16 12:38:14, Finnur wrote: > Will this function only be called when leaving a page? > > Also, is it correct to close all popup types upon leaving the page? (not saying > it is not, just thinking out loud). I only really changed the header file to try to reflect the new language. My expectation is this file will end up needing a lot of work to implement whatever policy we put in it. I maybe should just strip off the .cc files for this cl, since the intention is to focus more on making sure the API is something we're happy with. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:24: class PopupManager : public SinglePopupManagerDelegate { On 2014/05/16 12:38:14, Finnur wrote: > What is the scope/lifetime of this object? 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:35: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/16 12:38:14, Finnur wrote: > 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. :) 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // when calling this function. On 2014/05/16 12:38:14, Finnur wrote: > 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)? 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:42: virtual void WillClose(NativePopup popup) OVERRIDE; On 2014/05/16 12:38:14, Finnur wrote: > 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? 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). https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:66: void CloseAllPopups(); On 2014/05/16 12:38:14, Finnur wrote: > This is currently private. Won't this need to be public? 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. :-/ https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:80: PopupList child_dialogs_; On 2014/05/16 12:38:14, Finnur wrote: > We seem to be conflating dialogs and popups (bubbles aren't dialogs, per se, at > least not in the traditional sense). child_popups_ ? Yeah, I thought I got them all in the .h file, but I see I missed this one.
Yeah, I think this is headed in a better direction, some initial comments. Note, I'm fixing some WCMDM tests in http://crrev.com/282223002 Refining some of the existing code could go potentially help us move forward. Also, having the related automated tests working seems important. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/nat... File components/web_modal/native_web_contents_modal_dialog.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/nat... components/web_modal/native_web_contents_modal_dialog.h:23: // Use a void* since none of the gfx::Native* types are suitable for Interesting, NSWindow (gfx::NativeWindow) and NSView (gfx::NativeView) aren't applicable for neither bubbles nor dialogs? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/nat... components/web_modal/native_web_contents_modal_dialog.h:28: #endif On 2014/05/16 12:38:14, Finnur wrote: > I probably would merge these two #if defs. > > Out of curiosity... Anyone know the reason why we didn't append Ptr to these > names (as in NativeViewPtr and NativePopupPtr)? :) NativeWindow was at one point HWND, which is a handle, not a really a pointer. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:35: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/16 15:49:56, Greg Billock wrote: > On 2014/05/16 12:38:14, Finnur wrote: > > 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. :) > > 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. 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:42: virtual void WillClose(NativePopup popup) OVERRIDE; On 2014/05/16 15:49:56, Greg Billock wrote: > On 2014/05/16 12:38:14, Finnur wrote: > > 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? > > 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). Can't notifications of closure be handled through widget/window observation? Perhaps this is just a short-term way of integrating WCMDM?
This approach is looking good to me. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/nat... File components/web_modal/native_web_contents_modal_dialog.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/nat... components/web_modal/native_web_contents_modal_dialog.h:23: // Use a void* since none of the gfx::Native* types are suitable for On 2014/05/16 17:10:57, msw wrote: > Interesting, NSWindow (gfx::NativeWindow) and NSView (gfx::NativeView) aren't > applicable for neither bubbles nor dialogs? See https://codereview.chromium.org/12224020 for the extended discussion on why neither gfx::NativeWindow nor gfx::NativeView work for WCMDs. Similar logic may or may not also apply to popups. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:43: return; On 2014/05/16 12:38:14, Finnur wrote: > This not the first time I've seen code trying to deal with WillClose being sent > twice. I would have thought that to be a bug, no? > > (Yes, I know it is not your fault and that it is out of scope for this, I'm kind > of posing it as a question for the entire group, not you specifically). This was my comment from investigating this for the WCMD case. As I recall the problem is a mismatch between the views::DialogDelegate interface, which expects Cancel/Accept to take action *without* closing the window, and the usage of the TabModalConfirmDialog interface, which expects Cancel/Accept to close the dialog. This could be rationalized but it touches a lot of dialogs across both Views and Cocoa. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:47: child_dialogs_.erase(dlg); On 2014/05/16 12:38:14, Finnur wrote: > You don't erase in CloseAllPopups. Is that intended? Speaking based on the WCMDM implementation, yes. All dialogs invoke this function on closing and the dialog gets erased here, including those closed via CloseAllPopups. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:89: // Clear out any dialogs since we are leaving this page entirely. On 2014/05/16 15:49:56, Greg Billock wrote: > On 2014/05/16 12:38:14, Finnur wrote: > > Will this function only be called when leaving a page? > > > > Also, is it correct to close all popup types upon leaving the page? (not > saying > > it is not, just thinking out loud). > > I only really changed the header file to try to reflect the new language. My > expectation is this file will end up needing a lot of work to implement whatever > policy we put in it. I maybe should just strip off the .cc files for this cl, > since the intention is to focus more on making sure the API is something we're > happy with. I believe this function would be WCMD-specific. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.cc:105: CloseAllPopups(); On 2014/05/16 12:38:14, Finnur wrote: > Don't you need to check here to see whether this is the right WebContents? > Similar question -- is it right to close them all at this point? This is also WMCD-specific, and probably doesn't belong here anyway... Not closing on navigation to the same site is the right behavior for things like the cookies and certificate information dialogs, but not for e.g. the confirm form resubmission dialog. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // when calling this function. On 2014/05/16 15:49:56, Greg Billock wrote: > On 2014/05/16 12:38:14, Finnur wrote: > > 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)? > > 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. 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:42: virtual void WillClose(NativePopup popup) OVERRIDE; On 2014/05/16 17:10:57, msw wrote: > On 2014/05/16 15:49:56, Greg Billock wrote: > > On 2014/05/16 12:38:14, Finnur wrote: > > > 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? > > > > 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). > > Can't notifications of closure be handled through widget/window observation? > Perhaps this is just a short-term way of integrating WCMDM? 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:42: virtual content::WebContents* IsBoundToWebContents() = 0; GetBoundWebContents? IsBoundWebContents is odd for a non-bool function https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:52: // If this method causes a WillClose() call to the delegate, the manager This comment is misleading, both here and in the SingleWebContentsModalDialogManager. I believe Close should always result in a WillClose call on the delegate.
https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:35: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/16 17:10:57, msw wrote: > On 2014/05/16 15:49:56, Greg Billock wrote: > > On 2014/05/16 12:38:14, Finnur wrote: > > > 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. :) > > > > 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. > > 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. 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // when calling this function. On 2014/05/16 20:35:40, Mike Wittman wrote: > On 2014/05/16 15:49:56, Greg Billock wrote: > > On 2014/05/16 12:38:14, Finnur wrote: > > > 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)? > > > > 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. > > 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. 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? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:42: virtual void WillClose(NativePopup popup) OVERRIDE; On 2014/05/16 20:35:40, Mike Wittman wrote: > On 2014/05/16 17:10:57, msw wrote: > > On 2014/05/16 15:49:56, Greg Billock wrote: > > > On 2014/05/16 12:38:14, Finnur wrote: > > > > 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? > > > > > > 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). > > > > Can't notifications of closure be handled through widget/window observation? > > Perhaps this is just a short-term way of integrating WCMDM? > > 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. 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.
https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // when calling this function. On 2014/05/16 23:28:39, Greg Billock wrote: > On 2014/05/16 20:35:40, Mike Wittman wrote: > > On 2014/05/16 15:49:56, Greg Billock wrote: > > > On 2014/05/16 12:38:14, Finnur wrote: > > > > 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)? > > > > > > 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. > > > > 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. > > 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? 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.
Is there an overall bug filed for this work? https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // when calling this function. On 2014/05/17 00:24:06, Mike Wittman wrote: > On 2014/05/16 23:28:39, Greg Billock wrote: > > 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? > > 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. I'd also like this class to replace WCMDM, but perhaps not in the first CL.
https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:29: // may not be shown inline with this call, at a later asynchronous time, On 2014/05/16 12:38:14, Finnur wrote: > s/not// ? > s/asynchronous// ? Done. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // when calling this function. On 2014/05/20 16:51:32, msw wrote: > On 2014/05/17 00:24:06, Mike Wittman wrote: > > On 2014/05/16 23:28:39, Greg Billock wrote: > > > 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? > > > > 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. > > I'd also like this class to replace WCMDM, but perhaps not in the first CL. 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:47: explicit PopupManager(); On 2014/05/16 12:38:14, Finnur wrote: > nit: explicit? Done. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:80: PopupList child_dialogs_; On 2014/05/16 15:49:56, Greg Billock wrote: > On 2014/05/16 12:38:14, Finnur wrote: > > We seem to be conflating dialogs and popups (bubbles aren't dialogs, per se, > at > > least not in the traditional sense). child_popups_ ? > > Yeah, I thought I got them all in the .h file, but I see I missed this one. Done.
On 2014/05/20 16:51:32, msw wrote: > Is there an overall bug filed for this work? > > https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... > File components/web_modal/popup_manager.h (right): > > https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... > components/web_modal/popup_manager.h:38: // when calling this function. > On 2014/05/17 00:24:06, Mike Wittman wrote: > > On 2014/05/16 23:28:39, Greg Billock wrote: > > > 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? > > > > 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. > > I'd also like this class to replace WCMDM, but perhaps not in the first CL. Yes, I've put a bug. I've also added some code to Browser to indicate ownership and scope. Take a look and tell me what you think. All current callers to WCMDM would be compatible with this by either accessing it directly or using BrowserFinder with a WebContents.
https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:35: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/16 23:28:39, Greg Billock wrote: > On 2014/05/16 17:10:57, msw wrote: > > On 2014/05/16 15:49:56, Greg Billock wrote: > > > On 2014/05/16 12:38:14, Finnur wrote: > > > > 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. :) > > > > > > 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. > > > > 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. > > 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. 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. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:42: virtual content::WebContents* IsBoundToWebContents() = 0; On 2014/05/16 20:35:40, Mike Wittman wrote: > GetBoundWebContents? IsBoundWebContents is odd for a non-bool function I agree with Mike's comment here, can you try to address the comments you've missed? (it's difficult for us if you ask for more feedback before addressing open comments/questions) https://codereview.chromium.org/287123002/diff/60001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/287123002/diff/60001/chrome/browser/ui/browse... chrome/browser/ui/browser.h:825: scoped_ptr<web_modal::PopupManager> popup_manager_; Why should this be a scoped_ptr rather than a non-pointer class member? https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:36: bool IsPopupActiveInCurrentWebContents() const; 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). https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // Focus the topmost popup. IsPopupActiveInCurrentWebContents() must be true 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. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:40: void FocusTopmostPopup(); 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. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:48: struct PopupState { 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. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:54: bool close_on_interstitial_webui; 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. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/sin... components/web_modal/single_popup_manager.h:44: // Makes the popup visible. Only one popup will be shown at a time per tab. noting per-tab behavior here is incorrect (or jumping the gun) wrt bubbles.
https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:35: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/22 04:08:45, msw wrote: > On 2014/05/16 23:28:39, Greg Billock wrote: > > On 2014/05/16 17:10:57, msw wrote: > > > On 2014/05/16 15:49:56, Greg Billock wrote: > > > > On 2014/05/16 12:38:14, Finnur wrote: > > > > > 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. > :) > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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. reworked this substantially https://codereview.chromium.org/287123002/diff/40001/components/web_modal/pop... components/web_modal/popup_manager.h:39: void FocusTopmostPopup(); On 2014/05/16 12:38:14, Finnur wrote: > This function is not called at the moment. Who do you envision calling this when > this is fully implemented? added call site to cl https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:42: virtual content::WebContents* IsBoundToWebContents() = 0; On 2014/05/22 04:08:45, msw wrote: > On 2014/05/16 20:35:40, Mike Wittman wrote: > > GetBoundWebContents? IsBoundWebContents is odd for a non-bool function > > I agree with Mike's comment here, can you try to address the comments you've > missed? (it's difficult for us if you ask for more feedback before addressing > open comments/questions) Yeah, I've been focusing more on the PopupManager .h file, but I'll update this. https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:52: // If this method causes a WillClose() call to the delegate, the manager On 2014/05/16 20:35:40, Mike Wittman wrote: > This comment is misleading, both here and in the > SingleWebContentsModalDialogManager. I believe Close should always result in a > WillClose call on the delegate. I noticed that too reading this over. Changing. Any ideas about this land mine? The trouble is that the SPM needs deleting after close, but I worry deferring it ends up even more mysterious and dangerous. https://codereview.chromium.org/287123002/diff/60001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/287123002/diff/60001/chrome/browser/ui/browse... chrome/browser/ui/browser.h:825: scoped_ptr<web_modal::PopupManager> popup_manager_; On 2014/05/22 04:08:45, msw wrote: > Why should this be a scoped_ptr rather than a non-pointer class member? No particular reason right now -- in general scoped_ptr members are easier to inject test instances into and such, but that's probably not a concern here. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:36: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/22 04:08:45, msw wrote: > As per the older comment thread, consider IsPopupActive with a comment update. 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). > 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). https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:38: // Focus the topmost popup. IsPopupActiveInCurrentWebContents() must be true On 2014/05/22 04:08:45, msw wrote: > 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. Agreed. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:40: void FocusTopmostPopup(); On 2014/05/22 04:08:45, msw wrote: > 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. 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? https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:48: struct PopupState { On 2014/05/22 04:08:45, msw wrote: > 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. 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. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:54: bool close_on_interstitial_webui; On 2014/05/22 04:08:45, msw wrote: > 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. Sensible. I added this as a field of the SinglePopupManager. That should cover things from the popup manager perspective. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/sin... components/web_modal/single_popup_manager.h:44: // Makes the popup visible. Only one popup will be shown at a time per tab. On 2014/05/22 04:08:45, msw wrote: > noting per-tab behavior here is incorrect (or jumping the gun) wrt bubbles. I know what you mean; this is basically guidance for SPM implementers. I'll put this in the class comment.
https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:36: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/22 18:25:47, Greg Billock wrote: > 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. 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. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:40: void FocusTopmostPopup(); On 2014/05/22 18:25:47, Greg Billock wrote: > On 2014/05/22 04:08:45, msw wrote: > > 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. > > 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? 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)
Still making progress -- good. One comment: https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:40: void FocusTopmostPopup(); > 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? 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?
https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:40: void FocusTopmostPopup(); On 2014/05/23 15:29:52, Finnur wrote: > > 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? > > 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? 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.
https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/40001/components/web_modal/sin... components/web_modal/single_popup_manager.h:52: // If this method causes a WillClose() call to the delegate, the manager On 2014/05/22 18:25:47, Greg Billock wrote: > On 2014/05/16 20:35:40, Mike Wittman wrote: > > This comment is misleading, both here and in the > > SingleWebContentsModalDialogManager. I believe Close should always result in a > > WillClose call on the delegate. > > I noticed that too reading this over. Changing. > > Any ideas about this land mine? The trouble is that the SPM needs deleting after > close, but I worry deferring it ends up even more mysterious and dangerous. I don't see a better way to do this off-hand. In theory the SPM shouldn't have work to do post-close, so the risk shouldn't be that great. https://codereview.chromium.org/287123002/diff/80001/components/web_modal/pop... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/80001/components/web_modal/pop... components/web_modal/popup_manager.cc:44: // depending on the policy. I think we should be making decisions about showing/hiding popups in WasShown/WasHidden rather than here. This function is invoked on the WebContents receiving focus, which could conceivably happen repeatedly without the current WebContents changing. https://codereview.chromium.org/287123002/diff/80001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/80001/components/web_modal/sin... components/web_modal/single_popup_manager.h:81: virtual bool ShouldCloseOnNavigation() = 0; It would be good to eventually generalize this to support the same-origin logic that's currently in WebContentsModalDialogManager::DidNavigateMainFrame, and push that down to a dialog-specific configuration.
I think this is pretty close -- two possible directions: 1. refactor immediately (in this CL or the next) to move call sites to the PopupManager and make it transparently call through to WCMDM. or 2. Work more on the PopupManager to refine the API, then replace callers with references to the new API. I'm inclined for (1), and if you are too, I'll strip out most of the current impl code and do that to get a wedge in the door. Next CLs would then be putting all the WebContentsObservers in place to bring the WCMDM logic inside, followed by the bubble integration. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:36: bool IsPopupActiveInCurrentWebContents() const; On 2014/05/22 21:40:02, msw wrote: > On 2014/05/22 18:25:47, Greg Billock wrote: > > 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. > > 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. ok, let's head that direction. Added this API with a note that it's deprecated. https://codereview.chromium.org/287123002/diff/60001/components/web_modal/pop... components/web_modal/popup_manager.h:40: void FocusTopmostPopup(); On 2014/05/23 17:22:42, msw wrote: > On 2014/05/23 15:29:52, Finnur wrote: > > > 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? > > > > 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? > > 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. 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? https://codereview.chromium.org/287123002/diff/80001/components/web_modal/pop... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/80001/components/web_modal/pop... components/web_modal/popup_manager.cc:44: // depending on the policy. On 2014/05/23 19:02:35, Mike Wittman wrote: > I think we should be making decisions about showing/hiding popups in > WasShown/WasHidden rather than here. This function is invoked on the WebContents > receiving focus, which could conceivably happen repeatedly without the current > WebContents changing. I'm thinking the same thing. I added a note to that effect on the .h file, but yes, I'm suspicious the views calling code should move to WasShown. If its not working right on views, perhaps there's a bug somewhere in the window focus mgmt... In the meantime, I added a note to the call site that we could add a WasFocused(webcontents) to this API, which would basically just get called from the existing views code and do what it does. I agree I'm not sure that's really necessary, but if an aim of this CL or the next is to refactor existing usage to talk to it and not WCMDM that'd be a fast way forward. https://codereview.chromium.org/287123002/diff/80001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/80001/components/web_modal/sin... components/web_modal/single_popup_manager.h:81: virtual bool ShouldCloseOnNavigation() = 0; On 2014/05/23 19:02:35, Mike Wittman wrote: > It would be good to eventually generalize this to support the same-origin logic > that's currently in WebContentsModalDialogManager::DidNavigateMainFrame, and > push that down to a dialog-specific configuration. Yeah, I think that's right. Do you know what other knobs we need to parameterize that? I put a TODO in the file, but if you know the answer we can add it right now. :-)
On 2014/05/27 22:42:02, Greg Billock wrote: > I think this is pretty close -- two possible directions: 1. refactor immediately > (in this CL or the next) to move call sites to the PopupManager and make it > transparently call through to WCMDM. or 2. Work more on the PopupManager to > refine the API, then replace callers with references to the new API. I'm > inclined for (1), and if you are too, I'll strip out most of the current impl > code and do that to get a wedge in the door. Next CLs would then be putting all > the WebContentsObservers in place to bring the WCMDM logic inside, followed by > the bubble integration. (1) seems like the least risky and likely best strategy. There's enough subtle details around the WCMDs, and there are enough of them, that it's worth addressing this in small incremental steps. https://codereview.chromium.org/287123002/diff/80001/components/web_modal/pop... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/80001/components/web_modal/pop... components/web_modal/popup_manager.cc:44: // depending on the policy. On 2014/05/27 22:42:03, Greg Billock wrote: > On 2014/05/23 19:02:35, Mike Wittman wrote: > > I think we should be making decisions about showing/hiding popups in > > WasShown/WasHidden rather than here. This function is invoked on the > WebContents > > receiving focus, which could conceivably happen repeatedly without the current > > WebContents changing. > > I'm thinking the same thing. I added a note to that effect on the .h file, but > yes, I'm suspicious the views calling code should move to WasShown. If its not > working right on views, perhaps there's a bug somewhere in the window focus > mgmt... On the focus issue, it's also entirely possible that whatever issue led to the creation of this function is no longer present. My point in the original comment though is that this function, if it continues to exist, should only deal with focus. The comments here and in the header imply that it would also possibly change the shown/hidden state of the managed dialogs. > In the meantime, I added a note to the call site that we could add a > WasFocused(webcontents) to this API, which would basically just get called from > the existing views code and do what it does. I agree I'm not sure that's really > necessary, but if an aim of this CL or the next is to refactor existing usage to > talk to it and not WCMDM that'd be a fast way forward. https://codereview.chromium.org/287123002/diff/80001/components/web_modal/sin... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/80001/components/web_modal/sin... components/web_modal/single_popup_manager.h:81: virtual bool ShouldCloseOnNavigation() = 0; On 2014/05/27 22:42:03, Greg Billock wrote: > On 2014/05/23 19:02:35, Mike Wittman wrote: > > It would be good to eventually generalize this to support the same-origin > logic > > that's currently in WebContentsModalDialogManager::DidNavigateMainFrame, and > > push that down to a dialog-specific configuration. > > Yeah, I think that's right. Do you know what other knobs we need to parameterize > that? I put a TODO in the file, but if you know the answer we can add it right > now. :-) The cases I'm aware of that result in closing the dialog are: attaching an interstitial page, navigating to a page not on the same domain, and navigating to any other page. The latter two cases should be covered by a content::LoadCommittedDetails or even just previous and current URLs. I don't know that we have any similar state on attaching an interstitial page.
OK, I'm changing this CL to basically forward to the WCMDM for now, but there's a wrinkle: there are a few other contexts which can host web modals: native app windows, some login windows, etc. So using BrowserFromWebContents and looking there isn't right. We need a mapping from WebContents up to the PopupManager encompassing that WebContents. I'm tempted to just put this registry in the PopupManager itself, but that's a bit hard to swallow. It's only attractive because we currently go through the WCMDM to find this information, but that's a bit out of place -- we're really looking for the GetHostView() call to find the NativeView in which to create our windows. So I'm tempted to say that callers need to find the host window out-of-band vended from the same object they get the PopupManager from. Is that fair? It certainly works for Browser, albeit a bit beat-around-the-bush... (views::Widget::GetWidgetForNativeWindow(browser->window()->GetNativeWindow())->GetNativeView()) I think this is the biggest obstacle to getting the PopupManager checked in (albeit in just-forwards-to-WCMDM form for now). But it should re-orient the API in a window-scoped way and let us then just refactor behind that API to our hearts' content.
On 2014/05/29 18:47:25, Greg Billock wrote: > OK, I'm changing this CL to basically forward to the WCMDM for now, but there's > a wrinkle: there are a few other contexts which can host web modals: native app > windows, some login windows, etc. So using BrowserFromWebContents and looking > there isn't right. We need a mapping from WebContents up to the PopupManager > encompassing that WebContents. > > I'm tempted to just put this registry in the PopupManager itself, but that's a > bit hard to swallow. It's only attractive because we currently go through the > WCMDM to find this information, but that's a bit out of place -- we're really > looking for the GetHostView() call to find the NativeView in which to create our > windows. > > So I'm tempted to say that callers need to find the host window out-of-band > vended from the same object they get the PopupManager from. Is that fair? It > certainly works for Browser, albeit a bit beat-around-the-bush... > (views::Widget::GetWidgetForNativeWindow(browser->window()->GetNativeWindow())->GetNativeView()) > > I think this is the biggest obstacle to getting the PopupManager checked in > (albeit in just-forwards-to-WCMDM form for now). But it should re-orient the API > in a window-scoped way and let us then just refactor behind that API to our > hearts' content. ideas?
On 2014/06/02 18:58:45, Greg Billock wrote: > On 2014/05/29 18:47:25, Greg Billock wrote: > > OK, I'm changing this CL to basically forward to the WCMDM for now, but > there's > > a wrinkle: there are a few other contexts which can host web modals: native > app > > windows, some login windows, etc. So using BrowserFromWebContents and looking > > there isn't right. We need a mapping from WebContents up to the PopupManager > > encompassing that WebContents. > > > > I'm tempted to just put this registry in the PopupManager itself, but that's a > > bit hard to swallow. It's only attractive because we currently go through the > > WCMDM to find this information, but that's a bit out of place -- we're really > > looking for the GetHostView() call to find the NativeView in which to create > our > > windows. > > > > So I'm tempted to say that callers need to find the host window out-of-band > > vended from the same object they get the PopupManager from. Is that fair? It > > certainly works for Browser, albeit a bit beat-around-the-bush... > > > (views::Widget::GetWidgetForNativeWindow(browser->window()->GetNativeWindow())->GetNativeView()) > > > > I think this is the biggest obstacle to getting the PopupManager checked in > > (albeit in just-forwards-to-WCMDM form for now). But it should re-orient the > API > > in a window-scoped way and let us then just refactor behind that API to our > > hearts' content. > > ideas? Sorry for the delay... What about storing the PopupManager on the Browser/app window/login window, and adding a trivial WebContentsUserData that provides access to the PopupManager to all WebContents created by those objects? Ideally the dialog code ideally shouldn't have to know about the type of host window, at least for WCMDs.
https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.cc:12: #include "net/base/registry_controlled_domains/registry_controlled_domain.h" I think all these includes can be removed except for components/web_modal/web_contents_modal_dialog_manager.h. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.cc:61: void PopupManager::WillClose(NativePopup popup) { Forward on to WebContentsModalDialogManager::WillClose? https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:8: #include <deque> No need for this include. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:12: #include "content/public/browser/web_contents_user_data.h" Same here. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:13: #include "ui/gfx/native_widget_types.h" And here, I think. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:17: struct LoadCommittedDetails; No need for the previous two forward decls.
On 2014/06/02 19:35:10, Mike Wittman wrote: > On 2014/06/02 18:58:45, Greg Billock wrote: > > On 2014/05/29 18:47:25, Greg Billock wrote: > > > OK, I'm changing this CL to basically forward to the WCMDM for now, but > > there's > > > a wrinkle: there are a few other contexts which can host web modals: native > > app > > > windows, some login windows, etc. So using BrowserFromWebContents and > looking > > > there isn't right. We need a mapping from WebContents up to the PopupManager > > > encompassing that WebContents. > > > > > > I'm tempted to just put this registry in the PopupManager itself, but that's > a > > > bit hard to swallow. It's only attractive because we currently go through > the > > > WCMDM to find this information, but that's a bit out of place -- we're > really > > > looking for the GetHostView() call to find the NativeView in which to create > > our > > > windows. > > > > > > So I'm tempted to say that callers need to find the host window out-of-band > > > vended from the same object they get the PopupManager from. Is that fair? It > > > certainly works for Browser, albeit a bit beat-around-the-bush... > > > > > > (views::Widget::GetWidgetForNativeWindow(browser->window()->GetNativeWindow())->GetNativeView()) > > > > > > I think this is the biggest obstacle to getting the PopupManager checked in > > > (albeit in just-forwards-to-WCMDM form for now). But it should re-orient the > > API > > > in a window-scoped way and let us then just refactor behind that API to our > > > hearts' content. > > > > ideas? > > Sorry for the delay... What about storing the PopupManager on the Browser/app > window/login window, and adding a trivial WebContentsUserData that provides > access to the PopupManager to all WebContents created by those objects? Ideally > the dialog code ideally shouldn't have to know about the type of host window, at > least for WCMDs. Good plan.
ok, I have three spots that need popup manager: browser, appWindow, and the login window on chromeos. Is that it? https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.cc:12: #include "net/base/registry_controlled_domains/registry_controlled_domain.h" On 2014/06/02 19:54:50, Mike Wittman wrote: > I think all these includes can be removed except for > components/web_modal/web_contents_modal_dialog_manager.h. Done. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.cc:61: void PopupManager::WillClose(NativePopup popup) { On 2014/06/02 19:54:50, Mike Wittman wrote: > Forward on to WebContentsModalDialogManager::WillClose? I still need to do a bit more migrating. I haven't moved over the show methods yet to have the machinery to do WillClose. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:8: #include <deque> On 2014/06/02 19:54:50, Mike Wittman wrote: > No need for this include. Done. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:12: #include "content/public/browser/web_contents_user_data.h" On 2014/06/02 19:54:50, Mike Wittman wrote: > Same here. Done. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:12: #include "content/public/browser/web_contents_user_data.h" On 2014/06/02 19:54:50, Mike Wittman wrote: > Same here. Done. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:13: #include "ui/gfx/native_widget_types.h" On 2014/06/02 19:54:50, Mike Wittman wrote: > And here, I think. Done. https://codereview.chromium.org/287123002/diff/140001/components/web_modal/po... components/web_modal/popup_manager.h:17: struct LoadCommittedDetails; On 2014/06/02 19:54:50, Mike Wittman wrote: > No need for the previous two forward decls. Done.
On 2014/06/03 01:30:07, Greg Billock wrote: > ok, I have three spots that need popup manager: browser, appWindow, and the > login window on chromeos. Is that it? It looks like ExtensionviewHost has also been added since I worked on this. If you cover all the subclasses of web_modal::WebContentsModalDialogHost I think you should be OK.
On 2014/06/03 02:13:46, Mike Wittman wrote: > On 2014/06/03 01:30:07, Greg Billock wrote: > > ok, I have three spots that need popup manager: browser, appWindow, and the > > login window on chromeos. Is that it? > > It looks like ExtensionviewHost has also been added since I worked on this. If > you cover all the subclasses of web_modal::WebContentsModalDialogHost I think > you should be OK. I saw that, but I don't really understand it. It looks to me like EVH is for infobars/popups/dialogs created by extensions. They have a browser object available, so I think we should be using the popup manager available on that object as opposed to a particular one made for the EVH. (In practices are there places where popups get made on EVH surfaces? Perhaps file selector web modals or something? -- I don't see any on initial inspection.)
On 2014/06/04 18:53:38, Greg Billock wrote: > On 2014/06/03 02:13:46, Mike Wittman wrote: > > On 2014/06/03 01:30:07, Greg Billock wrote: > > > ok, I have three spots that need popup manager: browser, appWindow, and the > > > login window on chromeos. Is that it? > > > > It looks like ExtensionviewHost has also been added since I worked on this. If > > you cover all the subclasses of web_modal::WebContentsModalDialogHost I think > > you should be OK. > > I saw that, but I don't really understand it. It looks to me like EVH is for > infobars/popups/dialogs created by extensions. They have a browser object > available, so I think we should be using the popup manager available on that > object as opposed to a particular one made for the EVH. (In practices are there > places where popups get made on EVH surfaces? Perhaps file selector web modals > or something? -- I don't see any on initial inspection.) I'm not familiar with this either, but it's vending an ExtensionViewViews as the WCMD host so any popups would be parented to that View rather than the browser window. It's still not clear if these popups should ultimately be treated entirely separately or managed within the larger scope of the browser. Maybe treat them separately with their own PopupManager for now and revisit later whether they need to coordinate with the browser's PopupManager? That seems simpler and no worse than the current behavior.
On 2014/06/04 21:59:53, Mike Wittman wrote: > On 2014/06/04 18:53:38, Greg Billock wrote: > > On 2014/06/03 02:13:46, Mike Wittman wrote: > > > On 2014/06/03 01:30:07, Greg Billock wrote: > > > > ok, I have three spots that need popup manager: browser, appWindow, and > the > > > > login window on chromeos. Is that it? > > > > > > It looks like ExtensionviewHost has also been added since I worked on this. > If > > > you cover all the subclasses of web_modal::WebContentsModalDialogHost I > think > > > you should be OK. > > > > I saw that, but I don't really understand it. It looks to me like EVH is for > > infobars/popups/dialogs created by extensions. They have a browser object > > available, so I think we should be using the popup manager available on that > > object as opposed to a particular one made for the EVH. (In practices are > there > > places where popups get made on EVH surfaces? Perhaps file selector web modals > > or something? -- I don't see any on initial inspection.) > > I'm not familiar with this either, but it's vending an ExtensionViewViews as the > WCMD host so any popups would be parented to that View rather than the browser > window. It's still not clear if these popups should ultimately be treated > entirely separately or managed within the larger scope of the browser. Maybe > treat them separately with their own PopupManager for now and revisit later > whether they need to coordinate with the browser's PopupManager? That seems > simpler and no worse than the current behavior. True enough, I guess I'm just skeptical that's actually really happening. I'll dig into it a bit more and see if I can figure out why this class needs to be a WCMDH. In other news, I ended up bumping the API a bit to cover a couple cases around constrained windows and to help out with modal dialog creation. I just added the methods to the PopupManager instead of making a separate PopupManager Delegate/Host setup, as from what I can tell, the external users really only want to know the NativeView window handle and the max size. There's a bit of code in the constrained window management system that makes more use of the WCMDM and WCMDH to manage resizing. I think that's yours, right? Should that end up ported to PopupManager? Or stick with the WCMDM interface?
This is looking good. On 2014/06/04 22:44:24, Greg Billock wrote: > In other news, I ended up bumping the API a bit to cover a couple cases around > constrained windows and to help out with modal dialog creation. I just added the > methods to the PopupManager instead of making a separate PopupManager > Delegate/Host setup, as from what I can tell, the external users really only > want to know the NativeView window handle and the max size. There's a bit of > code in the constrained window management system that makes more use of the > WCMDM and WCMDH to manage resizing. I think that's yours, right? Should that end > up ported to PopupManager? Or stick with the WCMDM interface? For creation purposes removing the delegate and host makes sense, since the popup manager is now operating at roughly the same level as the host UI element, rather than at the WebContents level. The resizing probably should move to use PopupManager, but it may be good to retain a WCMDH-like interface for this since the UI-related responsibilities are distinct from the management-related responsibilities.
I just discovered AppModalDialogQueue exists for system-modal javascript dialogs; were others aware of this class? How do we think that should relate to this work? https://code.google.com/p/chromium/codesearch#search/&q=AppModalDialogQueue
On 2014/06/10 23:36:13, msw wrote: > I just discovered AppModalDialogQueue exists for system-modal javascript > dialogs; were others aware of this class? How do we think that should relate to > this work? > https://code.google.com/p/chromium/codesearch#search/&q=AppModalDialogQueue Very good questions. Not sure what the right answer is, but I was not aware of this class (or had completely blocked it out of my memory). Seems like it has been there since before the project went public...
On 2014/06/11 11:24:15, Finnur wrote: > On 2014/06/10 23:36:13, msw wrote: > > I just discovered AppModalDialogQueue exists for system-modal javascript > > dialogs; were others aware of this class? How do we think that should relate > to > > this work? > > https://code.google.com/p/chromium/codesearch#search/&q=AppModalDialogQueue > > Very good questions. Not sure what the right answer is, but I was not aware of > this class (or had completely blocked it out of my memory). > > Seems like it has been there since before the project went public... Personally I think JS alerts (which is what these are used for) ought to be tab-modal, not app-modal. I think the use case is a kind of alert mechanism where you want to have the tab that issues the alert seize control. Assuming that's good, which I'm not sure it is, don't we have notifications API with specific permissions for exactly this use case? In other news, I think I'm just going to punt figuring out the deal with the EVH and captive portal window and implement PopupManager on them for now. That'll let us land this and move on to stage 2.
On 2014/06/12 18:06:51, Greg Billock wrote: > On 2014/06/11 11:24:15, Finnur wrote: > > On 2014/06/10 23:36:13, msw wrote: > > > I just discovered AppModalDialogQueue exists for system-modal javascript > > > dialogs; were others aware of this class? How do we think that should relate > > to > > > this work? > > > https://code.google.com/p/chromium/codesearch#search/&q=AppModalDialogQueue > > > > Very good questions. Not sure what the right answer is, but I was not aware of > > this class (or had completely blocked it out of my memory). > > > > Seems like it has been there since before the project went public... > > Personally I think JS alerts (which is what these are used for) ought to be > tab-modal, not app-modal. I think the use case is a kind of alert mechanism > where you want to have the tab that issues the alert seize control. Assuming > that's good, which I'm not sure it is, don't we have notifications API with > specific permissions for exactly this use case? IIRC, JS alerts must be system-modal to avoid concurrent JS code execution. Then again, they seem to be window-modal in IE and tab-modal in Firefox :-/ It's worth verifying and following up separately, but out of scope here. > In other news, I think I'm just going to punt figuring out the deal with the EVH > and captive portal window and implement PopupManager on them for now. That'll > let us land this and move on to stage 2. Sounds good.
The CQ bit was checked by gbillock@chromium.org
The CQ bit was unchecked by gbillock@chromium.org
On 2014/06/12 21:29:17, Greg Billock wrote: > The CQ bit was unchecked by mailto:gbillock@chromium.org ok, this change is making its way to full test compliance. Please take a final look.
https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:903: popup_manager_->RegisterWith(contents); The WCMDM is disassociated from the Browser on tab close and tab detach. I would think we'll need similar behavior for the PopupManager. https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm (right): https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:36: // TODO(gbillock): Use PopupManager here. Do you intend to address the Mac usage in this CL? https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/views... File chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc (right): https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/views... chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:64: if (browser && browser->popup_manager()) Why not PopupManager::FromWebContents()? https://codereview.chromium.org/287123002/diff/420001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/420001/components/web_modal/po... components/web_modal/popup_manager.cc:72: return manager ? manager->IsDialogActive() : false; nit: manager && manager->IsDialogActive() https://codereview.chromium.org/287123002/diff/420001/components/web_modal/po... components/web_modal/popup_manager.cc:111: // TODO: implement, probably in terms of something in the host_ for now, Needs resolution.
https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:903: popup_manager_->RegisterWith(contents); On 2014/06/25 02:38:50, Mike Wittman wrote: > The WCMDM is disassociated from the Browser on tab close and tab detach. I would > think we'll need similar behavior for the PopupManager. Oh, I see what your saying -- it needs an api method to remove a WC even if the WC doesn't go away (eg tab drag to other Browser). will do. I added a new API for this and put hooks into TabClosingAt and TabDetachedAt. I don't think the non-Browser owners need to worry about this -- they are single-webcontents-owning environments that shouldn't need to do this, right? https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm (right): https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:36: // TODO(gbillock): Use PopupManager here. On 2014/06/25 02:38:50, Mike Wittman wrote: > Do you intend to address the Mac usage in this CL? Done. https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/views... File chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc (right): https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/views... chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:64: if (browser && browser->popup_manager()) On 2014/06/25 02:38:50, Mike Wittman wrote: > Why not PopupManager::FromWebContents()? Done. https://codereview.chromium.org/287123002/diff/420001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/420001/components/web_modal/po... components/web_modal/popup_manager.cc:72: return manager ? manager->IsDialogActive() : false; On 2014/06/25 02:38:50, Mike Wittman wrote: > nit: manager && manager->IsDialogActive() Done. https://codereview.chromium.org/287123002/diff/420001/components/web_modal/po... components/web_modal/popup_manager.cc:111: // TODO: implement, probably in terms of something in the host_ for now, On 2014/06/25 02:38:50, Mike Wittman wrote: > Needs resolution. Will do. I think long term we'll absorb WCMDM, but medium term we'll need to know all WCMDM in the associated Browser object. I need to double check the present register code is called after the WCMDM is placed so we can find it that way.
https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/287123002/diff/420001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:903: popup_manager_->RegisterWith(contents); On 2014/06/25 19:06:14, Greg Billock wrote: > I don't think the non-Browser owners need to worry about this -- they are > single-webcontents-owning environments that shouldn't need to do this, right? Yes, this behavior is unique to Browser. https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right): https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:215: if (browser && browser->popup_manager()->IsWebModalDialogActive( Can we use PopupManager::FromWebContents() here as well? https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... components/web_modal/popup_manager.cc:92: // popup managers with popups in-flight? For the tab drag case we'll at least need to transfer WCMDs associated with the tab's WebContents to the destination browser's PopupManager.
https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right): https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:215: if (browser && browser->popup_manager()->IsWebModalDialogActive( On 2014/06/25 21:12:44, Mike Wittman wrote: > Can we use PopupManager::FromWebContents() here as well? Done. https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... components/web_modal/popup_manager.cc:92: // popup managers with popups in-flight? On 2014/06/25 21:12:44, Mike Wittman wrote: > For the tab drag case we'll at least need to transfer WCMDs associated with the > tab's WebContents to the destination browser's PopupManager. Yeah, this is going to be fairly complicated and in the end the API may need to change to accommodate what we need to do.
On 2014/06/26 00:21:12, Greg Billock wrote: > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right): > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:215: if > (browser && browser->popup_manager()->IsWebModalDialogActive( > On 2014/06/25 21:12:44, Mike Wittman wrote: > > Can we use PopupManager::FromWebContents() here as well? > > Done. > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > File components/web_modal/popup_manager.cc (right): > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > components/web_modal/popup_manager.cc:92: // popup managers with popups > in-flight? > On 2014/06/25 21:12:44, Mike Wittman wrote: > > For the tab drag case we'll at least need to transfer WCMDs associated with > the > > tab's WebContents to the destination browser's PopupManager. > > Yeah, this is going to be fairly complicated and in the end the API may need to > change to accommodate what we need to do. OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. I'm not sure hot
On 2014/06/26 00:21:12, Greg Billock wrote: > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right): > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:215: if > (browser && browser->popup_manager()->IsWebModalDialogActive( > On 2014/06/25 21:12:44, Mike Wittman wrote: > > Can we use PopupManager::FromWebContents() here as well? > > Done. > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > File components/web_modal/popup_manager.cc (right): > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > components/web_modal/popup_manager.cc:92: // popup managers with popups > in-flight? > On 2014/06/25 21:12:44, Mike Wittman wrote: > > For the tab drag case we'll at least need to transfer WCMDs associated with > the > > tab's WebContents to the destination browser's PopupManager. > > Yeah, this is going to be fairly complicated and in the end the API may need to > change to accommodate what we need to do. OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. I'm not sure hot
On 2014/06/27 04:19:35, Greg Billock wrote: > On 2014/06/26 00:21:12, Greg Billock wrote: > > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right): > > > > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:215: if > > (browser && browser->popup_manager()->IsWebModalDialogActive( > > On 2014/06/25 21:12:44, Mike Wittman wrote: > > > Can we use PopupManager::FromWebContents() here as well? > > > > Done. > > > > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > > File components/web_modal/popup_manager.cc (right): > > > > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > > components/web_modal/popup_manager.cc:92: // popup managers with popups > > in-flight? > > On 2014/06/25 21:12:44, Mike Wittman wrote: > > > For the tab drag case we'll at least need to transfer WCMDs associated with > > the > > > tab's WebContents to the destination browser's PopupManager. > > > > Yeah, this is going to be fairly complicated and in the end the API may need > to > > change to accommodate what we need to do. > > OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. I'm > not sure hot sigh. codereview you are something else. OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. I'm not sure how that ends up functioning in real life, but the present plan is for the PopupManager to depend on that interface to get the host view and other important stuff. For now I can rig PopupManager to tolerate being created with a null WCMDH, but that seems like a pretty crippling gap in the API. Is there a plan for this or what?
On 2014/06/27 04:21:43, Greg Billock wrote: > On 2014/06/27 04:19:35, Greg Billock wrote: > > On 2014/06/26 00:21:12, Greg Billock wrote: > > > > > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm > (right): > > > > > > > > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:215: if > > > (browser && browser->popup_manager()->IsWebModalDialogActive( > > > On 2014/06/25 21:12:44, Mike Wittman wrote: > > > > Can we use PopupManager::FromWebContents() here as well? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > > > File components/web_modal/popup_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > > > components/web_modal/popup_manager.cc:92: // popup managers with popups > > > in-flight? > > > On 2014/06/25 21:12:44, Mike Wittman wrote: > > > > For the tab drag case we'll at least need to transfer WCMDs associated > with > > > the > > > > tab's WebContents to the destination browser's PopupManager. > > > > > > Yeah, this is going to be fairly complicated and in the end the API may need > > to > > > change to accommodate what we need to do. > > > > OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. > I'm > > not sure hot > > sigh. codereview you are something else. > > OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. I'm > not sure how that ends up functioning in real life, but the present plan is for > the PopupManager to depend on that interface to get the host view and other > important stuff. For now I can rig PopupManager to tolerate being created with a > null WCMDH, but that seems like a pretty crippling gap in the API. Is there a > plan for this or what? I think this is looking ready to commit now. Any other comments? I went ahead and relaxed the host constraint. I'm not thrilled about the state of the cocoa code there, but there's no additional exposure to it here.
On 2014/06/27 20:22:07, Greg Billock wrote: > On 2014/06/27 04:21:43, Greg Billock wrote: > > On 2014/06/27 04:19:35, Greg Billock wrote: > > > On 2014/06/26 00:21:12, Greg Billock wrote: > > > > > > > > > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > > > > File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/287123002/diff/440001/chrome/browser/ui/cocoa... > > > > chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:215: if > > > > (browser && browser->popup_manager()->IsWebModalDialogActive( > > > > On 2014/06/25 21:12:44, Mike Wittman wrote: > > > > > Can we use PopupManager::FromWebContents() here as well? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > > > > File components/web_modal/popup_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/287123002/diff/440001/components/web_modal/po... > > > > components/web_modal/popup_manager.cc:92: // popup managers with popups > > > > in-flight? > > > > On 2014/06/25 21:12:44, Mike Wittman wrote: > > > > > For the tab drag case we'll at least need to transfer WCMDs associated > > with > > > > the > > > > > tab's WebContents to the destination browser's PopupManager. > > > > > > > > Yeah, this is going to be fairly complicated and in the end the API may > need > > > to > > > > change to accommodate what we need to do. > > > > > > OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. > > I'm > > > not sure hot > > > > sigh. codereview you are something else. > > > > OK, new complication. BrowserWindowCocoa has no WebContentsModalDialogHost. > I'm > > not sure how that ends up functioning in real life, but the present plan is > for > > the PopupManager to depend on that interface to get the host view and other > > important stuff. For now I can rig PopupManager to tolerate being created with > a > > null WCMDH, but that seems like a pretty crippling gap in the API. Is there a > > plan for this or what? > > I think this is looking ready to commit now. Any other comments? I went ahead > and relaxed the host constraint. I'm not thrilled about the state of the cocoa > code there, but there's no additional exposure to it here. lgtm The Cocoa side of this unfortunately never reached parity with Views on implementation of the cross-platform interfaces, and I didn't have the Cocoa chops to pursue it myself. See http://crbug.com/166953.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/540001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
On 2014/06/27 22:34:40, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) Owners, please have a look: groby for cocoa benwells for apps finnur, take a look for extensions
https://codereview.chromium.org/287123002/diff/540001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm (right): https://codereview.chromium.org/287123002/diff/540001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:40: popup_manager->ShowModalDialog(this, web_contents); If the popup manager is scoped to web_contents, why do we need to pass in the contents? Also, is the use of web_contents instead of web_contents_ deliberate or accidental?
Its hard to review this meaningfully at the moment as (a) the bug doesn't explain much and (b) according to the description the policy this manager is implementing isn't known yet. Also, popup manager is a bit of a confusing name, I don't think this applies at all to popup browser windows, it only applies to bubbles and modal dialogs. https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc File apps/app_window.cc (right): https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc#newc... apps/app_window.cc:290: DCHECK(GetWebContentsModalDialogHost()); Nit: can you do DCHECK(popup_manager_.get()) a little later instead of calling GetWebContentsModalDialogHost twice? https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc#newc... apps/app_window.cc:293: popup_manager_->RegisterWith(web_contents); Does there need to be unregistering somewhere?
On 2014/06/30 01:16:52, benwells wrote: > Its hard to review this meaningfully at the moment as (a) the bug doesn't > explain much and (b) according to the description the policy this manager is > implementing isn't known yet. Yes, this change is part 1 of N. > Also, popup manager is a bit of a confusing name, I don't think this applies at > all to popup browser windows, it only applies to bubbles and modal dialogs. That's correct. It's talking about browser popups, not new browser windows. "BubbleAndWebModalDialogManager" is kind of a mouthful, though. :-) > https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc > File apps/app_window.cc (right): > > https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc#newc... > apps/app_window.cc:290: DCHECK(GetWebContentsModalDialogHost()); > Nit: can you do DCHECK(popup_manager_.get()) a little later instead of calling > GetWebContentsModalDialogHost twice? > > https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc#newc... > apps/app_window.cc:293: popup_manager_->RegisterWith(web_contents); > Does there need to be unregistering somewhere? The API supports it, but I didn't put it in for app_window and the other non-browser uses as my belief is that they are single-webcontents. If there are situations where app_window might swap in a new WebContents, then we should unregister.
https://codereview.chromium.org/287123002/diff/540001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm (right): https://codereview.chromium.org/287123002/diff/540001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:40: popup_manager->ShowModalDialog(this, web_contents); It's scoped to the browser; this registration isn't a scoping one. And you're right, it should be web_contents_. On 2014/06/28 02:04:05, groby wrote: > If the popup manager is scoped to web_contents, why do we need to pass in the > contents? > > Also, is the use of web_contents instead of web_contents_ deliberate or > accidental?
https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc File apps/app_window.cc (right): https://codereview.chromium.org/287123002/diff/540001/apps/app_window.cc#newc... apps/app_window.cc:290: DCHECK(GetWebContentsModalDialogHost()); On 2014/06/30 01:16:52, benwells wrote: > Nit: can you do DCHECK(popup_manager_.get()) a little later instead of calling > GetWebContentsModalDialogHost twice? Removed this, at least for now. It turns out Cocoa doesn't have one anyway.
media_galleries_scan_result_dialog_views.cc Was does that have to do with the CL. Was it unintentionally added? https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:326: popup_manager->IsWebModalDialogActive(contents)); Fits on one line? https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:593: parent_web_contents); nit: indentation. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:21: class PopupManagerRelay : public content::WebContentsUserData<PopupManager> { nit: Document purpose. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:26: virtual ~PopupManagerRelay() {} Does this need to be virtual? https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:28: base::WeakPtr<PopupManager> manager_; Make this private plus add a public accessor, instead of exposing it directly. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:29: }; Nit: Missing DISALLOW_etc? https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:82: manager->FocusTopmostDialog(); 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? https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:108: return relay->manager_.get(); 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)? https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:120: // or of owned WCMDMs. nit: This comment is hard to parse -- what does "re-implement in terms of" mean? https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:38: // Temporary method: Provides compatibility wth existing typo: s/wth/with/ https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:57: virtual void WasFocused(const content::WebContents* web_contents); There's no subclass overriding this. Do you need |override| for this? Same question applies to some other virtual functions in this class. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:59: // SupportsUserData-alike API for retrieving the associated window nit: s/SupportsUserData/Supports UserData/ https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:70: virtual void CloseAllDialogsForTesting(content::WebContents* web_contents); Does this need to be virtual? https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... components/web_modal/single_popup_manager.h:17: class SinglePopupManagerDelegate { I kind of would have expected to see this class within the SinglePopupManager class (SinglePopupManager::Delegate as opposed to SinglePopupManagerDelegate). Any particular reason for it being free-standing? https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... components/web_modal/single_popup_manager.h:61: virtual void Pulse() = 0; nit: Pulsate the popup, in order to draw the user's attention to it. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... components/web_modal/single_popup_manager.h:87: SinglePopupManager() {} nit: You have two ctors and two dtors in this .h file. I believe erg (or someone) did a pass at the codebase a while back to aggressively move these into a .cc file, to save on binary size -- otherwise the implementation of the default con-/de-structors would be copied to all the locations that use this class. By having a .cc you have one unit for that code. I know these classes don't (at least yet) contain much complexity in the implementation of the ctors and dtors, but you never know what happens in the future. Anyway, just thought I'd mention it since there's very little downsides, only upsides.
Ah, looks like that file has been deleted (media_galleries_scan_result_dialog_views.cc) on June 6th. You might need to sync to latest (at your convenience).
On 2014/06/30 14:01:18, Finnur wrote: > Ah, looks like that file has been deleted > (media_galleries_scan_result_dialog_views.cc) on June 6th. You might need to > sync to latest (at your convenience). Yes, it's gotten merge-replaced a couple times. :-(
https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:326: popup_manager->IsWebModalDialogActive(contents)); On 2014/06/30 13:53:36, Finnur wrote: > Fits on one line? Done. https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/287123002/diff/560001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:593: parent_web_contents); On 2014/06/30 13:53:36, Finnur wrote: > nit: indentation. Done. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:21: class PopupManagerRelay : public content::WebContentsUserData<PopupManager> { On 2014/06/30 13:53:36, Finnur wrote: > nit: Document purpose. Done. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:26: virtual ~PopupManagerRelay() {} On 2014/06/30 13:53:36, Finnur wrote: > Does this need to be virtual? I think so, for UserData. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:28: base::WeakPtr<PopupManager> manager_; On 2014/06/30 13:53:36, Finnur wrote: > Make this private plus add a public accessor, instead of exposing it directly. I think this is clear enough. There's no real encapsulation danger. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:29: }; On 2014/06/30 13:53:36, Finnur wrote: > Nit: Missing DISALLOW_etc? This class is pretty single-purpose, so putting a bunch of boilerplate in it just obscures its use. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:82: manager->FocusTopmostDialog(); On 2014/06/30 13:53:36, Finnur wrote: > 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? I feel like OnFocus raises the doubt that the popup manager gets focus, but I agree WasFocused isn't that clear either. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:108: return relay->manager_.get(); On 2014/06/30 13:53:36, Finnur wrote: > 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)? 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. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:120: // or of owned WCMDMs. On 2014/06/30 13:53:36, Finnur wrote: > nit: This comment is hard to parse -- what does "re-implement in terms of" mean? 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. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:38: // Temporary method: Provides compatibility wth existing On 2014/06/30 13:53:36, Finnur wrote: > typo: s/wth/with/ Done. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:57: virtual void WasFocused(const content::WebContents* web_contents); On 2014/06/30 13:53:36, Finnur wrote: > There's no subclass overriding this. Do you need |override| for this? Same > question applies to some other virtual functions in this class. Allows for test mocks to override it. This lets us get away with no TestAPI style interface. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:59: // SupportsUserData-alike API for retrieving the associated window On 2014/06/30 13:53:36, Finnur wrote: > nit: s/SupportsUserData/Supports UserData/ Called it WebContentsUserData-alike, which is better. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.h:70: virtual void CloseAllDialogsForTesting(content::WebContents* web_contents); On 2014/06/30 13:53:36, Finnur wrote: > Does this need to be virtual? Probably not. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... File components/web_modal/single_popup_manager.h (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... components/web_modal/single_popup_manager.h:17: class SinglePopupManagerDelegate { On 2014/06/30 13:53:36, Finnur wrote: > I kind of would have expected to see this class within the SinglePopupManager > class (SinglePopupManager::Delegate as opposed to SinglePopupManagerDelegate). > Any particular reason for it being free-standing? Not really. It's descending from the modal dlg mgr structure, is all. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... components/web_modal/single_popup_manager.h:61: virtual void Pulse() = 0; On 2014/06/30 13:53:36, Finnur wrote: > nit: Pulsate the popup, in order to draw the user's attention to it. Done. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/si... components/web_modal/single_popup_manager.h:87: SinglePopupManager() {} On 2014/06/30 13:53:36, Finnur wrote: > nit: You have two ctors and two dtors in this .h file. I believe erg (or > someone) did a pass at the codebase a while back to aggressively move these into > a .cc file, to save on binary size -- otherwise the implementation of the > default con-/de-structors would be copied to all the locations that use this > class. By having a .cc you have one unit for that code. I know these classes > don't (at least yet) contain much complexity in the implementation of the ctors > and dtors, but you never know what happens in the future. Anyway, just thought > I'd mention it since there's very little downsides, only upsides. These are interface stubs, which my impression was weren't nearly as heavy. I don't object to moving them, though.
https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); 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?
https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); On 2014/07/01 18:12:38, groby wrote: > 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? 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.
My main question is whether AppWindow needs to have one of these popup managers or not. Without having more details on what it does it is hard to know. https://codereview.chromium.org/287123002/diff/640001/components/web_modal/po... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/640001/components/web_modal/po... components/web_modal/popup_manager.h:67: void UnregisterWith(content::WebContents* web_contents); Please explain when you should and shouldn't unregister. https://codereview.chromium.org/287123002/diff/640001/components/web_modal/po... components/web_modal/popup_manager.h:69: // DEPRECATED. Why is there a deprecated function in new code?
Does the window ever support popups (bubbles or web modals)? If the answer is yes, then you need one. Basically, if there's any WebContentsModalDialogHost, then you need a popup manager for sure -- this pretty much means "all top-level windows," except I'm hoping to make an exception for the ChromeOS file manager dialog. https://codereview.chromium.org/287123002/diff/640001/components/web_modal/po... File components/web_modal/popup_manager.h (right): https://codereview.chromium.org/287123002/diff/640001/components/web_modal/po... components/web_modal/popup_manager.h:67: void UnregisterWith(content::WebContents* web_contents); On 2014/07/02 05:16:30, benwells wrote: > Please explain when you should and shouldn't unregister. You mean for clients? You shouldn't. The reason these are here at all is DEPS -- they should not be public API methods ideally. https://codereview.chromium.org/287123002/diff/640001/components/web_modal/po... components/web_modal/popup_manager.h:69: // DEPRECATED. On 2014/07/02 05:16:30, benwells wrote: > Why is there a deprecated function in new code? We're inheriting a bunch of code from WebContentsModalDialogManager. This interface will change as that gets absorbed.
https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); On 2014/07/01 19:33:19, Greg Billock wrote: > On 2014/07/01 18:12:38, groby wrote: > > 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? > > 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. 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.
https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); On 2014/07/02 20:13:55, groby wrote: > On 2014/07/01 19:33:19, Greg Billock wrote: > > On 2014/07/01 18:12:38, groby wrote: > > > 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? > > > > 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. > > 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. > > > 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.
Oh, and FWIW - Cocoa LGTM :) If you think the manager lookup thing is too much of a yak, LMK and I'll drop it. https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); > The problem to be > solved is "give me the right PopupManager for this WebContents" given those > constraints. Completely agreed - this is the problem we want to solve. > I agree this is a bit convoluted, but I think it's the right solution to that problem. 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.
https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); On 2014/07/03 01:11:25, groby wrote: > > The problem to be > > solved is "give me the right PopupManager for this WebContents" given those > > constraints. > Completely agreed - this is the problem we want to solve. > > > I agree this is a bit convoluted, but I think it's the right solution to that > problem. > > 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/ I'm not sure what you mean -- there's no knowledge of Browser here (there can't be). > 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(); 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. > just like WebContentsModalDialogManager does now. It's rather similar, and also 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. > 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. 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.
https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); > > 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/ > > I'm not sure what you mean -- there's no knowledge of Browser here (there can't > be). 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. > 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. 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.
https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... File components/web_modal/popup_manager.cc (right): https://codereview.chromium.org/287123002/diff/560001/components/web_modal/po... components/web_modal/popup_manager.cc:90: new PopupManagerRelay(weak_factory_.GetWeakPtr())); On 2014/07/03 02:23:44, groby wrote: > 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. 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. > 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. 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.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/640001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/660001
On 2014/07/08 18:13:35, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/660001 Trying to find some non-ooo owners for these files. If you are owners for chrome/browser/ui, components/ apps/ or chrome/browser/chromeos, please have a look at those files.
I can't review ObjC, but I looked at the rest of c/b/ui/. I'm not familiar enough with this to give it a functional review, so the comments below are basically style, mostly being confused about comment wording. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:451: // may not implement, and currently Cocoa does not either. 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?) https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:903: if (popup_manager_.get()) Nit: Remove get() (not necessary to NULL-check a scoped_ptr anymore) (3 places) https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:246: web_modal::PopupManager* popup_manager() const { Const functions should not return non-const pointers. Either return a const pointer or make the function non-const. (I'm aware that other code in this file violates this rule, but I don't want to require you to fix those cases before checking in.) https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:827: // window. Nit: What's the "overlapping this window" part mean? Are there other popup windows that come from this browser that don't overlap the window and aren't managed here? https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:255: // print requests or attach a user gesture or whatever we know. 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? https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:256: // Note: check because tests may not have a popup manager. Nit: Move this sentence above the TODO and rewrite: "In test code we may not have a popup manager." https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:318: // TODO(gbillock): This can be made more appropriate by asking the bubble Nit: What does "made more appropriate" mean? https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:320: // letting the bubble manager make the blocking call directly and don't use Nit: don't use -> not using https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_window_views.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views.cc:136: popup_manager->ShowModalDialog(widget->GetNativeWindow(), web_contents); Nit: Personally I'd just inline the temp, but up to you. (2 places)
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
removing myself as a chromeos/ reviewer -- someone from the more-specific attestation/ and login/ OWNERS files should take a look instead. adding bartfab@ and nkostylev@ to either review or recommend others.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
I'm not all that familiar with the extension_view_host.{h,cc} changes, so
removing myself as a reviewer. I'd say finnur is your best bet for those,
although looks like he's OOO until the 14th.
//components/web_model.gypi LGTM Feel free to add per-file OWNERS of that file in //components/OWNERS
I am not an owner of any of the code touched by this CL. Nikita is definitely the one who can review or reassign the login/ parts.
chromeos/login lgtm
https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:451: // may not implement, and currently Cocoa does not either. On 2014/07/08 19:13:54, Peter Kasting wrote: > 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?) 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. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:903: if (popup_manager_.get()) On 2014/07/08 19:13:54, Peter Kasting wrote: > Nit: Remove get() (not necessary to NULL-check a scoped_ptr anymore) (3 places) Done. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:246: web_modal::PopupManager* popup_manager() const { On 2014/07/08 19:13:54, Peter Kasting wrote: > Const functions should not return non-const pointers. Either return a const > pointer or make the function non-const. browser_commands uses Browser in a const context when calling this, and the accessor can't return a const object. Similar problems are probably why other code ignores this rule as well. > (I'm aware that other code in this file violates this rule, but I don't want to > require you to fix those cases before checking in.) https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:827: // window. On 2014/07/08 19:13:54, Peter Kasting wrote: > Nit: What's the "overlapping this window" part mean? Are there other popup > windows that come from this browser that don't overlap the window and aren't > managed here? Potentially, yes -- JS popups, for instance, which are a whole different bag of hurt. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:255: // print requests or attach a user gesture or whatever we know. On 2014/07/08 19:13:54, Peter Kasting wrote: > Nit: I don't know what "whatever we know" means here. Also, if this check is Me either. This is dependent on popup policy which is TBD. > only for tests, does this TODO really matter? Or are you saying this can affect > non-test code? 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. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:256: // Note: check because tests may not have a popup manager. On 2014/07/08 19:13:54, Peter Kasting wrote: > Nit: Move this sentence above the TODO and rewrite: "In test code we may not > have a popup manager." Done. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:318: // TODO(gbillock): This can be made more appropriate by asking the bubble On 2014/07/08 19:13:54, Peter Kasting wrote: > Nit: What does "made more appropriate" mean? "made better", "improved", "made more suitable or proper in the circumstances" IOW, this needs a refactor to move away from the tab strip model, which I think is said in the next sentence. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:320: // letting the bubble manager make the blocking call directly and don't use On 2014/07/08 19:13:54, Peter Kasting wrote: > Nit: don't use -> not using Done. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_window_views.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views.cc:136: popup_manager->ShowModalDialog(widget->GetNativeWindow(), web_contents); On 2014/07/08 19:13:55, Peter Kasting wrote: > Nit: Personally I'd just inline the temp, but up to you. (2 places) I'll leave it for now, although I don't feel that strongly.
https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:451: // may not implement, and currently Cocoa does not either. On 2014/07/09 19:28:12, Greg Billock wrote: > On 2014/07/08 19:13:54, Peter Kasting wrote: > > 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?) > > 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. 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. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:246: web_modal::PopupManager* popup_manager() const { On 2014/07/09 19:28:12, Greg Billock wrote: > On 2014/07/08 19:13:54, Peter Kasting wrote: > > Const functions should not return non-const pointers. Either return a const > > pointer or make the function non-const. > > browser_commands uses Browser in a const context when calling this, and the > accessor can't return a const object. Similar problems are probably why other > code ignores this rule as well. Then browser_commands needs to use a non-const Browser. If this is too big a change to fix here, do it as a separate CL before this one. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:827: // window. On 2014/07/09 19:28:12, Greg Billock wrote: > On 2014/07/08 19:13:54, Peter Kasting wrote: > > Nit: What's the "overlapping this window" part mean? Are there other popup > > windows that come from this browser that don't overlap the window and aren't > > managed here? > > Potentially, yes -- JS popups, for instance, which are a whole different bag of > hurt. I suggest clarifying this in your comment. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:255: // print requests or attach a user gesture or whatever we know. On 2014/07/09 19:28:12, Greg Billock wrote: > On 2014/07/08 19:13:54, Peter Kasting wrote: > > Nit: I don't know what "whatever we know" means here. Also, if this check is > > Me either. This is dependent on popup policy which is TBD. > > > only for tests, does this TODO really matter? Or are you saying this can > affect > > non-test code? > > 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. 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>." https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:318: // TODO(gbillock): This can be made more appropriate by asking the bubble On 2014/07/09 19:28:12, Greg Billock wrote: > On 2014/07/08 19:13:54, Peter Kasting wrote: > > Nit: What does "made more appropriate" mean? > > "made better", "improved", "made more suitable or proper in the circumstances" > > IOW, this needs a refactor to move away from the tab strip model, which I think > is said in the next sentence. I would phrase it more directly then by omitting the whole first clause: "Ask the bubble manager..."
https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:451: // may not implement, and currently Cocoa does not either. On 2014/07/09 19:41:41, Peter Kasting wrote: > On 2014/07/09 19:28:12, Greg Billock wrote: > > On 2014/07/08 19:13:54, Peter Kasting wrote: > > > 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?) > > > > 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. > > 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. Done. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:246: web_modal::PopupManager* popup_manager() const { On 2014/07/09 19:41:41, Peter Kasting wrote: > On 2014/07/09 19:28:12, Greg Billock wrote: > > On 2014/07/08 19:13:54, Peter Kasting wrote: > > > Const functions should not return non-const pointers. Either return a const > > > pointer or make the function non-const. > > > > browser_commands uses Browser in a const context when calling this, and the > > accessor can't return a const object. Similar problems are probably why other > > code ignores this rule as well. > > Then browser_commands needs to use a non-const Browser. If this is too big a > change to fix here, do it as a separate CL before this one. Done. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:827: // window. On 2014/07/09 19:41:41, Peter Kasting wrote: > On 2014/07/09 19:28:12, Greg Billock wrote: > > On 2014/07/08 19:13:54, Peter Kasting wrote: > > > Nit: What's the "overlapping this window" part mean? Are there other popup > > > windows that come from this browser that don't overlap the window and aren't > > > managed here? > > > > Potentially, yes -- JS popups, for instance, which are a whole different bag > of > > hurt. > > I suggest clarifying this in your comment. Done. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:255: // print requests or attach a user gesture or whatever we know. On 2014/07/09 19:41:41, Peter Kasting wrote: > On 2014/07/09 19:28:12, Greg Billock wrote: > > On 2014/07/08 19:13:54, Peter Kasting wrote: > > > Nit: I don't know what "whatever we know" means here. Also, if this check > is > > > > Me either. This is dependent on popup policy which is TBD. > > > > > only for tests, does this TODO really matter? Or are you saying this can > > affect > > > non-test code? > > > > 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. > > 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>." Done. https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/660001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:318: // TODO(gbillock): This can be made more appropriate by asking the bubble On 2014/07/09 19:41:41, Peter Kasting wrote: > On 2014/07/09 19:28:12, Greg Billock wrote: > > On 2014/07/08 19:13:54, Peter Kasting wrote: > > > Nit: What does "made more appropriate" mean? > > > > "made better", "improved", "made more suitable or proper in the circumstances" > > > > IOW, this needs a refactor to move away from the tab strip model, which I > think > > is said in the next sentence. > > I would phrase it more directly then by omitting the whole first clause: "Ask > the bubble manager..." Done.
LGTM https://codereview.chromium.org/287123002/diff/700001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/700001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:319: // blocked, or perhaps just by letting the bubble manager make the blocking Nit: Fix grammar in the second half of this comment now that the first half has changed.
https://codereview.chromium.org/287123002/diff/700001/chrome/browser/ui/tabs/... File chrome/browser/ui/tabs/tab_strip_model.cc (right): https://codereview.chromium.org/287123002/diff/700001/chrome/browser/ui/tabs/... chrome/browser/ui/tabs/tab_strip_model.cc:319: // blocked, or perhaps just by letting the bubble manager make the blocking On 2014/07/10 23:09:01, Peter Kasting wrote: > Nit: Fix grammar in the second half of this comment now that the first half has > changed. Done.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/720001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
On 2014/07/11 22:20:52, I haz the power (commit-bot) wrote: > FYI, CQ is re-trying this CL (attempt #1). > The failing builders are: > chromium_presubmit on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) > win_chromium_rel on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) Adding more apps/extensions OWNERS trying to find someone around.
I can at least take care of half of that problem (c/b/extensions). It LGTM.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/720001
On 2014/07/14 16:30:34, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/720001 Added more apps/ folks. TBR coming in 10...9...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/287123002/720001
Message was sent while issue was closed.
Change committed as 283099 |
