[ash-md] Animates a window closed while in overview mode

[ash-md] Animates a window closed while in overview mode

When a window is closed while in overview mode it is first scaled down
0.96x while fading out to 0.8 opacity. Then it is faded out completely
and the window closed which triggers overview re-layout.

BUG=622533
Committed: https://crrev.com/cab2a4a473efbf392a980d9caba64f70477c1426
Cr-Commit-Position: refs/heads/master@{#402797}

[varkha, 2016-06-28 01:04:33]

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

[commit-bot: I haz the power, 2016-06-28 01:05:37]

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

[varkha, 2016-06-28 01:18:51]

Patchset #1 (id:1) has been deleted

[varkha, 2016-06-28 01:18:58]

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

[commit-bot: I haz the power, 2016-06-28 01:19:23]

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

[varkha, 2016-06-28 01:21:00]

varkha@chromium.org changed reviewers:
+ bruthig@chromium.org, tdanderson@chromium.org

[varkha, 2016-06-28 01:21:00]

Can you please take a look? This should make the transition smoother for the
window that is closed in overview.
Thanks!

[commit-bot: I haz the power, 2016-06-28 01:57:47]

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

[commit-bot: I haz the power, 2016-06-28 01:57:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[varkha, 2016-06-28 03:46:11]

Patchset #1 (id:20001) has been deleted

[varkha, 2016-06-28 03:47:34]

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

[commit-bot: I haz the power, 2016-06-28 03:47:57]

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-06-28 04:37:46]

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

[commit-bot: I haz the power, 2016-06-28 04:37:47]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-06-28 15:28:02]

lgtm with nits

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/common/wm/ov...
File ash/common/wm/overview/window_selector_item.cc (right):

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/common/wm/ov...
ash/common/wm/overview/window_selector_item.cc:97: // Before closing window
animate the tile to shrink by this fraction of size.
Maybe specify what is meant by 'tile' (here and line 405)

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/common/wm/ov...
ash/common/wm/overview/window_selector_item.cc:404: // First animate opacity
first to intermediate value at the same time as
nit: remove one first

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/common/wm/ov...
ash/common/wm/overview/window_selector_item.cc:408: // Fade out the tile
effectively hiding it.
nit: tile,

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/common/wm/ov...
ash/common/wm/overview/window_selector_item.cc:613: OverviewAnimationType
animation_type) {
consider a DCHECK or similar to verify |opacity| is a legal opacity value?

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/common/wm/ov...
File ash/common/wm/overview/window_selector_item.h (right):

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/common/wm/ov...
ash/common/wm/overview/window_selector_item.h:143: // Animates opacity of the
transformed window and its caption to |opacity|
nit: "of |transform_window_| rather than 'of the transformed window'"

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/wm/overview/...
File ash/wm/overview/scoped_overview_animation_settings_aura.cc (right):

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/wm/overview/...
ash/wm/overview/scoped_overview_animation_settings_aura.cc:19: // The time
duration for layout update. Only different with Material Design;
nit: ;

Also reading this I find "only different with material design" confusing. Where
do I see the non-MD value?

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/wm/overview/...
ash/wm/overview/scoped_overview_animation_settings_aura.cc:20: const int
kCloseTransitionMillisecondsMD = 200;
nit: it was suggested to me in a separate CL to use Md rather than MD as a
suffix to conform to chrome naming conventions. Though you're consistently using
MD throughout overview code so not a big deal leaving it as-is.

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/wm/overview/...
File ash/wm/overview/window_selector_unittest.cc (right):

https://chromiumcodereview.appspot.com/2101843003/diff/40001/ash/wm/overview/...
ash/wm/overview/window_selector_unittest.cc:126:
ScopedTransformOverviewWindow::SetImmediateCloseForTests();
I will defer to Ben for any comments related to testing in this CL.

[varkha, 2016-06-28 23:50:37]

https://codereview.chromium.org/2101843003/diff/40001/ash/common/wm/overview/...
File ash/common/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2101843003/diff/40001/ash/common/wm/overview/...
ash/common/wm/overview/window_selector_item.cc:97: // Before closing window
animate the tile to shrink by this fraction of size.
On 2016/06/28 15:28:02, tdanderson wrote:
> Maybe specify what is meant by 'tile' (here and line 405)

Done.

https://codereview.chromium.org/2101843003/diff/40001/ash/common/wm/overview/...
ash/common/wm/overview/window_selector_item.cc:404: // First animate opacity
first to intermediate value at the same time as
On 2016/06/28 15:28:02, tdanderson wrote:
> nit: remove one first

Done.

https://codereview.chromium.org/2101843003/diff/40001/ash/common/wm/overview/...
ash/common/wm/overview/window_selector_item.cc:408: // Fade out the tile
effectively hiding it.
On 2016/06/28 15:28:02, tdanderson wrote:
> nit: tile,

Done.

https://codereview.chromium.org/2101843003/diff/40001/ash/common/wm/overview/...
ash/common/wm/overview/window_selector_item.cc:613: OverviewAnimationType
animation_type) {
On 2016/06/28 15:28:02, tdanderson wrote:
> consider a DCHECK or similar to verify |opacity| is a legal opacity value?

Done.

https://codereview.chromium.org/2101843003/diff/40001/ash/common/wm/overview/...
File ash/common/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/2101843003/diff/40001/ash/common/wm/overview/...
ash/common/wm/overview/window_selector_item.h:143: // Animates opacity of the
transformed window and its caption to |opacity|
On 2016/06/28 15:28:02, tdanderson wrote:
> nit: "of |transform_window_| rather than 'of the transformed window'"

Done.

https://codereview.chromium.org/2101843003/diff/40001/ash/wm/overview/scoped_...
File ash/wm/overview/scoped_overview_animation_settings_aura.cc (right):

https://codereview.chromium.org/2101843003/diff/40001/ash/wm/overview/scoped_...
ash/wm/overview/scoped_overview_animation_settings_aura.cc:19: // The time
duration for layout update. Only different with Material Design;
On 2016/06/28 15:28:02, tdanderson wrote:
> nit: ;
> 
> Also reading this I find "only different with material design" confusing.
Where
> do I see the non-MD value?

Done.

https://codereview.chromium.org/2101843003/diff/40001/ash/wm/overview/scoped_...
ash/wm/overview/scoped_overview_animation_settings_aura.cc:20: const int
kCloseTransitionMillisecondsMD = 200;
On 2016/06/28 15:28:02, tdanderson wrote:
> nit: it was suggested to me in a separate CL to use Md rather than MD as a
> suffix to conform to chrome naming conventions. Though you're consistently
using
> MD throughout overview code so not a big deal leaving it as-is.

Done.

https://codereview.chromium.org/2101843003/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/2101843003/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:126:
ScopedTransformOverviewWindow::SetImmediateCloseForTests();
On 2016/06/28 15:28:02, tdanderson wrote:
> I will defer to Ben for any comments related to testing in this CL.

Acknowledged.

[varkha, 2016-06-28 23:50:58]

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

[commit-bot: I haz the power, 2016-06-28 23:52:08]

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

[varkha, 2016-06-28 23:57:15]

Patchset #2 (id:60001) has been deleted

[varkha, 2016-06-28 23:57:37]

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

[varkha, 2016-06-28 23:58:58]

Description was changed from

==========
[ash-md] Animates a window closed while in overview mode

When a window is closed while in overview mode it is first scaled down
0.96x while fading out to 0.5 opacity. Then it is faded out completely
and the window closed which triggers overview re-layout.

BUG=622533
==========

to

==========
[ash-md] Animates a window closed while in overview mode

When a window is closed while in overview mode it is first scaled down
0.96x while fading out to 0.8 opacity. Then it is faded out completely
and the window closed which triggers overview re-layout.

BUG=622533
==========

[commit-bot: I haz the power, 2016-06-28 23:59:15]

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-06-29 01:34:45]

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

