[Mac] Redesign of the avatar menu button

chrome/browser/ui/cocoa/browser_window_controller_private.mm

Index: chrome/browser/ui/cocoa/browser_window_controller_private.mm
diff --git a/chrome/browser/ui/cocoa/browser_window_controller_private.mm b/chrome/browser/ui/cocoa/browser_window_controller_private.mm
index 208342012934a71c1790347faee2b018814bb934..444859fd612bd34fcdb4e5f611ae6ee4382b2b39 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller_private.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller_private.mm
@@ -18,6 +18,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window_state.h"
 #import "chrome/browser/ui/cocoa/browser/avatar_button_controller.h"
+#import "chrome/browser/ui/cocoa/browser/avatar_icon_controller.h"
 #import "chrome/browser/ui/cocoa/dev_tools_controller.h"
 #import "chrome/browser/ui/cocoa/fast_resize_view.h"
 #import "chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.h"
@@ -56,6 +57,10 @@ const CGFloat kAvatarRightOffset = 4;
 // incognito badge is present.
 const CGFloat kAvatarTabStripShrink = 18;
 
+// Width of the full screen icon. Used to position the AvatarButton to the
+// left of the icon.
+const CGFloat kFullscreenIconWidth = 30;
+
 // Insets for the location bar, used when the full toolbar is hidden.
 // TODO(viettrungluu): We can argue about the "correct" insetting; I like the
 // following best, though arguably 0 inset is better/more correct.
@@ -316,14 +321,24 @@ willPositionSheet:(NSWindow*)sheet
   // the right depends on it.
   if ([self shouldShowAvatar]) {
     NSView* avatarButton = [avatarButtonController_ view];
-    CGFloat buttonHeight = std::min(
-        static_cast<CGFloat>(profiles::kAvatarIconHeight), tabStripHeight);
-    [avatarButton setFrameSize:NSMakeSize(NSWidth([avatarButton frame]),
-                                          buttonHeight)];
-
-    // Actually place the badge *above* |maxY|, by +2 to miss the divider.
     CGFloat badgeXOffset = -kAvatarRightOffset;
-    CGFloat badgeYOffset = 2 * [[avatarButton superview] cr_lineWidth];
+    CGFloat badgeYOffset = 0;
+
+    if ([self shouldUseNewAvatarButton]) {
+      // The fullscreen icon is displayed to the right of the avatar button.
+      if (![self isFullscreen])
+        badgeXOffset -= kFullscreenIconWidth;
+
+      // Center the button vertically on the tabstrip.
+      badgeYOffset = (tabStripHeight - [avatarButton frame].size.height) /2;

[Nico, 2014/01/14 03:56:02]

NSHeight([avatarButton frame])


…don't you need to call setFrameSize: in this branch?

[noms (inactive), 2014/01/15 22:07:47]

I don't know how to test this (tabstrip needs to be smaller than the icon/button
height), but I trust it's here for a reason, so I've added it to the other
branch too.

On 2014/01/14 03:56:02, Nico wrote:

+    } else {
+      CGFloat buttonHeight = std::min(
+        static_cast<CGFloat>(profiles::kAvatarIconHeight), tabStripHeight);
+      // Actually place the badge *above* |maxY|, by +2 to miss the divider.
+      badgeYOffset = 2 * [[avatarButton superview] cr_lineWidth];
+      [avatarButton setFrameSize:NSMakeSize(NSWidth([avatarButton frame]),
+                                            buttonHeight)];
+    }
     NSPoint origin =
         NSMakePoint(width - NSWidth([avatarButton frame]) + badgeXOffset,
                     maxY + badgeYOffset);
@@ -343,10 +358,22 @@ willPositionSheet:(NSWindow*)sheet
     rightIndent += -[window fullScreenButtonOriginAdjustment].x;
   } else if ([self shouldShowAvatar]) {
     rightIndent += kAvatarTabStripShrink;
-    NSButton* labelButton = [avatarButtonController_ labelButtonView];
-    if (labelButton)
-      rightIndent += NSWidth([labelButton frame]) + kAvatarRightOffset;
+
+    if ([avatarButtonController_ isKindOfClass:[AvatarIconController class]]) {
+      NSButton* labelButton =
+          [static_cast<AvatarIconController*>(avatarButtonController_)
+              labelButtonView];
+      if (labelButton)
+        rightIndent += NSWidth([labelButton frame]) + kAvatarRightOffset;
+    }
   }
+
+  // If the new avatar button is displayed, we need to account for its width.
+  if ([self shouldShowAvatar] && [self shouldUseNewAvatarButton]) {
+    rightIndent += NSWidth([[avatarButtonController_ view] frame])
+        + kAvatarTabStripShrink;

[Nico, 2014/01/14 03:56:02]

Isn't this what the else-if 4 lines above does? Shouldn't this be an else to the
isKindOfClass: in there? (And shouldn't the isKindOfClass: there be spelled
![self shouldUseNewAvatarButton]?)

[noms (inactive), 2014/01/15 22:07:47]

Oops! Yes on all counts. On 2014/01/14 03:56:02, Nico wrote:

+  }
+
   [tabStripController_ setRightIndentForControls:rightIndent];
 
   // Go ahead and layout the tabs.
@@ -564,10 +591,11 @@ willPositionSheet:(NSWindow*)sheet
 
   // Move the incognito badge if present.
   if ([self shouldShowAvatar]) {
-    [[avatarButtonController_ view] removeFromSuperview];
-    [[avatarButtonController_ view] setHidden:YES];  // Will be shown in layout.
-    [[[destWindow contentView] superview] addSubview:
-        [avatarButtonController_ view]];
+    NSView* avatarButtonView = [avatarButtonController_ view];

[Nico, 2014/01/14 03:56:02]

Nice change!

[noms (inactive), 2014/01/15 22:07:47]

To preemptively balance the oops above, obviously.
On 2014/01/14 03:56:02, Nico wrote:

+
+    [avatarButtonView removeFromSuperview];
+    [avatarButtonView setHidden:YES];  // Will be shown in layout.
+    [[[destWindow contentView] superview] addSubview: avatarButtonView];
   }
 
   // Add the tab strip after setting the content view and moving the incognito