Gradient overlay for constrained window

Gradient overlay for constrained window

As a part of the new constrained window look and feel we're changing the the solid gray overlay to a gradient overlay. The overlay fades in on show and fades out on hide.

Screenshots:
    light background: http://i.imgur.com/GJj6L.png
    dark background: http://i.imgur.com/s37da.png

BUG=140520
TEST=Run with --enable-frameless-constrained-dialogs.
  Navigate to http://www.imagemator.com/
  Click "Click to pick an image."

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

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

[jamesr, 2012-08-14 19:08:24]

Seems mostly reasonable - you've almost got me convinced to hook up a better
animation system for WebPageOverlays.

I'll let Nico do the full review - he has more OWNERS powers than I do anyway.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay.cc (right):

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:80: shader->unref();
Could you use SkAutoTUnref<> here instead of manual derefing?

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:103: return end_time -
base::Time::Now();
we drive most web-based animations of of base::TimeTicks for better
precision+accuracy (for instance, if the user adjusts the system clock).

[Nico, 2012-08-14 21:14:12]

Looking pretty good!

I'm not sure if LinearAnimation actually helps, but it was written for more or
less this exact use case, so take a look at least.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay.cc (right):

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:35: } else {
DCHECK(state_ == HIDDEN);

Alternatively, you could switch(state_), then the compiler makes sure you handle
all cases (assuming you don't add a default: branch)

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:42: timer_.Start(FROM_HERE,
base::TimeDelta::FromMilliseconds(16), this,
constant

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:68: int alpha =
GetAnimationProgress() * 191;
This looks like a linear animation. Could ui/base/animation/linear_animation.h
make this class simpler?

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay.h (right):

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
I know, it's crazy how time flies when you're having a good time. But it's 2012
already.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.h:8: #include "base/basictypes.h"
not needed

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.h:10: #include
"base/memory/scoped_ptr.h"
not needed

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay_unittest.cc (right):

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay_unittest.cc:32: }
If it's not too difficult, a test that starts the animation twice in various
states and then checks that only one overlay is on the renderer would be good

[sail, 2012-08-15 05:26:19]

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay.cc (right):

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:35: } else {
On 2012/08/14 21:14:12, Nico wrote:
> DCHECK(state_ == HIDDEN);
> 
> Alternatively, you could switch(state_), then the compiler makes sure you
handle
> all cases (assuming you don't add a default: branch)

Done.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:42: timer_.Start(FROM_HERE,
base::TimeDelta::FromMilliseconds(16), this,
On 2012/08/14 21:14:12, Nico wrote:
> constant

Done.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:68: int alpha =
GetAnimationProgress() * 191;
On 2012/08/14 21:14:12, Nico wrote:
> This looks like a linear animation. Could ui/base/animation/linear_animation.h
> make this class simpler?

Done.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:80: shader->unref();
On 2012/08/14 19:08:24, jamesr wrote:
> Could you use SkAutoTUnref<> here instead of manual derefing?

Done.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:103: return end_time -
base::Time::Now();
On 2012/08/14 19:08:24, jamesr wrote:
> we drive most web-based animations of of base::TimeTicks for better
> precision+accuracy (for instance, if the user adjusts the system clock).

Done.
Switched to ui/base/animation which already uses base::TimeTicks.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay.h (right):

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/08/14 21:14:12, Nico wrote:
> I know, it's crazy how time flies when you're having a good time. But it's
2012
> already.

Done.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.h:8: #include "base/basictypes.h"
On 2012/08/14 21:14:12, Nico wrote:
> not needed

Done.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.h:10: #include
"base/memory/scoped_ptr.h"
On 2012/08/14 21:14:12, Nico wrote:
> not needed

Done.

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay_unittest.cc (right):

https://chromiumcodereview.appspot.com/10855151/diff/4001/chrome/renderer/web...
chrome/renderer/webview_animating_overlay_unittest.cc:32: }
On 2012/08/14 21:14:12, Nico wrote:
> If it's not too difficult, a test that starts the animation twice in various
> states and then checks that only one overlay is on the renderer would be good

Eek. This will require a WebKit patch. If you still want me to do this would it
be ok to it as a separate two part CL?

[Nico, 2012-08-15 18:26:14]

lgtm

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay.cc (right):

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:17: const int
kAnimationDurationMiliseconds = 5000;
5 seconds is surprisingly long

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:40: DCHECK(state_ == HIDDEN);
DCHECK_EQ

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:88: DCHECK(state_ ==
ANIMATING_OUT);
dcheck_eq

[sail, 2012-08-15 18:34:35]

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
File chrome/renderer/webview_animating_overlay.cc (right):

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:17: const int
kAnimationDurationMiliseconds = 5000;
On 2012/08/15 18:26:14, Nico wrote:
> 5 seconds is surprisingly long

Done.
Oops, debug code. Changed back to 500.

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:40: DCHECK(state_ == HIDDEN);
On 2012/08/15 18:26:14, Nico wrote:
> DCHECK_EQ

Done.

https://chromiumcodereview.appspot.com/10855151/diff/8003/chrome/renderer/web...
chrome/renderer/webview_animating_overlay.cc:88: DCHECK(state_ ==
ANIMATING_OUT);
On 2012/08/15 18:26:14, Nico wrote:
> dcheck_eq 

Done.

[sail, 2012-08-15 18:37:09]

joi: content/public/ OWNERS review
jamesr: content/render/ OWNERS review
ben: ui/base/animation OWNERS review

[jamesr, 2012-08-15 18:54:33]

content/renderer lgtm

[Jói, 2012-08-15 19:08:38]

LGTM for content/public.

[sail, 2012-08-15 22:41:16]

+sky for ui/base/animation OWNERS review

[sky, 2012-08-16 00:27:48]

LGTM

[commit-bot: I haz the power, 2012-08-16 00:32:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10855151/7004

[commit-bot: I haz the power, 2012-08-16 01:21:06]

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

[commit-bot: I haz the power, 2012-08-16 03:32:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10855151/9004

[commit-bot: I haz the power, 2012-08-16 05:45:56]

Try job failure for 10855151-9004 (retry) (retry) on linux_rel for steps
"interactive_ui_tests, unit_tests".
It's a second try, previously, steps "interactive_ui_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-16 14:56:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10855151/9004

[commit-bot: I haz the power, 2012-08-16 15:59:13]

Try job failure for 10855151-9004 (retry) on linux_rel for steps
"interactive_ui_tests, unit_tests".
It's a second try, previously, steps "interactive_ui_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-16 16:14:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10855151/9004

[commit-bot: I haz the power, 2012-08-16 17:52:42]

Try job failure for 10855151-9004 (retry) (retry) on linux_rel for step
"unit_tests".
It's a second try, previously, steps "interactive_ui_tests,
content_browsertests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-16 19:40:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10855151/9004

[commit-bot: I haz the power, 2012-08-16 21:19:25]

Try job failure for 10855151-9004 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-16 23:23:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10855151/9004

[commit-bot: I haz the power, 2012-08-17 00:58:44]

Try job failure for 10855151-9004 (retry) on linux_rel for steps
"interactive_ui_tests, unit_tests".
It's a second try, previously, steps "interactive_ui_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-17 05:19:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10855151/11026

[commit-bot: I haz the power, 2012-08-17 06:51:10]

Change committed as 152059