[MacViews] Implement AlwaysOnTop and VisibleOnAllWorkspaces.

[MacViews] Implement AlwaysOnTop and VisibleOnAllWorkspaces.

This works the same way as NativeAppWindowCocoa. Common code
is factored out to ui/gfx/mac/nswindow_frame_controls.

BUG=459877
Committed: https://crrev.com/37346b4ed633003ea14783395a491688ba8f8731
Cr-Commit-Position: refs/heads/master@{#329536}

[jackhou1, 2015-04-24 07:04:24]

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

[jackhou1, 2015-04-24 07:04:24]

tapted, what do you think?

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
File ui/gfx/mac/nswindow_frame_controls.mm (right):

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
ui/gfx/mac/nswindow_frame_controls.mm:58: if (!transient) {
The call from NativeWidgetMac::AlwaysOnTop should indicate if the window is
actually a transient window, e.g. a dialog or something. What's the logic for
that? If it's only app windows that can be always-on-top yet non-transient, then
this code should go in another function to be called by
NativeAppWindow[Cocoa|ViewsMac].

[tapted, 2015-05-06 02:00:19]

CL should have a summary like ~do this by factoring out common code from blah.

There should also be a cross-platform test for this in widget_unittest.cc. i.e.
something that just sets alwaysontop and checks that isalwaysontop changes.

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
File ui/gfx/mac/nswindow_frame_controls.h (right):

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
ui/gfx/mac/nswindow_frame_controls.h:8: #import <Cocoa/Cocoa.h>
Can you keep the forward-declare?

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
File ui/gfx/mac/nswindow_frame_controls.mm (right):

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
ui/gfx/mac/nswindow_frame_controls.mm:58: if (!transient) {
On 2015/04/24 07:04:24, jackhou1 (OOO until May 11) wrote:
> The call from NativeWidgetMac::AlwaysOnTop should indicate if the window is
> actually a transient window, e.g. a dialog or something. What's the logic for
> that? If it's only app windows that can be always-on-top yet non-transient,
then
> this code should go in another function to be called by
> NativeAppWindow[Cocoa|ViewsMac].

I would assume transient is always false (i.e. drop the extra argument for now
on SetNSWindowAlwaysOnTop).

I think Panels are always-on-top but still transient, but hopefully they are
going away. I can't think of anything else we would want always-on-top since
it's a crappy UX.

[jackhou1, 2015-05-12 01:42:12]

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
File ui/gfx/mac/nswindow_frame_controls.h (right):

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
ui/gfx/mac/nswindow_frame_controls.h:8: #import <Cocoa/Cocoa.h>
On 2015/05/06 02:00:18, tapted wrote:
> Can you keep the forward-declare? 

Done.

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
File ui/gfx/mac/nswindow_frame_controls.mm (right):

https://codereview.chromium.org/1105613002/diff/40001/ui/gfx/mac/nswindow_fra...
ui/gfx/mac/nswindow_frame_controls.mm:58: if (!transient) {
On 2015/05/06 02:00:19, tapted wrote:
> On 2015/04/24 07:04:24, jackhou1 (OOO until May 11) wrote:
> > The call from NativeWidgetMac::AlwaysOnTop should indicate if the window is
> > actually a transient window, e.g. a dialog or something. What's the logic
for
> > that? If it's only app windows that can be always-on-top yet non-transient,
> then
> > this code should go in another function to be called by
> > NativeAppWindow[Cocoa|ViewsMac].
> 
> I would assume transient is always false (i.e. drop the extra argument for now
> on SetNSWindowAlwaysOnTop).
> 
> I think Panels are always-on-top but still transient, but hopefully they are
> going away. I can't think of anything else we would want always-on-top since
> it's a crappy UX.

Done.

[tapted, 2015-05-12 04:05:35]

lgtm with a couple of tweaks

https://codereview.chromium.org/1105613002/diff/80001/ui/gfx/mac/nswindow_fra...
File ui/gfx/mac/nswindow_frame_controls.h (right):

https://codereview.chromium.org/1105613002/diff/80001/ui/gfx/mac/nswindow_fra...
ui/gfx/mac/nswindow_frame_controls.h:23: // Sets whether the window is
always-on-top. If the window is |transient| it
nit: update comment (wrt transient).

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

https://codereview.chromium.org/1105613002/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:87: SetAlwaysOnTop(params.keep_on_top);
I think... only call this when params.keep_on_top is true. But comment. i.e.

// Only set the always on top property during Init if it is true, since setting
// it can affect how the window is treated by Exposé.


Other options could be
 - only change collection behavior in SetAlwaysOnTop if it is being set `true`
  ( - but then should we try to restore when going to false?)
 - only change collection behavior in SetAlwaysOnTop if (collectionBehavior & (
NSWindowCollectionBehaviorManaged | NSWindowCollectionBehaviorTransient |
NSWindowCollectionBehaviorStationary)) is zero
  ( - i.e. nothing has tried to set it already, but then... what if something
later wants to set it?)


So I think if a window never opt in to always-on-top, we should just leave the
collection behavior alone.

[jackhou1, 2015-05-12 05:44:19]

https://codereview.chromium.org/1105613002/diff/80001/ui/gfx/mac/nswindow_fra...
File ui/gfx/mac/nswindow_frame_controls.h (right):

https://codereview.chromium.org/1105613002/diff/80001/ui/gfx/mac/nswindow_fra...
ui/gfx/mac/nswindow_frame_controls.h:23: // Sets whether the window is
always-on-top. If the window is |transient| it
On 2015/05/12 04:05:35, tapted wrote:
> nit: update comment (wrt transient).

Done.

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

https://codereview.chromium.org/1105613002/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:87: SetAlwaysOnTop(params.keep_on_top);
On 2015/05/12 04:05:35, tapted wrote:
> I think... only call this when params.keep_on_top is true. But comment. i.e.
> 
> // Only set the always on top property during Init if it is true, since
setting
> // it can affect how the window is treated by Exposé.
> 
> 
> Other options could be
>  - only change collection behavior in SetAlwaysOnTop if it is being set `true`
>   ( - but then should we try to restore when going to false?)
>  - only change collection behavior in SetAlwaysOnTop if (collectionBehavior &
(
> NSWindowCollectionBehaviorManaged | NSWindowCollectionBehaviorTransient |
> NSWindowCollectionBehaviorStationary)) is zero
>   ( - i.e. nothing has tried to set it already, but then... what if something
> later wants to set it?)
> 
> 
> So I think if a window never opt in to always-on-top, we should just leave the
> collection behavior alone.

Done.

[jackhou1, 2015-05-12 05:45:50]

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

[jackhou1, 2015-05-12 05:45:51]

thakis, please review for OWNERS

[Nico, 2015-05-12 16:05:37]

lgtm

[Nico, 2015-05-12 16:07:14]

(tapted, where were you missing OWNERS? (tapted, where were you missing OWNERS?
Just ui/gfx/mac? I feel that you should be able to OWNERS-approve patches like
this, so if you want you could be in ui/gfx/mac/OWNERS)

[jackhou1, 2015-05-13 00:07:10]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-05-13 00:07:10]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1105613002/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-05-13 00:07:39]

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

[commit-bot: I haz the power, 2015-05-13 00:12:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-13 00:12:54]

Patchset 6 (id:??) landed as
https://crrev.com/37346b4ed633003ea14783395a491688ba8f8731
Cr-Commit-Position: refs/heads/master@{#329536}