[commit-bot: I haz the power, 2016-06-29 01:34:46]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-06-29 13:56:49]

I will land this to get feedback on visuals.

[varkha, 2016-06-29 13:57:03]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-06-29 13:57:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2101843003/#ps80001
(title: "[ash-md] Animates a window closed while in overview mode (comments)")

[commit-bot: I haz the power, 2016-06-29 13:57:31]

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-06-29 14:02:58]

Description was changed from

==========
[ash-md] Animates a window closed while in overview mode

When a window is closed while in overview mode it is first scaled down
0.96x while fading out to 0.8 opacity. Then it is faded out completely
and the window closed which triggers overview re-layout.

BUG=622533
==========

to

==========
[ash-md] Animates a window closed while in overview mode

When a window is closed while in overview mode it is first scaled down
0.96x while fading out to 0.8 opacity. Then it is faded out completely
and the window closed which triggers overview re-layout.

BUG=622533
==========

[commit-bot: I haz the power, 2016-06-29 14:02:59]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-06-29 14:03:02]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 14:04:44]

Description was changed from

==========
[ash-md] Animates a window closed while in overview mode

When a window is closed while in overview mode it is first scaled down
0.96x while fading out to 0.8 opacity. Then it is faded out completely
and the window closed which triggers overview re-layout.

BUG=622533
==========

to

==========
[ash-md] Animates a window closed while in overview mode

When a window is closed while in overview mode it is first scaled down
0.96x while fading out to 0.8 opacity. Then it is faded out completely
and the window closed which triggers overview re-layout.

BUG=622533

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

[commit-bot: I haz the power, 2016-06-29 14:04:45]

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

[bruthig, 2016-06-30 15:33:08]

https://codereview.chromium.org/2101843003/diff/80001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/2101843003/diff/80001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:126:
ScopedTransformOverviewWindow::SetImmediateCloseForTests();
It would be better if SetImmediateCloseForTests() accepted a bool and was reset
during TearDown.