mash: wires up shadows for mash

mash: wires up shadows for mash

Specifically Widget::InitParams::shadow_type/shadow_elevation weren't
plumbed through for DesktopWindowTreeHostMus. This meant the types
weren't applied at to the aura::Window. They also weren't routed
through the PropertyConverter.

ShadowController is an EnvObserver. ShadowController runs both in mus
and clients. Previously ShadowController keyed off new aura::Window
creation to set the ShadowElevation property. This is problematic for
the mus case as we want mus to make the decisions about shadows
(unless a client has a specific requirement). To get this to work with
mus I added a new shadow elevation type, DEFAULT, which means the
shadow should be chosen based on the window type. ShadowController no
longer sets the shadow type, instead it keys of the existing type and
maps DEFAULT accordingly. This way mash sees DEFAULT and creates the
right shadow.

BUG=690546
670840
TEST=none
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2694213003
Cr-Commit-Position: refs/heads/master@{#450775}
Committed: https://chromium.googlesource.com/chromium/src/+/5c71eb349c1b9f3e0624ad6f27ba0b09c666337f

[sky, 2017-02-15 00:07:50]

Description was changed from

==========
mash: wires up shadows for mash

Specifically Widget::InitParams::shadow_type/shadow_elevation weren't
plumbed through for DesktopWindowTreeHostMus. This meant the types
weren't applied at to the aura::Window. They also weren't routed
through the PropertyConverter.

ShadowController is an EnvObserver. ShadowController runs both in mus
and clients. Previously ShadowController keyed off new aura::Window
creation to set the ShadowElevation property. This is problematic for
the mus case as we want mus to make the decisions about shadows
(unless a client has a specific requirement). To get this to work with
mus I added a new shadow elevation type, DEFAULT, which means the
shadow should be chosen based on the window type. ShadowController no
longer sets the shadow type, instead it keys of the existing type and
maps DEFAULT accordingly. This way mash sees DEFAULT and creates the
right shadow.

BUG=690546 670840
TEST=none
R=msw@chromium.org
==========

to

==========
mash: wires up shadows for mash

Specifically Widget::InitParams::shadow_type/shadow_elevation weren't
plumbed through for DesktopWindowTreeHostMus. This meant the types
weren't applied at to the aura::Window. They also weren't routed
through the PropertyConverter.

ShadowController is an EnvObserver. ShadowController runs both in mus
and clients. Previously ShadowController keyed off new aura::Window
creation to set the ShadowElevation property. This is problematic for
the mus case as we want mus to make the decisions about shadows
(unless a client has a specific requirement). To get this to work with
mus I added a new shadow elevation type, DEFAULT, which means the
shadow should be chosen based on the window type. ShadowController no
longer sets the shadow type, instead it keys of the existing type and
maps DEFAULT accordingly. This way mash sees DEFAULT and creates the
right shadow.

BUG=690546 670840
TEST=none
R=msw@chromium.org
==========

[sky, 2017-02-15 00:07:50]

sky@chromium.org changed reviewers:
+ reveman@chromium.org, tsepez@chromium.org

[sky, 2017-02-15 00:08:37]

reveman: exo (I move GetShadowElevation to the exo test as that is the only
place using the function).
tsepez: mojom
msw: the rest

[sky, 2017-02-15 00:08:44]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 00:09:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2017-02-15 00:34:40]

lgtm

[msw, 2017-02-15 01:37:29]

+CC estade (he recently added the shadow elevation code)

https://codereview.chromium.org/2694213003/diff/20001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:323: return
property_converter_->IsTransportNameRegistered(name);
aside: I wonder if we should log when properties are prevented from changing.

https://codereview.chromium.org/2694213003/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:491: LOG(ERROR) << "focus="
optional nit: "Focused window: "? (to match format above)

https://codereview.chromium.org/2694213003/diff/20001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:158:
mojo::ConvertTo<TransportType>(static_cast<int32_t>(init_params.type));
Hmm, these init params don't go through the property converter, iirc, but for
consistency, should we replace int32_t here with PrimitiveType (int64_t)?
(similar q for bools below: kFocusable, kAlwaysOnTop, kRemoveStandardFrame)

