Add Fade-out/Fade-in animation during output-configuration change.

Add Fade-out/Fade-in animation during output-configuration change.


BUG=127493
TEST=manually checked on lumpy


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149382

[oshima, 2012-07-26 05:27:48]

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
File ash/display/output_configurator_animation.cc (right):

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:80: ClearHidingLayers();
Will this call the callback? My guess is that such scenario is like "cancel and
start new mode change", so it shouldn't call the callback, so we should make
sure that won't happen.

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:99:
hiding_layer->SetTransform(ui::Transform());
just wondering. why you need this?

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:158: hiding_layers_.begin(),
hiding_layers_.end());
what happen to the layer for detaching display? Won't they be deleted when the
root window gets deleted?

Alternatively, you can set the layer to the root window's property, and clear
hiding layers on the all available root windows.

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
File ash/display/output_configurator_animation.h (right):

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.h:19: }
//  namespace ui
(or you can remove "// namespace aura" as it's small. just be consistent)

http://codereview.chromium.org/10817028/diff/21001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10817028/diff/21001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:763: base::Unretained(this),
new_state));
what happen if the display configuration changed during animation?

http://codereview.chromium.org/10817028/diff/21001/ui/compositor/scoped_layer...
File ui/compositor/scoped_layer_animation_settings.cc (left):

http://codereview.chromium.org/10817028/diff/21001/ui/compositor/scoped_layer...
ui/compositor/scoped_layer_animation_settings.cc:74: 
revert this file?

[Jun Mukai, 2012-07-26 09:44:02]

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
File ash/display/output_configurator_animation.cc (right):

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:80: ClearHidingLayers();
On 2012/07/26 05:27:49, oshima wrote:
> Will this call the callback? My guess is that such scenario is like "cancel
and
> start new mode change", so it shouldn't call the callback, so we should make
> sure that won't happen.

I put a debug code and made sure ClearHidingLayers() will cancel its callback
too.  Thanks for the clarification.  Added comment on the header file.

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:99:
hiding_layer->SetTransform(ui::Transform());
On 2012/07/26 05:27:49, oshima wrote:
> just wondering. why you need this?

oh, I was wrong and it's not necessary anymore.  Removed.

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:158: hiding_layers_.begin(),
hiding_layers_.end());
On 2012/07/26 05:27:49, oshima wrote:
> what happen to the layer for detaching display? Won't they be deleted when the
> root window gets deleted?
> 
> Alternatively, you can set the layer to the root window's property, and clear
> hiding layers on the all available root windows.

I just checked ui/compositor/layer.cc and noticed that ~Layer() doesn't delete
its children.  Rather the children remain with no-parent, so deleting them here
seem safe.

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
File ash/display/output_configurator_animation.h (right):

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.h:19: }
On 2012/07/26 05:27:49, oshima wrote:
> //  namespace ui
> (or you can remove "// namespace aura" as it's small. just be consistent)

Done.

http://codereview.chromium.org/10817028/diff/21001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

http://codereview.chromium.org/10817028/diff/21001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:763: base::Unretained(this),
new_state));
On 2012/07/26 05:27:49, oshima wrote:
> what happen if the display configuration changed during animation?

Added the output_state_ guards for STATE_INVALID/HEADLESS/SINGLE to
SetDisplayModeInternal() too.

Then, this just delays the call of previous SetDisplayMode() and it's as safe as
the existing code.

Note that the output_state_ guard is still necessary for this new
SetDisplayMode() too.  Without the guard, Ctrl-F4 will make fade-out/fade-in
animation with no effects in case of single-display.

[oshima, 2012-07-26 14:08:16]

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
File ash/display/output_configurator_animation.cc (right):

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:80: ClearHidingLayers();
On 2012/07/26 09:44:03, Jun Mukai wrote:
> On 2012/07/26 05:27:49, oshima wrote:
> > Will this call the callback? My guess is that such scenario is like "cancel
> and
> > start new mode change", so it shouldn't call the callback, so we should make
> > sure that won't happen.
> 
> I put a debug code and made sure ClearHidingLayers() will cancel its callback
> too.  Thanks for the clarification.  Added comment on the header file.

How they're canceled? ImplicitAnimationObserver calls OnSingleTaskCompleted when
aborted.
If it indeed doesn't call callback_.Run(), won't observer instance leak?

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:158: hiding_layers_.begin(),
hiding_layers_.end());
On 2012/07/26 09:44:03, Jun Mukai wrote:
> On 2012/07/26 05:27:49, oshima wrote:
> > what happen to the layer for detaching display? Won't they be deleted when
the
> > root window gets deleted?
> > 
> > Alternatively, you can set the layer to the root window's property, and
clear
> > hiding layers on the all available root windows.
> 
> I just checked ui/compositor/layer.cc and noticed that ~Layer() doesn't delete
> its children.  Rather the children remain with no-parent, so deleting them
here
> seem safe.
Ok, so looks like it doesn't delete layer when root window is deleted, or
animation is completed. Isn't it better to clear all layers when animation is
completed?

