MacViews: Allow views::Widgets to be parented off NSWindows

MacViews: Allow views::Widgets to be parented off NSWindows

Currently, a views::Widget on Mac can't be owned by a NSWindow that is
not also a views::Widget. This would mean that to add a toolkit-views
dialog parented to the browser, the whole browser window would need to
be ported to views first.

This CL abstracts the responsibilities of a NativeViewMac's parent
window into a BridgedNativeWidgetOwner interface which
BridgedNativeWidget implements to retain the current behaviour.

WidgetOwnerNSWindowAdapter is a lightweight class that implements this
to allow the parent to be any native NSView. The parent window closing
will also close the child, and (for appropriate child types) the child
window frame is offset by the parent view's position on screen.

BUG=447086
TEST=views_unittests' NativeWidgetMacTest.NonWidgetParent

Committed: https://crrev.com/adc5eefaf1611f00b4b3482baa07bb10a335c681
Cr-Commit-Position: refs/heads/master@{#329097}

[tapted, 2015-05-06 12:11:05]

Patchset #5 (id:80001) has been deleted

[tapted, 2015-05-07 12:39:34]

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

[tapted, 2015-05-07 12:40:54]

Patchset #6 (id:120001) has been deleted

[tapted, 2015-05-07 12:56:47]

Hi Andre, please take a look.

[Andre, 2015-05-07 17:32:38]

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:683: return parent()->IsVisibleParent();
How about,
  return parent() ? parent()->IsVisibleParent() && window_visible_
                  : window_visible_;
I think it's more readable, and you probably won't need the comment anymore.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_owner.h (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_owner.h:25: virtual NSWindow* GetNSWindow()
= 0;
const?
(also ChildWIndowOffset and IsVisibleParent).

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_owner.h:28: virtual gfx::Vector2d
ChildWindowOffset() = 0;
How about GetChildWindowOffset? Use of Get prefix seems to be common style in
our C++ codebase.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/widget_...
File ui/views/cocoa/widget_owner_nswindow_adapter.mm (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/widget_...
ui/views/cocoa/widget_owner_nswindow_adapter.mm:21: -
(id)initWithAdapter:(views::WidgetOwnerNSWindowAdapter*)adapter;
Use instancetype for the return type.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/widget_...
ui/views/cocoa/widget_owner_nswindow_adapter.mm:72: NSRect rect_in_screen = {{0,
0}, {0, 0}};
NSZeroRect

https://codereview.chromium.org/1063933003/diff/140001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:10: #import
"base/mac/scoped_nsobject.h"
Sorts before base/run_loop.h.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:19: #include
"ui/views/widget/native_widget_private.h"
Sorts after ui/views/test/...

[tapted, 2015-05-07 23:43:51]

(note there's a small rebase in native_widget_mac.mm)

Thanks Andre!

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:683: return parent()->IsVisibleParent();
On 2015/05/07 17:32:37, Andre wrote:
> How about,
>   return parent() ? parent()->IsVisibleParent() && window_visible_
>                   : window_visible_;
> I think it's more readable, and you probably won't need the comment anymore.
> 

Done (also put the recursive call after the sequence point)

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_owner.h (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_owner.h:25: virtual NSWindow* GetNSWindow()
= 0;
On 2015/05/07 17:32:37, Andre wrote:
> const?
> (also ChildWIndowOffset and IsVisibleParent).

Done.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_owner.h:28: virtual gfx::Vector2d
ChildWindowOffset() = 0;
On 2015/05/07 17:32:37, Andre wrote:
> How about GetChildWindowOffset? Use of Get prefix seems to be common style in
> our C++ codebase.

Done.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/widget_...
File ui/views/cocoa/widget_owner_nswindow_adapter.mm (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/widget_...
ui/views/cocoa/widget_owner_nswindow_adapter.mm:21: -
(id)initWithAdapter:(views::WidgetOwnerNSWindowAdapter*)adapter;
On 2015/05/07 17:32:37, Andre wrote:
> Use instancetype for the return type.

Done.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/cocoa/widget_...
ui/views/cocoa/widget_owner_nswindow_adapter.mm:72: NSRect rect_in_screen = {{0,
0}, {0, 0}};
On 2015/05/07 17:32:37, Andre wrote:
> NSZeroRect

Done.

https://codereview.chromium.org/1063933003/diff/140001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1063933003/diff/140001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:10: #import
"base/mac/scoped_nsobject.h"
On 2015/05/07 17:32:38, Andre wrote:
> Sorts before base/run_loop.h.

Whoops - forgot header sort checks don't check .mm files

https://codereview.chromium.org/1063933003/diff/140001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:19: #include
"ui/views/widget/native_widget_private.h"
On 2015/05/07 17:32:37, Andre wrote:
> Sorts after ui/views/test/...

Done.

[Andre, 2015-05-07 23:48:19]

LGTM

[tapted, 2015-05-07 23:49:30]

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

[tapted, 2015-05-07 23:49:31]

+sky for views.gyp OWNERS (or any other comments you may have :) - Thanks!

[sky, 2015-05-08 16:41:26]

LGTM with fixed constness.

https://codereview.chromium.org/1063933003/diff/160001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1063933003/diff/160001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.h:156: BridgedNativeWidgetOwner* parent()
const { return parent_; }
Returning a non-const pointer from a const function allows you to circumvent
const'ness. The old code is better. If you need a const version too, then it
should return a const BridgedNativeWidgetOwner*.

[tapted, 2015-05-09 05:00:21]

https://codereview.chromium.org/1063933003/diff/160001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1063933003/diff/160001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.h:156: BridgedNativeWidgetOwner* parent()
const { return parent_; }
On 2015/05/08 16:41:26, sky wrote:
> Returning a non-const pointer from a const function allows you to circumvent
> const'ness. The old code is better. If you need a const version too, then it
> should return a const BridgedNativeWidgetOwner*.

Done. Went back to the old stuff here. Actually, just returning a `const
BridgedNativeWidgetOwner*` here works, but it breaks down for
BridgedNativeWidgetOwner::GetNSWindow() [which has the same problem]. Having a
`const NSWindow*` is usually a trap since objective-C interfaces don't support
const methods.

But only minor changes allow GetChildWindowOffset() and IsVisibleParent() to
remain const.

[tapted, 2015-05-09 05:00:32]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-05-09 05:00:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
andresantoso@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1063933003/#ps180001
(title: "fix constness")

[commit-bot: I haz the power, 2015-05-09 05:01:02]

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

[commit-bot: I haz the power, 2015-05-09 12:15:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-09 12:15:51]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[tapted, 2015-05-10 00:28:17]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-05-10 00:29:25]

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

[commit-bot: I haz the power, 2015-05-10 04:32:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-10 04:32:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[tapted, 2015-05-10 23:19:02]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-05-10 23:19:53]

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

[commit-bot: I haz the power, 2015-05-11 05:59:14]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-05-11 06:00:06]

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