Add a new GetSystemColor method to native theme.

Add a new GetSystemColor method to native theme.
Currently it only knows about the dialog box background color. The intent is to add other colors, such as the text
highlighting color (issue 100229). This CL also fixes issue
104229 by returning an opaque color on Aura.


BUG=104229
TEST=none


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

[James Cook, 2011-11-18 23:22:49]

One comment below.

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme.h
File ui/gfx/native_theme.h (right):

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme.h#newcode22
ui/gfx/native_theme.h:22: // platform, such as Windows or Linux. It also
supplies default colors for
Thanks for adding file/class level docs.

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme_base.cc#...
ui/gfx/native_theme_base.cc:65: return SkColorSetRGB(200, 200, 200);
We typically define colors like this as constants in an anonymous namespace,
like:

namespace {

const SkColor kDialogBackgroundColor = ...;

}

even if they are used only once.  That also might imply that the enum should be
named kDialogBackground or kDialogBackgroundColorId or some such.

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme_win.h
File ui/gfx/native_theme_win.h (right):

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme_win.h#ne...
ui/gfx/native_theme_win.h:114: const ExtraParams& extra) const OVERRIDE;
Thanks for fixing these.

[benrg, 2011-11-19 00:20:22]

Moved hardcoded colors into constants. Feel free to critique my choice of
names...

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/7001/ui/gfx/native_theme_base.cc#...
ui/gfx/native_theme_base.cc:65: return SkColorSetRGB(200, 200, 200);
On 2011/11/18 23:22:50, James Cook (Chromium) wrote:
> We typically define colors like this as constants in an anonymous namespace,
> like:
> 
> namespace {
> 
> const SkColor kDialogBackgroundColor = ...;
> 
> }
> 
> even if they are used only once.  That also might imply that the enum should
be
> named kDialogBackground or kDialogBackgroundColorId or some such.

Done.

[James Cook, 2011-11-19 00:28:14]

One more suggestion, then I'd send it to either sky@ or ben@ for final review.

http://codereview.chromium.org/8597015/diff/7002/ui/gfx/native_theme.h
File ui/gfx/native_theme.h (right):

http://codereview.chromium.org/8597015/diff/7002/ui/gfx/native_theme.h#newcode19
ui/gfx/native_theme.h:19: const SkColor kColorYouShouldNeverSee =
SkColorSetRGB(255, 0, 128);
I wouldn't do this in a header file, even though the color ends up as an
unsigned int.  It just looks too much like you're initializing an object.  We're
super-paranoid about global object init/destruct around here.

I would either just leave copies of the color in each .cc file that needs it, or
create a one-line utility function in the base implementation to get it.

[James Cook, 2011-11-19 03:02:45]

Were you ready for me to review this again?  I didn't see an email but did so
anyway.

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:22: const SkColor kInvalidColorIdColor =
SkColorSetRGB(255, 0, 128);
I like this name better.

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:23: }
The style guide likes namespaces to end with two spaces and a comment:

}  // namespace

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_gtk.cc
File ui/gfx/native_theme_gtk.cc (right):

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_gtk.cc#...
ui/gfx/native_theme_gtk.cc:17: }
}  // namespace

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_win.cc#...
ui/gfx/native_theme_win.cc:62: const SkColor kInvalidColorIdColor =
SkColorSetRGB(255, 0, 128);
nit: We usually list constants before functions in namespace blocks.

[benrg, 2011-11-21 18:32:40]

ben, can I get owner's approval on this?

http://codereview.chromium.org/8597015/diff/7002/ui/gfx/native_theme.h
File ui/gfx/native_theme.h (right):

http://codereview.chromium.org/8597015/diff/7002/ui/gfx/native_theme.h#newcode19
ui/gfx/native_theme.h:19: const SkColor kColorYouShouldNeverSee =
SkColorSetRGB(255, 0, 128);
It seems silly to add a public method for code that should never be hit, so I
just duplicated the color.

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:23: }
On 2011/11/19 03:02:46, James Cook (Chromium) wrote:
> The style guide likes namespaces to end with two spaces and a comment:
> 
> }  // namespace

Done.

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/8597015/diff/13001/ui/gfx/native_theme_win.cc#...
ui/gfx/native_theme_win.cc:62: const SkColor kInvalidColorIdColor =
SkColorSetRGB(255, 0, 128);
On 2011/11/19 03:02:46, James Cook (Chromium) wrote:
> nit: We usually list constants before functions in namespace blocks.

Done.

[James Cook, 2011-11-21 18:54:08]

LGTM, assuming Ben's OK with the overall approach.  Thanks for persevering
through the style-nit gauntlet!

[Ben Goodger (Google), 2011-11-21 18:59:59]