http://codereview.chromium.org/10817028/diff/21001/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

http://codereview.chromium.org/10817028/diff/21001/chromeos/display/output_co...
chromeos/display/output_configurator.h:73: }
move this to .cc, and forward decl base::Closure.

http://codereview.chromium.org/10817028/diff/18003/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10817028/diff/18003/ash/ash.gyp#newcode67
ash/ash.gyp:67: 'display/output_configurator_animation.h',
sort

[Jun Mukai, 2012-07-27 02:22:53]

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
File ash/display/output_configurator_animation.cc (right):

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:80: ClearHidingLayers();
On 2012/07/26 14:08:17, oshima wrote:
> On 2012/07/26 09:44:03, Jun Mukai wrote:
> > On 2012/07/26 05:27:49, oshima wrote:
> > > Will this call the callback? My guess is that such scenario is like
"cancel
> > and
> > > start new mode change", so it shouldn't call the callback, so we should
make
> > > sure that won't happen.
> > 
> > I put a debug code and made sure ClearHidingLayers() will cancel its
callback
> > too.  Thanks for the clarification.  Added comment on the header file.
> 
> How they're canceled? ImplicitAnimationObserver calls OnSingleTaskCompleted
when
> aborted.
> If it indeed doesn't call callback_.Run(), won't observer instance leak?

Sorry I haven't looked this deeper.
I noticed that animation abort event won't arrive unless the observer overrides
RequiresNotificationWhenAnimatorDestroyed() which returns false by default. 
That's why the callback doesn't run although layer is deleted.

However, this means memory leak since no one is removing the animation observer
instances.  To release the observers and distinguish normal end event and abort
event, I changed the code to override ui::LayerAnimationObserver rather than
Implicit...

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:158: hiding_layers_.begin(),
hiding_layers_.end());
On 2012/07/26 14:08:17, oshima wrote:
> On 2012/07/26 09:44:03, Jun Mukai wrote:
> > On 2012/07/26 05:27:49, oshima wrote:
> > > what happen to the layer for detaching display? Won't they be deleted when
> the
> > > root window gets deleted?
> > > 
> > > Alternatively, you can set the layer to the root window's property, and
> clear
> > > hiding layers on the all available root windows.
> > 
> > I just checked ui/compositor/layer.cc and noticed that ~Layer() doesn't
delete
> > its children.  Rather the children remain with no-parent, so deleting them
> here
> > seem safe.
> Ok, so looks like it doesn't delete layer when root window is deleted, or
> animation is completed. Isn't it better to clear all layers when animation is
> completed?

Sorry, what do you mean?  The layers are deleted at the end of
OnDisplayModeChanged() as its callback.  At the end of WillDisplayModeChange()'s
animation, the black layers are covering over the root windows and cannot be
deleted.

Oh, probably it makes sense to set a timer or something to delete layers in case
that OnDisplayModeChanged() isn't called due to some unknown errors?

http://codereview.chromium.org/10817028/diff/18003/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10817028/diff/18003/ash/ash.gyp#newcode67
ash/ash.gyp:67: 'display/output_configurator_animation.h',
On 2012/07/26 14:08:17, oshima wrote:
> sort 

Done.

[Jun Mukai, 2012-07-27 02:36:21]

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
File ash/display/output_configurator_animation.cc (right):

http://codereview.chromium.org/10817028/diff/21001/ash/display/output_configu...
ash/display/output_configurator_animation.cc:158: hiding_layers_.begin(),
hiding_layers_.end());
On 2012/07/27 02:22:53, Jun Mukai wrote:
> On 2012/07/26 14:08:17, oshima wrote:
> > On 2012/07/26 09:44:03, Jun Mukai wrote:
> > > On 2012/07/26 05:27:49, oshima wrote:
> > > > what happen to the layer for detaching display? Won't they be deleted
when
> > the
> > > > root window gets deleted?
> > > > 
> > > > Alternatively, you can set the layer to the root window's property, and
> > clear
> > > > hiding layers on the all available root windows.
> > > 
> > > I just checked ui/compositor/layer.cc and noticed that ~Layer() doesn't
> delete
> > > its children.  Rather the children remain with no-parent, so deleting them
> > here
> > > seem safe.
> > Ok, so looks like it doesn't delete layer when root window is deleted, or
> > animation is completed. Isn't it better to clear all layers when animation
is
> > completed?
> 
> Sorry, what do you mean?  The layers are deleted at the end of
> OnDisplayModeChanged() as its callback.  At the end of
WillDisplayModeChange()'s
> animation, the black layers are covering over the root windows and cannot be
> deleted.
> 
> Oh, probably it makes sense to set a timer or something to delete layers in
case
> that OnDisplayModeChanged() isn't called due to some unknown errors?

