Reland r107720 - Enable the new layer animation framework.

Reland r107720 - Enable the new layer animation framework.

Depends on http://codereview.chromium.org/8247009/

BUG=None
TEST=None

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

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

[sky, 2011-10-20 20:30:30]

http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
File chrome/browser/ui/touch/animation/screen_rotation.cc (right):

http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
chrome/browser/ui/touch/animation/screen_rotation.cc:129: const gfx::Rect bounds
= delegate_->GetBoundsForAnimation();
const gfx::Rect&

http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
File chrome/browser/ui/touch/animation/screen_rotation_animator.h (right):

http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
chrome/browser/ui/touch/animation/screen_rotation_animator.h:9: #include
"ui/gfx/compositor/layer_animator.h"
can you forward declare LayerAnimator?

http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
chrome/browser/ui/touch/animation/screen_rotation_animator.h:20: };
Add private: DISALLOW_IMPLICIT_CONSTRUCTORS

http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/dummy_laye...
File ui/gfx/compositor/dummy_layer_animation_delegate.h (right):

http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/dummy_laye...
ui/gfx/compositor/dummy_layer_animation_delegate.h:17: class COMPOSITOR_EXPORT
DummyLayerAnimationDelegate
Why are you renaming this? Test is a better name than Dummy.

http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer.cc#n...
ui/gfx/compositor/layer.cc:109: animator->SetDelegate(this);
Seems like you should allow NULL here to reset to default.

http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_element.h (right):

http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.h:75: void
SetTarget(LayerAnimationDelegate* delegate) const;
This is an odd way to extract the target value. How about:

 struct TargetValue {
    transform, opacity ...
  };

 GetTargetValue(TargetValue* target_value);

[Ian Vollick, 2011-10-20 21:24:10]

On 2011/10/20 20:30:30, sky wrote:
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
> File chrome/browser/ui/touch/animation/screen_rotation.cc (right):
> 
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
> chrome/browser/ui/touch/animation/screen_rotation.cc:129: const gfx::Rect
bounds
> = delegate_->GetBoundsForAnimation();
> const gfx::Rect&
Done.
> 
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
> File chrome/browser/ui/touch/animation/screen_rotation_animator.h (right):
> 
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
> chrome/browser/ui/touch/animation/screen_rotation_animator.h:9: #include
> "ui/gfx/compositor/layer_animator.h"
> can you forward declare LayerAnimator?
Yes, done.
> 
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
> chrome/browser/ui/touch/animation/screen_rotation_animator.h:20: };
> Add private: DISALLOW_IMPLICIT_CONSTRUCTORS
Done.
> 
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/dummy_laye...
> File ui/gfx/compositor/dummy_layer_animation_delegate.h (right):
> 
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/dummy_laye...
> ui/gfx/compositor/dummy_layer_animation_delegate.h:17: class COMPOSITOR_EXPORT
> DummyLayerAnimationDelegate
> Why are you renaming this? Test is a better name than Dummy.
I realized that I was being shortsighted with this class. I need to make use of
it in other places than in tests so the Test prefix felt wrong. I intend to use
it for blending sequences. At time t, I'll Progress sequence a at t with a dummy
delegate, do the same for b, and then interpolate between the dummies. It's also
been useful as a throwaway target for animations (for collecting target values).
> 
> http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer.cc
> File ui/gfx/compositor/layer.cc (right):
> 
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer.cc#n...
> ui/gfx/compositor/layer.cc:109: animator->SetDelegate(this);
> Seems like you should allow NULL here to reset to default.
Good point. I now allow NULL which resets the animator (and the next time
GetAnimator is called, it will get the default animator).
> 
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animation_element.h (right):
> 
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.h:75: void
> SetTarget(LayerAnimationDelegate* delegate) const;
> This is an odd way to extract the target value. How about:
> 
>  struct TargetValue {
>     transform, opacity ...
>   };
> 
>  GetTargetValue(TargetValue* target_value);