Welcome to Google, land of OCD style-guide enforcement:

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme.h
File ui/gfx/native_theme.h (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme.h#newco...
ui/gfx/native_theme.h:219: kDialogBackgroundColorId
OCD requires me to suggest putting the constant part of the name first:
kColorId_DialogBackground

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:69: // and NativeThemeChromeos and overridden by
NativeThemeGtk.
comment moves inside function.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:70: SkColor NativeThemeBase::GetSystemColor(ColorId
color_id) const {
order in .cc must match .h

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.h
File ui/gfx/native_theme_base.h (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.h#...
ui/gfx/native_theme_base.h:30: // This implementation returns hardcoded default
colors.
no comments or NL above overrides, just run it straight after Paint( above.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.cc#...
ui/gfx/native_theme_win.cc:1691: SkColor NativeThemeWin::GetSystemColor(ColorId
color_id) const {
and update .cc to match .h file location

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.h
File ui/gfx/native_theme_win.h (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.h#n...
ui/gfx/native_theme_win.h:100: virtual SkColor GetSystemColor(ColorId color_id)
const OVERRIDE;
Move this to be adjacent to the other NativeTheme overrides.

http://codereview.chromium.org/8597015/diff/20003/ui/views/window/dialog_clie...
File ui/views/window/dialog_client_view.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:308:
GetSystemColor(gfx::NativeTheme::kDialogBackgroundColorId);
reformat as:

gfx::NativeTheme::instance()->GetSystemColor(
    gfx::NativeTheme::kColorId_DialogBackground);
^
+-- 4 space indent for wrapped params.

[benrg, 2011-11-21 19:54:20]

Going to lunch; I hope this builds.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme.h
File ui/gfx/native_theme.h (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme.h#newco...
ui/gfx/native_theme.h:219: kDialogBackgroundColorId
On 2011/11/21 18:59:59, Ben Goodger (Google) wrote:
> OCD requires me to suggest putting the constant part of the name first:
> kColorId_DialogBackground

Done.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:69: // and NativeThemeChromeos and overridden by
NativeThemeGtk.
On 2011/11/21 18:59:59, Ben Goodger (Google) wrote:
> comment moves inside function.

Done.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:70: SkColor NativeThemeBase::GetSystemColor(ColorId
color_id) const {
On 2011/11/21 18:59:59, Ben Goodger (Google) wrote:
> order in .cc must match .h

Independently for each protection level, I guess; is this in the style guide?

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.h
File ui/gfx/native_theme_base.h (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.h#...
ui/gfx/native_theme_base.h:30: // This implementation returns hardcoded default
colors.
On 2011/11/21 18:59:59, Ben Goodger (Google) wrote:
> no comments or NL above overrides, just run it straight after Paint( above.

Done.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.cc#...
ui/gfx/native_theme_win.cc:1691: SkColor NativeThemeWin::GetSystemColor(ColorId
color_id) const {
On 2011/11/21 18:59:59, Ben Goodger (Google) wrote:
> and update .cc to match .h file location

Done, but this puts GetSystemColor in the middle of the part-related methods. I
wonder if I should move GetSystemColor to the top everywhere to avoid that.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.h
File ui/gfx/native_theme_win.h (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_win.h#n...
ui/gfx/native_theme_win.h:100: virtual SkColor GetSystemColor(ColorId color_id)
const OVERRIDE;
On 2011/11/21 18:59:59, Ben Goodger (Google) wrote:
> Move this to be adjacent to the other NativeTheme overrides.

Done.

http://codereview.chromium.org/8597015/diff/20003/ui/views/window/dialog_clie...
File ui/views/window/dialog_client_view.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:308:
GetSystemColor(gfx::NativeTheme::kDialogBackgroundColorId);
On 2011/11/21 18:59:59, Ben Goodger (Google) wrote:
> reformat as:
> 
> gfx::NativeTheme::instance()->GetSystemColor(
>     gfx::NativeTheme::kColorId_DialogBackground);
> ^
> +-- 4 space indent for wrapped params.

Done.

[benrg, 2011-11-22 00:46:33]

PTAL. (It builds; test failures seem unrelated.)

[Ben Goodger (Google), 2011-11-22 15:49:50]

Forgot to press submit on this yesterday I guess...

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:70: SkColor NativeThemeBase::GetSystemColor(ColorId
color_id) const {
Sort of, it's in the Chrome-specific overrides:

http://dev.chromium.org/developers/coding-style

[benrg, 2011-11-22 21:43:49]

The method orders in the .cc and .h files didn't match before my change (see my
inline comments). I could rearrange the functions I didn't modify, but I think
that should be a different CL.

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc
File ui/gfx/native_theme_base.cc (right):

http://codereview.chromium.org/8597015/diff/20003/ui/gfx/native_theme_base.cc...
ui/gfx/native_theme_base.cc:70: SkColor NativeThemeBase::GetSystemColor(ColorId
color_id) const {
In the newest patch set, NativeThemeBase::GetSystemColor is in order with
respect to the other public methods of NativeThemeBase. The public, protected,
and private method definitions are interleaved with each other, but I would have
to rearrange a lot of functions that I didn't modify to fix that.

http://codereview.chromium.org/8597015/diff/33001/ui/gfx/native_theme_win.cc
File ui/gfx/native_theme_win.cc (right):

http://codereview.chromium.org/8597015/diff/33001/ui/gfx/native_theme_win.cc#...
ui/gfx/native_theme_win.cc:1703: 
Until other methods are rearranged, there's no correct place for this. It has to
be before PaintToNonPlatformCanvas() but after Paint().

[Ben Goodger (Google), 2011-11-22 22:13:13]

I prefer not to make the problem worse. Please just insert it after the adjacent
function in the header. You don't need to rearrange the other functions in this
CL.

[benrg, 2011-11-22 22:30:45]

GetSystemColor is now after the previous method (but not before the next method)
in all three implementations.

[Ben Goodger (Google), 2011-11-22 22:31:42]

Thanks. LGTM.

[commit-bot: I haz the power, 2011-11-22 22:35:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/8597015/35002

[commit-bot: I haz the power, 2011-11-23 01:00:18]

Change committed as 111268