Replace WM shadow types (on/off) and styles (small/inactive/active)

Replace WM shadow types (on/off) and styles (small/inactive/active)
with elevation values instead.

This is necessary to allow different windows of the same UI or WM window
type to specify different shadow appearances. This also makes more
sense in that active windows aren't the only ones to get big shadows.

Concretely, this is a requirement for updating Toast shadows.

BUG=608852, 676223
TBR=reveman@chromium.org

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

[Evan Stade, 2016-12-21 04:54:24]

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

[commit-bot: I haz the power, 2016-12-21 04:54:37]

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, 2016-12-21 05:09:38]

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

[commit-bot: I haz the power, 2016-12-21 05:09:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-12-21 16:57:56]

estade@chromium.org changed reviewers:
+ jamescook@chromium.org, sky@chromium.org

[Evan Stade, 2016-12-21 16:57:57]

+sky, jamescook (I know you're both OOO, but so is everyone it seems. No rush.)

This patch will break some unit tests but before I fix them, wdyt? The good
parts of the patch are outlined in the CL desc (I like that we stop using
"ACTIVE" shadows for windows that are not necessarily active in the WM sense).
The only part I feel slightly guilty about is adding a param to
Widget::InitParams which is only used on aura, however there seems to be
precedent for platform-specific init params.

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

https://codereview.chromium.org/2596743002/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:237: int shadow_elevation;
Do you think the type for this param should be ui::ShadowElevation?

[sky, 2017-01-03 22:35:38]

https://codereview.chromium.org/2596743002/diff/20001/ash/common/wm/overview/...
File ash/common/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2596743002/diff/20001/ash/common/wm/overview/...
ash/common/wm/overview/window_grid.cc:679:
selector_shadow_->Init(::wm::ShadowElevation::BIG);
Inside ShadowController you define kActiveNormalShadowElevation, should this
code use kActiveNormalShadowElevation rather than explicitly BIG?

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

https://codereview.chromium.org/2596743002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:147: SetShadowElevation(window_,
wm::ShadowElevation::NONE);
Isn't this the default?

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

https://codereview.chromium.org/2596743002/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:237: int shadow_elevation;
On 2016/12/21 16:57:57, Evan Stade wrote:
> Do you think the type for this param should be ui::ShadowElevation?

Yes. Additionally do we need both shadow_type and shadow_elevation?

https://codereview.chromium.org/2596743002/diff/20001/ui/wm/core/shadow_types.h
File ui/wm/core/shadow_types.h (right):

https://codereview.chromium.org/2596743002/diff/20001/ui/wm/core/shadow_types...
ui/wm/core/shadow_types.h:21: NONE = 0,
It may be obvious to you, but I would document the value of the enum is used in
the shadow itself.

https://codereview.chromium.org/2596743002/diff/20001/ui/wm/core/shadow_types...
ui/wm/core/shadow_types.h:24: BIG = 24,
optional: I would have went with LARGE (mostly because menus are small, medium
and large), but if you prefer BIG, stick with it.

[Evan Stade, 2017-01-04 00:20:45]

https://codereview.chromium.org/2596743002/diff/20001/ash/common/wm/overview/...
File ash/common/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2596743002/diff/20001/ash/common/wm/overview/...
ash/common/wm/overview/window_grid.cc:679:
selector_shadow_->Init(::wm::ShadowElevation::BIG);
On 2017/01/03 22:35:38, sky (OOO) wrote:
> Inside ShadowController you define kActiveNormalShadowElevation, should this
> code use kActiveNormalShadowElevation rather than explicitly BIG?

IMO it's better that it doesn't, because the selector is not the same thing as
the "active" window. (If it were, you wouldn't have this code here.) So now if
we decide to change the size of large shadows across the board, we just change
the enum value to something else, but if we just want to adjust how big shadows
are for the active window, we can do that without affecting unrelated shadows.

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

https://codereview.chromium.org/2596743002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:147: SetShadowElevation(window_,
wm::ShadowElevation::NONE);
On 2017/01/03 22:35:38, sky wrote:
> Isn't this the default?

The default depends on the platform and the type of window.

https://cs.chromium.org/chromium/src/ui/views/widget/widget.h?rcl=0&l=199

It's true that some windows will default to no shadow. Passing NONE allows
callers to remove shadows for windows that /do/ have a shadow by default.

See also changes to shadow_controller.cc where I try to make this more obvious
via more explicit function names.

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

https://codereview.chromium.org/2596743002/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:237: int shadow_elevation;
On 2017/01/03 22:35:38, sky wrote:
> On 2016/12/21 16:57:57, Evan Stade wrote:
> > Do you think the type for this param should be ui::ShadowElevation?
> 
> Yes.

Done.

> Additionally do we need both shadow_type and shadow_elevation?

I believe we do because of the way ShadowType is used in MenuHost::InitMenuHost.
We want to be able to control whether there is a shadow (ignoring platform
default), but to leave the elevation as the default.

https://codereview.chromium.org/2596743002/diff/20001/ui/wm/core/shadow_types.h
File ui/wm/core/shadow_types.h (right):

