[Mac] Show a warning in the new avatar button/menu if there's an auth error

chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm

Index: chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
diff --git a/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm b/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
index dbacedb55c10b49c0194a04406f9fd55bcd548d0..95951ef78cdb91919834837dae8222efdadd6417 100644
--- a/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
+++ b/chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
+#include "components/signin/core/browser/signin_error_controller.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #import "ui/base/cocoa/appkit_utils.h"
@@ -19,6 +20,8 @@
 #include "ui/base/l10n/l10n_util_mac.h"
 #include "ui/base/nine_image_painter_factory.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image_skia_operations.h"
+#include "ui/gfx/image/image_skia_util_mac.h"
 #include "ui/gfx/text_elider.h"
 
 namespace {
@@ -28,6 +31,8 @@ const CGFloat kButtonDefaultPadding = 5;
 const CGFloat kButtonHeight = 27;
 const CGFloat kButtonTitleImageSpacing = 10;
 const CGFloat kMaxButtonContentWidth = 100;
+const CGFloat kAuthErrorIconWidth = 13;
+const CGFloat kAuthErrorIconHeight = 11;
 
 const ui::NinePartImageIds kNormalBorderImageIds =
     IMAGE_GRID(IDR_AVATAR_MAC_BUTTON_NORMAL);
@@ -49,21 +54,34 @@ NSImage* GetImageFromResourceID(int resourceId) {
 @interface CustomThemeButtonCell : NSButtonCell {
  @private
    BOOL isThemedWindow_;
+   BOOL hasError_;
+   base::scoped_nsobject<NSImage> authErrorImage_;

[groby-ooo-7-16, 2014/06/10 19:28:14]

authenticationErrorImage_, please. (This is Cocoa. Verbosity is a must :)

[noms (inactive), 2014/06/17 17:07:32]

Done.

 }
 - (void)setIsThemedWindow:(BOOL)isThemedWindow;
+- (void)setHasError:(BOOL)hasError;
+
 @end
 
 @implementation CustomThemeButtonCell
 - (id)initWithThemedWindow:(BOOL)isThemedWindow {
   if ((self = [super init])) {
     isThemedWindow_ = isThemedWindow;
+    hasError_ = NO;
+
+    gfx::ImageSkia icon = gfx::ImageSkiaOperations::CreateResizedImage(

[groby-ooo-7-16, 2014/06/10 19:28:14]

Why are we resizing a static resource? Can we instead just check in a properly
sized one?

[noms (inactive), 2014/06/11 15:12:46]

Mike didn't want me to check in duplicate, just resized, images, so he asked me
to do the resizing. You guys can fight it out. I'd prefer the duplicate
resource, personally, because a) it's already there and b) the resizing code is
scary :)

On 2014/06/10 19:28:14, groby wrote:

[msw, 2014/06/11 16:42:36]

Don't block this CL on that, you can use the duplicate resource, I'm just sad
that we have a dozen nearly identical icons.

[groby-ooo-7-16, 2014/06/11 17:19:09]

Not a blocking issue for me, either - I'm just sad we have code to resize static
image resources :)

(Mike: Just to make your day, we have them all in multiple densities, too ;)

[msw, 2014/06/11 17:33:59]

My concern wasn't entirely about binary size, we have inconsistent iconography.
See http://crbug.com/378467: Chrome has too many distinct warning icons.

[noms (inactive), 2014/06/17 17:07:32]

Done.

+        *ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+            IDR_ICON_PROFILES_ACCOUNT_BUTTON_ERROR).ToImageSkia(),
+        skia::ImageOperations::RESIZE_BEST,
+        gfx::Size(kAuthErrorIconWidth, kAuthErrorIconHeight));
+    authErrorImage_.reset([gfx::NSImageFromImageSkia(icon) retain]);
   }
   return self;
 }
 
 - (NSSize)cellSize {
   NSSize buttonSize = [super cellSize];
-  buttonSize.width += 2 * kButtonPadding - 2 * kButtonDefaultPadding;
+  CGFloat errorWidth = hasError_ ? kAuthErrorIconWidth : 0;
+  buttonSize.width += 2 * (kButtonPadding - kButtonDefaultPadding) + errorWidth;
   buttonSize.height = kButtonHeight;
   return buttonSize;
 }
