[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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/profiles/avatar_button_controller.h"
 
 #include "base/mac/foundation_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #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"
 #import "ui/base/cocoa/hover_image_button.h"
 #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 {
 
 const CGFloat kButtonPadding = 12;
 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);
 const ui::NinePartImageIds kHoverBorderImageIds =
     IMAGE_GRID(IDR_AVATAR_MAC_BUTTON_HOVER);
 const ui::NinePartImageIds kPressedBorderImageIds =
     IMAGE_GRID(IDR_AVATAR_MAC_BUTTON_PRESSED);
 const ui::NinePartImageIds kThemedBorderImageIds =
     IMAGE_GRID(IDR_AVATAR_THEMED_MAC_BUTTON_NORMAL);
 
 NSImage* GetImageFromResourceID(int resourceId) {
   return ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed(
       resourceId).ToNSImage();
 }
 
 }  // namespace
 
 // Button cell with a custom border given by a set of nine-patch image grids.
 @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;
 }
 
 - (NSRect)drawTitle:(NSAttributedString*)title
           withFrame:(NSRect)frame
              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
   // was chosen by visual inspection.
   frame = NSOffsetRect(frame, kButtonDefaultPadding - kButtonPadding, 2);
   [super drawImage:image withFrame:frame inView:controlView];
 }
 
 - (void)drawBezelWithFrame:(NSRect)frame
                     inView:(NSView*)controlView {
   HoverState hoverState =
       [base::mac::ObjCCastStrict<HoverImageButton>(controlView) hoverState];
   ui::NinePartImageIds imageIds = kNormalBorderImageIds;
   if (isThemedWindow_)
     imageIds = kThemedBorderImageIds;
 
   if (hoverState == kHoverStateMouseDown)
     imageIds = kPressedBorderImageIds;
   else if (hoverState == kHoverStateMouseOver)
     imageIds = kHoverBorderImageIds;
   ui::DrawNinePartImage(frame, imageIds, NSCompositeSourceOver, 1.0, true);
 }
 
 - (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
 
 @implementation AvatarButtonController
 
 - (id)initWithBrowser:(Browser*)browser {
   if ((self = [super initWithBrowser:browser])) {
     ThemeService* themeService =
         ThemeServiceFactory::GetForProfile(browser->profile());
     isThemedWindow_ = !themeService->UsingSystemTheme();
 
     HoverImageButton* hoverButton =
         [[HoverImageButton alloc] initWithFrame:NSZeroRect];
     [hoverButton setDefaultImage:GetImageFromResourceID(
         IDR_AVATAR_MAC_BUTTON_DROPARROW)];
     [hoverButton setHoverImage:GetImageFromResourceID(
         IDR_AVATAR_MAC_BUTTON_DROPARROW_HOVER)];
     [hoverButton setPressedImage:GetImageFromResourceID(
         IDR_AVATAR_MAC_BUTTON_DROPARROW_PRESSED)];
 
     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_];
 
     [button_ setBezelStyle:NSShadowlessSquareBezelStyle];
     [button_ setButtonType:NSMomentaryChangeButton];
     [button_ setBordered:YES];
     // This is a workaround for an issue in the HoverImageButton where the
     // button is initially sized incorrectly unless a default image is provided.
     [button_ setImage:GetImageFromResourceID(IDR_AVATAR_MAC_BUTTON_DROPARROW)];
     [button_ setImagePosition:NSImageRight];
     [button_ setAutoresizingMask:NSViewMinXMargin | NSViewMinYMargin];
     [button_ setTarget:self];
     [button_ setAction:@selector(buttonClicked:)];
 
     [self updateAvatarButtonAndLayoutParent:NO];
 
     NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
     [center addObserver:self
                selector:@selector(themeDidChangeNotification:)
                    name:kBrowserThemeDidChangeNotification
                  object:nil];
-
   }
   return self;
 }
 
 - (void)dealloc {
   [[NSNotificationCenter defaultCenter] removeObserver:self];
   [super dealloc];
 }
 
 - (void)themeDidChangeNotification:(NSNotification*)aNotification {
@@ skipping 12 matching lines @@
   base::string16 name = profiles::GetAvatarNameForProfile(browser_->profile());
   int maxTextWidth = kMaxButtonContentWidth - [[button_ image] size].width;
   return gfx::ElideText(name, gfx::FontList(gfx::Font([button_ font])),
                         maxTextWidth, gfx::ELIDE_TAIL);
 }
 
 - (void)updateAvatarButtonAndLayoutParent:(BOOL)layoutParent {
   // 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];
 
   NSColor* foregroundColor;
   if (browser_->profile()->IsGuestSession()) {
     foregroundColor = [NSColor colorWithCalibratedWhite:1.0 alpha:0.9];
     [shadow setShadowColor:[NSColor colorWithCalibratedWhite:0.0 alpha:0.4]];
   } else if (!isThemedWindow_) {
     foregroundColor = [NSColor blackColor];
@@ skipping 29 matching lines @@
 
   if (layoutParent) {
     // Because the width of the button might have changed, the parent browser
     // frame needs to recalculate the button bounds and redraw it.
     [[BrowserWindowController
         browserWindowControllerForWindow:browser_->window()->GetNativeWindow()]
         layoutSubviews];
   }
 }
 
+- (void)updateErrorStatus:(BOOL)hasError {
+  [[button_ cell] setHasError:hasError];
+  [self updateAvatarButtonAndLayoutParent:YES];
+}
+
 @end