Implemented screen rotation animation experiment.

Implemented screen rotation animation experiment.

This patch implements 2 different animations for when the  screen orientation changes.  These animations are still experimental and will be disabled by default.

Depends on: https://codereview.chromium.org/983933002/

BUG=337596
Committed: https://crrev.com/37f9cad0a5a945bda2d4ebfe2217db7711572718
Cr-Commit-Position: refs/heads/master@{#320391}

[bruthig, 2015-03-03 22:24:18]

bruthig@chromium.org changed reviewers:
+ mukai@chromium.org, oshima@chromium.org

[bruthig, 2015-03-03 22:24:19]

Can you please review this patch for me?

mukai@chromium.org: Please review changes in
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc

oshima@chromium.org: Please review changes in everything except
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc

[Jun Mukai, 2015-03-03 23:36:42]

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:168: bool should_scale) {
Indent seems wrong. Run git cl format.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:172: const gfx::Rect
original_screen_bounds_f = root_window->GetTargetBounds();
What does the _f prefix mean?  Did you intend to use RectF?

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:224:
screen_rotation->set_target_opacity(1.0f);
Why these values aren't in constructor params?

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:231:
animator->StartAnimation(animation_sequence.release());
Could we use ScopedLayerAnimationSettings instead of using LayerAnimator
directly?  If it's just applying a transform, I think that's better.

With ScopedLayerAnimationSettings, ui/compositor/closure_animation_observer.h
can care about the handling events for animation ends and reduce your layer
observer code.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:304: "partial-rotation" == switch_value
? 250 : 2500),
You should declare constants for those values, like
const int kRotationDurationInMs = 250;
const int kRotationDurationSlowInMs = 2500;
so that you can share the values below.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:306: Is180DegreeFlip(current_rotation,
new_rotation) ? 160 : 70,
What do these 160/70 mean?

[bruthig, 2015-03-04 15:31:36]

I've addressed mukai@'s comments from patch set 1.  Can you guys have another
look?

mukai@chromium.org: Please review changes in
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc

oshima@chromium.org: Please review changes in everything except
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:168: bool should_scale) {
On 2015/03/03 23:36:42, Jun Mukai wrote:
> Indent seems wrong. Run git cl format.

Done.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:172: const gfx::Rect
original_screen_bounds_f = root_window->GetTargetBounds();
On 2015/03/03 23:36:41, Jun Mukai wrote:
> What does the _f prefix mean?  Did you intend to use RectF?

Ya.  I just made both *_screen_bounds variables RectF's and dropped the _f
suffix.

Done.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:224:
screen_rotation->set_target_opacity(1.0f);
On 2015/03/03 23:36:41, Jun Mukai wrote:
> Why these values aren't in constructor params?

I thought the default values for them were reasonable and I thought reducing the
number of parameters in the constructor simplified it for re-use.  Also these
values are not necessary during construction.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:231:
animator->StartAnimation(animation_sequence.release());
On 2015/03/03 23:36:41, Jun Mukai wrote:
> Could we use ScopedLayerAnimationSettings instead of using LayerAnimator
> directly?  If it's just applying a transform, I think that's better.
> 
> With ScopedLayerAnimationSettings, ui/compositor/closure_animation_observer.h
> can care about the handling events for animation ends and reduce your layer
> observer code.

No I don't think it's possible to use the ScopedLayerAnimationSettings directly
because we are applying a compound interpolated transform and we can't achieve
the rotation pivot without it.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:304: "partial-rotation" == switch_value
? 250 : 2500),
On 2015/03/03 23:36:41, Jun Mukai wrote:
> You should declare constants for those values, like
> const int kRotationDurationInMs = 250;
> const int kRotationDurationSlowInMs = 2500;
> so that you can share the values below.

Done.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:306: Is180DegreeFlip(current_rotation,
new_rotation) ? 160 : 70,
On 2015/03/03 23:36:41, Jun Mukai wrote:
> What do these 160/70 mean?

I attempted to clarify by adding the kPartialRotationDegrees constant.

Done.

[Jun Mukai, 2015-03-04 19:46:20]

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:172: const gfx::Rect
original_screen_bounds_f = root_window->GetTargetBounds();
On 2015/03/04 15:31:36, bruthig wrote:
> On 2015/03/03 23:36:41, Jun Mukai wrote:
> > What does the _f prefix mean?  Did you intend to use RectF?
> 
> Ya.  I just made both *_screen_bounds variables RectF's and dropped the _f
> suffix.
> 
> Done.