My reasons for doing this way were that I liked that the only class that an
element knew how to interact with was a delegate, and a method that sets a
delegate to the target value felt like it might have future applicability for
things other than extracting the target values. Also, the TargetValue is so
close to the dummy delegate (which I'll need for blending) it seems like a shame
that both should exist. I could certainly change this, though -- please let me
know if I should go ahead and switch this.

Thanks,

Ian

[sky, 2011-10-20 21:54:56]

On Thu, Oct 20, 2011 at 2:24 PM,  <vollick@chromium.org> wrote:
> On 2011/10/20 20:30:30, sky wrote:
>
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
>>
>> File chrome/browser/ui/touch/animation/screen_rotation.cc (right):
>
>
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
>>
>> chrome/browser/ui/touch/animation/screen_rotation.cc:129: const gfx::Rect
>
> bounds
>>
>> = delegate_->GetBoundsForAnimation();
>> const gfx::Rect&
>
> Done.
>
>
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
>>
>> File chrome/browser/ui/touch/animation/screen_rotation_animator.h (right):
>
>
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
>>
>> chrome/browser/ui/touch/animation/screen_rotation_animator.h:9: #include
>> "ui/gfx/compositor/layer_animator.h"
>> can you forward declare LayerAnimator?
>
> Yes, done.
>
>
>
http://codereview.chromium.org/8362006/diff/2002/chrome/browser/ui/touch/anim...
>>
>> chrome/browser/ui/touch/animation/screen_rotation_animator.h:20: };
>> Add private: DISALLOW_IMPLICIT_CONSTRUCTORS
>
> Done.
>
>
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/dummy_laye...
>>
>> File ui/gfx/compositor/dummy_layer_animation_delegate.h (right):
>
>
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/dummy_laye...
>>
>> ui/gfx/compositor/dummy_layer_animation_delegate.h:17: class
>> COMPOSITOR_EXPORT
>> DummyLayerAnimationDelegate
>> Why are you renaming this? Test is a better name than Dummy.
>
> I realized that I was being shortsighted with this class. I need to make use
> of
> it in other places than in tests so the Test prefix felt wrong. I intend to
> use
> it for blending sequences. At time t, I'll Progress sequence a at t with a
> dummy
> delegate, do the same for b, and then interpolate between the dummies. It's
> also
> been useful as a throwaway target for animations (for collecting target
> values).
>
>>
>> http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer.cc
>> File ui/gfx/compositor/layer.cc (right):
>
>
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer.cc#n...
>>
>> ui/gfx/compositor/layer.cc:109: animator->SetDelegate(this);
>> Seems like you should allow NULL here to reset to default.
>
> Good point. I now allow NULL which resets the animator (and the next time
> GetAnimator is called, it will get the default animator).
>
>
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer_anim...
>>
>> File ui/gfx/compositor/layer_animation_element.h (right):
>
>
>
http://codereview.chromium.org/8362006/diff/2002/ui/gfx/compositor/layer_anim...
>>
>> ui/gfx/compositor/layer_animation_element.h:75: void
>> SetTarget(LayerAnimationDelegate* delegate) const;
>> This is an odd way to extract the target value. How about:
>
>>  struct TargetValue {
>>     transform, opacity ...
>>   };
>
>>  GetTargetValue(TargetValue* target_value);
>
> My reasons for doing this way were that I liked that the only class that an
> element knew how to interact with was a delegate, and a method that sets a
> delegate to the target value felt like it might have future applicability
> for
> things other than extracting the target values. Also, the TargetValue is so
> close to the dummy delegate (which I'll need for blending) it seems like a
> shame
> that both should exist. I could certainly change this, though -- please let
> me
> know if I should go ahead and switch this.

Go for clarity. Passing in struct with the possible values to set is
much clearer than taking a delegate with loads of methods on it.

  -Scott

[Ian Vollick, 2011-10-20 22:16:15]

> Go for clarity. Passing in struct with the possible values to set is
> much clearer than taking a delegate with loads of methods on it.
>

Done. 

Ian

[sky, 2011-10-20 23:31:34]

LGTM

http://codereview.chromium.org/8362006/diff/2004/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animation_element.h (right):

http://codereview.chromium.org/8362006/diff/2004/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animation_element.h:35: float opacity;
initialize opacity to 0.

[Ian Vollick, 2011-10-24 21:15:29]

Hi Scott,

I'm sorry to make more changes to this CL, but I needed to update aura to work
with the new API.

Noteworthy changes:
 - Screen rotation is much, much simpler and lives in ui/aura. It's just a layer
animation element that takes a (relative) number of degrees to turn.
 - ui/aura/desktop.cc now starts rotations with ALT+SHIFT+UP, ALT+SHIFT+RIGHT,
etc. Has to do a little bit of work to figure out how much to turn to end up in
the desired orientation.
 - ui/aura_shell/shell.cc SetAnimation is gone, so to support temporarily
changing the animation settings I created a LayerAnimator::TransientSettings
class that lets you change the animation settings, but restores them when it is
destructed.

Ian

On 2011/10/20 23:31:34, sky wrote:
> LGTM
> 
>
http://codereview.chromium.org/8362006/diff/2004/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animation_element.h (right):
> 
>
http://codereview.chromium.org/8362006/diff/2004/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animation_element.h:35: float opacity;
> initialize opacity to 0.

Done.

[sky, 2011-10-24 22:56:44]

Oy. I've gone numb to looking at this whole patch. Can I diff between
any of the patchsets to see the interesting changes?

  -Scott

On Mon, Oct 24, 2011 at 2:15 PM,  <vollick@chromium.org> wrote:
> Hi Scott,
>
> I'm sorry to make more changes to this CL, but I needed to update aura to
> work
> with the new API.
>
> Noteworthy changes:
>  - Screen rotation is much, much simpler and lives in ui/aura. It's just a
> layer
> animation element that takes a (relative) number of degrees to turn.
>  - ui/aura/desktop.cc now starts rotations with ALT+SHIFT+UP,
> ALT+SHIFT+RIGHT,
> etc. Has to do a little bit of work to figure out how much to turn to end up
> in
> the desired orientation.
>  - ui/aura_shell/shell.cc SetAnimation is gone, so to support temporarily
> changing the animation settings I created a LayerAnimator::TransientSettings
> class that lets you change the animation settings, but restores them when it
> is
> destructed.
>
> Ian
>
> On 2011/10/20 23:31:34, sky wrote:
>>
>> LGTM
>
>
>
http://codereview.chromium.org/8362006/diff/2004/ui/gfx/compositor/layer_anim...
>>
>> File ui/gfx/compositor/layer_animation_element.h (right):
>
>
>
http://codereview.chromium.org/8362006/diff/2004/ui/gfx/compositor/layer_anim...
>>
>> ui/gfx/compositor/layer_animation_element.h:35: float opacity;
>> initialize opacity to 0.
>
> Done.
>
>
>
> http://codereview.chromium.org/8362006/
>

[sky, 2011-10-25 00:26:59]

One question. With the current code on trunk I can start an animation for
bounds, and subsequent changes to other properties are immediate and don't
effect the bounds change. Is that still the case with this code?

  -Scott

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animator.h (right):

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:110: class TransientSettings {
This name is confusing. Maybe ScopedSettings or something. Be sure and add a
description too.

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:112: TransientSettings(LayerAnimator*
animator);
explicit

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:120: };
DISALLOW_COPY_AND_ASSIGN

[vollick1, 2011-10-25 02:02:01]

On Mon, Oct 24, 2011 at 8:26 PM, <sky@chromium.org> wrote:

> One question. With the current code on trunk I can start an animation for
> bounds, and subsequent changes to other properties are immediate and don't
> effect the bounds change. Is that still the case with this code?
>

Yes, that's still the case. Here are two examples (both of which assume
you're using the default animator that doesn't cause implicit animations).

