Use system themes where possible in Omnibox, dropdown, and security info bubb...

Use system themes where possible in Omnibox edit and dropdown; in other cases, provide a pair of colors and use whichever contrasts more with the relevant background color, to maintain readability.

BUG=92, 18367
TEST=Use the omnibox on various non-default and high-contrast system themes.  Observe that everything is always readable.

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

[Peter Kasting, 2009-09-03 21:27:36]

I'm not sure that both TOOLKIT_VIEWS and non-TOOLKIT_VIEWS GTK builds will
compile correctly with this change as-is.  I'll watch the trybot.  In the
meantime, this is good enough to start reviewing.

[sky, 2009-09-03 23:26:58]

http://codereview.chromium.org/200009/diff/4001/4002
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/200009/diff/4001/4002#newcode2008
Line 2008: emphasize ? LocationBarView::DEEMPHASIZED_TEXT :
LocationBarView::TEXT));
This seems weird. emphasize bets DEEMPHASIZED_TEXT?

http://codereview.chromium.org/200009/diff/4001/4003
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/200009/diff/4001/4003#newcode31
Line 31: enum ResultViewState {
nit: put these in an anonymous namespace.

http://codereview.chromium.org/200009/diff/4001/4003#newcode47
Line 47: bool initialized = false;
You never set this to true. Did you mean to make it static and set it true in
the if?

http://codereview.chromium.org/200009/diff/4001/4003#newcode545
Line 545: const ResultViewState state = GetState();
nit: don't go const happy on me here.

http://codereview.chromium.org/200009/diff/4001/4003#newcode849
Line 849: contents_canvas.FillRectInt(GetColor(NORMAL, BACKGROUND), 0, 0,
width(),
Use drawColor for filling in the complete bounds.

http://codereview.chromium.org/200009/diff/4001/4005
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/200009/diff/4001/4005#newcode215
Line 215: NOT_SECURE,
nit: = 0

http://codereview.chromium.org/200009/diff/4001/4005#newcode220
Line 220: bool initialized = false;
Should be static, and set to true in the if block.

http://codereview.chromium.org/200009/diff/4001/4005#newcode221
Line 221: SkColor colors[NUM_STATES][NUM_KINDS];
should be static

http://codereview.chromium.org/200009/diff/4001/4005#newcode243
Line 243: static const SkColor kDarkNotSecureText = SkColorSetRGB(200, 0, 0);
If colors[][] is static, why bother making any variables in this if block
static?

[Peter Kasting, 2009-09-04 00:47:08]

New snapshot uploaded.  In this one I have added some temporary #ifs around the
lines which account for info bubbles changing their background color.  My intent
is to commit the change without this change's modifications to info_bubble.cc
and _with_ these temporary #ifs in place, tell Anthony to merge that to Chrome
3, and then do an immediate followup checkin that modifies info_bubble.cc and
removes the #ifs, which should _not_ be merged to Chrome 3.

http://codereview.chromium.org/200009/diff/4001/4002
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/200009/diff/4001/4002#newcode2008
Line 2008: emphasize ? LocationBarView::DEEMPHASIZED_TEXT :
LocationBarView::TEXT));
On 2009/09/03 23:26:58, sky wrote:
> This seems weird. emphasize bets DEEMPHASIZED_TEXT?

Added a comment to help explain this oddity.

http://codereview.chromium.org/200009/diff/4001/4003
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/200009/diff/4001/4003#newcode31
Line 31: enum ResultViewState {
On 2009/09/03 23:26:58, sky wrote:
> nit: put these in an anonymous namespace.

Done; since I had the namespace here I went ahead and extended it to cover the
other constants up here too.

http://codereview.chromium.org/200009/diff/4001/4003#newcode47
Line 47: bool initialized = false;
On 2009/09/03 23:26:58, sky wrote:
> You never set this to true. Did you mean to make it static and set it true in
> the if?

Yes.  Done.

http://codereview.chromium.org/200009/diff/4001/4003#newcode849
Line 849: contents_canvas.FillRectInt(GetColor(NORMAL, BACKGROUND), 0, 0,
width(),
On 2009/09/03 23:26:58, sky wrote:
> Use drawColor for filling in the complete bounds.

Fixed (three places)

http://codereview.chromium.org/200009/diff/4001/4005
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/200009/diff/4001/4005#newcode215
Line 215: NOT_SECURE,
On 2009/09/03 23:26:58, sky wrote:
> nit: = 0

Done.

http://codereview.chromium.org/200009/diff/4001/4005#newcode220
Line 220: bool initialized = false;
On 2009/09/03 23:26:58, sky wrote:
> Should be static, and set to true in the if block.

Yep.

http://codereview.chromium.org/200009/diff/4001/4005#newcode221
Line 221: SkColor colors[NUM_STATES][NUM_KINDS];
On 2009/09/03 23:26:58, sky wrote:
> should be static

Yep.  (Also fixed in other file.)

http://codereview.chromium.org/200009/diff/4001/4005#newcode243
Line 243: static const SkColor kDarkNotSecureText = SkColorSetRGB(200, 0, 0);
On 2009/09/03 23:26:58, sky wrote:
> If colors[][] is static, why bother making any variables in this if block
> static?

Habit.  I suspect the compiler will generate the same thing.  changed to just
"const" to be shorter (also in other file).

[sky, 2009-09-04 14:05:14]

This fails on linux toolkit_views with the following error:

/usr/local/google/gold/ld:
/secondary/builds/build3/src/sconsbuild/Debug/lib/libbrowser.a(autocomplete_edit_view_gtk.o):
in function
AutocompleteEditViewGtk::SetBaseColor():/secondary/builds/build3/src/chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:529:
undefined reference to 'LocationBarViewGtk::kBackgroundColorByLevel'

[Peter Kasting, 2009-09-04 18:33:18]

On 2009/09/04 14:05:14, sky wrote:
> This fails on linux toolkit_views with the following error:
> 
> /usr/local/google/gold/ld:
>
/secondary/builds/build3/src/sconsbuild/Debug/lib/libbrowser.a(autocomplete_edit_view_gtk.o):
> in function
>
AutocompleteEditViewGtk::SetBaseColor():/secondary/builds/build3/src/chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:529:
> undefined reference to 'LocationBarViewGtk::kBackgroundColorByLevel'

Try the new snapshot.

[sky, 2009-09-04 19:48:53]

LGTM, and it compiles!