Ah, actually I'm okay with _f prefix itself, I wondered because this is just a
Rect but has _f prefix.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:224:
screen_rotation->set_target_opacity(1.0f);
On 2015/03/04 15:31:36, bruthig wrote:
> On 2015/03/03 23:36:41, Jun Mukai wrote:
> > Why these values aren't in constructor params?
> 
> I thought the default values for them were reasonable and I thought reducing
the
> number of parameters in the constructor simplified it for re-use.  Also these
> values are not necessary during construction.

I still don't see the reason why these two fields are different. I feel like
tween type and target opacity are just animation parameters, they are same as
end-degree or pivot or duration or something else.
Also anyways these parameters come from the arguments of this RotateScreen
function, they don't complicate the interface.

So if it's better to separate these two makes sense, consider to pass the layer
only as the constructor parameter and set other parameters as optional.

If those animation parameters should be constructor params, consider passing
tween type and target opacity as the constructor params as well. The parameters
are already very complicated, adding two more doesn't have much differences.

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:231:
animator->StartAnimation(animation_sequence.release());
On 2015/03/04 15:31:36, bruthig wrote:
> On 2015/03/03 23:36:41, Jun Mukai wrote:
> > Could we use ScopedLayerAnimationSettings instead of using LayerAnimator
> > directly?  If it's just applying a transform, I think that's better.
> > 
> > With ScopedLayerAnimationSettings,
ui/compositor/closure_animation_observer.h
> > can care about the handling events for animation ends and reduce your layer
> > observer code.
> 
> No I don't think it's possible to use the ScopedLayerAnimationSettings
directly
> because we are applying a compound interpolated transform and we can't achieve
> the rotation pivot without it.

got it. Sorry for my poor comment.

[oshima, 2015-03-04 21:31:35]

can you update the CL description and explain what kind of rotate animation this
CL implements?

https://codereview.chromium.org/975943002/diff/20001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/975943002/diff/20001/ash/ash_switches.cc#newc...
ash/ash_switches.cc:60: "ash-enable-screen-rotation_animation";
nit: ash-screen-rotation-animation (-type, or something like that with no
-enable)