@@ -73,13 +91,29 @@ NSImage* GetImageFromResourceID(int resourceId) {
              inView:(NSView*)controlView {
   frame.origin.x = kButtonPadding;
   // Ensure there's always a padding between the text and the image.
-  frame.size.width -= kButtonTitleImageSpacing;
+  frame.size.width -= hasError_ ? 2 * kButtonTitleImageSpacing :
+                                  kButtonTitleImageSpacing;
   return [super drawTitle:title withFrame:frame inView:controlView];
 }
 
 - (void)drawImage:(NSImage*)image
         withFrame:(NSRect)frame
            inView:(NSView*)controlView {
+  // If there's an auth error, draw a warning icon before the cell image.
+  if (hasError_) {
+    NSSize imageSize = [authErrorImage_ size];
+    NSRect rect = NSMakeRect(
+        frame.origin.x - imageSize.width - kButtonTitleImageSpacing,

[groby-ooo-7-16, 2014/06/10 19:28:14]

Am I misreading this, or are you planning to draw outside the frame given to you
in drawImage? (In which case, eeew! ;)

It'd probably cleaner to draw the error icon in -drawInteriorWithFrame:, and
then call super's -drawInterior with a reduced frame. It also will consolidate
the handling of the spacing between errorimage and title.

[noms (inactive), 2014/06/17 17:07:32]

Hmm, so the layout of the button is: 
[padding][title][spacing][optional error icon][optional spacing][drop down
arrow].

I was planning (and succeeding) on committing the "draw outside a frame sin" , 
so I moved the drawing of the icon in -drawTitle though, so that it's being
drawn inside
a given frame. Does it look ok?

On 2014/06/10 19:28:14, groby wrote:

[groby-ooo-7-16, 2014/06/17 17:16:41]

It looked at first glance... let me recheck

+        (kButtonHeight - imageSize.height) / 2,
+        imageSize.width,
+        imageSize.height);
+    [authErrorImage_ drawInRect:rect
+                       fromRect:NSZeroRect
+                      operation:NSCompositeSourceOver
+                       fraction:1.0
+                 respectFlipped:YES
+                          hints:nil];
+  }
   // For the x-offset, we need to undo the default padding and apply the
   // new one. For the y-offset, increasing the button height means we need
   // to move the image a little down to align it nicely with the text; this
@@ -106,11 +140,17 @@ NSImage* GetImageFromResourceID(int resourceId) {
 - (void)setIsThemedWindow:(BOOL)isThemedWindow {
   isThemedWindow_ = isThemedWindow;
 }
+
+- (void)setHasError:(BOOL)hasError {

[groby-ooo-7-16, 2014/06/10 19:28:14]

Assuming that having an error on an account is rare, I'd suggest getting rid of
hasError_ and just resetting authErrorImage_ appropriately.

[noms (inactive), 2014/06/17 17:07:32]

Done.

+  hasError_ = hasError;
+}
+
 @end
 
 @interface AvatarButtonController (Private)
 - (base::string16)getElidedAvatarName;
 - (void)updateAvatarButtonAndLayoutParent:(BOOL)layoutParent;
+- (void)updateErrorStatus:(BOOL)hasError;
 - (void)dealloc;
 - (void)themeDidChangeNotification:(NSNotification*)aNotification;
 @end
@@ -135,6 +175,10 @@ NSImage* GetImageFromResourceID(int resourceId) {
     button_.reset(hoverButton);
     base::scoped_nsobject<CustomThemeButtonCell> cell(
         [[CustomThemeButtonCell alloc] initWithThemedWindow:isThemedWindow_]);
+    SigninErrorController* error =
+        profiles::GetSigninErrorController(browser->profile());
+    if (error)
+      [cell setHasError:error->HasError()];

[groby-ooo-7-16, 2014/06/10 19:28:14]

I'd actually remove this - it gets rid of the dependency on the
SigninErrorController. 

Either have callers call -updateErrorStatus: immediately after
-initWithBrowser:, or if you don't trust the caller, pass in as a param. (I'd
trust the caller :)

[noms (inactive), 2014/06/17 17:07:32]

Hmm, but then it adds that dependency to the BrowserWindowController. Isn't that
worse?

On 2014/06/10 19:28:14, groby wrote:

     [button_ setCell:cell.get()];
     [self setView:button_];
 
@@ -156,7 +200,6 @@ NSImage* GetImageFromResourceID(int resourceId) {
                selector:@selector(themeDidChangeNotification:)
                    name:kBrowserThemeDidChangeNotification
                  object:nil];
-
   }
   return self;
 }
@@ -189,7 +232,6 @@ NSImage* GetImageFromResourceID(int resourceId) {
   // The button text has a black foreground and a white drop shadow for regular
   // windows, and a light text with a dark drop shadow for guest windows
   // which are themed with a dark background.
-  // TODO(noms): Figure out something similar for themed windows, if possible.
   base::scoped_nsobject<NSShadow> shadow([[NSShadow alloc] init]);
   [shadow setShadowOffset:NSMakeSize(0, -1)];
   [shadow setShadowBlurRadius:0];
@@ -239,4 +281,9 @@ NSImage* GetImageFromResourceID(int resourceId) {
   }
 }
 
+- (void)updateErrorStatus:(BOOL)hasError {
+  [[button_ cell] setHasError:hasError];
+  [self updateAvatarButtonAndLayoutParent:YES];
+}
+
 @end