a OneShotTimer is added

[oshima, 2012-07-27 06:22:56]

plus, please stop time in OnDisplayModeChanged.

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
File ash/display/output_configurator_animation.cc (right):

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:27: // CallbackRunningObserver
accepts several layer animations and
s/several/multiple/

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:83: CallbackRunningObserver*
observer_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:87: bool completed_successfully_;
animation_aborted_ maybe more intuitive, but i'll leave it to owner's review.

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:90: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:148: base::Unretained(this)));
this can be canceled, but I assume this only happens when next animation is
started, so they'll eventually be deleted. Can you add comment for this
scenario?

[Jun Mukai, 2012-07-27 06:35:05]

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
File ash/display/output_configurator_animation.cc (right):

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:27: // CallbackRunningObserver
accepts several layer animations and
On 2012/07/27 06:22:57, oshima wrote:
> s/several/multiple/

Done.

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:83: CallbackRunningObserver*
observer_;
On 2012/07/27 06:22:57, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:87: bool completed_successfully_;
On 2012/07/27 06:22:57, oshima wrote:
> animation_aborted_ maybe more intuitive, but i'll leave it to owner's review.

Agree with you and changed to animation_aborted_ (although I'm slightly negative
with if (!aborted) style...)

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:90: };
On 2012/07/27 06:22:57, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/10817028/diff/21003/ash/display/output_configu...
ash/display/output_configurator_animation.cc:148: base::Unretained(this)));
On 2012/07/27 06:22:57, oshima wrote:
> this can be canceled, but I assume this only happens when next animation is
> started, so they'll eventually be deleted. Can you add comment for this
> scenario?

Done.

[oshima_google, 2012-07-27 06:40:37]

lgtm

[Jun Mukai, 2012-07-27 08:46:37]

ben, please review this as the OWNER

[Ben Goodger (Google), 2012-07-27 17:17:07]

http://codereview.chromium.org/10817028/diff/14009/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

http://codereview.chromium.org/10817028/diff/14009/chromeos/display/output_co...
chromeos/display/output_configurator.h:64: class AnimationDelegate {
I don't like how this is called "Animation*" since Animation to me implies
something very tied to the UI.

Can this instead be Observer or something like that?

e.g. class OutputConfiguratorObserver

[Jun Mukai, 2012-07-30 11:28:45]

http://codereview.chromium.org/10817028/diff/14009/chromeos/display/output_co...
File chromeos/display/output_configurator.h (right):

http://codereview.chromium.org/10817028/diff/14009/chromeos/display/output_co...
chromeos/display/output_configurator.h:64: class AnimationDelegate {
On 2012/07/27 17:17:07, Ben Goodger (Google) wrote:
> I don't like how this is called "Animation*" since Animation to me implies
> something very tied to the UI.
> 
> Can this instead be Observer or something like that?
> 
> e.g. class OutputConfiguratorObserver

Agree to rename this class, but I'm afraid that what this class does is not an
observer's task, since WillDisplayModeChange() accepts a callback and runs it. 
I think "observer" will just receive several events, right?

Then, I rather think to reorganize the code again like:
- create OutputConfiguratorObserver which triggers OnDisplayModeChanged() only
- ash::Shell doesn't have chromeos::OutputConfigurator directly.  Rather it has
ash::OutputConfigurator, which provides the interface to
chromeos::OutputConfigurator but conducts the fade-out animation before the
actual task.
- ash::OutputConfigurator inherits OutputConfiguratorObserver so that it knows
when fade-in animation will run.

Let me take a day to do this.

[Jun Mukai, 2012-07-31 11:40:48]

Uploaded a new patch set.  Could you take a look again?

[Ben Goodger (Google), 2012-07-31 15:53:34]

lgtm

[commit-bot: I haz the power, 2012-07-31 23:06:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10817028/27001

[commit-bot: I haz the power, 2012-08-01 04:39:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10817028/27001

[commit-bot: I haz the power, 2012-08-01 04:39:20]

Failed to apply patch for ash/accelerators/accelerator_controller.cc:
While running patch -p1 --forward --force;
patching file ash/accelerators/accelerator_controller.cc
Hunk #3 FAILED at 408.
1 out of 3 hunks FAILED -- saving rejects to file
ash/accelerators/accelerator_controller.cc.rej

[commit-bot: I haz the power, 2012-08-01 05:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10817028/15006

[commit-bot: I haz the power, 2012-08-01 06:42:55]

Change committed as 149382