enable implies boolean flag.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animation.cc (right):

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animation.cc:53: bool
ScreenRotationAnimation::OnProgress(double t,
current? I see all other places uses t, but they should probably be updated.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:66: scoped_ptr<ui::LayerTreeOwner>
CloneWindowLayers(aura::Window* window) {
Can you use wm::RecreateLayers in ui/wm/core/window_util.h?

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:183: const gfx::RectF
original_screen_bounds = root_window->GetTargetBounds();
What happens if the root window is destroyed while animating?

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:298: if ("" == switch_value) {
switch_value.empty()

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:305: "partial-rotation-slow" ==
switch_value) {
can you define const with the comments that explains how they differ?

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:313: : 90 - kPartialRotationDegrees,
can you move this out of constructor to make this easier to read?

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:320: : kRotationDurationSlowInMs),
ditto

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:325: DCHECK(false);
NOTREACHED()

[bruthig, 2015-03-06 16:00:33]

mukai@, oshima@ can you please have another look?

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/1/ash/rotator/screen_rotation_...
ash/rotator/screen_rotation_animator.cc:224:
screen_rotation->set_target_opacity(1.0f);
On 2015/03/04 19:46:20, Jun Mukai wrote:
> On 2015/03/04 15:31:36, bruthig wrote:
> > On 2015/03/03 23:36:41, Jun Mukai wrote:
> > > Why these values aren't in constructor params?
> > 
> > I thought the default values for them were reasonable and I thought reducing
> the
> > number of parameters in the constructor simplified it for re-use.  Also
these
> > values are not necessary during construction.
> 
> I still don't see the reason why these two fields are different. I feel like
> tween type and target opacity are just animation parameters, they are same as
> end-degree or pivot or duration or something else.
> Also anyways these parameters come from the arguments of this RotateScreen
> function, they don't complicate the interface.
> 
> So if it's better to separate these two makes sense, consider to pass the
layer
> only as the constructor parameter and set other parameters as optional.
> 
> If those animation parameters should be constructor params, consider passing
> tween type and target opacity as the constructor params as well. The
parameters
> are already very complicated, adding two more doesn't have much differences.

Done.

https://codereview.chromium.org/975943002/diff/20001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/975943002/diff/20001/ash/ash_switches.cc#newc...
ash/ash_switches.cc:60: "ash-enable-screen-rotation_animation";
On 2015/03/04 21:31:35, oshima wrote:
> nit: ash-screen-rotation-animation (-type, or something like that with no
> -enable)
> 
> enable implies boolean flag.

Done.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animation.cc (right):

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animation.cc:53: bool
ScreenRotationAnimation::OnProgress(double t,
On 2015/03/04 21:31:35, oshima wrote:
> current? I see all other places uses t, but they should probably be updated.

Done.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:66: scoped_ptr<ui::LayerTreeOwner>
CloneWindowLayers(aura::Window* window) {
On 2015/03/04 21:31:35, oshima wrote:
> Can you use wm::RecreateLayers in ui/wm/core/window_util.h?

Not with the current implementation of wm::RecreateLayers.  However I have a fix
in review for it here: https://codereview.chromium.org/983933002/
I have included the fix in this review but will not submit until the other CL
lands.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:183: const gfx::RectF
original_screen_bounds = root_window->GetTargetBounds();
On 2015/03/04 21:31:35, oshima wrote:
> What happens if the root window is destroyed while animating?

As best I can tell, the RootWindow will complete all animations on it's layer
which will recursively complete all animations on child layers.  This should
cause the LayerCleanupObserver to cleanup before anything else is destroyed.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:298: if ("" == switch_value) {
On 2015/03/04 21:31:35, oshima wrote:
> switch_value.empty()

Done.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:305: "partial-rotation-slow" ==
switch_value) {
On 2015/03/04 21:31:35, oshima wrote:
> can you define const with the comments that explains how they differ?

Done.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:313: : 90 - kPartialRotationDegrees,
On 2015/03/04 21:31:35, oshima wrote:
> can you move this out of constructor to make this easier to read?

Done.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:320: : kRotationDurationSlowInMs),
On 2015/03/04 21:31:35, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:325: DCHECK(false);
On 2015/03/04 21:31:35, oshima wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/975943002/diff/40001/ui/compositor/layer_owne...
File ui/compositor/layer_owner.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ui/compositor/layer_owne...
ui/compositor/layer_owner.cc:75: 
This file will NOT be submitted with this CL.  It is already in flight here:
https://codereview.chromium.org/983933002/ and is required by this CL.

[oshima, 2015-03-06 16:13:45]

On 2015/03/04 21:31:35, oshima wrote:
> can you update the CL description and explain what kind of rotate animation
this
> CL implements?

You missed this comment.

> 
> https://codereview.chromium.org/975943002/diff/20001/ash/ash_switches.cc
> File ash/ash_switches.cc (right):
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/ash_switches.cc#newc...
> ash/ash_switches.cc:60: "ash-enable-screen-rotation_animation";
> nit: ash-screen-rotation-animation (-type, or something like that with no
> -enable)
> 
> enable implies boolean flag.
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> File ash/rotator/screen_rotation_animation.cc (right):
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animation.cc:53: bool
> ScreenRotationAnimation::OnProgress(double t,
> current? I see all other places uses t, but they should probably be updated.
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> File ash/rotator/screen_rotation_animator.cc (right):
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animator.cc:66: scoped_ptr<ui::LayerTreeOwner>
> CloneWindowLayers(aura::Window* window) {
> Can you use wm::RecreateLayers in ui/wm/core/window_util.h?
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animator.cc:183: const gfx::RectF
> original_screen_bounds = root_window->GetTargetBounds();
> What happens if the root window is destroyed while animating?
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animator.cc:298: if ("" == switch_value) {
> switch_value.empty()
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animator.cc:305: "partial-rotation-slow" ==
> switch_value) {
> can you define const with the comments that explains how they differ?
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animator.cc:313: : 90 - kPartialRotationDegrees,
> can you move this out of constructor to make this easier to read?
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animator.cc:320: : kRotationDurationSlowInMs),
> ditto
> 
>
https://codereview.chromium.org/975943002/diff/20001/ash/rotator/screen_rotat...
> ash/rotator/screen_rotation_animator.cc:325: DCHECK(false);
> NOTREACHED()

[bruthig, 2015-03-06 16:28:56]

Opps sorry oshima@, I did miss that one comment.  I've updated it now, hopefully
that is what you were looking for.

[oshima, 2015-03-06 20:59:33]

can you add a bit more explanation about the animations this implement. Or you
can describe them in the bug and just refer to it.

https://codereview.chromium.org/975943002/diff/40001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ash/ash_switches.cc#newc...
ash/ash_switches.cc:58: // Specifies the screen rotation animation to use.
nit: Can you list the possible values with the pointer to the doc in
screen_rotation_animator.cc ?

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:154: DCHECK(sequence == sequence_);
nit: DCHECK_EQ

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:211: // rendered.
instead, can't we simply apply the transform to the root, but apply the
transform that cancel it to the old layer?

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:319: gfx::Tween::FAST_OUT_LINEAR_IN,
true /* should_scale */);
showing stretched image sounds a bit weird.  Didn't "full rotation, no scale"
work well?

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.h (right):

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.h:27: gfx::Display display_;
looks like it's using display_id only. Can you just use id and nuke another
ctor?

[bruthig, 2015-03-09 21:04:27]

oshima@, can you have another look?

https://codereview.chromium.org/975943002/diff/40001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ash/ash_switches.cc#newc...
ash/ash_switches.cc:58: // Specifies the screen rotation animation to use.
On 2015/03/06 20:59:32, oshima wrote:
> nit: Can you list the possible values with the pointer to the doc in
> screen_rotation_animator.cc ?

Done.

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:154: DCHECK(sequence == sequence_);
On 2015/03/06 20:59:32, oshima wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:211: // rendered.
On 2015/03/06 20:59:32, oshima wrote:
> instead, can't we simply apply the transform to the root, but apply the
> transform that cancel it to the old layer?

I'm hesitant to implement it like that for a couple of reasons.

1) The benefit is unclear to me.

2) To implement it as suggested the old_layer animation sequence would have to
apply the inverse transform of the interpolated transform from the root's
animation sequence at each call to ScreenRotationAnimation::OnProgress(...).  I
feel like from a code maintenance and understand-ability perspective this is not
a win.

3) This would couple the two animations and would make it impossible to animate
the opacity of the new and old layer trees independently.

4) I consider having to add the old_layer as a child (instead of a sibling) is a
work-around in the first place.  I wish there was, or I knew how, to get the
old_layer to render without having to add it as a child.

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:319: gfx::Tween::FAST_OUT_LINEAR_IN,
true /* should_scale */);
On 2015/03/06 20:59:32, oshima wrote:
> showing stretched image sounds a bit weird.  Didn't "full rotation, no scale"
> work well?

