Use 0.2f opacity only when the shadow animation is applied.

Use 0.2f opacity only when the shadow animation is applied.

The opacity stays 0.2f if the window is created and shown in inactive state because the opacity is set back to 1.0f only when the animation is applied. This CL uses 1.0f opacity as initial opacity and when transitioning from/to small shadow.

BUG=433574
TEST=manual test. See the bug for repro step.

Committed: https://crrev.com/2f994844a6cdf6977e8960becb8dd5e94398a973
Cr-Commit-Position: refs/heads/master@{#308187}

[oshima, 2014-12-11 18:57:04]

oshima@chromium.org changed reviewers:
+ jamescook@chromium.org

[oshima, 2014-12-11 18:57:05]

James, you seems to be the author of the original code. Could you please take a
look?

[James Cook, 2014-12-11 19:53:17]

Are you sure this is right?  Do the underlying art assets have transparency?

I seem to remember we used fully-opaque assets, then intentionally made the
inactive window shadows more transparent.

Is this actually a bug?  Inactive windows should not have much of a shadow.

[oshima, 2014-12-11 20:38:22]

On 2014/12/11 19:53:17, James Cook wrote:
> Are you sure this is right?  Do the underlying art assets have transparency?

Here is my interpretation of how this is expected work.

1) when switching to / from small shadow, no animation, so we can just set to
1.0f.

2) when switching from inactive to active, we switch the active asssets
immedicately, but animate the opacity from 0.2 to 1.0f.

3) when switching from active to inactive, we animate the opacity from 1.0f to
0.2f first, then switch to the inative assets and set opacity back to 1.0f.
(See OnImplicitAnimationsCompleted)

> I seem to remember we used fully-opaque assets, then intentionally made the
> inactive window shadows more transparent.
> Is this actually a bug?  Inactive windows should not have much of a shadow.

Assets have gradient alpha channel, and IDR_AURA_SHADOW_INACTIVE has less shadow
(less opaque).

[James Cook, 2014-12-11 22:34:04]

LGTM with nit

Yep, that's how it's supposed to work. I remember now.

https://codereview.chromium.org/798673002/diff/1/ui/wm/core/shadow.cc
File ui/wm/core/shadow.cc (right):

https://codereview.chromium.org/798673002/diff/1/ui/wm/core/shadow.cc#newcode16
ui/wm/core/shadow.cc:16: const float kActiveShadowOpacity = 1.0f;
I would just delete this constant and inline the use of 1.0f. I think the
existence of this variable (and its name) confused me because it implies that in
the steady-state the assets are drawn with non-1.0 opacity.

[oshima, 2014-12-12 21:22:15]

oshima@chromium.org changed reviewers:
+ sadrul@chromium.org

[oshima, 2014-12-12 21:22:16]

sadrul -> owners

[oshima, 2014-12-12 21:23:45]

https://codereview.chromium.org/798673002/diff/1/ui/wm/core/shadow.cc
File ui/wm/core/shadow.cc (right):

https://codereview.chromium.org/798673002/diff/1/ui/wm/core/shadow.cc#newcode16
ui/wm/core/shadow.cc:16: const float kActiveShadowOpacity = 1.0f;
On 2014/12/11 22:34:04, James Cook wrote:
> I would just delete this constant and inline the use of 1.0f. I think the
> existence of this variable (and its name) confused me because it implies that
in
> the steady-state the assets are drawn with non-1.0 opacity.

Done.

https://codereview.chromium.org/798673002/diff/1/ui/wm/core/shadow.cc#newcode79
ui/wm/core/shadow.cc:79: shadow_layer_->SetOpacity(1.0f);
I removed this because 1.0f is default.

[sadrul, 2014-12-12 21:26:54]

lgtm

[oshima, 2014-12-12 21:33:08]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-12-12 21:34:01]

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

[commit-bot: I haz the power, 2014-12-12 22:52:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-12 22:53:35]

Patchset 3 (id:??) landed as
https://crrev.com/2f994844a6cdf6977e8960becb8dd5e94398a973
Cr-Commit-Position: refs/heads/master@{#308187}