(Views only) Add a gradient background to the tabstrip of the view tabbed pane implementation.

(Views only) Add a gradient background to the tabstrip of the view tabbed pane implementation.

BUG=138063
TEST=none


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

[markusheintz_, 2012-08-08 12:28:46]

Hey Ben could you please review this CL? Thanks a lot. I also uploaded before
and after screenshots (http://crbug.com/138063)

[msw, 2012-08-08 17:12:20]

Driveby!

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background.cc
File ui/views/background.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background...
ui/views/background.cc:118: SkColor color1, SkColor color2, float start, float
end) {
nit: one param per line

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background.h
File ui/views/background.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background...
ui/views/background.h:64: // the gradient. Both values are specified as
percentage.
nit: consider "Both |start| and |end| range from 0 (top) to 1 (bottom)."

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background...
ui/views/background.h:66: SkColor color1, SkColor color2, float start, float
end);
nit: one param per line

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/controls/t...
File ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/controls/t...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:129: const float
gradient_start = 0.7f;
nit: please compare snapshots or get values from UX, the gradient looks
smaller/lighter than the spec at first glance.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/controls/t...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:178:
OnPaintBackground(canvas);
Perhaps just call View::OnPaint here (or above the SkPaint decl)?

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc
File ui/views/painter.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:23: GradientPainter(bool horizontal, SkColor top, SkColor
bottom, double start,
nit: one param per line

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:64: // The start of the gradient specified as percentage of
the total size of the
nit: s/percentage/ratio/, ditto below

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:66: double start_;
Consider using SkScalar/floats here, instead of doubles (for use in SkPoints).

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:189: SkColor c1, SkColor c2, float start, float end) {
nit: one param per line

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.h
File ui/views/painter.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.h#...
ui/views/painter.h:37: static Painter* CreateVerticalGradientRange(SkColor c1,
SkColor c2,
nit: one param per line

[msw, 2012-08-08 18:29:50]

http://codereview.chromium.org/10823229/diff/4001/ui/views/controls/tabbed_pa...
File ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc (right):

http://codereview.chromium.org/10823229/diff/4001/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:129: const float
gradient_start = 0.7f;
nit: i think this needs const naming kGradientStart; same for end.

[markusheintz_, 2012-08-08 21:53:55]

I've changed the CL to support arbitrary multi color gradients. The colors I use
are the exact same colors of the CROS webui tabs.

You want to have a particular look at the painter where I use plain arrays
instead of smart pointers. Initially I wanted to use vectors, but then I decided
to use arrays similar to the skia code.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background.cc
File ui/views/background.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background...
ui/views/background.cc:118: SkColor color1, SkColor color2, float start, float
end) {
On 2012/08/08 17:12:20, msw wrote:
> nit: one param per line

Done.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background.h
File ui/views/background.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background...
ui/views/background.h:64: // the gradient. Both values are specified as
percentage.
On 2012/08/08 17:12:20, msw wrote:
> nit: consider "Both |start| and |end| range from 0 (top) to 1 (bottom)."
obsolete

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/background...
ui/views/background.h:66: SkColor color1, SkColor color2, float start, float
end);
On 2012/08/08 17:12:20, msw wrote:
> nit: one param per line

Done.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/controls/t...
File ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/controls/t...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:129: const float
gradient_start = 0.7f;
On 2012/08/08 17:12:20, msw wrote:
> nit: please compare snapshots or get values from UX, the gradient looks
> smaller/lighter than the spec at first glance.

It's a bit tricky because the shadows around the tab are still missing. 

The reference values are the once that are used on ChromeOS. It's easy to
observe them since this is webUI und you can just use the dev tools.
The values  are 0%    #ff ff ff  
                        60%  #ff ff ff 
                        80%  #fa fa fa
                      100%  #f2 f2 f2

These values are not the same as in the mocks.

If changed this now to make it use the same values as on CROS, These are not the
same values as in the mocks.

I will bring this UI in the final UI review before launch.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/controls/t...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:129: const float
gradient_start = 0.7f;
On 2012/08/08 18:29:50, msw wrote:
> nit: i think this needs const naming kGradientStart; same for end.
obsolete.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/controls/t...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:178:
OnPaintBackground(canvas);
On 2012/08/08 17:12:20, msw wrote:
> Perhaps just call View::OnPaint here (or above the SkPaint decl)?
Moved above the SkPaint.

I deliberately didn't call the View::OnPaint as I only want to draw the
Background.

What I also could do (and already did in a prototyp) is get rid of the custom
OnPaint method and use a custom Border impl.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc
File ui/views/painter.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:23: GradientPainter(bool horizontal, SkColor top, SkColor
bottom, double start,
On 2012/08/08 17:12:20, msw wrote:
> nit: one param per line

Done.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:64: // The start of the gradient specified as percentage of
the total size of the
On 2012/08/08 17:12:20, msw wrote:
> nit: s/percentage/ratio/, ditto below
obsolete

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:66: double start_;
On 2012/08/08 17:12:20, msw wrote:
> Consider using SkScalar/floats here, instead of doubles (for use in SkPoints).

Done.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.cc...
ui/views/painter.cc:189: SkColor c1, SkColor c2, float start, float end) {
On 2012/08/08 17:12:20, msw wrote:
> nit: one param per line

Done.

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.h
File ui/views/painter.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/4001/ui/views/painter.h#...
ui/views/painter.h:37: static Painter* CreateVerticalGradientRange(SkColor c1,
SkColor c2,
On 2012/08/08 17:12:20, msw wrote:
> nit: one param per line

Done.

[msw, 2012-08-08 23:13:53]

more nits :)

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/background.h
File ui/views/background.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/backgroun...
ui/views/background.h:62: // Creates a background that containa a vertical
gradient. The gradient can
nit: contains

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/backgroun...
ui/views/background.h:69: int count);
nit: use size_t?

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/controls/...
File ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/controls/...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:128: SkColor colors[]
= {
nit: mention where these values were derived from?

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/controls/...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:182: // Draw the
TabStrip border.
nit: blank line above (separate bg / border).

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.cc
File ui/views/painter.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:26: int count)
nit: ditto for size_t

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:27: : horizontal_(horizontal), count_(count) {
nit: I think if the params and initializer list don't fit on one line together,
the initializer list should also be one init per line.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:31: for (int i = 0; i < count_; ++i) {
nit: ditto for size_t

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:38: delete [] pos_;
nit: no space in "delete[]", same below.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:70: // The relatove positions of the corresponding gradient
colors.
nit: relative

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:200: Painter* Painter::CreateVerticalGradientRange(
nit: use MultiColor / Range terminology consistently.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:201: SkColor* colors, SkScalar* pos, int count) {
nit: one param per line

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.h
File ui/views/painter.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.h...
ui/views/painter.h:38: // Creates a painter that draws a multi color gradient.
|colors| contains the
nit: multi-color

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.h...
ui/views/painter.h:44: int count);
nit: ditto for size_t

[markusheintz_, 2012-08-09 01:00:15]

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/background.h
File ui/views/background.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/backgroun...
ui/views/background.h:62: // Creates a background that containa a vertical
gradient. The gradient can
On 2012/08/08 23:13:53, msw wrote:
> nit: contains

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/backgroun...
ui/views/background.h:69: int count);
On 2012/08/08 23:13:53, msw wrote:
> nit: use size_t?

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/controls/...
File ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/controls/...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:128: SkColor colors[]
= {
On 2012/08/08 23:13:53, msw wrote:
> nit: mention where these values were derived from?

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/controls/...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:182: // Draw the
TabStrip border.
On 2012/08/08 23:13:53, msw wrote:
> nit: blank line above (separate bg / border).

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.cc
File ui/views/painter.cc (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:26: int count)
On 2012/08/08 23:13:53, msw wrote:
> nit: ditto for size_t

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:27: : horizontal_(horizontal), count_(count) {
On 2012/08/08 23:13:53, msw wrote:
> nit: I think if the params and initializer list don't fit on one line
together,
> the initializer list should also be one init per line.

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:31: for (int i = 0; i < count_; ++i) {
On 2012/08/08 23:13:53, msw wrote:
> nit: ditto for size_t

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:38: delete [] pos_;
On 2012/08/08 23:13:53, msw wrote:
> nit: no space in "delete[]", same below.

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:70: // The relatove positions of the corresponding gradient
colors.
On 2012/08/08 23:13:53, msw wrote:
> nit: relative

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:200: Painter* Painter::CreateVerticalGradientRange(
On 2012/08/08 23:13:53, msw wrote:
> nit: use MultiColor / Range terminology consistently.

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.c...
ui/views/painter.cc:201: SkColor* colors, SkScalar* pos, int count) {
On 2012/08/08 23:13:53, msw wrote:
> nit: one param per line

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.h
File ui/views/painter.h (right):

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.h...
ui/views/painter.h:38: // Creates a painter that draws a multi color gradient.
|colors| contains the
On 2012/08/08 23:13:53, msw wrote:
> nit: multi-color

Done.

https://chromiumcodereview.appspot.com/10823229/diff/11002/ui/views/painter.h...
ui/views/painter.h:44: int count);
On 2012/08/08 23:13:53, msw wrote:
> nit: ditto for size_t

Done.

[msw, 2012-08-09 01:26:59]

LGTM, but wait for Ben's review, thanks!

[Ben Goodger (Google), 2012-08-09 15:39:13]

lgtm

[commit-bot: I haz the power, 2012-08-09 16:35:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10823229/3006

[commit-bot: I haz the power, 2012-08-09 19:04:05]

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

[commit-bot: I haz the power, 2012-08-09 19:07:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10823229/3006

[commit-bot: I haz the power, 2012-08-09 20:19:46]

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

[commit-bot: I haz the power, 2012-08-10 09:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10823229/3006

[commit-bot: I haz the power, 2012-08-10 12:45:50]

Change committed as 151017