I showed that implementation to some developers close by and the general feeling
was it didn't look all that great because of the large black area's while the
old layer faded out.

The two implementations captured in this review are ones specifically asked for
by designers.

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.h (right):

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.h:27: gfx::Display display_;
On 2015/03/06 20:59:32, oshima wrote:
> looks like it's using display_id only. Can you just use id and nuke another
> ctor?

Done.

[oshima, 2015-03-10 22:20:38]

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:211: // rendered.
On 2015/03/09 21:04:27, bruthig wrote:
> On 2015/03/06 20:59:32, oshima wrote:
> > instead, can't we simply apply the transform to the root, but apply the
> > transform that cancel it to the old layer?
> 
> I'm hesitant to implement it like that for a couple of reasons.
> 
> 1) The benefit is unclear to me.
> 
> 2) To implement it as suggested the old_layer animation sequence would have to
> apply the inverse transform of the interpolated transform from the root's
> animation sequence at each call to ScreenRotationAnimation::OnProgress(...). 
I
> feel like from a code maintenance and understand-ability perspective this is
not
> a win.
> 
> 3) This would couple the two animations and would make it impossible to
animate
> the opacity of the new and old layer trees independently.
> 
> 4) I consider having to add the old_layer as a child (instead of a sibling) is
a
> work-around in the first place.  I wish there was, or I knew how, to get the
> old_layer to render without having to add it as a child.

We can actually have a layer with NOT_DRAWN under root window, and move children
there.
This way, you can just apply transform to one layer.

