M20 Ash: Animate window header transition from active to inactive

M20 Ash: Animate window header transition from active to inactive

BUG=116583
TEST=Manual

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

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

[James Cook, 2012-03-19 19:25:58]

A few comments.

Also, what changed in the art asset you updated?

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.cc#newc...
ash/wm/frame_painter.cc:64: const int kActivationCrossfadeDurationMS = 100;
I think the shadow animation uses 200 ms.  Could you check and make sure they
match?  Also, I think we usually use the suffix "Ms" instead of "MS".

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.cc#newc...
ash/wm/frame_painter.cc:69: SkPaint paint,
Are these cheap to copy?  If not, pass a pointer?

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.cc#newc...
ash/wm/frame_painter.cc:236: crossfade_animation_->Reset();
Does the animation have to be reset if it was just created?

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.h
File ash/wm/frame_painter.h (right):

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.h#newco...
ash/wm/frame_painter.h:40: ~FramePainter();
must be virtual, clang will complain

[pkotwicz, 2012-03-20 00:59:46]

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.cc#newc...
ash/wm/frame_painter.cc:64: const int kActivationCrossfadeDurationMS = 100;
On 2012/03/19 19:25:58, James Cook (Chromium) wrote:
> I think the shadow animation uses 200 ms.  Could you check and make sure they
> match?  Also, I think we usually use the suffix "Ms" instead of "MS".

Done.

http://codereview.chromium.org/9720043/diff/3001/ash/wm/frame_painter.cc#newc...
ash/wm/frame_painter.cc:236: crossfade_animation_->Reset();
On 2012/03/19 19:25:58, James Cook (Chromium) wrote:
> Does the animation have to be reset if it was just created?

Done.

[James Cook, 2012-03-20 03:37:03]

LGTM assuming the compile errors are fixed and it runs OK.

http://codereview.chromium.org/9720043/diff/8002/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

http://codereview.chromium.org/9720043/diff/8002/ash/wm/frame_painter.cc#newc...
ash/wm/frame_painter.cc:86: paint.setShader(shader);
paint-> ?

http://codereview.chromium.org/9720043/diff/8002/ash/wm/frame_painter.cc#newc...
ash/wm/frame_painter.cc:90: canvas->sk_canvas()->drawPath(path, paint);
*paint ?

[Ben Goodger (Google), 2012-03-20 15:41:01]

LGTM.

you may want to test this on an alex too at some point to see what the perf is
like.

[pkotwicz, 2012-03-26 23:44:19]

Tested on Alex and the performance seems reasonable

[commit-bot: I haz the power, 2012-03-26 23:46:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/9720043/12001

[commit-bot: I haz the power, 2012-03-27 02:01:15]

Try job failure for 9720043-12001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-27 14:31:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/9720043/12001

[commit-bot: I haz the power, 2012-03-27 18:07:14]

Change committed as 129220