Use light images in colored app frames, when the frame color is dark.

apps/ui/views/app_window_frame_view.cc

Index: apps/ui/views/app_window_frame_view.cc
diff --git a/apps/ui/views/app_window_frame_view.cc b/apps/ui/views/app_window_frame_view.cc
index db4d52b4709df3c62cc3cd26a0337c0b54ae23d9..db26be9d3a8c5908ff0235eddd64fcee70d43c57 100644
--- a/apps/ui/views/app_window_frame_view.cc
+++ b/apps/ui/views/app_window_frame_view.cc
@@ -15,6 +15,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
+#include "ui/gfx/color_utils.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/path.h"
 #include "ui/views/controls/button/image_button.h"
@@ -75,9 +76,6 @@ void AppWindowFrameView::Init() {
     AddChildView(close_button_);

[Matt Giuca, 2014/05/09 08:33:11]

// STATE_NORMAL images are set in SetButtonImagesForFrame, not here.

(or similar)

[benwells, 2014/05/09 09:06:05]

Done.

     maximize_button_ = new views::ImageButton(this);
     maximize_button_->SetImage(
-        views::CustomButton::STATE_NORMAL,
-        rb.GetNativeImageNamed(IDR_APP_WINDOW_MAXIMIZE).ToImageSkia());
-    maximize_button_->SetImage(
         views::CustomButton::STATE_HOVERED,
         rb.GetNativeImageNamed(IDR_APP_WINDOW_MAXIMIZE_H).ToImageSkia());
     maximize_button_->SetImage(
@@ -91,9 +89,6 @@ void AppWindowFrameView::Init() {
     AddChildView(maximize_button_);
     restore_button_ = new views::ImageButton(this);
     restore_button_->SetImage(
-        views::CustomButton::STATE_NORMAL,
-        rb.GetNativeImageNamed(IDR_APP_WINDOW_RESTORE).ToImageSkia());
-    restore_button_->SetImage(
         views::CustomButton::STATE_HOVERED,
         rb.GetNativeImageNamed(IDR_APP_WINDOW_RESTORE_H).ToImageSkia());
     restore_button_->SetImage(
@@ -104,9 +99,6 @@ void AppWindowFrameView::Init() {
     AddChildView(restore_button_);
     minimize_button_ = new views::ImageButton(this);
     minimize_button_->SetImage(
-        views::CustomButton::STATE_NORMAL,
-        rb.GetNativeImageNamed(IDR_APP_WINDOW_MINIMIZE).ToImageSkia());
-    minimize_button_->SetImage(
         views::CustomButton::STATE_HOVERED,
         rb.GetNativeImageNamed(IDR_APP_WINDOW_MINIMIZE_H).ToImageSkia());
     minimize_button_->SetImage(
@@ -115,6 +107,8 @@ void AppWindowFrameView::Init() {
     minimize_button_->SetAccessibleName(
         l10n_util::GetStringUTF16(IDS_APP_ACCNAME_MINIMIZE));
     AddChildView(minimize_button_);
+
+    SetButtonImagesForFrame();
   }
 }
 
@@ -304,7 +298,8 @@ void AppWindowFrameView::OnPaint(gfx::Canvas* canvas) {
         rb.GetNativeImageNamed(IDR_APP_WINDOW_CLOSE_U).ToImageSkia());
   }
 
-  // TODO(jeremya): different look for inactive?
+  SetButtonImagesForFrame();
+  // TODO(benwells): different look for inactive by default.
   SkPaint paint;
   paint.setAntiAlias(false);
   paint.setStyle(SkPaint::kFill_Style);
@@ -366,4 +361,41 @@ void AppWindowFrameView::ButtonPressed(views::Button* sender,
     widget_->Minimize();
 }
 
+void AppWindowFrameView::SetButtonImagesForFrame() {
+  DCHECK(draw_frame_);
+
+  unsigned char frame_luma =
+      widget_->IsActive()
+          ? color_utils::GetLuminanceForColor(active_frame_color_)

[Matt Giuca, 2014/05/09 08:33:11]

Optional: Can I suggest a small refactor: add a method GetFrameColor that
returns widget_->IsActive() ? active_frame_color_ : innactive_frame_color_, and
use it here?

If not: Can you simplify this a bit:
unsigned char frame_luma = color_utils::GetLuminanceForColor(
    widget_->IsActive() ? active_frame_color_ :
    inactive_frame_color_);
(+ClangFormat)

[benwells, 2014/05/09 09:06:05]

Done.

+          : color_utils::GetLuminanceForColor(inactive_frame_color_);
+
+  // If the frame is dark, we should use the light images so they have
+  // some contrast.
+  const unsigned char kLuminanceThreshold = 100;
+  bool use_light = frame_luma < kLuminanceThreshold;
+
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();

[Matt Giuca, 2014/05/09 08:33:11]

const

(I looked up GetSharedInstance and I'm shocked that it returns a non-const
reference... but you shouldn't need non-const here.)

[benwells, 2014/05/09 09:06:05]

Yeah resource bundle isn't very consty. Needs to be non-const.

+  if (use_light) {
+    maximize_button_->SetImage(
+        views::CustomButton::STATE_NORMAL,
+        rb.GetNativeImageNamed(IDR_APP_WINDOW_MAXIMIZE_L).ToImageSkia());
+    restore_button_->SetImage(
+        views::CustomButton::STATE_NORMAL,
+        rb.GetNativeImageNamed(IDR_APP_WINDOW_RESTORE_L).ToImageSkia());
+    minimize_button_->SetImage(
+        views::CustomButton::STATE_NORMAL,
+        rb.GetNativeImageNamed(IDR_APP_WINDOW_MINIMIZE_L).ToImageSkia());
+  } else {
+    maximize_button_->SetImage(
+        views::CustomButton::STATE_NORMAL,
+        rb.GetNativeImageNamed(IDR_APP_WINDOW_MAXIMIZE).ToImageSkia());
+    restore_button_->SetImage(
+        views::CustomButton::STATE_NORMAL,
+        rb.GetNativeImageNamed(IDR_APP_WINDOW_RESTORE).ToImageSkia());
+    minimize_button_->SetImage(
+        views::CustomButton::STATE_NORMAL,
+        rb.GetNativeImageNamed(IDR_APP_WINDOW_MINIMIZE).ToImageSkia());
+  }
+}
+
 }  // namespace apps