It's a cheaper and simpler. If you still don't like it, I'm fine as is.

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:215: continue;
can you add old layer after animations are applied?

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:319: gfx::Tween::FAST_OUT_LINEAR_IN,
true /* should_scale */);
On 2015/03/09 21:04:27, bruthig wrote:
> On 2015/03/06 20:59:32, oshima wrote:
> > showing stretched image sounds a bit weird.  Didn't "full rotation, no
scale"
> > work well?
> 
> I showed that implementation to some developers close by and the general
feeling
> was it didn't look all that great because of the large black area's while the
> old layer faded out.
> 
> The two implementations captured in this review are ones specifically asked
for
> by designers.


I tried it on my desktop and showing stretched image didn't look great either.
I think we just need try it on the field though.

[bruthig, 2015-03-11 14:40:07]

I've added the TODO to track the layer copying improvement.
oshima@ can you please have a look?

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:211: // rendered.
On 2015/03/10 22:20:38, oshima wrote:
> On 2015/03/09 21:04:27, bruthig wrote:
> > On 2015/03/06 20:59:32, oshima wrote:
> > > instead, can't we simply apply the transform to the root, but apply the
> > > transform that cancel it to the old layer?
> > 
> > I'm hesitant to implement it like that for a couple of reasons.
> > 
> > 1) The benefit is unclear to me.
> > 
> > 2) To implement it as suggested the old_layer animation sequence would have
to
> > apply the inverse transform of the interpolated transform from the root's
> > animation sequence at each call to ScreenRotationAnimation::OnProgress(...).

> I
> > feel like from a code maintenance and understand-ability perspective this is
> not
> > a win.
> > 
> > 3) This would couple the two animations and would make it impossible to
> animate
> > the opacity of the new and old layer trees independently.
> > 
> > 4) I consider having to add the old_layer as a child (instead of a sibling)
is
> a
> > work-around in the first place.  I wish there was, or I knew how, to get the
> > old_layer to render without having to add it as a child.
> 
> We can actually have a layer with NOT_DRAWN under root window, and move
children
> there.
> This way, you can just apply transform to one layer.
> 
> It's a cheaper and simpler. If you still don't like it, I'm fine as is.

Added a TODO to implement this.

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:215: continue;
On 2015/03/10 22:20:38, oshima wrote:
> can you add old layer after animations are applied?

Am I correct to assume that this was an alternative suggestion to the NOT_DRAWN
layer above?

[oshima, 2015-03-11 22:43:43]

lgtm

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/975943002/diff/40001/ash/rotator/screen_rotat...
ash/rotator/screen_rotation_animator.cc:215: continue;
On 2015/03/11 14:40:07, bruthig wrote:
> On 2015/03/10 22:20:38, oshima wrote:
> > can you add old layer after animations are applied?
> 
> Am I correct to assume that this was an alternative suggestion to the
NOT_DRAWN
> layer above?

Yes. If you're going to change, you may leave it as is.

[bruthig, 2015-03-12 13:48:35]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2015-03-12 13:48:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/975943002/120001

[commit-bot: I haz the power, 2015-03-12 14:10:46]

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

[commit-bot: I haz the power, 2015-03-12 14:10:49]

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

[bruthig, 2015-03-12 18:28:29]

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

[bruthig, 2015-03-12 18:28:30]

mpearson@chromium.org: Please review changes in
- tools/metrics/histograms/histograms.xml

mukai@chromium.org: Please review changes in
- chrome/browser/ui/webui/options/chromeos/display_options_handler.cc

[Mark P, 2015-03-12 20:07:51]

histograms.xml lgtm

[Jun Mukai, 2015-03-12 21:06:42]

display_options_handler LGTM

[bruthig, 2015-03-12 21:16:02]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-03-12 21:16:02]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/975943002/#ps140001
(title: "Added ash-screen-rotation-animation entry to histograms.xml.")

[commit-bot: I haz the power, 2015-03-12 21:16:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/975943002/140001

[commit-bot: I haz the power, 2015-03-12 22:29:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-12 22:29:34]

Patchset 8 (id:??) landed as
https://crrev.com/37f9cad0a5a945bda2d4ebfe2217db7711572718
Cr-Commit-Position: refs/heads/master@{#320391}