// implicit animation
{
  ScopedSettings settings(layer()->GetAnimator()); // I will rename
TransientSettings -> ScopedSettings
  settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(250));
  layer()->SetBounds(new_bounds); // causes a bounds animation
}
layer()->SetTransform(new_transform); // immediately sets the new transform,
but the bounds still animate.

// explicit animation
layer()->GetAnimator()->ScheduleAnimation(new MyBoundsAnimation()); //
starts a bounds animation
layer()->SetTransform(new_transform); // immediately sets the new transform,
but the bounds still animate.

Ian

[sky, 2011-10-25 04:00:55]

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer.h
File ui/gfx/compositor/layer.h (right):

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer.h#ne...
ui/gfx/compositor/layer.h:90: void SetAnimator(LayerAnimator* animator);
document ownership.

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
File ui/gfx/compositor/layer_animator.h (right):

http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
ui/gfx/compositor/layer_animator.h:78: void
StartAnimation(LayerAnimationElement* animation);
Style guide doesn't like methods with the same name that take different
arguments (which is why View has SetBounds and SetBoundsRect). You should also
group the new methods together and say they are cover methods that create a
sequence for you.

[Ian Vollick, 2011-10-25 19:40:31]

On 2011/10/25 04:00:55, sky wrote:
> http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer.h
> File ui/gfx/compositor/layer.h (right):
> 
>
http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer.h#ne...
> ui/gfx/compositor/layer.h:90: void SetAnimator(LayerAnimator* animator);
> document ownership.
Done.
> 
>
http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
> File ui/gfx/compositor/layer_animator.h (right):
> 
>
http://codereview.chromium.org/8362006/diff/8001/ui/gfx/compositor/layer_anim...
> ui/gfx/compositor/layer_animator.h:78: void
> StartAnimation(LayerAnimationElement* animation);
> Style guide doesn't like methods with the same name that take different
> arguments (which is why View has SetBounds and SetBoundsRect). You should also
> group the new methods together and say they are cover methods that create a
> sequence for you.
Done.

