Use native theme colors for textfields; etc.

Use native theme colors for textfields; etc.

Make LocationBarView::GetColor use NativeTheme colors.
(except the CrOS transparent Omnibox background color)

Add NativeTheme read-only textfield color/background IDs.
Cache a set of Windows system colors in NativeThemeWin.
Update cached colors via gfx::SysColorChangeListener impl.

Init colors in NativeTextfieldViews, not TextfieldViewsModel.
Refactor Textfield and NativeTextfieldViews color code.

Nix frivolous AutocompleteTextfield::PaintChildren OVERRIDE.
Nix Textfield cursor color settings and visibility kludge.
(replace with proper cursor enabled accessors for views)

TODO(followup): Fix related existing issues (not affected here):
(existing textfields don't update with system color changes)
(bubbles seem to cache the system colors at startup...)

BUG=134766
TEST=Views textfields are created with the current system colors.
R=sky@chromium.org,varunjain@chromium.org,samarth@chromium.org
TBR=marja@chromium.org

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

[msw, 2012-12-04 18:40:07]

Hey Scott, please take a look; thanks! Is formalizing read-only textfield colors
good, or should they just use default colors? Also, should read-only omniboxes
(in popups) inherit the read-only textfield background color?

Varun: please take a look at the location_bar_view.cc change.

https://chromiumcodereview.appspot.com/11421204/diff/7018/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://chromiumcodereview.appspot.com/11421204/diff/7018/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:121: #if
defined(OS_CHROMEOS)
Varun: Is this still needed? It's purpose isn't explained in crrev.com/152436

[sky, 2012-12-04 22:38:14]

https://chromiumcodereview.appspot.com/11421204/diff/7018/ui/native_theme/nat...
File ui/native_theme/native_theme.h (right):

https://chromiumcodereview.appspot.com/11421204/diff/7018/ui/native_theme/nat...
ui/native_theme/native_theme.h:40: class NATIVE_THEME_EXPORT NativeTheme :
public gfx::SysColorChangeListener {
Since only windows actually implement OnSysColorChange(), can we move this to
NativeThemeWin?

https://chromiumcodereview.appspot.com/11421204/diff/7018/ui/native_theme/nat...
File ui/native_theme/native_theme_win.cc (right):

https://chromiumcodereview.appspot.com/11421204/diff/7018/ui/native_theme/nat...
ui/native_theme/native_theme_win.cc:332: OnSysColorChange();
I don't like calling a virtual method from a constructor. How about adding a
ResetColors() or something that this and OnSysColorChange call into.

https://chromiumcodereview.appspot.com/11421204/diff/7018/ui/native_theme/nat...
ui/native_theme/native_theme_win.cc:345: for (int i = 0; i <
arraysize(kSystemColors); ++i)
nit: use {} for the for loop.

[varunjain, 2012-12-04 23:05:30]

https://chromiumcodereview.appspot.com/11421204/diff/7018/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://chromiumcodereview.appspot.com/11421204/diff/7018/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:121: #if
defined(OS_CHROMEOS)
On 2012/12/04 18:40:07, msw wrote:
> Varun: Is this still needed? It's purpose isn't explained in crrev.com/152436

This was done because we want the omnibox to have the background color of the
asset used to draw the omnibox (browser_omnibox_border.png). This is only really
used for chromeos as for win, the color is obtained from the GetSysColor call.
Now that you are using native theme (awesome!), this is not needed, but you will
have to change the default color in native_theme_aura to this value to avoid
causing any regressions.

https://chromiumcodereview.appspot.com/11421204/diff/7018/ui/native_theme/nat...
File ui/native_theme/native_theme_aura.cc (right):

https://chromiumcodereview.appspot.com/11421204/diff/7018/ui/native_theme/nat...
ui/native_theme/native_theme_aura.cc:44: const SkColor
kTextfieldDefaultBackground = SK_ColorWHITE;
this one

[msw, 2012-12-05 02:09:56]

Unfortunately, a transparent textfield background breaks ChromeOS Omnibox
dropdown results (and likely more).

Patch Set 5 added kColorId_OmniboxDefaultBackground, but then ui/native_theme
has a semantic dependency on chrome/browser/ui/views/omnibox, which seems wrong.

Instead, Patch Set 6 leaves a CrOS #ifdef in LocationBarView::GetColor() and
restores the explicit Omnibox background color setting in
OmniboxViewViews::Init(). It's not ideal, but no worse than status-quo.

Please take a look; thanks!

https://codereview.chromium.org/11421204/diff/7018/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/11421204/diff/7018/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:121: #if
defined(OS_CHROMEOS)
On 2012/12/04 23:05:31, varunjain wrote:
> On 2012/12/04 18:40:07, msw wrote:
> > Varun: Is this still needed? It's purpose isn't explained in
crrev.com/152436
> 
> This was done because we want the omnibox to have the background color of the
> asset used to draw the omnibox (browser_omnibox_border.png). This is only
really
> used for chromeos as for win, the color is obtained from the GetSysColor call.
> Now that you are using native theme (awesome!), this is not needed, but you
will
> have to change the default color in native_theme_aura to this value to avoid
> causing any regressions.

I in-lined the CrOS case into GetColor, as explained above.

https://codereview.chromium.org/11421204/diff/7018/ui/native_theme/native_the...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/11421204/diff/7018/ui/native_theme/native_the...
ui/native_theme/native_theme.h:40: class NATIVE_THEME_EXPORT NativeTheme :
public gfx::SysColorChangeListener {
On 2012/12/04 22:38:14, sky wrote:
> Since only windows actually implement OnSysColorChange(), can we move this to
> NativeThemeWin?

Done.

https://codereview.chromium.org/11421204/diff/7018/ui/native_theme/native_the...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/11421204/diff/7018/ui/native_theme/native_the...
ui/native_theme/native_theme_aura.cc:44: const SkColor
kTextfieldDefaultBackground = SK_ColorWHITE;
On 2012/12/04 23:05:31, varunjain wrote:
> this one

I in-lined the CrOS case into GetColor, as explained above.

https://codereview.chromium.org/11421204/diff/7018/ui/native_theme/native_the...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/11421204/diff/7018/ui/native_theme/native_the...
ui/native_theme/native_theme_win.cc:332: OnSysColorChange();
On 2012/12/04 22:38:14, sky wrote:
> I don't like calling a virtual method from a constructor. How about adding a
> ResetColors() or something that this and OnSysColorChange call into.

Done.

https://codereview.chromium.org/11421204/diff/7018/ui/native_theme/native_the...
ui/native_theme/native_theme_win.cc:345: for (int i = 0; i <
arraysize(kSystemColors); ++i)
On 2012/12/04 22:38:14, sky wrote:
> nit: use {} for the for loop.

Done.

[msw, 2012-12-05 22:23:25]

+cc samarth@ for conflicting http://crrev.com/171160
I've replaced that new (hacky) use of textfield cursor color settings with
proper cursor visibility hooks for Views; please take a look and LMK if you'd
like me to split up the change.

[samarth, 2012-12-05 23:38:21]

+mathp

The code looks OK.  Let patch in your change to test that it still works as
expected for us.

Thanks for the heads up!
Samarth

https://codereview.chromium.org/11421204/diff/31001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/11421204/diff/31001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:598: visible_caret_color_
: textfield_->background_color());
You can kill visible_caret_color_.

[msw, 2012-12-06 00:03:51]

Done, LMK if you have any problems testing the patch.

https://codereview.chromium.org/11421204/diff/31001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/11421204/diff/31001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:598: visible_caret_color_
: textfield_->background_color());
On 2012/12/05 23:38:21, samarth wrote:
> You can kill visible_caret_color_.

Done.

[samarth, 2012-12-06 18:19:29]

Tested the latest patchset and it works. LGTM.

Thanks!
Samarth

[varunjain, 2012-12-06 18:36:58]

LGTM. As mentioned in the comment, I think the proper fix is to change the
omnibox popup background color. But that seems out of scope of this CL.

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:333: // Chrome OS
requires a transparent omnibox background color.
I agree that native_theme should not have a separate color for omnibox. I think
the omnibox popup (nothing else seems to be getting affected) should not be
using textfieldBackgroundColor. It should probably use the background color of a
popup. There is none defined in native_theme, closest seems to be
kColorId_MenuBackgroundColor

[msw, 2012-12-06 18:54:46]

Scott, please take another look as you have time; thanks!

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:333: // Chrome OS
requires a transparent omnibox background color.
On 2012/12/06 18:36:58, varunjain wrote:
> I agree that native_theme should not have a separate color for omnibox. I
think
> the omnibox popup (nothing else seems to be getting affected) should not be
> using textfieldBackgroundColor. It should probably use the background color of
a
> popup. There is none defined in native_theme, closest seems to be
> kColorId_MenuBackgroundColor

That might be feasible for a follow-up change. It may make more sense to offer
control over the presence of the solid color background used in
NativeTextfieldViews.

A problem with either approach (and the current code) is that DEEMPHASIZED_TEXT
and SECURITY_TEXT calculate colors using the BACKGROUND color, which is actually
the unspecified color within the images, not the transparent white supplied
here.

[sky, 2012-12-06 20:43:56]

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:333: // Chrome OS
requires a transparent omnibox background color.
On 2012/12/06 18:54:46, msw wrote:
> On 2012/12/06 18:36:58, varunjain wrote:
> > I agree that native_theme should not have a separate color for omnibox. I
> think
> > the omnibox popup (nothing else seems to be getting affected) should not be
> > using textfieldBackgroundColor. It should probably use the background color
of
> a
> > popup. There is none defined in native_theme, closest seems to be
> > kColorId_MenuBackgroundColor
> 
> That might be feasible for a follow-up change. It may make more sense to offer
> control over the presence of the solid color background used in
> NativeTextfieldViews.
> 
> A problem with either approach (and the current code) is that
DEEMPHASIZED_TEXT
> and SECURITY_TEXT calculate colors using the BACKGROUND color, which is
actually
> the unspecified color within the images, not the transparent white supplied
> here.

Doesn't the transparent background apply to OS_WIN when native theme ==
nativethemeaura?

https://codereview.chromium.org/11421204/diff/20006/ui/native_theme/native_th...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/11421204/diff/20006/ui/native_theme/native_th...
ui/native_theme/native_theme_win.cc:175: void NativeThemeWin::CloseHandles()
const {
I believe internally we invoke this when the theme changes. Can we get rid of
those calls now in favor of SysColorChangeListener now? The renderer may
directly invoke CloseHandles too, you'll have to make sure that is ok.

[msw, 2012-12-07 02:04:01]

Responses inline, no code changes (yet); please take a look.

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:333: // Chrome OS
requires a transparent omnibox background color.
On 2012/12/06 20:43:57, sky wrote:
> On 2012/12/06 18:54:46, msw wrote:
> > On 2012/12/06 18:36:58, varunjain wrote:
> > > I agree that native_theme should not have a separate color for omnibox. I
> > think
> > > the omnibox popup (nothing else seems to be getting affected) should not
be
> > > using textfieldBackgroundColor. It should probably use the background
color
> of
> > a
> > > popup. There is none defined in native_theme, closest seems to be
> > > kColorId_MenuBackgroundColor
> > 
> > That might be feasible for a follow-up change. It may make more sense to
offer
> > control over the presence of the solid color background used in
> > NativeTextfieldViews.
> > 
> > A problem with either approach (and the current code) is that
> DEEMPHASIZED_TEXT
> > and SECURITY_TEXT calculate colors using the BACKGROUND color, which is
> actually
> > the unspecified color within the images, not the transparent white supplied
> > here.
> 
> Doesn't the transparent background apply to OS_WIN when native theme ==
> nativethemeaura?

Hmm, not afaik; I think this is okay as-is. The old kOmniboxBackgroundColor
definition was transparent white for CrOS and opaque white for Win. My new code
preserves the local transparent white for CrOS and uses the equivalent
NativeThemeAura value for kColorId_TextfieldDefaultBackground
(kTextfieldDefaultBackground == SK_ColorWHITE) on Aura.

https://codereview.chromium.org/11421204/diff/20006/ui/native_theme/native_th...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/11421204/diff/20006/ui/native_theme/native_th...
ui/native_theme/native_theme_win.cc:175: void NativeThemeWin::CloseHandles()
const {
On 2012/12/06 20:43:57, sky wrote:
> I believe internally we invoke this when the theme changes. Can we get rid of
> those calls now in favor of SysColorChangeListener now? The renderer may
> directly invoke CloseHandles too, you'll have to make sure that is ok.

Hmm, I'm not entirely sure if that's correct, since they're respectively invoked
on WM_THEMECHANGED and WM_SYSCOLORCHANGE. I'd prefer to avoid changing that in
this same CL.

[sky, 2012-12-07 16:35:12]

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:333: // Chrome OS
requires a transparent omnibox background color.
On 2012/12/07 02:04:01, msw wrote:
> On 2012/12/06 20:43:57, sky wrote:
> > On 2012/12/06 18:54:46, msw wrote:
> > > On 2012/12/06 18:36:58, varunjain wrote:
> > > > I agree that native_theme should not have a separate color for omnibox.
I
> > > think
> > > > the omnibox popup (nothing else seems to be getting affected) should not
> be
> > > > using textfieldBackgroundColor. It should probably use the background
> color
> > of
> > > a
> > > > popup. There is none defined in native_theme, closest seems to be
> > > > kColorId_MenuBackgroundColor
> > > 
> > > That might be feasible for a follow-up change. It may make more sense to
> offer
> > > control over the presence of the solid color background used in
> > > NativeTextfieldViews.
> > > 
> > > A problem with either approach (and the current code) is that
> > DEEMPHASIZED_TEXT
> > > and SECURITY_TEXT calculate colors using the BACKGROUND color, which is
> > actually
> > > the unspecified color within the images, not the transparent white
supplied
> > > here.
> > 
> > Doesn't the transparent background apply to OS_WIN when native theme ==
> > nativethemeaura?
> 
> Hmm, not afaik; I think this is okay as-is. The old kOmniboxBackgroundColor
> definition was transparent white for CrOS and opaque white for Win. My new
code
> preserves the local transparent white for CrOS and uses the equivalent
> NativeThemeAura value for kColorId_TextfieldDefaultBackground
> (kTextfieldDefaultBackground == SK_ColorWHITE) on Aura.

I'm still confused. OS_CHROMEOS and OS_WIN when the theme == NativeThemeAura
should get the same colors. If you're saying native_theme->GetSystemColor for
!OS_CHROMEOS does the right thing, why do we need the ifdef at all?

[msw, 2012-12-07 18:22:28]

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:333: // Chrome OS
requires a transparent omnibox background color.
On 2012/12/07 16:35:12, sky wrote:
> On 2012/12/07 02:04:01, msw wrote:
> > On 2012/12/06 20:43:57, sky wrote:
> > > Doesn't the transparent background apply to OS_WIN when native theme ==
> > > nativethemeaura?
> > 
> > Hmm, not afaik; I think this is okay as-is. The old kOmniboxBackgroundColor
> > definition was transparent white for CrOS and opaque white for Win. My new
> code
> > preserves the local transparent white for CrOS and uses the equivalent
> > NativeThemeAura value for kColorId_TextfieldDefaultBackground
> > (kTextfieldDefaultBackground == SK_ColorWHITE) on Aura.
> 
> I'm still confused. OS_CHROMEOS and OS_WIN when the theme == NativeThemeAura
> should get the same colors. If you're saying native_theme->GetSystemColor for
> !OS_CHROMEOS does the right thing, why do we need the ifdef at all?

This change preserves the existing behavior, which contradicts your assertion.
Prior to my change, OS_CHROMEOS and OS_WIN when the theme == NativeThemeAura get
different colors. They'd both wind up outside the #if block and return
kOmniboxBackgroundColor, defined as transparent (0, 255, 255, 255) for CrOS and
normal (255, 255, 255, 255) for Win (see left side line 123). I can change/test
that, but I'm not sure that's our intent. Switching to transparent white here
breaks the non-Aura Win omnibox result dropdown, not sure if it would also break
Aura Win. Sorry for the trouble :(

[sky, 2012-12-07 18:58:02]

LGTM

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11421204/diff/20006/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:333: // Chrome OS
requires a transparent omnibox background color.
On 2012/12/07 18:22:28, msw wrote:
> On 2012/12/07 16:35:12, sky wrote:
> > On 2012/12/07 02:04:01, msw wrote:
> > > On 2012/12/06 20:43:57, sky wrote:
> > > > Doesn't the transparent background apply to OS_WIN when native theme ==
> > > > nativethemeaura?
> > > 
> > > Hmm, not afaik; I think this is okay as-is. The old
kOmniboxBackgroundColor
> > > definition was transparent white for CrOS and opaque white for Win. My new
> > code
> > > preserves the local transparent white for CrOS and uses the equivalent
> > > NativeThemeAura value for kColorId_TextfieldDefaultBackground
> > > (kTextfieldDefaultBackground == SK_ColorWHITE) on Aura.
> > 
> > I'm still confused. OS_CHROMEOS and OS_WIN when the theme == NativeThemeAura
> > should get the same colors. If you're saying native_theme->GetSystemColor
for
> > !OS_CHROMEOS does the right thing, why do we need the ifdef at all?
> 
> This change preserves the existing behavior, which contradicts your assertion.
> Prior to my change, OS_CHROMEOS and OS_WIN when the theme == NativeThemeAura
get
> different colors. They'd both wind up outside the #if block and return
> kOmniboxBackgroundColor, defined as transparent (0, 255, 255, 255) for CrOS
and
> normal (255, 255, 255, 255) for Win (see left side line 123). I can
change/test
> that, but I'm not sure that's our intent. Switching to transparent white here
> breaks the non-Aura Win omnibox result dropdown, not sure if it would also
break
> Aura Win. Sorry for the trouble :(

Ah, ok. I see it now.

[commit-bot: I haz the power, 2012-12-07 19:04:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/11421204/20006

[commit-bot: I haz the power, 2012-12-07 19:05:06]

Presubmit check for 11421204-20006 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      ui/native_theme/native_theme_win.h:15

Presubmit checks took 2.9s to calculate.

[msw - DO NOT USE, 2012-12-07 19:10:37]

Hey Marja (or anyone on the review), can you help with the presubmit warning:
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      ui/native_theme/native_theme_win.h:15
Perhaps I should put map after windows.h or some blank lines between?

https://chromiumcodereview.appspot.com/11421204/diff/20006/ui/native_theme/na...
File ui/native_theme/native_theme_win.h (right):

https://chromiumcodereview.appspot.com/11421204/diff/20006/ui/native_theme/na...
ui/native_theme/native_theme_win.h:15: #include <windows.h>
Hmm, including uxtheme.h before windows.h causes build errors.
I'll find out if there's a better solution or CQ workaround from marja@.

[commit-bot: I haz the power, 2012-12-07 19:41:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/11421204/20006

[commit-bot: I haz the power, 2012-12-07 19:42:12]

Presubmit check for 11421204-20006 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      ui/native_theme/native_theme_win.h:15

Presubmit checks took 1.9s to calculate.

[marja, 2012-12-10 09:32:17]

Hmm, the #include check doesn't cover this special case, and I guess there's no
way to tell CQ to bypass the presubmit checks. (I'll ask maruel@ what we should
do about it.) Sorry for the extra trouble. Can you git cl dcommit this instead
of using the CQ?

[marja, 2012-12-10 17:58:04]

The #include check is now fixed so that it uses only a "notify" level when
trying to commit -> CQ should process it nicely.

[msw, 2012-12-10 18:58:46]

On 2012/12/10 17:58:04, marja wrote:
> The #include check is now fixed so that it uses only a "notify" level when
> trying to commit -> CQ should process it nicely.

Cool, I'll give that a shot or just dcommit if needed; thanks!

[commit-bot: I haz the power, 2012-12-10 18:59:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/11421204/43002

[commit-bot: I haz the power, 2012-12-10 22:04:19]

Change committed as 172142