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

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

Index: chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
diff --git a/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm b/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
index ea26e90dc3ef73cd94a95cf74770d8392f846bab..61b2771dbafd454d870488cb57921af54b3bd8fd 100644
--- a/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
+++ b/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
@@ -230,6 +230,31 @@ NSView* BuildTitleCard(NSRect frame_rect,
   return container.autorelease();
 }
 
+bool HasAuthError(Profile* profile) {

[msw, 2014/06/03 04:15:31]

nit: this is used twice, is a helper necessary?

[noms (inactive), 2014/06/06 20:33:44]

I thought it looked a little nicer, because the three lines of code look a
little annoying, but I can inline it if you want to. 
On 2014/06/03 04:15:31, msw wrote:

+  const SigninErrorController* error =
+      profiles::GetSigninErrorController(profile);
+  return error && error->HasError();
+}
+
+std::string GetAuthErrorAccountId(Profile* profile) {

[msw, 2014/06/03 04:15:31]

nit: this is used once, please inline it.

[noms (inactive), 2014/06/06 20:33:44]

Done.

+  SigninErrorController* error =
+      ProfileOAuth2TokenServiceFactory::GetForProfile(profile)->
+          signin_error_controller();
+  if (!error)

[msw, 2014/06/03 04:15:31]

nit: return error ? error->error_account_id() : std::string() if you keep this.

[noms (inactive), 2014/06/06 20:33:44]

Done.

+    return std::string();
+
+  return error->error_account_id();
+}
+
+std::string GetAuthErrorUsername(Profile* profile) {

[msw, 2014/06/03 04:15:31]

nit: this is used once, please inline it.

[noms (inactive), 2014/06/06 20:33:43]

Done.

+  const SigninErrorController* error =
+      profiles::GetSigninErrorController(profile);
+  if (!error)

[msw, 2014/06/03 04:15:31]

nit: return error ? error->error_username() : std::string() if you keep this.

[noms (inactive), 2014/06/06 20:33:44]

Done.

+    return std::string();
+
+  return error->error_username();
+}
+
 }  // namespace
 
 // Class that listens to changes to the OAuth2Tokens for the active profile,
@@ -281,7 +306,8 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
     profiles::BubbleViewMode viewMode = [controller_ viewMode];
     if (viewMode == profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT ||
         viewMode == profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN ||
-        viewMode == profiles::BUBBLE_VIEW_MODE_GAIA_ADD_ACCOUNT) {
+        viewMode == profiles::BUBBLE_VIEW_MODE_GAIA_ADD_ACCOUNT ||
+        viewMode == profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH) {
       [controller_ initMenuContentsWithView:
           profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT];
     }
@@ -736,11 +762,13 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
                      frameOrigin:(NSPoint)frameOrigin
                           action:(SEL)action;
 
-// Creates an email account button with |title| and a remove icon. |tag|
+// Creates an email account button with |title| and a remove icon. If
+// |reauthRequired| is true, the button also displays a warning icon. |tag|
 // indicates which account the button refers to.
 - (NSButton*)accountButtonWithRect:(NSRect)rect
                              title:(const std::string&)title
-                               tag:(int)tag;
+                               tag:(int)tag
+                    reauthRequired:(BOOL)reauthRequired;
 
 @end
 
@@ -816,6 +844,11 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   [self initMenuContentsWithView:profiles::BUBBLE_VIEW_MODE_ACCOUNT_REMOVAL];
 }
 