[sky, 2011-10-25 20:10:27]

LGTM

[Ian Vollick, 2011-10-26 15:15:37]

Merge was a bit awkward as the old framework is still being used.
The only noteworthy changes are between the last two patch sets for the
following files:
 - ui/aura_shell/workspace.cc
 - ui/aura_shell/workspace_manager.cc
 - ui/aura/window.h
 - ui/aura/window.cc
I replaced aura::Window::ApplyDefaultAnimationSettings with a class
aura::Window::ScopedAnimationSettings. Creating the scoped settings and applying
the defaults used to be two steps, but now it's one. Just create an instance of
aura::Window::ScopedAnimationSettings.

[sky, 2011-10-26 15:39:15]

http://codereview.chromium.org/8362006/diff/20046/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/8362006/diff/20046/ui/aura/window.h#newcode260
ui/aura/window.h:260: class ScopedAnimationSettings : public
ui::LayerAnimator::ScopedSettings {
Could we make ui::LayerAnimator::ScopedSettings set the duration to 200ms and
remove this class entirely?

http://codereview.chromium.org/8362006/diff/20046/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.cc (right):

http://codereview.chromium.org/8362006/diff/20046/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:213: //TRACE_EVENT0("LayerAnimator",
"Step");
Remove if not needed.

[Ian Vollick, 2011-10-26 16:17:55]

On 2011/10/26 15:39:15, sky wrote:
> http://codereview.chromium.org/8362006/diff/20046/ui/aura/window.h
> File ui/aura/window.h (right):
> 
> http://codereview.chromium.org/8362006/diff/20046/ui/aura/window.h#newcode260
> ui/aura/window.h:260: class ScopedAnimationSettings : public
> ui::LayerAnimator::ScopedSettings {
> Could we make ui::LayerAnimator::ScopedSettings set the duration to 200ms and
> remove this class entirely?
Done. Changed the documentation for ui::LayerAnimator::ScopedSettings to mention
the new behavior.
> 
>
http://codereview.chromium.org/8362006/diff/20046/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animator.cc (right):
> 
>
http://codereview.chromium.org/8362006/diff/20046/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:213: //TRACE_EVENT0("LayerAnimator",
> "Step");
> Remove if not needed.
Done.

[sky, 2011-10-26 16:24:27]

LGTM

On Wed, Oct 26, 2011 at 9:17 AM,  <vollick@chromium.org> wrote:
> On 2011/10/26 15:39:15, sky wrote:
>>
>> http://codereview.chromium.org/8362006/diff/20046/ui/aura/window.h
>> File ui/aura/window.h (right):
>
>>
>> http://codereview.chromium.org/8362006/diff/20046/ui/aura/window.h#newcode260
>> ui/aura/window.h:260: class ScopedAnimationSettings : public
>> ui::LayerAnimator::ScopedSettings {
>> Could we make ui::LayerAnimator::ScopedSettings set the duration to 200ms
>> and
>> remove this class entirely?
>
> Done. Changed the documentation for ui::LayerAnimator::ScopedSettings to
> mention
> the new behavior.
>
>
>
http://codereview.chromium.org/8362006/diff/20046/ui/gfx/compositor/layer_ani...
>>
>> File ui/gfx/compositor/layer_animator.cc (right):
>
>
>
http://codereview.chromium.org/8362006/diff/20046/ui/gfx/compositor/layer_ani...
>>
>> ui/gfx/compositor/layer_animator.cc:213: //TRACE_EVENT0("LayerAnimator",
>> "Step");
>> Remove if not needed.
>
> Done.
>
>
>
> http://codereview.chromium.org/8362006/
>