MacViews: Implement scrollbar theming.

MacViews: Implement scrollbar theming.

The default views scrollbars look out of place on Mac OS X.
As a start, theme the scrollbar drawing to look like the native non-overlay
scrollbars. It looks like a simple round-rect that is inset from the track,
with a line border next to the content area.

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

[Andre, 2014-08-07 22:19:33]

Trent PTAL.

[tapted, 2014-08-11 01:00:58]

I don't think it's worth going half-way (but we could possibly defer the
fade-in/out on hover, and overlay scrollbars).

But Aura platforms already have code to support overlay scrollbars - there's
probably some code that we can share, and it's possible that will make some of
the stuff here redundant.

It's still possible there's no overlap, in which case a CL like this is probably
fine to land too, but we should check first.

Also, we usually put screenshots on the BUG for UI changes.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_mac.h (right):

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.h:32: virtual void PaintScrollbarTrack(
Nit: place these above the PaintMenu* functions, to match declaration order in
the base class

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:20: const SkColor kScrollerTrackColor =
SkColorSetRGB(0xF8, 0xF8, 0xF8);
Can we make this a horizontal 3-stage gradient, to match a little closer?

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:22: const SkColor kScrollerThumbColor =
SkColorSetRGB(0xC3, 0xC3, 0xC3);
These should have an alpha component, so the track gradient shows through. it
looks like there's also a PaintStateTransition(..) function that will allow the
fade-in effect to be done -- can we do this by refactoring some code out of
native_theme_aura for some code re-use?

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:227: void
NativeThemeMac::PaintScrollbarTrack(
(nit: move up in the .mm file too)

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:234: paint.setColor(track_color_);
probably something like paint.setShader(SkGradientShader::CreateLinear(..));

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:261: if (part == kScrollbarVerticalThumb)
Maybe a switch statement here, with NOTREACHED(). (i.e. follow
NativeThemeWin::PaintScrollbarThumb(..) more closely). There are also "gripper"
parts, (which common theme ignores).

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:279: set_scrollbar_button_length(0);
Perhaps also override PaintArrowButton to be a no-op? there's some complex
drawing code there it would be nice to avoid.

[Andre, 2014-08-12 21:42:38]

Added screenshots to the bug.
There's an issue where PaintScrollbarCorner(..) is never called, I'm looking
into it.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_mac.h (right):

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.h:32: virtual void PaintScrollbarTrack(
On 2014/08/11 01:00:58, tapted wrote:
> Nit: place these above the PaintMenu* functions, to match declaration order in
> the base class

Done.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:20: const SkColor kScrollerTrackColor =
SkColorSetRGB(0xF8, 0xF8, 0xF8);
On 2014/08/11 01:00:58, tapted wrote:
> Can we make this a horizontal 3-stage gradient, to match a little closer?
> 

Done.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:22: const SkColor kScrollerThumbColor =
SkColorSetRGB(0xC3, 0xC3, 0xC3);
On 2014/08/11 01:00:58, tapted wrote:
> These should have an alpha component, so the track gradient shows through. it
> looks like there's also a PaintStateTransition(..) function that will allow
the
> fade-in effect to be done -- can we do this by refactoring some code out of
> native_theme_aura for some code re-use?

Added alpha component, although the effect is very subtle.
PaintStateTransition(..) appears to be unused and never called.
I think we should tackle fade effects in a separate CL since it will involve
modifying views code.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:227: void
NativeThemeMac::PaintScrollbarTrack(
On 2014/08/11 01:00:58, tapted wrote:
> (nit: move up in the .mm file too)

Done.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:234: paint.setColor(track_color_);
On 2014/08/11 01:00:58, tapted wrote:
> probably something like paint.setShader(SkGradientShader::CreateLinear(..)); 

Done.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:261: if (part == kScrollbarVerticalThumb)
On 2014/08/11 01:00:58, tapted wrote:
> Maybe a switch statement here, with NOTREACHED(). (i.e. follow
> NativeThemeWin::PaintScrollbarThumb(..) more closely). There are also
"gripper"
> parts, (which common theme ignores).

Done.

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:279: set_scrollbar_button_length(0);
On 2014/08/11 01:00:58, tapted wrote:
> Perhaps also override PaintArrowButton to be a no-op? there's some complex
> drawing code there it would be nice to avoid.

I've added a scrollbar_button_length_ check in NativeThemeBase::Paint() instead.
WDYT?

[tapted, 2014-08-13 01:13:29]

lgtm with the following

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/456473002/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:279: set_scrollbar_button_length(0);
On 2014/08/12 21:42:38, Andre wrote:
> On 2014/08/11 01:00:58, tapted wrote:
> > Perhaps also override PaintArrowButton to be a no-op? there's some complex
> > drawing code there it would be nice to avoid.
> 
> I've added a scrollbar_button_length_ check in NativeThemeBase::Paint()
instead.
> WDYT?

lg

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_base.cc:239: if (scrollbar_button_length_ > 0)
nit: perhaps a comment (here, or - probably better - on the data member in the
header): // A button length of 0 means no buttons are drawn.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:30: const int kScrollerThumbInset = 3;
nit: this one probably needs a comment - it's a bit subtle that it's inset from
both the ends and the sides.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:206: SkPoint gradient_bounds[2];
nit: perhaps a comment in here saying which style is being targeted (e.g. OSX
10.7 - 10.10 ?), and that the non-overlay style is always used.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:211: gradient_bounds[0].set(rect.x(),
rect.y() + kScrollerTrackBorderWidth);
nit: DCHECK_EQ(part, kScrollbarHorizontalTrack);

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:218: 4,
nit: 4 -> arraysize(kScrollerTrackGradientColors)  (from macros.h)

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:264: SkPaint paint;
Looking at the screenshots.. I think we want paint.setAntiAlias(true); after
this

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:265: paint.setColor(state == kHovered ?
thumb_active_color_
hm - it's odd there are protected data members in the base class - style guide
doesn't allow those. See what owners say maybe

[Andre, 2014-08-13 20:04:22]

Added corner painting.
Refined painting, paint outer border line.
Updated screenshot on the bug.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_base.cc:239: if (scrollbar_button_length_ > 0)
On 2014/08/13 01:13:29, tapted wrote:
> nit: perhaps a comment (here, or - probably better - on the data member in the
> header): // A button length of 0 means no buttons are drawn.

Done.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:30: const int kScrollerThumbInset = 3;
On 2014/08/13 01:13:29, tapted wrote:
> nit: this one probably needs a comment - it's a bit subtle that it's inset
from
> both the ends and the sides.

Done.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:206: SkPoint gradient_bounds[2];
On 2014/08/13 01:13:29, tapted wrote:
> nit: perhaps a comment in here saying which style is being targeted (e.g. OSX
> 10.7 - 10.10 ?), and that the non-overlay style is always used.

Done.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:211: gradient_bounds[0].set(rect.x(),
rect.y() + kScrollerTrackBorderWidth);
On 2014/08/13 01:13:29, tapted wrote:
> nit: DCHECK_EQ(part, kScrollbarHorizontalTrack);

Done.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:218: 4,
On 2014/08/13 01:13:29, tapted wrote:
> nit: 4 -> arraysize(kScrollerTrackGradientColors)  (from macros.h)

Done.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:264: SkPaint paint;
On 2014/08/13 01:13:29, tapted wrote:
> Looking at the screenshots.. I think we want paint.setAntiAlias(true); after
> this

Done.

https://codereview.chromium.org/456473002/diff/60001/ui/native_theme/native_t...
ui/native_theme/native_theme_mac.mm:265: paint.setColor(state == kHovered ?
thumb_active_color_
On 2014/08/13 01:13:29, tapted wrote:
> hm - it's odd there are protected data members in the base class - style guide
> doesn't allow those. See what owners say maybe

Acknowledged.

[Andre, 2014-08-13 20:04:49]

sky@ PTAL.

[sky, 2014-08-13 22:46:39]

LGTM

[Andre, 2014-08-13 22:47:50]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:51:00]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andresantoso@chromium.org/456473002/10...

[commit-bot: I haz the power, 2014-08-14 08:01:34]

Committed patchset #4 (100001) as 289486