+- (IBAction)showAccountReauthView:(id)sender {
+  DCHECK(!isGuestSession_);
+  [self initMenuContentsWithView:profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH];
+}
+
 - (IBAction)removeAccount:(id)sender {
   DCHECK(!accountIdToRemove_.empty());
   ProfileOAuth2TokenServiceFactory::GetPlatformSpecificForProfile(
@@ -903,6 +936,13 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
     // Guest profiles do not have a token service.
     isGuestSession_ = browser_->profile()->IsGuestSession();
 
+    // If view mode is PROFILE_CHOOSER but there is an auth error, force
+    // ACCOUNT_MANAGEMENT mode.
+    if (viewMode_ == profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER &&
+        HasAuthError(browser_->profile())) {
+      viewMode_ = profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT;
+    }
+
     [[self bubble] setAlignment:info_bubble::kAlignRightEdgeToAnchorEdge];
     [[self bubble] setArrowLocation:info_bubble::kNoArrow];
     [[self bubble] setBackgroundColor:GetDialogBackgroundColor()];
@@ -1481,21 +1521,28 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   std::vector<std::string>accounts =
       profiles::GetSecondaryAccountsForProfile(profile, primaryAccount);
 
+  // Get state of authentication error, if any.

[msw, 2014/06/03 04:15:31]

nit: this comment should be made more helpful or removed.

[noms (inactive), 2014/06/06 20:33:43]

Done.

+  std::string errorAccountId = GetAuthErrorAccountId(profile);
+
   rect.origin.y = 0;
   for (size_t i = 0; i < accounts.size(); ++i) {
     // Save the original email address, as the button text could be elided.
     currentProfileAccounts_[i] = accounts[i];
-    NSButton* accountButton = [self accountButtonWithRect:rect
-                                                    title:accounts[i]
-                                                      tag:i];
+    NSButton* accountButton =
+        [self accountButtonWithRect:rect
+                              title:accounts[i]
+                                tag:i
+                     reauthRequired:errorAccountId == accounts[i]];

[msw, 2014/06/03 04:15:31]

Couldn't multiple accounts have auth errors at the same time?

[noms (inactive), 2014/06/06 20:33:44]

They could, but the way it works right now is that the SigninErrorController
will only return one error at a time. Once you resolve that, you might get
another error, for a different account.
On 2014/06/03 04:15:31, msw wrote:

[msw, 2014/06/09 19:02:38]

It's also unclear to me how this retains the authorization error for
account/button A if another error appears for account/button B. Does
SigninErrorController only express an authorization error for one account at a
time, and then express subsequent account errors once the first error has been
resolved? (if so, why?). Can you add a comment explaining the pattern here, it's
definitely not obvious. 

[noms (inactive), 2014/06/10 15:02:47]

A little background: this error controller used to be used in the hot dog menu,
where we could only badge the menu for one error at a time. So the controller
sort of queues up errors. If you're badging account A with an error, and account
B also has an error, the controller will only care about the account A error
until it gets resolved. Once it gets resolved, it goes and looks for any other
errors. This works the same in the Windows menu too.

In theory, now that we *could* display multiple errors in parallel, the
controller could change, but that's a bit of a giant yak shave that I don't
thing belongs in this CL. :)

On 2014/06/09 19:02:38, msw wrote:

     [container addSubview:accountButton];
     rect.origin.y = NSMaxY([accountButton frame]);
   }
 
   // The primary account should always be listed first.
-  NSButton* accountButton = [self accountButtonWithRect:rect
-                                                  title:primaryAccount
-                                                    tag:kPrimaryProfileTag];
+  NSButton* accountButton =
+      [self accountButtonWithRect:rect
+                            title:primaryAccount
+                              tag:kPrimaryProfileTag
+                   reauthRequired:errorAccountId == primaryAccount];
   [container addSubview:accountButton];
   [container setFrameSize:NSMakeSize(NSWidth([container frame]),
                                      NSMaxY([accountButton frame]))];
@@ -1507,20 +1554,39 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
       [[NSView alloc] initWithFrame:NSZeroRect]);
   CGFloat yOffset = 0;
 
-  bool addSecondaryAccount =
-      viewMode_ == profiles::BUBBLE_VIEW_MODE_GAIA_ADD_ACCOUNT;
-  signin::Source source = addSecondaryAccount ?
-      signin::SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT :
-      signin::SOURCE_AVATAR_BUBBLE_SIGN_IN;
+  GURL url;
+  int messageId;

[msw, 2014/06/03 04:15:31]

nit: initialize this value

[noms (inactive), 2014/06/06 20:33:44]

Done.

+  switch (viewMode_) {
+    case profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN:
+      url = signin::GetPromoURL(signin::SOURCE_AVATAR_BUBBLE_SIGN_IN,
+                                false /* auto_close */,
+                                true /* is_constrained */);
+      messageId = IDS_PROFILES_GAIA_SIGNIN_TITLE;
+      break;
+    case profiles::BUBBLE_VIEW_MODE_GAIA_ADD_ACCOUNT:
+      url = signin::GetPromoURL(signin::SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT,
+                                false /* auto_close */,
+                                true /* is_constrained */);
+      messageId = IDS_PROFILES_GAIA_ADD_ACCOUNT_TITLE;
+      break;
+    case profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH: {

[msw, 2014/06/03 04:15:31]

nit: you shouldn't need these curly braces.

[noms (inactive), 2014/06/06 20:33:44]

Without the curly braces, the compiler is unhappy with defining the
SigninErrorController in only one of the cases, since all the cases are in the
same scope. I've moved the definition to outside the switch. 

+      DCHECK(HasAuthError(browser_->profile()));
+      url = signin::GetReauthURL(browser_->profile(),
+                                 GetAuthErrorUsername(browser_->profile()));
+      messageId = IDS_PROFILES_GAIA_REAUTH_TITLE;
+      break;
+    }
+    default:
+      NOTREACHED() << "Called with invalid mode=" << viewMode_;
+      return NULL;
+  }
 
   webContents_.reset(content::WebContents::Create(
       content::WebContents::CreateParams(browser_->profile())));