https://codereview.chromium.org/2596743002/diff/20001/ui/wm/core/shadow_types...
ui/wm/core/shadow_types.h:21: NONE = 0,
On 2017/01/03 22:35:38, sky wrote:
> It may be obvious to you, but I would document the value of the enum is used
in
> the shadow itself.

Done.

https://codereview.chromium.org/2596743002/diff/20001/ui/wm/core/shadow_types...
ui/wm/core/shadow_types.h:24: BIG = 24,
On 2017/01/03 22:35:38, sky wrote:
> optional: I would have went with LARGE (mostly because menus are small, medium
> and large), but if you prefer BIG, stick with it.

I have no preference, done.

[Evan Stade, 2017-01-04 00:20:59]

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

[commit-bot: I haz the power, 2017-01-04 00:21:38]

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

[sky, 2017-01-04 00:32:20]

LGTM

[commit-bot: I haz the power, 2017-01-04 00:56:47]

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

[commit-bot: I haz the power, 2017-01-04 00:56:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2017-01-04 16:51:45]

Code LGTM but it looks like you need to update some unit tests.

[Evan Stade, 2017-01-04 17:21:14]

Description was changed from

==========
Replace WM shadow types (on/off) and styles (small/inactive/active)
with elevation values instead.

This is necessary to allow different windows of the same UI or WM window
type to specify different shadow appearances. This also makes more
sense in that active windows aren't the only ones to get big shadows.

Concretely, this is a requirement for updating Toast shadows.

BUG=608852,676223
==========

to

==========
Replace WM shadow types (on/off) and styles (small/inactive/active)
with elevation values instead.

This is necessary to allow different windows of the same UI or WM window
type to specify different shadow appearances. This also makes more
sense in that active windows aren't the only ones to get big shadows.

Concretely, this is a requirement for updating Toast shadows.

BUG=608852,676223
TBR=reveman@chromium.org
==========

[Evan Stade, 2017-01-04 17:21:14]

estade@chromium.org changed reviewers:
+ reveman@chromium.org

[Evan Stade, 2017-01-04 17:21:35]

thanks!

tbr reveman for mechanical changes in components/exo/shell_surface.cc

[Evan Stade, 2017-01-04 17:21:40]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-01-04 17:21:41]

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

[commit-bot: I haz the power, 2017-01-04 17:22:00]

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

[Evan Stade, 2017-01-04 17:54:34]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-01-04 17:54:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2596743002/#ps80001
(title: "rebase and fix more tests")

[commit-bot: I haz the power, 2017-01-04 17:54:44]

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-01-04 18:27:33]

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

[commit-bot: I haz the power, 2017-01-04 18:27:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2017-01-04 18:45:16]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-01-04 18:45:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2596743002/#ps100001
(title: "one more mechanical change")

[commit-bot: I haz the power, 2017-01-04 18:45:49]

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

[Evan Stade, 2017-01-04 19:57:39]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-01-04 19:57:44]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-01-04 19:58:02]

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-01-04 20:06:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483559864911930,
"parent_rev": "e34e8cd26cf4c9bd47bf69d3c2d6a98e80e8e29d", "commit_rev":
"560a290e84695e38b7ba64388c48cb459fd02350"}

[commit-bot: I haz the power, 2017-01-04 20:06:39]

Description was changed from

==========
Replace WM shadow types (on/off) and styles (small/inactive/active)
with elevation values instead.

This is necessary to allow different windows of the same UI or WM window
type to specify different shadow appearances. This also makes more
sense in that active windows aren't the only ones to get big shadows.

Concretely, this is a requirement for updating Toast shadows.

BUG=608852,676223
TBR=reveman@chromium.org
==========

to

==========
Replace WM shadow types (on/off) and styles (small/inactive/active)
with elevation values instead.

This is necessary to allow different windows of the same UI or WM window
type to specify different shadow appearances. This also makes more
sense in that active windows aren't the only ones to get big shadows.

Concretely, this is a requirement for updating Toast shadows.

BUG=608852,676223
TBR=reveman@chromium.org

Review-Url: https://codereview.chromium.org/2596743002
==========

[commit-bot: I haz the power, 2017-01-04 20:06:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2017-01-04 20:09:40]

Description was changed from

==========
Replace WM shadow types (on/off) and styles (small/inactive/active)
with elevation values instead.

This is necessary to allow different windows of the same UI or WM window
type to specify different shadow appearances. This also makes more
sense in that active windows aren't the only ones to get big shadows.

Concretely, this is a requirement for updating Toast shadows.

BUG=608852,676223
TBR=reveman@chromium.org

Review-Url: https://codereview.chromium.org/2596743002
==========

to

==========
Replace WM shadow types (on/off) and styles (small/inactive/active)
with elevation values instead.

This is necessary to allow different windows of the same UI or WM window
type to specify different shadow appearances. This also makes more
sense in that active windows aren't the only ones to get big shadows.

Concretely, this is a requirement for updating Toast shadows.

BUG=608852,676223
TBR=reveman@chromium.org

Committed: https://crrev.com/ba7b9d74bdd0fa555c7997c459ddd0d6c05cc78a
Cr-Commit-Position: refs/heads/master@{#441441}
==========

[commit-bot: I haz the power, 2017-01-04 20:09:41]

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

[reveman, 2017-01-05 03:21:40]

lgtm