Animate the App List overlay when dialogs are opened

Animate the App List overlay when dialogs are opened

Currently, the App List overlay opens and closes without animating. This
looks strange the dialog itself animates as it opens. Add an animation
delay so the overlay animates slowly open to match the sub-dialogs.

BUG=395494
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287492

[benwells, 2014-07-31 06:14:14]

punting to calamity to review :)

[calamity, 2014-07-31 07:58:11]

https://codereview.chromium.org/429823002/diff/1/ui/app_list/views/app_list_v...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/429823002/diff/1/ui/app_list/views/app_list_v...
ui/app_list/views/app_list_view.cc:236: if (visible == true) {
This can just be

if (visible) {

But if you get rid of SetVisible(false) below, you can just use the ternary
operator for the SetOpacity argument.

https://codereview.chromium.org/429823002/diff/1/ui/app_list/views/app_list_v...
ui/app_list/views/app_list_view.cc:241: overlay_view_->SetVisible(false);
How does this work? Won't the SetVisible(false) immediately hide the whole view,
messing up the animation?

I think you'll need to observe the animation here.

[sashab, 2014-08-04 01:19:33]

https://codereview.chromium.org/429823002/diff/1/ui/app_list/views/app_list_v...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/429823002/diff/1/ui/app_list/views/app_list_v...
ui/app_list/views/app_list_view.cc:236: if (visible == true) {
On 2014/07/31 07:58:11, calamity wrote:
> This can just be
> 
> if (visible) {
> 
> But if you get rid of SetVisible(false) below, you can just use the ternary
> operator for the SetOpacity argument.

Hmm... I originally thought this might be more confusing. But it looks OK. Done.

https://codereview.chromium.org/429823002/diff/1/ui/app_list/views/app_list_v...
ui/app_list/views/app_list_view.cc:241: overlay_view_->SetVisible(false);
On 2014/07/31 07:58:11, calamity wrote:
> How does this work? Won't the SetVisible(false) immediately hide the whole
view,
> messing up the animation?
> 
> I think you'll need to observe the animation here.

Yup, it works - no matter how long I set the delay, the animation fully
completes before hiding the layer.
The ScopedLayerAnimationSettings must take visibility into account. I can't find
the code for this though...

Also, if you close the dialog while the animation is running, it disappears
instantly.

[sashab, 2014-08-04 02:59:36]

calamity: Not a bad idea with the overlay observer. Thoughts?

[calamity, 2014-08-04 03:33:47]

Was there any reason you couldn't just use the observer as it was?

https://codereview.chromium.org/429823002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/429823002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:233: if (visible == false) {
Use !x over 'x == false'.

https://codereview.chromium.org/429823002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:234:
animation_observer_->set_frame(GetBubbleFrameView());
Does the background need to repaint? If not, pass NULL and handle !frame_ in the
animation observer.

[sashab, 2014-08-04 05:39:16]

Thanks calamity.

https://codereview.chromium.org/429823002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/429823002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:233: if (visible == false) {
On 2014/08/04 03:33:47, calamity wrote:
> Use !x over 'x == false'.

Done.

https://codereview.chromium.org/429823002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:234:
animation_observer_->set_frame(GetBubbleFrameView());
On 2014/08/04 03:33:47, calamity wrote:
> Does the background need to repaint? If not, pass NULL and handle !frame_ in
the
> animation observer.

Done.

[calamity, 2014-08-04 06:50:17]

lgtm w/ nit.

https://codereview.chromium.org/429823002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/429823002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:236: // animation_observer_ here.
Call animation_observer_->set_frame_view(NULL) here.

[sashab, 2014-08-05 01:17:55]

https://codereview.chromium.org/429823002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/429823002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:236: // animation_observer_ here.
On 2014/08/04 06:50:17, calamity wrote:
> Call animation_observer_->set_frame_view(NULL) here.

It's not necessary (NULL is the default), but I see why you'd want to do it for
clarity. Done.

[sashab, 2014-08-05 01:18:04]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-08-05 01:20:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/429823002/80001

[commit-bot: I haz the power, 2014-08-05 04:37:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-05 08:11:53]

Change committed as 287492