MacViews: Style BUTTON_STYLE buttons using the "modern" UI

ui/views/controls/button/label_button.cc

Index: ui/views/controls/button/label_button.cc
diff --git a/ui/views/controls/button/label_button.cc b/ui/views/controls/button/label_button.cc
index f5b07e474368a2d8484444a0ed37887ddf5a3ff0..ac655be5541d66306454e728fddaa9de4e7a0595 100644
--- a/ui/views/controls/button/label_button.cc
+++ b/ui/views/controls/button/label_button.cc
@@ -30,12 +30,6 @@ namespace {
 // The default spacing between the icon and text.
 const int kSpacing = 5;
 
-#if !(defined(OS_LINUX) && !defined(OS_CHROMEOS))
-// Default text and shadow colors for STYLE_BUTTON.
-const SkColor kStyleButtonTextColor = SK_ColorBLACK;
-const SkColor kStyleButtonShadowColor = SK_ColorWHITE;
-#endif
-
 const gfx::FontList& GetDefaultNormalFontList() {
   static base::LazyInstance<gfx::FontList>::Leaky font_list =
       LAZY_INSTANCE_INITIALIZER;
@@ -165,8 +159,8 @@ void LabelButton::SetFontList(const gfx::FontList& font_list) {
   cached_normal_font_list_ = font_list;
   cached_bold_font_list_ = font_list.DeriveWithStyle(
       font_list.GetFontStyle() | gfx::Font::BOLD);
-  label_->SetFontList(is_default_ ?
-      cached_bold_font_list_ : cached_normal_font_list_);
+  bool bold = PlatformStyle::kDefaultLabelButtonHasBoldFont && is_default_;

[sky, 2016/04/07 17:16:31]

Seems like you can skip cached_bold_font_list_ entirely if
kDefaultLabelButtonHasBoldFont is false. Maybe make cached_bold_font_list_  a
scoped_ptr?

[tapted, 2016/04/08 08:35:33]

gfx::FontList is itself just a scoped_refptr, and default-constructing a
gfx::FontList is cheap now that it just references `g_default_impl` in
font_list.cc. So a scoped_ptr might not add much. However, you're right that I'm
missing out on cases where it's fine to leave cached_bold_font_list_ untouched,
so I've fixed that.


(I guess an orthogonal question is whether the caching really adds much on any
platform, since it should be rare for a button to switch between
SetIsDefault(true/false)...)

[sky, 2016/04/08 16:24:22]

I think you're right and am fine to nuke the caching. I think that will simplify
the code too.

+  label_->SetFontList(bold ? cached_bold_font_list_ : cached_normal_font_list_);
 }
 
 void LabelButton::SetElideBehavior(gfx::ElideBehavior elide_behavior) {
@@ -198,9 +192,10 @@ void LabelButton::SetIsDefault(bool is_default) {
   ui::Accelerator accel(ui::VKEY_RETURN, ui::EF_NONE);
   is_default_ ? AddAccelerator(accel) : RemoveAccelerator(accel);
 
-  label_->SetFontList(
-      is_default ? cached_bold_font_list_ : cached_normal_font_list_);
+  bool bold = PlatformStyle::kDefaultLabelButtonHasBoldFont && is_default_;
+  label_->SetFontList(bold ? cached_bold_font_list_ : cached_normal_font_list_);
   InvalidateLayout();
+  HighlightStateChanged();
 }
 
 void LabelButton::SetStyle(ButtonStyle style) {
@@ -215,7 +210,8 @@ void LabelButton::SetStyle(ButtonStyle style) {
   SetFocusPainter(nullptr);
   SetHorizontalAlignment(gfx::ALIGN_CENTER);
   SetFocusable(true);
-  SetMinSize(gfx::Size(70, 33));
+  SetMinSize(gfx::Size(PlatformStyle::kMinLabelButtonWidth,
+                       PlatformStyle::kMinLabelButtonHeight));
 
   // Themed borders will be set once the button is added to a Widget, since that
   // provides the value of GetNativeTheme().
@@ -241,7 +237,7 @@ gfx::Size LabelButton::GetPreferredSize() const {
   Label label(GetText(), cached_normal_font_list_);
   label.SetShadows(label_->shadows());
 
-  if (style() == STYLE_BUTTON) {
+  if (style_ == STYLE_BUTTON && PlatformStyle::kDefaultLabelButtonHasBoldFont) {
     // Some text appears wider when rendered normally than when rendered bold.
     // Accommodate the widest, as buttons may show bold and shouldn't resize.
     const int current_width = label.GetPreferredSize().width();
@@ -396,6 +392,7 @@ void LabelButton::OnBlur() {
 void LabelButton::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   ResetColorsFromNativeTheme();
   UpdateThemedBorder();
+  HighlightStateChanged();
   // Invalidate the layout to pickup the new insets from the border.
   InvalidateLayout();
 }
@@ -440,9 +437,7 @@ gfx::Point LabelButton::GetInkDropCenter() const {
 void LabelButton::StateChanged() {
   const gfx::Size previous_image_size(image_->GetPreferredSize());
   UpdateImage();
-  const SkColor color = button_state_colors_[state()];
-  if (state() != STATE_DISABLED && label_->enabled_color() != color)
-    label_->SetEnabledColor(color);
+  HighlightStateChanged();
   label_->SetEnabled(state() != STATE_DISABLED);
   if (image_->GetPreferredSize() != previous_image_size)
     Layout();
@@ -467,38 +462,21 @@ void LabelButton::ResetColorsFromNativeTheme() {
     theme->GetSystemColor(ui::NativeTheme::kColorId_ButtonDisabledColor),
   };
 
-  // Certain styles do not change text color when hovered or pressed.
-  bool constant_text_color = false;
   // Use hardcoded colors for inverted color scheme support and STYLE_BUTTON.
   if (color_utils::IsInvertedColorScheme()) {
-    constant_text_color = true;
     colors[STATE_NORMAL] = SK_ColorWHITE;
+    colors[STATE_HOVERED] = colors[STATE_PRESSED] = colors[STATE_NORMAL];

[sky, 2016/04/07 17:16:31]

nit: a single assignment setting all to WHITE is more readable.

[tapted, 2016/04/08 08:35:33]

Done.

     label_->SetBackgroundColor(SK_ColorBLACK);
     label_->set_background(Background::CreateSolidBackground(SK_ColorBLACK));
     label_->SetAutoColorReadabilityEnabled(true);
     label_->SetShadows(gfx::ShadowValues());
   } else if (style() == STYLE_BUTTON) {
-    // TODO(erg): This is disabled on desktop linux because of the binary asset
-    // confusion. These details should either be pushed into ui::NativeThemeWin
-    // or should be obsoleted by rendering buttons with paint calls instead of
-    // with static assets. http://crbug.com/350498
-#if !(defined(OS_LINUX) && !defined(OS_CHROMEOS))
-    constant_text_color = true;
-    colors[STATE_NORMAL] = kStyleButtonTextColor;
-    label_->SetBackgroundColor(theme->GetSystemColor(
-        ui::NativeTheme::kColorId_ButtonBackgroundColor));
-    label_->SetAutoColorReadabilityEnabled(false);
-    label_->SetShadows(gfx::ShadowValues(
-        1, gfx::ShadowValue(gfx::Vector2d(0, 1), 0, kStyleButtonShadowColor)));
-#endif
-    label_->set_background(NULL);
+    PlatformStyle::ApplyLabelButtonTextStyle(label_, &colors);
+    label_->set_background(nullptr);
   } else {
-    label_->set_background(NULL);
+    label_->set_background(nullptr);
   }
 
-  if (constant_text_color)
-    colors[STATE_HOVERED] = colors[STATE_PRESSED] = colors[STATE_NORMAL];
-
   for (size_t state = STATE_NORMAL; state < STATE_COUNT; ++state) {
     if (!explicitly_set_colors_[state]) {
       SetTextColor(static_cast<ButtonState>(state), colors[state]);
@@ -573,4 +551,13 @@ void LabelButton::ResetCachedPreferredSize() {
   cached_preferred_size_ = gfx::Size();
 }
 
+void LabelButton::HighlightStateChanged() {

[sky, 2016/04/07 17:16:31]

This function is confusingly named as there is no 'highlight' state, and it's
invoked at random ties. I think a more specific name would be better, perhaps
ResetLabelEnabledColor().

[tapted, 2016/04/08 08:35:33]

Done.

+  const SkColor color =

[sky, 2016/04/07 17:16:31]

If you're going to use const (which is good), be consistent. You don't use it
for 'bool bold' in this file when you could as well.

[tapted, 2016/04/08 08:35:33]

Done.

+      explicitly_set_colors_[state()]
+          ? button_state_colors_[state()]
+          : PlatformStyle::TextColorForButton(button_state_colors_, *this);
+  if (state() != STATE_DISABLED && label_->enabled_color() != color)
+    label_->SetEnabledColor(color);
+}
+
 }  // namespace views