[MacViews] Position modal sheets using ModalDialogHost.

[MacViews] Position modal sheets using ModalDialogHost.

This makes ViewsNSWindowDelegate override
-[NSWindow window:willPositionSheet:usingRect:] and call the newly
added NativeWidgetMac::SheetPositionY().

BrowserFrameMac implements SheetPositionY() using the same
ModalDialogHost as Views browsers.

Note that on Views, modal dialogs appear below the omnibox whereas on
Cocoa they appear below the bookmarks bar, but this can be addressed
later as MacViews UX is not finalized.

BUG=524847
Committed: https://crrev.com/c5fd2cddccff942e467846c4cf3ba5b9fc089b34
Cr-Commit-Position: refs/heads/master@{#346008}

[jackhou1, 2015-08-26 02:10:04]

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

[jackhou1, 2015-08-26 02:10:05]

tapted, PTAL

[tapted, 2015-08-26 03:20:41]

nits mostly..

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/app...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:25: return
NSHeight([ns_window frame]) - NSMaxY([[ns_window contentView] frame]);
how does this look for frameless widgets? (is it consistent with Cocoa?)

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_frame_mac.mm:36: NSView* host_view =
dialog_host->GetHostView();
I think I need a comment about the logic here.

E.g. mention why not simply GetDialogPosition(..).y() and a summary

Also does this work equivalently well in mac_views_browser?

And, do you think it's worth separating GetDialogPosition() into
GetChildModalDialogPosition() and GetWindowModalDialogPosition()?

https://codereview.chromium.org/1315043003/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac.mm:109: return 0;
would window height - content view height be a better default?

[jackhou1, 2015-08-26 07:02:09]

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/app...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:25: return
NSHeight([ns_window frame]) - NSMaxY([[ns_window contentView] frame]);
On 2015/08/26 03:20:41, tapted wrote:
> how does this look for frameless widgets? (is it consistent with Cocoa?)

I had tried this with the USB selector dialog, but it turns out that's actually
WebContents modal. I'm not sure if there are any window-modal dialogs on app
windows. There's only one override of -willPositionSheet in the codebase, and
it's on BrowserWindowControllerPrivate, so presumably any modal sheets on app
windows will do the default, which is to show it at the top of the contentView
frame.

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_frame_mac.mm:36: NSView* host_view =
dialog_host->GetHostView();
On 2015/08/26 03:20:41, tapted wrote:
> I think I need a comment about the logic here.
> 
> E.g. mention why not simply GetDialogPosition(..).y() and a summary

Done.

> Also does this work equivalently well in mac_views_browser?

This only applies to mac_views_browser since positioning is done by the parent
window.
 
> And, do you think it's worth separating GetDialogPosition() into
> GetChildModalDialogPosition() and GetWindowModalDialogPosition()?

Nup, I think this is the only caller that wants the position relative to the
window.

https://codereview.chromium.org/1315043003/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac.mm:109: return 0;
On 2015/08/26 03:20:41, tapted wrote:
> would window height - content view height be a better default?

Done.

[tapted, 2015-08-26 07:27:49]

some suggestions...

https://codereview.chromium.org/1315043003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:38: //
ModalDialogHost::GetDialogPosition is relative to the host view.
nit: GetDialogPosition()

https://codereview.chromium.org/1315043003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:40: NSHeight([[host_view
window] frame]) - NSMaxY([host_view frame]);
So I think this assumes host_view is the contentView (i.e. its frame is in the
same coordinate space as the window bounds), which might not be the case... I.e.
this only works if |dialog_host| and all its parent views are positioned at
(0,0) in their respective parents.

The idiomatic way to do this is something like

NSPoint pointInHost = flip([host_view frame],
dialog_host->GetDialogPosition(gfx::Size());
NSPoint pointInWindow = [host_view convertPoint:pointInHost toView:nil];
return gfx::Point(flip([window frame], pointInWindow)).y();

But.. I guess that's a bit horrible. If you don't like it, perhaps comment about
the assumption, and do something like

DCHECK_EQ(0, NSMinY([host_view frame]);  // Actually all the parents must be at
0 as well.
int height_delta = NSHeight([[host_view window] frame]) - NSHeight([host_view
frame]);

i.e. make it clear that it's only necessary to concern ourselves with the
difference in the heights of the host view and the window.

https://codereview.chromium.org/1315043003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:110: return NSHeight([ns_window frame]) -
NSMaxY([[ns_window contentView] frame]);
If it makes it simpler... maybe make SheetPositionY return the distance from the
bottom of the window?

Then this would be

NSView* view = GetNativeView()
return [view convertPoint:NSMakePoint(0, NSHeight([view frame])) toView:nil].y;


but this is OK too.

[jackhou1, 2015-08-27 05:38:09]

https://codereview.chromium.org/1315043003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:38: //
ModalDialogHost::GetDialogPosition is relative to the host view.
On 2015/08/26 07:27:48, tapted wrote:
> nit: GetDialogPosition()

Done.

https://codereview.chromium.org/1315043003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:40: NSHeight([[host_view
window] frame]) - NSMaxY([host_view frame]);
On 2015/08/26 07:27:48, tapted wrote:
> So I think this assumes host_view is the contentView (i.e. its frame is in the
> same coordinate space as the window bounds), which might not be the case...
I.e.
> this only works if |dialog_host| and all its parent views are positioned at
> (0,0) in their respective parents.
> 
> The idiomatic way to do this is something like
> 
> NSPoint pointInHost = flip([host_view frame],
> dialog_host->GetDialogPosition(gfx::Size());
> NSPoint pointInWindow = [host_view convertPoint:pointInHost toView:nil];
> return gfx::Point(flip([window frame], pointInWindow)).y();
> 
> But.. I guess that's a bit horrible. If you don't like it, perhaps comment
about
> the assumption, and do something like
> 
> DCHECK_EQ(0, NSMinY([host_view frame]);  // Actually all the parents must be
at
> 0 as well.
> int height_delta = NSHeight([[host_view window] frame]) - NSHeight([host_view
> frame]);
> 
> i.e. make it clear that it's only necessary to concern ourselves with the
> difference in the heights of the host view and the window.

Changed to use -convertPoint to get the host_view's top edge.

https://codereview.chromium.org/1315043003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1315043003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:110: return NSHeight([ns_window frame]) -
NSMaxY([[ns_window contentView] frame]);
On 2015/08/26 07:27:48, tapted wrote:
> If it makes it simpler... maybe make SheetPositionY return the distance from
the
> bottom of the window?
> 
> Then this would be
> 
> NSView* view = GetNativeView()
> return [view convertPoint:NSMakePoint(0, NSHeight([view frame]))
toView:nil].y;
> 
> 
> but this is OK too.

Done.

It makes the BrowserFrameMac version simpler too.

[tapted, 2015-08-27 07:03:13]

lgtm

[jackhou1, 2015-08-27 07:11:03]

jackhou@chromium.org changed reviewers:
+ sky@chromium.org

[jackhou1, 2015-08-27 07:11:03]

sky, please review for OWNERS of c/b/ui/views/frame/

[sky, 2015-08-27 16:49:35]

LGTM

[jackhou1, 2015-08-27 22:28:05]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-08-27 22:28:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315043003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315043003/60001

[commit-bot: I haz the power, 2015-08-27 22:37:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-27 22:38:43]

Patchset 4 (id:??) landed as
https://crrev.com/c5fd2cddccff942e467846c4cf3ba5b9fc089b34
Cr-Commit-Position: refs/heads/master@{#346008}