-  webContents_->GetController().LoadURL(
-      signin::GetPromoURL(
-          source, false /* auto_close */, true /* is_constrained */),
-      content::Referrer(),
-      content::PAGE_TRANSITION_AUTO_TOPLEVEL,
-      std::string());
+  webContents_->GetController().LoadURL(url,
+                                        content::Referrer(),
+                                        content::PAGE_TRANSITION_AUTO_TOPLEVEL,
+                                        std::string());
   NSView* webview = webContents_->GetNativeView();
   [webview setFrameSize:NSMakeSize(kFixedGaiaViewWidth, kFixedGaiaViewHeight)];
   [container addSubview:webview];
@@ -1533,9 +1599,8 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   yOffset = NSMaxY([separator frame]) + kSmallVerticalSpacing;
 
   NSView* titleView = BuildTitleCard(
-      NSMakeRect(0, yOffset, kFixedGaiaViewWidth,0),
-      addSecondaryAccount ? IDS_PROFILES_GAIA_ADD_ACCOUNT_TITLE :
-                            IDS_PROFILES_GAIA_SIGNIN_TITLE,
+      NSMakeRect(0, yOffset, kFixedGaiaViewWidth, 0),
+      messageId,
       self /* backButtonTarget*/,
       @selector(navigateBackFromSigninPage:) /* backButtonAction */);
   [container addSubview:titleView];
@@ -1724,29 +1789,45 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
 
 - (NSButton*)accountButtonWithRect:(NSRect)rect
                              title:(const std::string&)title
-                               tag:(int)tag {
+                               tag:(int)tag
+                    reauthRequired:(BOOL)reauthRequired {
+  ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
+  NSImage* deleteImage = rb->GetNativeImageNamed(IDR_CLOSE_1).AsNSImage();
+  CGFloat deleteImageWidth = [deleteImage size].width;
+
+  NSImage* reauthImage = nil;
+  CGFloat reauthImageWidth = 0;
+  if (reauthRequired) {
+    reauthImage = rb->GetNativeImageNamed(
+        IDR_ICON_PROFILES_ACCOUNT_BUTTON_ERROR).AsNSImage();
+    reauthImageWidth = [reauthImage size].width;

[msw, 2014/06/03 04:15:31]

Do you need to add some padding, like kHorizontalSpacing, here or below?

[noms (inactive), 2014/06/06 20:33:44]

Done.

+  }
+
+  CGFloat availableTextWidth = rect.size.width - kHorizontalSpacing -
+      deleteImageWidth - reauthImageWidth;
+
   NSColor* backgroundColor = gfx::SkColorToCalibratedNSColor(
       profiles::kAvatarBubbleAccountsBackgroundColor);
   base::scoped_nsobject<BackgroundColorHoverButton> button(
       [[BackgroundColorHoverButton alloc] initWithFrame:rect
                                       imageTitleSpacing:0
                                         backgroundColor:backgroundColor]);
-  ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
-  NSImage* defaultImage = rb->GetNativeImageNamed(IDR_CLOSE_1).AsNSImage();
-  CGFloat kDeleteButtonWidth = [defaultImage size].width;
-  CGFloat availableWidth = rect.size.width -
-      kDeleteButtonWidth - kHorizontalSpacing;
-  [button setTitle:ElideEmail(title, availableWidth)];
+  [button setTitle:ElideEmail(title, availableTextWidth)];
   [button setAlignment:NSLeftTextAlignment];
   [button setBordered:NO];
+  if (reauthRequired) {
+    [button setTarget:self];
+    [button setAction:@selector(showAccountReauthView:)];
+    [button setTag:tag];
+  }
 
   // Delete button.
-  rect.origin = NSMakePoint(availableWidth, 0);
-  rect.size.width = kDeleteButtonWidth;
+  rect.origin = NSMakePoint(availableTextWidth + reauthImageWidth, 0);
+  rect.size.width = deleteImageWidth;
   base::scoped_nsobject<HoverImageButton> deleteButton(
       [[HoverImageButton alloc] initWithFrame:rect]);
   [deleteButton setBordered:NO];
-  [deleteButton setDefaultImage:defaultImage];
+  [deleteButton setDefaultImage:deleteImage];
   [deleteButton setHoverImage:rb->GetNativeImageNamed(
       IDR_CLOSE_1_H).ToNSImage()];
   [deleteButton setPressedImage:rb->GetNativeImageNamed(
@@ -1756,6 +1837,16 @@ class ActiveProfileObserverBridge : public AvatarMenuObserver,
   [deleteButton setTag:tag];
 
   [button addSubview:deleteButton];
+
+  // Warning icon.
+  if (reauthRequired) {
+    base::scoped_nsobject<NSImageView> reauthIcon(
+        [[NSImageView alloc] initWithFrame:NSMakeRect(
+            availableTextWidth, 0, reauthImageWidth, rect.size.height)]);
+    [reauthIcon setImage:reauthImage];
+    [button addSubview:reauthIcon];
+  }
+
   return button.autorelease();
 }