MacViews: Unify web contents modal dialog types

MacViews: Unify web contents modal dialog types

The #if around NativeWebContentsModalDialog is a problem for building
the MacViews browser. Unify them to be gfx::NativeWindow on all platforms.
Refactor the cocoa side to move code from ConstrainedWindowMac into
SingleWebContentsDialogManagerCocoa, while keeping
ConstrainedWindowMac's interface the same as before.
The Views side only needs simple changes from NativeView to NativeWindow.

BUG=425229
Committed: https://crrev.com/5f1263bb318f072343f59dec4b310a522c80b9a2
Cr-Commit-Position: refs/heads/master@{#318813}

[Andre, 2015-01-27 00:28:15]

andresantoso@chromium.org changed reviewers:
+ wittman@chromium.org

[Andre, 2015-01-27 00:28:16]

Hey Mike, looking for your advice on this.
We are porting Views to Mac.
I'm hoping to be able to do this without any #ifdef (there is the gyp flag
mac_views_browser in chrome_browser_ui.gypi).
The typedef for NativeWebContentsModalDialog and NativePopup is a
problem, because on a MacViews browser we want to run Views dialogs
on Mac. Ideally the types should be the same whether os is Mac or not.
I found it a tricky to get back a ConstrainedWindowMac* from the
SingleWebContentsDialogManager.
What do you think is the best way to achieve this?

[Mike Wittman, 2015-01-27 23:09:58]

On 2015/01/27 00:28:16, Andre wrote:
> Hey Mike, looking for your advice on this.
> We are porting Views to Mac.
> I'm hoping to be able to do this without any #ifdef (there is the gyp flag
> mac_views_browser in chrome_browser_ui.gypi).
> The typedef for NativeWebContentsModalDialog and NativePopup is a
> problem, because on a MacViews browser we want to run Views dialogs
> on Mac. Ideally the types should be the same whether os is Mac or not.
> I found it a tricky to get back a ConstrainedWindowMac* from the
> SingleWebContentsDialogManager.
> What do you think is the best way to achieve this?

Hi Andre, sorry for the delayed response; I'm paging this stuff back in having
not looked at it in a long time.

The goal of removing the #ifdef is a good one. We would have liked to do this
when initially implementing this stuff but didn't have sufficient Mac developer
support to do so. See here for the history:
https://codereview.chromium.org/12221055.

As background, the WCMD interfaces are designed with the notion that WCMDs are a
simple gfx::NativeView/gfx::NativeWindow that can be displayed on any
WebContentsModalDialogHost. (Previously, these dialogs were tightly tied to
being displayed on the browser window, in platform specific code.) Views was
refactored to support this, but only minimal refactoring occurred in the Mac
code, so the dialogs there remain tied to being displayed on the browser window.

The basic requirement of the NativeWebContentsModalDialog type is that it has a
1-1 relationship with the corresponding dialog. Notably, since a single browser
window or a single WebContents can host multiple WCMDs the
NativeWebContentsModalDialog can't be anything that has a 1-1 relationship with
either of those. I'm mostly unfamiliar with the details of the Mac
implementation, so I don't know how your sheetWindow implementations fit with
this requirement.

Hope that helps.

[Andre, 2015-02-17 22:22:30]

andresantoso@chromium.org changed reviewers:
+ tapted@chromium.org

[Andre, 2015-02-17 22:22:31]

Thanks Mike!
Trent, do you have a better idea?
Basically I want to add web_contents_modal_dialog_manager_views.cc to
the mac_views_browser target.

[tapted, 2015-02-19 11:35:40]

On 2015/02/17 22:22:31, Andre wrote:
> Thanks Mike!
> Trent, do you have a better idea?
> Basically I want to add web_contents_modal_dialog_manager_views.cc to
> the mac_views_browser target.

Sorry - this keeps falling off my queue (this week is busy!). I've only had a
brief look. It reminded my a bit of my "Parent the AppInfoDialog using a
ModalDialogHost" experiment in https://codereview.chromium.org/719203003/ . But
I haven't thought enough to tell if they relate yet.

