Make views::Label and views::Link auto-color themselves to be readable over their background colo...

Make views::Label and views::Link auto-color themselves to be readable over their background color.  Unfortunately since they can't automatically determine what that backgruond color is it has to be manually set.  (I suppose I could try to add some crazy hierarchy-walking code looking for views::Background objects, but that seems like a bad move.)  There is also a disable switch, which I use in a few places where I couldn't figure out what the background color actually is, or where updating it is really annoying.

In theory, we might want to apply this to other controls like text buttons, but since those are usually rendered with explicit background images there's probably not a ton of win there.

This also makes some other cleanup changes like converting a few wstrings to string16s, de-inlining some class member functions, etc.  Some of these were mandated by the presubmitter :/

BUG=92
TEST=Things still look OK
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105189

[Peter Kasting, 2011-10-10 23:41:56]

sail: Avatar menu changes (mostly just to de-inline function definitions) and
FYI
sky: Rest

Scott, I ended up going the "opt-out" route for color balancing rather than "opt
in" because opt-out is nicer in principle (things "just work" for a11y) and
practically it didn't seem like I actually needed to opt out in very many
places.

A few notes - first_run_search_engine_view.cc changed so much because I wanted
to move construction of the child views until during/after the initial
ViewHierarchyChanged call, so that we could safely call GetThemeProvider() to
get the background color (can't call this before the view has an ancestor
widget).  I also ended up changing the ConfirmSetDefaultSearchProvider() API to
pass a Profile* instead of a TemplateURLService* because the ultimate endpoint
had to calculate a Profile anyway, and so this way was a little more direct (and
more parallel to the sister function ConfirmAddSearchProvider()).

[sail, 2011-10-10 23:49:34]

LGTM!
Sorry again for the merge issues.

http://codereview.chromium.org/8221027/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/8221027/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:191:
edit_link_->SetEnabledColor(SkColorSetRGB(0xe3, 0xed, 0xf6));
Merge error:
 edit_link_->SetHasFocusBorder(true);

http://codereview.chromium.org/8221027/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:245:
set_background(IsHighlighted() ? views::Background::CreateSolidBackground(
cool!

[Peter Kasting, 2011-10-11 00:02:37]

http://codereview.chromium.org/8221027/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/8221027/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:191:
edit_link_->SetEnabledColor(SkColorSetRGB(0xe3, 0xed, 0xf6));
On 2011/10/10 23:49:34, sail wrote:
> Merge error:
>  edit_link_->SetHasFocusBorder(true);	

Thanks!  I totally missed that.

[sky, 2011-10-11 00:42:34]

I only looked at label/link and want to rubber stamp the rest of the changes. If
you want me to review it all, let me know.

  -Scott

http://codereview.chromium.org/8221027/diff/1/views/controls/label.cc
File views/controls/label.cc (right):

http://codereview.chromium.org/8221027/diff/1/views/controls/label.cc#newcode29
views/controls/label.cc:29: SkColor kDefaultEnabledColor;
Having these away from where you initialize them makes it easy to try and use
one and not realize it wasn't initialized. It looks like you only use them in
Init, could they be static there?

http://codereview.chromium.org/8221027/diff/1/views/controls/label.cc#newcode33
views/controls/label.cc:33: };
nit: no ;

http://codereview.chromium.org/8221027/diff/1/views/controls/link.cc
File views/controls/link.cc (right):

http://codereview.chromium.org/8221027/diff/1/views/controls/link.cc#newcode29
views/controls/link.cc:29: SkColor kDefaultEnabledColor;
Same comment here as in Label.

[Peter Kasting, 2011-10-11 01:05:47]

Unfortunately, Scott, you should probably review everything.

In particular, any place where I disable auto color readability, if it's
actually possible to get the correct background color there, I should probably
be doing that instead.

http://codereview.chromium.org/8221027/diff/1/views/controls/label.cc
File views/controls/label.cc (right):

http://codereview.chromium.org/8221027/diff/1/views/controls/label.cc#newcode29
views/controls/label.cc:29: SkColor kDefaultEnabledColor;
On 2011/10/11 00:42:35, sky wrote:
> Having these away from where you initialize them makes it easy to try and use
> one and not realize it wasn't initialized. It looks like you only use them in
> Init, could they be static there?

Sure.  Moved.  (Also for link.cc.)

[sky, 2011-10-11 14:54:10]

LGTM

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_view.cc (right):

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_view.cc:221: void
UpdateView::InitLabel(views::Label** label, SkColor background_color) {
nit: out params should be last.

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/chromeos/opti...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/chromeos/opti...
chrome/browser/chromeos/options/vpn_config_view.cc:546:
error_label_->SetAutoColorReadabilityEnabled(false);
I tend to think you don't need this for the subclasses of
ChildNetworkConfigView, but I'm not positive. If you ask the author I'm sure
they could tell you.

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_fetcher_unittest.cc (right):

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:33: Profile*
profile);
Since you're touching this code, how about adding OVERRIDE

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/ui/search_eng...
File chrome/browser/ui/search_engines/template_url_fetcher_ui_callbacks.h
(right):

http://codereview.chromium.org/8221027/diff/2027/chrome/browser/ui/search_eng...
chrome/browser/ui/search_engines/template_url_fetcher_ui_callbacks.h:28:
Profile* profile);
OVERRIDE