https://codereview.chromium.org/2694213003/diff/20001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:177: if (init_params.remove_standard_frame) {
q: always set this property, like we do for kFocusable and kAlwaysOnTop?

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host.h (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host.h:172: virtual bool
ShouldCreateVisibiltyController() const = 0;
nit: VisibilityController (missing an 'i')

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:486: return false;
I guess the wm shadow (and other VisibilityController functionality) is
cros-specific and so this is okay? Were we previously creating visibility
controllers for DesktopNativeWidgetAura on Windows? (perhaps they don't use
TRANSLUCENT_WINDOW?) It'd be nice if VisibilityController had a class comment

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1192: return false;
ditto q: was it already cros-only (not used on linux desktop)?

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.h (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.h:60: // If necessary sets the
ShadowElevation of |window| from |params|.
optional nit: comma after necessary

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.h:61: static void
SetShadowElevationFromInitParams(
Could this just be part of InitNativeWidget? (why isn't that used for
DesktopWindowTreeHostMus::Init? Are we missing any other aura window
initialization steps in that function?)

https://codereview.chromium.org/2694213003/diff/20001/ui/wm/core/shadow_contr...
File ui/wm/core/shadow_controller.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/wm/core/shadow_contr...
ui/wm/core/shadow_controller.cc:57: ShadowElevation
GetShadowElevationUseDefault(aura::Window* window) {
optional nit: replace 'Use' with 'Convert', 'Lookup', or similar?

[commit-bot: I haz the power, 2017-02-15 02:16:24]

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

[commit-bot: I haz the power, 2017-02-15 02:16:25]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-15 03:58:25]

https://codereview.chromium.org/2694213003/diff/20001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:323: return
property_converter_->IsTransportNameRegistered(name);
On 2017/02/15 01:37:29, msw wrote:
> aside: I wonder if we should log when properties are prevented from changing.

Good idea. Done.

https://codereview.chromium.org/2694213003/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:491: LOG(ERROR) << "focus="
On 2017/02/15 01:37:29, msw wrote:
> optional nit: "Focused window: "? (to match format above)

Done.

https://codereview.chromium.org/2694213003/diff/20001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:158:
mojo::ConvertTo<TransportType>(static_cast<int32_t>(init_params.type));
On 2017/02/15 01:37:29, msw wrote:
> Hmm, these init params don't go through the property converter, iirc, but for
> consistency, should we replace int32_t here with PrimitiveType (int64_t)?
> (similar q for bools below: kFocusable, kAlwaysOnTop, kRemoveStandardFrame)

I didn't see a pressing need to as these values don't make it into window
properties.

https://codereview.chromium.org/2694213003/diff/20001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:177: if (init_params.remove_standard_frame) {
On 2017/02/15 01:37:29, msw wrote:
> q: always set this property, like we do for kFocusable and kAlwaysOnTop?

Done.

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host.h (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host.h:172: virtual bool
ShouldCreateVisibiltyController() const = 0;
On 2017/02/15 01:37:29, msw wrote:
> nit: VisibilityController (missing an 'i')

Done.

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:486: return false;
On 2017/02/15 01:37:29, msw wrote:
> I guess the wm shadow (and other VisibilityController functionality) is
> cros-specific and so this is okay? Were we previously creating visibility
> controllers for DesktopNativeWidgetAura on Windows? (perhaps they don't use
> TRANSLUCENT_WINDOW?) It'd be nice if VisibilityController had a class comment

This is wrong, it should be true.

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1192: return false;
On 2017/02/15 01:37:29, msw wrote:
> ditto q: was it already cros-only (not used on linux desktop)?

Same comment, should be true.

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.h (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.h:60: // If necessary sets the
ShadowElevation of |window| from |params|.
On 2017/02/15 01:37:29, msw wrote:
> optional nit: comma after necessary

Done.

https://codereview.chromium.org/2694213003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.h:61: static void
SetShadowElevationFromInitParams(
On 2017/02/15 01:37:29, msw wrote:
> Could this just be part of InitNativeWidget? (why isn't that used for
> DesktopWindowTreeHostMus::Init? Are we missing any other aura window
> initialization steps in that function?)

It's not necessarily for NativeWidgetMac. I think we have most other parts of
NativeWidgetAura::InitNativeWidget, but I'll take a closer look separately.

https://codereview.chromium.org/2694213003/diff/20001/ui/wm/core/shadow_contr...
File ui/wm/core/shadow_controller.cc (right):

https://codereview.chromium.org/2694213003/diff/20001/ui/wm/core/shadow_contr...
ui/wm/core/shadow_controller.cc:57: ShadowElevation
GetShadowElevationUseDefault(aura::Window* window) {
On 2017/02/15 01:37:29, msw wrote:
> optional nit: replace 'Use' with 'Convert', 'Lookup', or similar?

Done.

[sky, 2017-02-15 03:58:28]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 03:59:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 05:26:30]

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

[commit-bot: I haz the power, 2017-02-15 05:26:31]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-15 16:29:08]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 16:29:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 17:57:02]

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

[commit-bot: I haz the power, 2017-02-15 17:57:04]

Dry run: This issue passed the CQ dry run.

[msw, 2017-02-15 18:04:30]

lgtm

[reveman, 2017-02-15 18:09:52]

components/exo lgtm

[sky, 2017-02-15 18:18:55]

The CQ bit was checked by sky@chromium.org

[sky, 2017-02-15 18:18:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2694213003/#ps60001
(title: "defaults test")

[commit-bot: I haz the power, 2017-02-15 18:20:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 19:39:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487182735410930,
"parent_rev": "068272b789d612b69453c24f47dd0d9f4708aedf", "commit_rev":
"5c71eb349c1b9f3e0624ad6f27ba0b09c666337f"}

[commit-bot: I haz the power, 2017-02-15 19:43:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487182735410930,
"parent_rev": "068272b789d612b69453c24f47dd0d9f4708aedf", "commit_rev":
"5c71eb349c1b9f3e0624ad6f27ba0b09c666337f"}

[commit-bot: I haz the power, 2017-02-15 19:43:52]

Description was changed from

==========
mash: wires up shadows for mash

Specifically Widget::InitParams::shadow_type/shadow_elevation weren't
plumbed through for DesktopWindowTreeHostMus. This meant the types
weren't applied at to the aura::Window. They also weren't routed
through the PropertyConverter.

ShadowController is an EnvObserver. ShadowController runs both in mus
and clients. Previously ShadowController keyed off new aura::Window
creation to set the ShadowElevation property. This is problematic for
the mus case as we want mus to make the decisions about shadows
(unless a client has a specific requirement). To get this to work with
mus I added a new shadow elevation type, DEFAULT, which means the
shadow should be chosen based on the window type. ShadowController no
longer sets the shadow type, instead it keys of the existing type and
maps DEFAULT accordingly. This way mash sees DEFAULT and creates the
right shadow.

BUG=690546 670840
TEST=none
R=msw@chromium.org
==========

to

==========
mash: wires up shadows for mash

Specifically Widget::InitParams::shadow_type/shadow_elevation weren't
plumbed through for DesktopWindowTreeHostMus. This meant the types
weren't applied at to the aura::Window. They also weren't routed
through the PropertyConverter.

ShadowController is an EnvObserver. ShadowController runs both in mus
and clients. Previously ShadowController keyed off new aura::Window
creation to set the ShadowElevation property. This is problematic for
the mus case as we want mus to make the decisions about shadows
(unless a client has a specific requirement). To get this to work with
mus I added a new shadow elevation type, DEFAULT, which means the
shadow should be chosen based on the window type. ShadowController no
longer sets the shadow type, instead it keys of the existing type and
maps DEFAULT accordingly. This way mash sees DEFAULT and creates the
right shadow.

BUG=690546 670840
TEST=none
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2694213003
Cr-Commit-Position: refs/heads/master@{#450775}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c71eb349c1b9f3e0624ad6f27ba...
==========

[commit-bot: I haz the power, 2017-02-15 19:43:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5c71eb349c1b9f3e0624ad6f27ba...