There are also two modal dialog Widget tests I need to do something with --
https://codereview.chromium.org/782673002/diff/1250001/ui/views/widget/widget...
. The window-modal one might involve using the sheet APIs in some way, and could
relate here.

[tapted, 2015-02-23 22:33:36]

On 2015/02/19 11:35:40, tapted wrote:
> On 2015/02/17 22:22:31, Andre wrote:
> > Thanks Mike!
> > Trent, do you have a better idea?
> > Basically I want to add web_contents_modal_dialog_manager_views.cc to
> > the mac_views_browser target.
> 
> Sorry - this keeps falling off my queue (this week is busy!). I've only had a
> brief look. It reminded my a bit of my "Parent the AppInfoDialog using a
> ModalDialogHost" experiment in https://codereview.chromium.org/719203003/ .
But
> I haven't thought enough to tell if they relate yet.
> 
> There are also two modal dialog Widget tests I need to do something with --
>
https://codereview.chromium.org/782673002/diff/1250001/ui/views/widget/widget...
> . The window-modal one might involve using the sheet APIs in some way, and
could
> relate here.

I pinged andre offline for a clarification, and was going to reply there, but I
might as well put it on record :p.

If the concern is around the change in ConstrainedWindowMac, (bypassing
PopupManager and using WebContentsModalDialogManager directly). I'd be inclined
not to worry too much. (disclaimer: I haven't fully absorbed all the dialog code
yet -- it's not very familiar to me).

My reading is that web_modal::PopupManager is incomplete, the code has gone
stale, and the original author has changed teams. I remember a conversation I
had discussing just tearing it all out (some kind of revert of
https://codereview.chromium.org/287123002/) -- I think with gbillock, but it
mustn't have been archived. I do remember being sad that it might get torn out
though -- the API seemed kinda nice.

I think our options are to "opt-in" more completely to PopupManager -- take
ownership of it, and refactor that API to do what we want. Or to start cutting
it out of the picture. And if the future is toolkit-views everywhere a simpler
abstraction (i.e. no PopupManager, or a views-only PopupManager toolkit) might
be the way to go about it.

https://codereview.chromium.org/866263008/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm (right):

https://codereview.chromium.org/866263008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:15:
NOTREACHED();
Perhaps not for this CL... but if we want to keep web_modal::PopupManager I'd be
more "brute-force" about this:
 * Remove CreateNativeWebModalManager completely
 * have web_contents_modal_dialog_manager.cc call the
NativeWebContentsModalDialogManagerViews directly
 * Move web_contents_modal_dialog_manager_views.cc to components/web_modal
 * Add a views (+optional aura) dependency into components/web_modal
 * Stop compiling components/web_modal on non-toolkit-views Mac

Optional:
 * move constrained_window/constrained_window_views.cc into web_modal and delete
components/constrained_window


Note that this approach (i.e. defining a function in the web_modal namespace
outside of components/web_modal to satisfy the linker) is "wrong" anyway, and
it's irritated me when I've been moving other files around.

It's wrong because it adds a linker backreference to components/web_modal.so
which makes it impossible for components/web_modal to actually be a proper
component -- web_modal has to be described as a static_library in its gypi, and
that prevents anything depending on web_modal from being a component as well.

So, if this is only really used for views, I think the right solution is to just
add the views dependency and make web_modal a proper component library.

[Andre, 2015-02-24 21:31:31]

Thanks Trent!
PTAL at https://crbug.com/157161 for the original motivation.
Patchset #5 (rough) is another idea that is more along that spirit of asking a
manager to present a
native dialog on a web content.
Instead of ConstrainedWindowMac using WCMDM, we make WCMDM use
ConstrainedWindowMac,
and CollectedCookiesMac shows how this can be invoked.
Personally I'm OK with making Cocoa stop using WCMDM if we see limited value,
but there was a
specific effort to include Cocoa, so ....

[tapted, 2015-02-25 06:23:28]

some initial comments. not sure how accurate the comments are - lifetime of
windows on Cocoa tied to C++ objects always bends my brain.

Maybe there's a nice abstraction be overriding NSWindowController with our own
that ties itself to the dialog manager -- it sucks that dialogs have to be
careful to manage their own lifetime on Cocoa with things like DeleteSoon

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/content_settings/collected_cookies_mac.h (right):

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/content_settings/collected_cookies_mac.h:41:
content::WebContents* web_contents_;  // weak
This weak reference spooks me out a little. It's not clear that it's guaranteed
to outlive the dialog.

CollectedCookiesMac is currently self-deleting. I think the
WebContentsModalDialogManager ensures it gets closed, so perhaps it would be
clearer to have a weak reference to the WebContentsModalDialogManager instead,
with a comment like

// Weak. Closes this dialog.

Or "Weak. Owns this." except that while it's self-deleting that's a bit of a
gray statement.

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/content_settings/collected_cookies_mac.mm (right):

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/content_settings/collected_cookies_mac.mm:82:
base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
I'm not familiar with the CollectedCookies dialog.. but the guarantee that this
can be called only once may have been lost. i.e. both the observer and
closeSheet: call PerformClose() -- can they fight?

Maybe the Delete can be triggered via windowWillClose?

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm (right):

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:56:
SingleWebContentsDialogManagerDelegate* native_delegate_;
Lifetime comment?

[Mike Wittman, 2015-02-25 20:40:53]

This looks like a fruitful approach to me. At a minimum, moving the Mac code
towards the WCMDM behavior expectations will make it easier to reason about
further changes to support MacViews for these dialogs.

Re PopupManager: I concur with the assessment in #7. If the goal is to move to
Views-everywhere, then removing the PopupManager in favor of the WCMDM seems the
best approach. If and when we eventually address the PopupManager functionality
it will be a fair amount easier if we don't have to do it across
toolkit-independent interfaces.

+gbillock since he was working on this.

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm (right):

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:33:
window_.reset(new ConstrainedWindowMac(
I think this preserves the existing creation/show/hide behavior so should be OK
for now.

If you're moving towards a Mac implementation more in line with the spirit of
the desired interfaces, you may want/need to address https://crbug.com/264032 at
some point. This would use the cross-platform show/hide logic in the WCMDM for
Mac. Without that change it may be difficult to reason about the show/hide
behavior due to the two different implementations.

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:40:
window_->CloseWebContentsModalDialog();
Add null check. window_ can be null here if the dialog is created but not shown
before being destroyed. This occurs e.g. when another WCMD is already being
displayed when this one is created, then the tab is closed.

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:44:
window_->FocusWebContentsModalDialog();
DCHECK(window_.get())

This shouldn't get called unless this window has been shown.

https://codereview.chromium.org/866263008/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:48:
window_->PulseWebContentsModalDialog();
Ditto.

[Andre, 2015-02-26 05:55:57]

Turns out many tests expect ConstrainedWindowMac to use WCMDM so they
failed.
Mike, Trent, PTAL at patchset 6. It makes ConstrainedWindowMac use WCMDM
again, but moves most of its code down to WCMDMCocoa.

I decided not to change CollectedCookiesMac for now. Closing the dialog by
calling WillClose() does not seem correct. The Views implementation just
closes the Widget which is observed by WCMDMViews, but I can't just call
-close on the sheet window on Cocoa. Mike, how do you think that should
work?

[Mike Wittman, 2015-02-26 19:57:38]

On 2015/02/26 05:55:57, Andre wrote:
> Turns out many tests expect ConstrainedWindowMac to use WCMDM so they
> failed.
> Mike, Trent, PTAL at patchset 6. It makes ConstrainedWindowMac use WCMDM
> again, but moves most of its code down to WCMDMCocoa.
> 
> I decided not to change CollectedCookiesMac for now. Closing the dialog by
> calling WillClose() does not seem correct. The Views implementation just
> closes the Widget which is observed by WCMDMViews, but I can't just call
> -close on the sheet window on Cocoa. Mike, how do you think that should
> work?

I don't know about the constraints of closing windows under Cocoa, but let me
give some background on the expected sequence of events around close:

By design, the WCMD interfaces are intended to treat the WCMD as a "regular"
dialog that can be closed via whatever toolkit-native window close/destroy
mechanism is available. When this is done, it is expected that
WCMDM::WillClose() is invoked to let the WCMDM know to stop managing the dialog
and delete the single manager. On Windows at least, we require this behavior
because the WCMD can be closed as a result of the NativeWidget closing, with no
explicit action taken in code to close the dialog. This occurs e.g. when the
browser window containing the WCMD is closed by the user. This sequence is also
triggered when dialogs close through their normal UI mechanisms
(accept/cancel/etc).

Additionally we need to explicitly close the dialog when the WebContents is
destroyed on tab close. This is what SWCMDM::Close() is intended to handle, and
if the previous mechanism is in place, we should be able to do this by simply
invoking the toolkit-native window close/destroy mechanism.

If you can't call -close in the latter case, do you have another mechanism for
triggering window close/destroy in Cocoa? Is there a Cocoa event you can listen
to for window close that can signal WillClose() back to the WCMDM?
NSWindowWillCloseNotification?

[tapted, 2015-02-26 23:34:49]

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:50:
base::scoped_nsprotocol<id<ConstrainedWindowSheet>> sheet_;
is this still needed?

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:36:
make_scoped_ptr(native_manager_));
This feels a bit like "cheating" to me :). i.e. passing a scoped_ptr while
holding on to a reference.

I think the observer-interface could be formalized more. That is, instead of
being just an observer, inherit from a `CocoaDialogOwner` (something like
ui::LayerOwner I guess), or perhaps `CocoaModalDialogClient` (or delegate..).
This way the WebContentsModalDialogManagerCocoa can be responsible for
clobbering the reference, rather than relying on client code to add observers
and correctly nulling-out references.

I think there is currently only be one observer, so observer-style isn't really
needed.

The CocoaModalDialogClient base is initialized first in the constructor
initializer list with the webcontents. (the data member probably isn't needed on
ConstrainedWindowMa as well)

The code here would be just `super::ShowSheet(sheet)`, being

CocoaModalDialogClient::ShowSheet(id<ConstrainedWindowSheet> sheet) {
  auto manager = WebContentsModalDialogManager::FromWebContents(web_contents_);
  scoped_ptr<SingleWebContentsModalDialogManagerCocoa> native_manager(
      new SingleWebContentsModalDialogManagerCocoa(this, sheet, manager));
  WebContentsModalDialogManager::FromWebContents(web_contents_)
      ->ShowDialogWithManager([sheet_ sheetWindow], native_manager.Pass());
}

[maybe you can even pass web_contents to ShowSheet as well, and not have a data
member]

And the constructor,

SingleWebContentsModalDialogManagerCocoa(CocoaModalDialogClient* client, sheet)
    : client_(client) {
  /* other stuff */
  client->SetManager(this);
}

and then SingleWebContentsModalDialogManagerCocoa just does
client_->SetManager(nullptr); whenever it needs to.

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:41: if
(native_manager_)
I *think* this should be DCHECK(!GetManager());  [is there a codepath where that
fails?]

i.e. the destructor is usually too late for these things -- in my experience, if
the window hasn't been closed by this point then something is leaking.

Maybe see WindowedInstallDialogController in
chrome/browser/ui/cocoa/extensions/windowed_install_dialog_controller.h for
ideas

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:46: if
(native_manager_)
in a `CocoaModalDialgClient` world this would be

if (GetManager())
  GetManager()->Close();

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h:27:
@property(readonly) NSWindow* sheetWindow;
nonatomic?

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.h (right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.h:8: #include
"base/mac/scoped_nsobject.h"
nit: import

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.h:14: class
WebContentsModalDialogManagerCocoa
It's a mouthful, but perhaps prefix with 'Single'. As it is, I think this is a
bad name. It implies the class inherits from WebContentsModalDialogManager, but
that's who owns it. Also (nit): needs a class comment.

[Andre, 2015-02-28 00:09:46]

Patchset #7 (id:120001) has been deleted

[Andre, 2015-03-02 02:55:56]

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h:50:
base::scoped_nsprotocol<id<ConstrainedWindowSheet>> sheet_;
On 2015/02/26 23:34:48, tapted wrote:
> is this still needed?

Deleted.

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:36:
make_scoped_ptr(native_manager_));
On 2015/02/26 23:34:49, tapted wrote:
> This feels a bit like "cheating" to me :). i.e. passing a scoped_ptr while
> holding on to a reference.
> 
> I think the observer-interface could be formalized more. That is, instead of
> being just an observer, inherit from a `CocoaDialogOwner` (something like
> ui::LayerOwner I guess), or perhaps `CocoaModalDialogClient` (or delegate..).
> This way the WebContentsModalDialogManagerCocoa can be responsible for
> clobbering the reference, rather than relying on client code to add observers
> and correctly nulling-out references.
> 
> I think there is currently only be one observer, so observer-style isn't
really
> needed.
> 
> The CocoaModalDialogClient base is initialized first in the constructor
> initializer list with the webcontents. (the data member probably isn't needed
on
> ConstrainedWindowMa as well)
> 
> The code here would be just `super::ShowSheet(sheet)`, being
> 
> CocoaModalDialogClient::ShowSheet(id<ConstrainedWindowSheet> sheet) {
>   auto manager =
WebContentsModalDialogManager::FromWebContents(web_contents_);
>   scoped_ptr<SingleWebContentsModalDialogManagerCocoa> native_manager(
>       new SingleWebContentsModalDialogManagerCocoa(this, sheet, manager));
>   WebContentsModalDialogManager::FromWebContents(web_contents_)
>       ->ShowDialogWithManager([sheet_ sheetWindow], native_manager.Pass());
> }
> 
> [maybe you can even pass web_contents to ShowSheet as well, and not have a
data
> member]
> 
> And the constructor,
> 
> SingleWebContentsModalDialogManagerCocoa(CocoaModalDialogClient* client,
sheet)
>     : client_(client) {
>   /* other stuff */
>   client->SetManager(this);
> }
> 
> and then SingleWebContentsModalDialogManagerCocoa just does
> client_->SetManager(nullptr); whenever it needs to.

Done, PTAL.
But the separation between ModalDialogClient and ConstrainedWindowMac does not
feel that
useful now. Maybe we should just merge them?

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:41: if
(native_manager_)
On 2015/02/26 23:34:49, tapted wrote:
> I *think* this should be DCHECK(!GetManager());  [is there a codepath where
that
> fails?]
> 
> i.e. the destructor is usually too late for these things -- in my experience,
if
> the window hasn't been closed by this point then something is leaking.
> 
> Maybe see WindowedInstallDialogController in
> chrome/browser/ui/cocoa/extensions/windowed_install_dialog_controller.h for
> ideas

Done. Added the DCHECK in ~ModalDialogClientCocoa.

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:46: if
(native_manager_)
On 2015/02/26 23:34:49, tapted wrote:
> in a `CocoaModalDialgClient` world this would be
> 
> if (GetManager())
>   GetManager()->Close();

Done.
I went with calling super's Close() to match how Show() is done.

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h:27:
@property(readonly) NSWindow* sheetWindow;
On 2015/02/26 23:34:49, tapted wrote:
> nonatomic?

I wasn't sure what readonly+nonatomic means.
I suppose if an adopting class does a @synthesize, the generated getter might
unnecessarily use a lock even
thought there is no setter? Seems like it should not.
But it doesn't hurt and it's commonly done in our codebase.
Done.

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.h (right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.h:8: #include
"base/mac/scoped_nsobject.h"
On 2015/02/26 23:34:49, tapted wrote:
> nit: import

Done.

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.h:14: class
WebContentsModalDialogManagerCocoa
On 2015/02/26 23:34:49, tapted wrote:
> It's a mouthful, but perhaps prefix with 'Single'. As it is, I think this is a
> bad name. It implies the class inherits from WebContentsModalDialogManager,
but
> that's who owns it. Also (nit): needs a class comment.

Done.

[tapted, 2015-03-02 04:28:50]

lgtm % nits

Maybe say a bit more in the CL description about the approach too.

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:36:
make_scoped_ptr(native_manager_));
On 2015/03/02 02:55:56, Andre wrote:
> Done, PTAL.
> But the separation between ModalDialogClient and ConstrainedWindowMac does not
> feel that
> useful now. Maybe we should just merge them?

I'm not opposed to merging them, but there might be a curly lifetime issue that
makes it not-quite-fit.. (i guess we'd have to try it :). But this seems
reasonably neat to me.

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/modal_dialog_client_cocoa.h (right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/modal_dialog_client_cocoa.h:9: #include
"base/memory/scoped_ptr.h"
nit: move to .mm?

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm
(right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:54:
client_->OnDialogClosing();  // client might delete itself here.
nit: client -> |client_|  (or "Client")

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.h (right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.h:10: #include
"base/mac/scoped_nsobject.h"
nit: import

[Mike Wittman, 2015-03-02 18:35:53]

lgtm

Please be sure to test the close behavior when the browser window is closed
while a dialog is being shown.

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm
(right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
nit: Copyright 2015

[Andre, 2015-03-02 18:47:31]

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:36:
make_scoped_ptr(native_manager_));
On 2015/03/02 04:28:50, tapted wrote:
> On 2015/03/02 02:55:56, Andre wrote:
> > Done, PTAL.
> > But the separation between ModalDialogClient and ConstrainedWindowMac does
not
> > feel that
> > useful now. Maybe we should just merge them?
> 
> I'm not opposed to merging them, but there might be a curly lifetime issue
that
> makes it not-quite-fit.. (i guess we'd have to try it :). But this seems
> reasonably neat to me.

I merged them in the latest patch, it looks good.

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/modal_dialog_client_cocoa.h (right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/modal_dialog_client_cocoa.h:9: #include
"base/memory/scoped_ptr.h"
On 2015/03/02 04:28:50, tapted wrote:
> nit: move to .mm?

Done.

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm
(right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:54:
client_->OnDialogClosing();  // client might delete itself here.
On 2015/03/02 04:28:50, tapted wrote:
> nit: client -> |client_|  (or "Client")

Done.

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.h (right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.h:10: #include
"base/mac/scoped_nsobject.h"
On 2015/03/02 04:28:50, tapted wrote:
> nit: import

Done.

[Andre, 2015-03-02 18:48:27]

On 2015/03/02 18:35:53, Mike Wittman wrote:
> lgtm
> 
> Please be sure to test the close behavior when the browser window is closed
> while a dialog is being shown.

Yep, I tested that. Thanks!

[Andre, 2015-03-02 18:50:23]

andresantoso@chromium.org changed reviewers:
+ rsesek@chromium.org

[Andre, 2015-03-02 18:50:24]

+rsesek for Cocoa.
+msw for c/b/ui/views.
Thanks!

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm
(right):

https://codereview.chromium.org/866263008/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2015/03/02 18:35:53, Mike Wittman wrote:
> nit: Copyright 2015

Done.

[Andre, 2015-03-02 18:50:38]

andresantoso@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2015-03-02 19:26:10]

c/b/ui/views lgtm

[Robert Sesek, 2015-03-02 22:15:27]

https://codereview.chromium.org/866263008/diff/180001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/180001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:30: auto
manager = WebContentsModalDialogManager::FromWebContents(web_contents);
Can |manager| be null?

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
File components/web_modal/native_web_contents_modal_dialog.h (right):

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
components/web_modal/native_web_contents_modal_dialog.h:12: // TODO(gbillock):
rename this file
It seems like this entire file could be removed and the gfx types could be use
instead.

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
components/web_modal/native_web_contents_modal_dialog.h:14: typedef
gfx::NativeWindow NativeWebContentsModalDialog;
|using| is now preferred.

[tapted, 2015-03-02 22:25:46]

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:36:
make_scoped_ptr(native_manager_));
On 2015/03/02 18:47:31, Andre wrote:
> On 2015/03/02 04:28:50, tapted wrote:
> > On 2015/03/02 02:55:56, Andre wrote:
> > > Done, PTAL.
> > > But the separation between ModalDialogClient and ConstrainedWindowMac does
> not
> > > feel that
> > > useful now. Maybe we should just merge them?
> > 
> > I'm not opposed to merging them, but there might be a curly lifetime issue
> that
> > makes it not-quite-fit.. (i guess we'd have to try it :). But this seems
> > reasonably neat to me.
> 
> I merged them in the latest patch, it looks good.

still lgtm after the merge.

[Andre, 2015-03-02 22:45:38]

https://codereview.chromium.org/866263008/diff/180001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/180001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:30: auto
manager = WebContentsModalDialogManager::FromWebContents(web_contents);
On 2015/03/02 22:15:27, Robert Sesek wrote:
> Can |manager| be null?

For these FromWebContents kinds, it seems like it's pretty common practice to
assume non-null (set up by TabHelpers).
It might be null if a test does not set it up.

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
File components/web_modal/native_web_contents_modal_dialog.h (right):

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
components/web_modal/native_web_contents_modal_dialog.h:12: // TODO(gbillock):
rename this file
On 2015/03/02 22:15:27, Robert Sesek wrote:
> It seems like this entire file could be removed and the gfx types could be use
> instead.

There are quite a bit of call sites that will need to be adjusted.
I can do this replacement in a separate CL if wittman@ and msw@ approves.

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
components/web_modal/native_web_contents_modal_dialog.h:14: typedef
gfx::NativeWindow NativeWebContentsModalDialog;
On 2015/03/02 22:15:27, Robert Sesek wrote:
> |using| is now preferred.

Done.

[Mike Wittman, 2015-03-02 22:48:31]

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
File components/web_modal/native_web_contents_modal_dialog.h (right):

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
components/web_modal/native_web_contents_modal_dialog.h:12: // TODO(gbillock):
rename this file
On 2015/03/02 22:45:38, Andre wrote:
> On 2015/03/02 22:15:27, Robert Sesek wrote:
> > It seems like this entire file could be removed and the gfx types could be
use
> > instead.
> 
> There are quite a bit of call sites that will need to be adjusted.
> I can do this replacement in a separate CL if wittman@ and msw@ approves.

Replacing types and removing this file in a separate CL sgtm.

[Robert Sesek, 2015-03-02 22:50:22]

LGTM

https://codereview.chromium.org/866263008/diff/180001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
(right):

https://codereview.chromium.org/866263008/diff/180001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm:30: auto
manager = WebContentsModalDialogManager::FromWebContents(web_contents);
On 2015/03/02 22:45:38, Andre wrote:
> On 2015/03/02 22:15:27, Robert Sesek wrote:
> > Can |manager| be null?
> 
> For these FromWebContents kinds, it seems like it's pretty common practice to
> assume non-null (set up by TabHelpers).
> It might be null if a test does not set it up.

OK. The popup manager on the LHS was null-checked, which is why I asked.

[msw, 2015-03-02 23:07:02]

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
File components/web_modal/native_web_contents_modal_dialog.h (right):

https://codereview.chromium.org/866263008/diff/180001/components/web_modal/na...
components/web_modal/native_web_contents_modal_dialog.h:12: // TODO(gbillock):
rename this file
On 2015/03/02 22:48:31, Mike Wittman wrote:
> On 2015/03/02 22:45:38, Andre wrote:
> > On 2015/03/02 22:15:27, Robert Sesek wrote:
> > > It seems like this entire file could be removed and the gfx types could be
> use
> > > instead.
> > 
> > There are quite a bit of call sites that will need to be adjusted.
> > I can do this replacement in a separate CL if wittman@ and msw@ approves.
> 
> Replacing types and removing this file in a separate CL sgtm.

Ditto, I think that's a great suggestion.

[Andre, 2015-03-03 01:04:51]

The CQ bit was checked by andresantoso@chromium.org

[Andre, 2015-03-03 01:04:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
wittman@chromium.org, tapted@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/866263008/#ps200001
(title: "Fix for rsesek")

[commit-bot: I haz the power, 2015-03-03 01:05:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/866263008/200001

[commit-bot: I haz the power, 2015-03-03 01:37:23]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-03-03 01:38:29]

Patchset 10 (id:??) landed as
https://crrev.com/5f1263bb318f072343f59dec4b310a522c80b9a2
Cr-Commit-Position: refs/heads/master@{#318813}