[ash-md] Fades overview header in and out

[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and animates its bounds and opacity such that it appears to take over the real header. Only once the "fake" header is opaque a mask or alpha shape is applied to the window to hide its original header after which the "fake" header becomes translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is restored for the overview mode from the minimized state and is thus animated from the shelf item.

BUG=624608, 645076
TEST=Most changes are only really visible under a great slow-down but watching closely the files app header transform into overview mode should be much less abrupt.

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

[varkha, 2016-08-12 21:12:27]

varkha@chromium.org changed reviewers:
+ sky@chromium.org

[varkha, 2016-08-12 21:12:28]

sky@, this is still preliminary WIP but I wanted to see if what I am doing in
WmWindow to get the un-minimizing animation work properly (I need to grow the
"fake" window header) from the window's shelf item's icon rather than from its
restored bounds when a window is hidden before getting into overview mode).
Can you please take a look at ash/aura/*, ash/mus/* and ash/common/wm_window.h
changes? Is there a simpler way that doesn't expand WmWindow?

Thanks!

[sky, 2016-08-12 21:48:36]

Unfortunately what you have is the best we have at this point. Hopefully we'll
get animation in soon, at which point code like this can go away.

[varkha, 2016-08-12 22:02:11]

On 2016/08/12 21:48:36, sky wrote:
> Unfortunately what you have is the best we have at this point. Hopefully we'll
> get animation in soon, at which point code like this can go away.

OK, thanks! I need to do a bit more of cleanup to make sure things are fading in
and out when I want them to. I will then ping you for OWNERS review.

[varkha, 2016-08-24 00:11:49]

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

[commit-bot: I haz the power, 2016-08-24 00:12:25]

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

[varkha, 2016-08-24 00:42:27]

Patchset #4 (id:60001) has been deleted

[varkha, 2016-08-24 00:42:33]

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

[commit-bot: I haz the power, 2016-08-24 00:43:05]

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

[varkha, 2016-08-24 00:43:50]

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

[varkha, 2016-08-24 00:43:50]

bruthig@, can you please take a look in ash/common/wm/overview/ and
ash/wm/overview/ ? Thanks!

[varkha, 2016-08-24 00:47:29]

Description was changed from

==========
[ash-md] Fades overview header in and out

WIP - not for review yet.

BUG=624608
==========

to

==========
[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and
animates its bounds and opacity such that it appears to take over the real
header. Only once the "fake" header is opaque a mask or alpha shape is applied
to the window to hide its original header after which the "fake" header becomes
translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is
restored for the overview mode from the minimized state and is thus animated
from the shelf item.

BUG=624608
==========

[commit-bot: I haz the power, 2016-08-24 01:23:57]

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

[commit-bot: I haz the power, 2016-08-24 01:23:57]

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

[varkha, 2016-08-24 18:36:40]

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

[commit-bot: I haz the power, 2016-08-24 18:37:22]

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-08-24 20:26:57]

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

[commit-bot: I haz the power, 2016-08-24 20:26:58]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-08-30 17:57:33]

https://codereview.chromium.org/2239233002/diff/100001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/100001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:303: // corners. Its
layout needs to be update when setting a transform.
spelling-nit: update->updated

https://codereview.chromium.org/2239233002/diff/100001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:309: // Alpha shape
is only used to to hide the window header and only when
spelling-nit: "to to" -> "to"

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:292: void
OnLayerAnimationAborted(ui::LayerAnimationSequence* sequence) override {}
Would it make sense to perform the same actions in Aborted as in Ended?

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:323: bool use_mask_;
I find it confusing that there is a bool |use_mask_| and an actual |mask_| that
is used even when |use_mask_| is false.  Also |use_mask_| and |use_shape_|
appear to be functionally equivalent?  Perhaps some docs would help or combining
|use_mask_| and |use_shape_| into an enum if they are mutually exclusive. ;) 
Additionally if there are some mutually exclusive states perhaps having distinct
classes makes more sense.

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

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_grid.cc:411:
PrepareForUsingMasksOrShapes(windows_in_root.size());
Correct me if I'm wrong but there is no need for this to be set for each
WindowGrid.  IMO the WindowSelector should be defining whether to use masks or
shapes and the WindowSelectorItem can ask the WindowSelector for this
configuration instead of using statics.

Furthermore, I think we will actually run into an inconsistent state because the
WindowSelectorItem::use_mask_ and use_shape_ are defined as statics.  Consider
multiple WindowGrid instances of which the last one to be constructed only has 1
window.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_grid.cc:1046: windows_to_use_masks =
kMaxWindowsCountToHideHeaderWithMasks;
You could consider adding a warning when the commend line specifies a value but
you don't actually use it.  Same applies below.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_grid.cc:1049: if
(command_line->HasSwitch(switches::kAshMaxWindowsToUseShapeInOverview) &&
Consider wrapping this duplicate logic in a helper function. e.g.

int ReadCmdLineInt(int default, char[] switch);

Invoked like so:

int windows_to_use_masks = ReadCmdLineInt(kMaxWindowsCountToHideHeaderWithMasks,
switches::kAshMaxWindowsToUseMaskInOverview);

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

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:108: const int
kTransitionMilliseconds = 300;
Should this have 'static' just like the rest of the consts?

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:563: params.activatable =
views::Widget::InitParams::Activatable::ACTIVATABLE_NO;
I feel like re-using |params| like this creates more risk than what it's worth.
e.g. If this block overwrites an attribute set in
ConfigureWidgetInitParamsForContainer() but it never gets reset below.  It would
be much safer, and future proof to create a copy of |params| here and modify the
copy as needed.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:729:
ui::ScopedLayerAnimationSettings animation_settings(
For consistency, would it make more sense to add an OverviewAnimationType and
use a ScopedOverviewAnimationSettingsAura object here instead?

Maybe we need to add a
ScopedOverviewAnimationSettings::AddObserver(ImplicitAnimationObserver*
observer) method.

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

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:193: bool first_time_;
nit: A more explicit var name would be very helpful.

https://codereview.chromium.org/2239233002/diff/120001/ash/wm/overview/scoped...
File ash/wm/overview/scoped_overview_animation_settings_aura.cc (right):

https://codereview.chromium.org/2239233002/diff/120001/ash/wm/overview/scoped...
ash/wm/overview/scoped_overview_animation_settings_aura.cc:73: if (material)
nit: Can this be an else block from the block starting on line 65?

[varkha, 2016-09-02 11:12:45]

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

[commit-bot: I haz the power, 2016-09-02 11:12:56]

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

[varkha, 2016-09-02 11:22:51]

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:292: void
OnLayerAnimationAborted(ui::LayerAnimationSequence* sequence) override {}
On 2016/08/30 17:57:33, bruthig wrote:
> Would it make sense to perform the same actions in Aborted as in Ended?

I've reworked this and the observer now lives in window_selector_item.cc. From
my debugging I get Aborted when cancelling overview mode while animating and so
we should not hide the header in this case.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:323: bool use_mask_;
On 2016/08/30 17:57:33, bruthig wrote:
> I find it confusing that there is a bool |use_mask_| and an actual |mask_|
that
> is used even when |use_mask_| is false.  Also |use_mask_| and |use_shape_|
> appear to be functionally equivalent?  Perhaps some docs would help or
combining
> |use_mask_| and |use_shape_| into an enum if they are mutually exclusive. ;) 
> Additionally if there are some mutually exclusive states perhaps having
distinct
> classes makes more sense.

Masks will be removed in the next CL and there will be no need for those flags.

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

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_grid.cc:411:
PrepareForUsingMasksOrShapes(windows_in_root.size());
On 2016/08/30 17:57:33, bruthig wrote:
> Correct me if I'm wrong but there is no need for this to be set for each
> WindowGrid.  IMO the WindowSelector should be defining whether to use masks or
> shapes and the WindowSelectorItem can ask the WindowSelector for this
> configuration instead of using statics.
> 
> Furthermore, I think we will actually run into an inconsistent state because
the
> WindowSelectorItem::use_mask_ and use_shape_ are defined as statics.  Consider
> multiple WindowGrid instances of which the last one to be constructed only has
1
> window.

There is since this is dependent on number of windows in each grid (to have the
corner radius correct). I will remove the masks and rounded corners at the
bottom of the window in the next CL however so this will be simplified.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_grid.cc:1046: windows_to_use_masks =
kMaxWindowsCountToHideHeaderWithMasks;
On 2016/08/30 17:57:33, bruthig wrote:
> You could consider adding a warning when the commend line specifies a value
but
> you don't actually use it.  Same applies below.

This flag was temporary scaffolding and Will be removed.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_grid.cc:1049: if
(command_line->HasSwitch(switches::kAshMaxWindowsToUseShapeInOverview) &&
On 2016/08/30 17:57:33, bruthig wrote:
> Consider wrapping this duplicate logic in a helper function. e.g.
> 
> int ReadCmdLineInt(int default, char[] switch);
> 
> Invoked like so:
> 
> int windows_to_use_masks =
ReadCmdLineInt(kMaxWindowsCountToHideHeaderWithMasks,
> switches::kAshMaxWindowsToUseMaskInOverview);

Ditto, will keep this in mind.

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

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:108: const int
kTransitionMilliseconds = 300;
On 2016/08/30 17:57:33, bruthig wrote:
> Should this have 'static' just like the rest of the consts?

Done.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:563: params.activatable =
views::Widget::InitParams::Activatable::ACTIVATABLE_NO;
On 2016/08/30 17:57:33, bruthig wrote:
> I feel like re-using |params| like this creates more risk than what it's
worth.
> e.g. If this block overwrites an attribute set in
> ConfigureWidgetInitParamsForContainer() but it never gets reset below.  It
would
> be much safer, and future proof to create a copy of |params| here and modify
the
> copy as needed.

I've managed to kill the second widget with some rework. PTAL.

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:729:
ui::ScopedLayerAnimationSettings animation_settings(
On 2016/08/30 17:57:33, bruthig wrote:
> For consistency, would it make more sense to add an OverviewAnimationType and
> use a ScopedOverviewAnimationSettingsAura object here instead?
> 
> Maybe we need to add a
> ScopedOverviewAnimationSettings::AddObserver(ImplicitAnimationObserver*
> observer) method.

Yes, will do.

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

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:193: bool first_time_;
On 2016/08/30 17:57:33, bruthig wrote:
> nit: A more explicit var name would be very helpful.

How is this?

https://codereview.chromium.org/2239233002/diff/120001/ash/wm/overview/scoped...
File ash/wm/overview/scoped_overview_animation_settings_aura.cc (right):

https://codereview.chromium.org/2239233002/diff/120001/ash/wm/overview/scoped...
ash/wm/overview/scoped_overview_animation_settings_aura.cc:73: if (material)
On 2016/08/30 17:57:33, bruthig wrote:
> nit: Can this be an else block from the block starting on line 65?

Done.

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

https://codereview.chromium.org/2239233002/diff/180001/ui/wm/core/shadow.cc#n...
ui/wm/core/shadow.cc:202: interior_inset_ + kRoundedCornerRadius,
This small bug is fairly recent - https://codereview.chromium.org/2083083004. I
will upload this separately - here and in a mus copy ash/mus/shadow.cc.

[commit-bot: I haz the power, 2016-09-02 11:56:36]

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

[commit-bot: I haz the power, 2016-09-02 11:56:37]

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-09-02 15:33:36]

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

[commit-bot: I haz the power, 2016-09-02 15:33:51]

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

[varkha, 2016-09-02 15:43:56]

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

https://codereview.chromium.org/2239233002/diff/180001/ui/wm/core/shadow.cc#n...
ui/wm/core/shadow.cc:202: interior_inset_ + kRoundedCornerRadius,
On 2016/09/02 11:22:51, varkha wrote:
> This small bug is fairly recent - https://codereview.chromium.org/2083083004.
I
> will upload this separately - here and in a mus copy ash/mus/shadow.cc.

Done. https://codereview.chromium.org/2302773005/

[bruthig, 2016-09-02 16:00:13]

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/120001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:323: bool use_mask_;
On 2016/09/02 11:22:51, varkha wrote:
> On 2016/08/30 17:57:33, bruthig wrote:
> > I find it confusing that there is a bool |use_mask_| and an actual |mask_|
> that
> > is used even when |use_mask_| is false.  Also |use_mask_| and |use_shape_|
> > appear to be functionally equivalent?  Perhaps some docs would help or
> combining
> > |use_mask_| and |use_shape_| into an enum if they are mutually exclusive. ;)

> > Additionally if there are some mutually exclusive states perhaps having
> distinct
> > classes makes more sense.
> 
> Masks will be removed in the next CL and there will be no need for those
flags.

Acknowledged.

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:301:
ShowHeaderAndResetShape();
AFAICT it is not guaranteed that HideHeaderAndSetShape() will be called on
|this| at this point.  Thus we either need to ensure ShowHeaderAndResetShape()
is allowed to be called in such a case, or you should add some tracking meta
data to know when to invoke it.  At the very least a comment to this effect
would be helpful somewhere.

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

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:203: void
SetBackgroundOpacity(float opacity) {
nit: IMO this would be better named as AnimateBackgroundOpacity().

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:256:
animation_->SetSlideDuration(400);
nit: Can the 400 be made a named constant like kSelectorFadeInMilliseconds?

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:282:
std::unique_ptr<gfx::SlideAnimation> animation_;
Is a SlideAnimation required or would a LinearAnimation be sufficient?

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:515:
transform_window_.HideHeaderAndSetShape(use_mask_, use_shape_, radius);
[Discussed offline]  Since HideHeaderAndSetShape() is now called when an
animation ends it is possible to use incorrect values of the static |use_mask_|
and |use_shape_| variables if animations are ending in an interleaved fashion
across different WindowGrid instances.

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:625: background_view_ = new
RoundedContainerView(
nit: It would be helpful to have a comment about who owns the
|background_view_|,  i.e. the |caption_container_view_|.

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:704:
window_label_button_view_->SetVisible(true);
Am I correct that this SetVisible() call invokes an implicit animation that the
|background_view_| is going to observe?  If so you should probably attach the
observer before kicking off the animation.  If a test is using a zero duration
animation the observer might not be attached in time.

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

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:197: // True initially until the
UpdateHeaderLayout is invoked for the first time.
This seems inconsistent with where |first_time_update_| is set to true. i.e. in
WindowSelectorItem::CreateWindowLabel().  I would have expected it to be set at
the bottom of WindowSelectorItem::UpdateHeaderLayout().

[commit-bot: I haz the power, 2016-09-02 16:51:13]

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

[commit-bot: I haz the power, 2016-09-02 16:51:14]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-09-02 21:33:04]

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

[commit-bot: I haz the power, 2016-09-02 21:33:34]

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-09-02 21:37:17]

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

[commit-bot: I haz the power, 2016-09-02 21:37:18]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[varkha, 2016-09-02 21:38:37]

Still working through the comments while optimizing the looks. The latest patch
supports properly animating the color on the way out as well as no crashes when
randomly clicking while pressing F5.

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

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:256:
animation_->SetSlideDuration(400);
On 2016/09/02 16:00:12, bruthig wrote:
> nit: Can the 400 be made a named constant like kSelectorFadeInMilliseconds?

Done.

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

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:197: // True initially until the
UpdateHeaderLayout is invoked for the first time.
On 2016/09/02 16:00:13, bruthig wrote:
> This seems inconsistent with where |first_time_update_| is set to true. i.e.
in
> WindowSelectorItem::CreateWindowLabel().  I would have expected it to be set
at
> the bottom of WindowSelectorItem::UpdateHeaderLayout().

I have removed this in favor for passing the stage (enter / update / exit) into
UpdateHeaderLayout.

[varkha, 2016-09-02 21:41:04]

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

[commit-bot: I haz the power, 2016-09-02 21:41: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-09-02 22:31:18]

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

[commit-bot: I haz the power, 2016-09-02 22:31:19]

Dry run: Exceeded global retry quota

[varkha, 2016-09-08 00:09:50]

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:301:
ShowHeaderAndResetShape();
On 2016/09/02 16:00:12, bruthig wrote:
> AFAICT it is not guaranteed that HideHeaderAndSetShape() will be called on
> |this| at this point.  Thus we either need to ensure ShowHeaderAndResetShape()
> is allowed to be called in such a case, or you should add some tracking meta
> data to know when to invoke it.  At the very least a comment to this effect
> would be helpful somewhere.

Done. Added a comment in the header since I think it is safe to call
ShowHeaderAndResetShape() even when HideHeaderAndSetShape() is not going to be
called.

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

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:203: void
SetBackgroundOpacity(float opacity) {
On 2016/09/02 16:00:12, bruthig wrote:
> nit: IMO this would be better named as AnimateBackgroundOpacity().

Done.

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:282:
std::unique_ptr<gfx::SlideAnimation> animation_;
On 2016/09/02 16:00:12, bruthig wrote:
> Is a SlideAnimation required or would a LinearAnimation be sufficient?

SlideAnimation saves from some of the headaches like choosing a frame rate. I
think it is simpler than having to recreate current value logic.

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:515:
transform_window_.HideHeaderAndSetShape(use_mask_, use_shape_, radius);
On 2016/09/02 16:00:12, bruthig wrote:
> [Discussed offline]  Since HideHeaderAndSetShape() is now called when an
> animation ends it is possible to use incorrect values of the static
|use_mask_|
> and |use_shape_| variables if animations are ending in an interleaved fashion
> across different WindowGrid instances.

Agreed. I will be posting removal of those command line switches immediately
after.

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:625: background_view_ = new
RoundedContainerView(
On 2016/09/02 16:00:12, bruthig wrote:
> nit: It would be helpful to have a comment about who owns the
> |background_view_|,  i.e. the |caption_container_view_|.

Done.

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:704:
window_label_button_view_->SetVisible(true);
On 2016/09/02 16:00:12, bruthig wrote:
> Am I correct that this SetVisible() call invokes an implicit animation that
the
> |background_view_| is going to observe?  If so you should probably attach the
> observer before kicking off the animation.  If a test is using a zero duration
> animation the observer might not be attached in time.

I don't think so. |window_label_button_view_| is just one of the views so I
think there are no implicit animations here.

[varkha, 2016-09-08 00:10:00]

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

[commit-bot: I haz the power, 2016-09-08 00:10:23]

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-09-08 00:13:51]

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

[commit-bot: I haz the power, 2016-09-08 00:13:53]

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

[varkha, 2016-09-08 16:24:29]

Description was changed from

==========
[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and
animates its bounds and opacity such that it appears to take over the real
header. Only once the "fake" header is opaque a mask or alpha shape is applied
to the window to hide its original header after which the "fake" header becomes
translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is
restored for the overview mode from the minimized state and is thus animated
from the shelf item.

BUG=624608
==========

to

==========
[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and
animates its bounds and opacity such that it appears to take over the real
header. Only once the "fake" header is opaque a mask or alpha shape is applied
to the window to hide its original header after which the "fake" header becomes
translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is
restored for the overview mode from the minimized state and is thus animated
from the shelf item.

BUG=624608, 645076
TEST=Most changes are only really visible under a great slow-down but watching
closely the files app header transform into overview mode should be much less
abrupt.
==========

[varkha, 2016-09-08 16:33:43]

Patchset #14 (id:280001) has been deleted

[varkha, 2016-09-08 16:33:54]

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

[commit-bot: I haz the power, 2016-09-08 16:34:10]

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-09-08 17:44:51]

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

[commit-bot: I haz the power, 2016-09-08 17:44:53]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-09-08 17:51:15]

sky@, can you also comment on this CL? Thanks!

[sky, 2016-09-08 18:30:17]

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_overview_animation_settings.h (right):

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_overview_animation_settings.h:19: virtual void
AddObserver(ui::ImplicitAnimationObserver* observer) = 0;
Don't you need a remove?

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:524:
window()->GetLayer()->SchedulePaint(bounds);
window()->SchedulePaint() ? That said, bounds in screen coordinates, but you're
passing it to window which is not necessarily screen coordaintes. Don't you need
to convert?

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:529: SkRegion* region
= new SkRegion;
std::unique_ptr and then std::move on 533.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:530:
region->setRect(RectToSkIRect(bounds));
Similar comment about bounds, shouldn't they be converted to coordaintes of
window()?

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

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:165: class
WindowSelectorItem::RoundedContainerView
Please update the description to indicate what this class now does. In
particular the animations and layer usage aren't at all obvious.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:188: void ObserveLayer(ui::Layer*
layer) {
Add descriptions for all your functions please.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:189: layer_ = layer;
What if layer_ is non-null?

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:247:
SchedulePaintInRect(GetLocalBounds());
SchedulePaint()?

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

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:49: void Disconnect() { listener_
= nullptr; }
How about a more obvious RemoveListener? Or ResetListener?

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:71: void Disconnect() { listener_
= nullptr; }
Similar comment.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:161: enum HeaderFadeInMode {
Generally we're moving toward enum class for better scoping and less errors.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:162: Enter,
Style for enums is ALL_CAPS.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm_window_p...
File ash/common/wm_window_property.h (right):

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm_window_p...
ash/common/wm_window_property.h:26: // Type int. See aura::client::kTopViewColor
for details.
Shouldn't the type be an SkColor? SkColor is uint32_t, which isn't really the
same as int.

[bruthig, 2016-09-08 18:32:50]

Mostly nits, 1 concern about the ImplicitAnimationObserver.

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

https://codereview.chromium.org/2239233002/diff/180001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:704:
window_label_button_view_->SetVisible(true);
On 2016/09/08 00:09:50, varkha wrote:
> On 2016/09/02 16:00:12, bruthig wrote:
> > Am I correct that this SetVisible() call invokes an implicit animation that
> the
> > |background_view_| is going to observe?  If so you should probably attach
the
> > observer before kicking off the animation.  If a test is using a zero
duration
> > animation the observer might not be attached in time.
> 
> I don't think so. |window_label_button_view_| is just one of the views so I
> think there are no implicit animations here.

Acknowledged.

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

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:190:
layer_->GetAnimator()->AddObserver(this);
nit: As a reader I wonder about the lifetime of |this| and whether the
LayerAnimator will outlive this and possibly notify an already destroyed
instance.  Might be worth a comment at the least or a
layer_->RemoveObserver(this) call in ~RoundedContainerView().

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:201: // Tests complete animations
immediately. Emulate by invoking the callback.
Won't the layer animations still notify AnimationEnded() when using the
ZERO_DURATION ScaleMode?  If not I would think it should be fixed to do so and
this might be worthy of a TODO here.

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:831:
animation_settings->AddObserver(observer.get());
Out of curiosity I looked into how long the layer animator actually points to
this |observer| and it appears to me that it will be removed sooner than we
originally thought.  From what I can tell |observer| will be removed when
~ScopedLayerAnimationSettings()  is called from
~ScopedOverviewAnimationSettings() dtor. i.e. The observer is removed at the end
of this method.

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

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:49: void Disconnect() { listener_
= nullptr; }
nit: I find it really odd that Button::listener_ is protected yet everything
seems to use Button::set_listener().  No action requested but it might be better
to use set_listener().

[varkha, 2016-09-09 02:25:26]

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

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:190:
layer_->GetAnimator()->AddObserver(this);
On 2016/09/08 18:32:49, bruthig wrote:
> nit: As a reader I wonder about the lifetime of |this| and whether the
> LayerAnimator will outlive this and possibly notify an already destroyed
> instance.  Might be worth a comment at the least or a
> layer_->RemoveObserver(this) call in ~RoundedContainerView().

Done.

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:201: // Tests complete animations
immediately. Emulate by invoking the callback.
On 2016/09/08 18:32:49, bruthig wrote:
> Won't the layer animations still notify AnimationEnded() when using the
> ZERO_DURATION ScaleMode?  If not I would think it should be fixed to do so and
> this might be worthy of a TODO here.

This is not an implicit property animation and so the AnimationEnded() is not
called in tests.

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:831:
animation_settings->AddObserver(observer.get());
On 2016/09/08 18:32:49, bruthig wrote:
> Out of curiosity I looked into how long the layer animator actually points to
> this |observer| and it appears to me that it will be removed sooner than we
> originally thought.  From what I can tell |observer| will be removed when
> ~ScopedLayerAnimationSettings()  is called from
> ~ScopedOverviewAnimationSettings() dtor. i.e. The observer is removed at the
end
> of this method.

The subtlety is that the observer is removed from the animator but not from the
sequences that are already started. See [1] vs. [2].

[1] - removing observers for all started sequences -
https://cs.chromium.org/chromium/src/ui/compositor/layer_animator.cc?type=cs&...

[2] - removing just the observer from the ScopedLayerAnimationSettings but
leaving it in the sequences -
https://cs.chromium.org/chromium/src/ui/compositor/scoped_layer_animation_set...

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

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:49: void Disconnect() { listener_
= nullptr; }
On 2016/09/08 18:32:50, bruthig wrote:
> nit: I find it really odd that Button::listener_ is protected yet everything
> seems to use Button::set_listener().  No action requested but it might be
better
> to use set_listener().

There is no Button::set_listener() (but there is one in Checkbox. I can (in a
separate CL of course) add a protected set_listener() to Button, make
|listener_| private and expose set_listener in Checkbox.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_overview_animation_settings.h (right):

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_overview_animation_settings.h:19: virtual void
AddObserver(ui::ImplicitAnimationObserver* observer) = 0;
On 2016/09/08 18:30:16, sky wrote:
> Don't you need a remove?

This gets a bit tricky.
The short answer is, I don't think so - it gets removed here [1].
This observer is added here [2] and NOT removed when
ScopedOverviewAnimationSettings goes out of scope because we do not remove
observers from the already running sequences - see [3]. So the observer stays
observing the scheduled animations and then gets removed before
OnImplicitAnimationsCompleted() is called which would do "delete this" on the
observer disposing of the owned widget_.

[1] -
https://cs.chromium.org/chromium/src/ui/compositor/layer_animation_observer.c...

[2]
https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
line 831.

[3] See how we call animator_->observers_.RemoveObserver() (and not
animator_->RemoveObserver() which would have removed the observer from the
running sequences) here -
https://cs.chromium.org/chromium/src/ui/compositor/scoped_layer_animation_set...

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
File ash/common/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:524:
window()->GetLayer()->SchedulePaint(bounds);
On 2016/09/08 18:30:16, sky wrote:
> window()->SchedulePaint() ? That said, bounds in screen coordinates, but
you're
> passing it to window which is not necessarily screen coordaintes. Don't you
need
> to convert?

I haven't seen WmWindow::SchedulePaint(). |bounds| are initialized with size()
so they are relative to the layer already, no?

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:529: SkRegion* region
= new SkRegion;
On 2016/09/08 18:30:16, sky wrote:
> std::unique_ptr and then std::move on 533.

Done.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/scoped_transform_overview_window.cc:530:
region->setRect(RectToSkIRect(bounds));
On 2016/09/08 18:30:16, sky wrote:
> Similar comment about bounds, shouldn't they be converted to coordaintes of
> window()?

Same, bounds are initialized with size() so they are local to the layer.

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

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:165: class
WindowSelectorItem::RoundedContainerView
On 2016/09/08 18:30:16, sky wrote:
> Please update the description to indicate what this class now does. In
> particular the animations and layer usage aren't at all obvious.

Done.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:188: void ObserveLayer(ui::Layer*
layer) {
On 2016/09/08 18:30:16, sky wrote:
> Add descriptions for all your functions please.

Done.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:189: layer_ = layer;
On 2016/09/08 18:30:16, sky wrote:
> What if layer_ is non-null?

Added a DCHECK() and a comment. This should be only called once upon entering
overview.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:247:
SchedulePaintInRect(GetLocalBounds());
On 2016/09/08 18:30:16, sky wrote:
> SchedulePaint()?

Done.

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

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:49: void Disconnect() { listener_
= nullptr; }
On 2016/09/08 18:30:17, sky wrote:
> How about a more obvious RemoveListener? Or ResetListener?

Done.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:71: void Disconnect() { listener_
= nullptr; }
On 2016/09/08 18:30:16, sky wrote:
> Similar comment.

Done.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:161: enum HeaderFadeInMode {
On 2016/09/08 18:30:16, sky wrote:
> Generally we're moving toward enum class for better scoping and less errors.

Done.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.h:162: Enter,
On 2016/09/08 18:30:17, sky wrote:
> Style for enums is ALL_CAPS.

Done.

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm_window_p...
File ash/common/wm_window_property.h (right):

https://codereview.chromium.org/2239233002/diff/300001/ash/common/wm_window_p...
ash/common/wm_window_property.h:26: // Type int. See aura::client::kTopViewColor
for details.
On 2016/09/08 18:30:17, sky wrote:
> Shouldn't the type be an SkColor? SkColor is uint32_t, which isn't really the
> same as int.

Done.

[varkha, 2016-09-09 02:32:15]

Patchset #15 (id:320001) has been deleted

[varkha, 2016-09-09 02:32:26]

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

[commit-bot: I haz the power, 2016-09-09 02:32: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-09-09 03:41:07]

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

[commit-bot: I haz the power, 2016-09-09 03:41:08]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-09-09 13:58:27]

lgtm

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

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:201: // Tests complete animations
immediately. Emulate by invoking the callback.
On 2016/09/09 02:25:25, varkha wrote:
> On 2016/09/08 18:32:49, bruthig wrote:
> > Won't the layer animations still notify AnimationEnded() when using the
> > ZERO_DURATION ScaleMode?  If not I would think it should be fixed to do so
and
> > this might be worthy of a TODO here.
> 
> This is not an implicit property animation and so the AnimationEnded() is not
> called in tests.

Ah, I think I get it.  So |animation_| is a SlideAnimation which is based off of
the AnimationContainer::timer_ which is not hookable during tests and runs in
real clock time?  I think this issue applies then https://crbug.com/630684. 
Maybe a TODO referencing it make sense.

So as it stands now, you are artificially invoking AnimationEnded() immediately
but the |animation_| will probably call AnimationEnded() again later when it
ends or is destroyed.  That seems reasonable.

https://codereview.chromium.org/2239233002/diff/260001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:831:
animation_settings->AddObserver(observer.get());
On 2016/09/09 02:25:25, varkha wrote:
> On 2016/09/08 18:32:49, bruthig wrote:
> > Out of curiosity I looked into how long the layer animator actually points
to
> > this |observer| and it appears to me that it will be removed sooner than we
> > originally thought.  From what I can tell |observer| will be removed when
> > ~ScopedLayerAnimationSettings()  is called from
> > ~ScopedOverviewAnimationSettings() dtor. i.e. The observer is removed at the
> end
> > of this method.
> 
> The subtlety is that the observer is removed from the animator but not from
the
> sequences that are already started. See [1] vs. [2].
> 
> [1] - removing observers for all started sequences -
>
https://cs.chromium.org/chromium/src/ui/compositor/layer_animator.cc?type=cs&...
> 
> [2] - removing just the observer from the ScopedLayerAnimationSettings but
> leaving it in the sequences -
>
https://cs.chromium.org/chromium/src/ui/compositor/scoped_layer_animation_set...

Acknowledged.

[sky, 2016-09-09 16:13:20]

Thanks for the improved docs!
Just a couple of suggestions on naming.

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

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:170: // - Opacity animation. The
header is initially same color as the original
Thanks for improving the description!

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:192: previous_color_(background),
I was mildly confused by these names and what they mean. Did you consider
initial_color and target_color? That better indicates what they are used for.

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:217: void
AnimateColor(gfx::Tween::Type tween_type, int duration) {
If this is expected to only be called once and while layer_ is null, it would be
nice to assert and document that.

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:258: last_alpha_ =
SkColorGetA(color);
You seem to calculate this, but it's never used. Is it needed?

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:273: WmWindow* label_window =
WmLookup::Get()->GetWindowForWidget(GetWidget());
It isn't obvious why you need to restack here. Please document why.

[varkha, 2016-09-09 16:37:22]

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

[commit-bot: I haz the power, 2016-09-09 16:37:45]

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

[varkha, 2016-09-09 16:37:46]

Thanks! PTAL.

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

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:170: // - Opacity animation. The
header is initially same color as the original
On 2016/09/09 16:13:20, sky wrote:
> Thanks for improving the description!

Acknowledged.

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:192: previous_color_(background),
On 2016/09/09 16:13:20, sky wrote:
> I was mildly confused by these names and what they mean. Did you consider
> initial_color and target_color? That better indicates what they are used for.

Done.

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:217: void
AnimateColor(gfx::Tween::Type tween_type, int duration) {
On 2016/09/09 16:13:19, sky wrote:
> If this is expected to only be called once and while layer_ is null, it would
be
> nice to assert and document that.

Done.

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:258: last_alpha_ =
SkColorGetA(color);
On 2016/09/09 16:13:19, sky wrote:
> You seem to calculate this, but it's never used. Is it needed?

Done. Good catch, thanks! Scaffolding.

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:273: WmWindow* label_window =
WmLookup::Get()->GetWindowForWidget(GetWidget());
On 2016/09/09 16:13:20, sky wrote:
> It isn't obvious why you need to restack here. Please document why.

Done.

[varkha, 2016-09-09 16:47:08]

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

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:273: WmWindow* label_window =
WmLookup::Get()->GetWindowForWidget(GetWidget());
On 2016/09/09 16:37:46, varkha wrote:
> On 2016/09/09 16:13:20, sky wrote:
> > It isn't obvious why you need to restack here. Please document why.
> 
> Done.

Got the comments wrong. Correcting now...

[varkha, 2016-09-09 16:51:15]

Patchset #16 (id:360001) has been deleted

[varkha, 2016-09-09 16:51:47]

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

https://codereview.chromium.org/2239233002/diff/340001/ash/common/wm/overview...
ash/common/wm/overview/window_selector_item.cc:273: WmWindow* label_window =
WmLookup::Get()->GetWindowForWidget(GetWidget());
On 2016/09/09 16:47:08, varkha wrote:
> On 2016/09/09 16:37:46, varkha wrote:
> > On 2016/09/09 16:13:20, sky wrote:
> > > It isn't obvious why you need to restack here. Please document why.
> > 
> > Done.
> 
> Got the comments wrong. Correcting now...

Done.

[varkha, 2016-09-09 16:52:30]

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

[commit-bot: I haz the power, 2016-09-09 16:53:08]

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-09-09 17:50:11]

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

[commit-bot: I haz the power, 2016-09-09 17:50:13]

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

[varkha, 2016-09-09 19:26:44]

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

[commit-bot: I haz the power, 2016-09-09 19:27:03]

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

[sky, 2016-09-09 20:29:37]

LGTM

[commit-bot: I haz the power, 2016-09-09 20:43:02]

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

[commit-bot: I haz the power, 2016-09-09 20:43:03]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-09-09 22:00:23]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-09-09 22:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2239233002/#ps400001
(title: "[ash-md] Fades overview header in and out (fixed a new test
assertion)")

[commit-bot: I haz the power, 2016-09-09 22:01:01]

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-09-09 22:06:37]

Description was changed from

==========
[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and
animates its bounds and opacity such that it appears to take over the real
header. Only once the "fake" header is opaque a mask or alpha shape is applied
to the window to hide its original header after which the "fake" header becomes
translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is
restored for the overview mode from the minimized state and is thus animated
from the shelf item.

BUG=624608, 645076
TEST=Most changes are only really visible under a great slow-down but watching
closely the files app header transform into overview mode should be much less
abrupt.
==========

to

==========
[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and
animates its bounds and opacity such that it appears to take over the real
header. Only once the "fake" header is opaque a mask or alpha shape is applied
to the window to hide its original header after which the "fake" header becomes
translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is
restored for the overview mode from the minimized state and is thus animated
from the shelf item.

BUG=624608, 645076
TEST=Most changes are only really visible under a great slow-down but watching
closely the files app header transform into overview mode should be much less
abrupt.
==========

[commit-bot: I haz the power, 2016-09-09 22:06:38]

Committed patchset #17 (id:400001)

[commit-bot: I haz the power, 2016-09-09 22:08:07]

Description was changed from

==========
[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and
animates its bounds and opacity such that it appears to take over the real
header. Only once the "fake" header is opaque a mask or alpha shape is applied
to the window to hide its original header after which the "fake" header becomes
translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is
restored for the overview mode from the minimized state and is thus animated
from the shelf item.

BUG=624608, 645076
TEST=Most changes are only really visible under a great slow-down but watching
closely the files app header transform into overview mode should be much less
abrupt.
==========

to

==========
[ash-md] Fades overview header in and out

This change installs an additional header on top of the real window's header and
animates its bounds and opacity such that it appears to take over the real
header. Only once the "fake" header is opaque a mask or alpha shape is applied
to the window to hide its original header after which the "fake" header becomes
translucent to conform to MD overview mode spec.

This creates a visually smoother transition into overview mode than before.

Special care is taken to animate the "fake" header in case when the window is
restored for the overview mode from the minimized state and is thus animated
from the shelf item.

BUG=624608, 645076
TEST=Most changes are only really visible under a great slow-down but watching
closely the files app header transform into overview mode should be much less
abrupt.

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

[commit-bot: I haz the power, 2016-09-09 22:08:09]

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

[dmurph, 2016-09-10 01:49:28]

A revert of this CL (patchset #17 id:400001) has been created in
https://codereview.chromium.org/2329433003/ by dmurph@chromium.org.

The reason for reverting is: This is causing a use-after-free and crashing stuff
:(

BUG=645708.