[Mac] Redesign of the avatar menu button

chrome/browser/ui/cocoa/browser_window_controller_private.mm

 // Copyright (c) 2012 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/browser_window_controller_private.h"
 
 #include <cmath>
 
 #include "base/command_line.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/fullscreen.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_info_util.h"
 #include "chrome/browser/ui/bookmarks/bookmark_tab_helper.h"
 #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"
 #import "chrome/browser/ui/cocoa/floating_bar_backing_view.h"
 #import "chrome/browser/ui/cocoa/framed_browser_window.h"
 #import "chrome/browser/ui/cocoa/fullscreen_mode_controller.h"
 #import "chrome/browser/ui/cocoa/fullscreen_window.h"
 #import "chrome/browser/ui/cocoa/infobars/infobar_container_controller.h"
 #include "chrome/browser/ui/cocoa/last_active_browser_cocoa.h"
 #import "chrome/browser/ui/cocoa/nsview_additions.h"
@@ skipping 18 matching lines @@
 
 namespace {
 
 // Space between the incognito badge and the right edge of the window.
 const CGFloat kAvatarRightOffset = 4;
 
 // The amount by which to shrink the tab strip (on the right) when the
 // 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.
 const CGFloat kLocBarLeftRightInset = 1;
 const CGFloat kLocBarTopInset = 0;
 const CGFloat kLocBarBottomInset = 1;
 
 }  // namespace
 
 @implementation BrowserWindowController(Private)
@@ skipping 240 matching lines @@
   [tabStripView setFrame:NSMakeRect(0, maxY, width, tabStripHeight)];
 
   // Set left indentation based on fullscreen mode status.
   [tabStripController_ setLeftIndentForControls:(fullscreen ? 0 :
       [[tabStripController_ class] defaultLeftIndentForControls])];
 
   // Lay out the icognito/avatar badge because calculating the indentation on
   // the right depends on it.
   if ([self shouldShowAvatar]) {
     NSView* avatarButton = [avatarButtonController_ view];
-    CGFloat buttonHeight = std::min(
+    CGFloat badgeXOffset = -kAvatarRightOffset;
+    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);
-    [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];
+      // 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);
     [avatarButton setFrameOrigin:origin];
     [avatarButton setHidden:NO];  // Make sure it's shown.
   }
 
   // Calculate the right indentation.  The default indentation built into the
   // tabstrip leaves enough room for the fullscreen button or presentation mode
   // toggle button on Lion.  On non-Lion systems, the right indent needs to be
   // adjusted to make room for the new tab button when an avatar is present.
   CGFloat rightIndent = 0;
   if (base::mac::IsOSLionOrLater() &&
       [[self window] isKindOfClass:[FramedBrowserWindow class]]) {
     FramedBrowserWindow* window =
         static_cast<FramedBrowserWindow*>([self window]);
     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.
   [tabStripController_ layoutTabsWithoutAnimation];
 
   return maxY;
 }
 
 - (CGFloat)layoutToolbarAtMinX:(CGFloat)minX
                           maxY:(CGFloat)maxY
@@ skipping 197 matching lines @@
   // With this call, valgrind complains that a "Conditional jump or move depends
   // on uninitialised value(s)".  The error happens in -[NSThemeFrame
   // drawOverlayRect:].  I'm pretty convinced this is an Apple bug, but there is
   // no visual impact.  I have been unable to tickle it away with other window
   // or view manipulation Cocoa calls.  Stack added to suppressions_mac.txt.
   [contentView setAutoresizesSubviews:YES];
   [destWindow setContentView:contentView];
 
   // 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
   // badge (if any), so that the tab strip will be on top (in the z-order).
   if ([self hasTabStrip])
     [[[destWindow contentView] superview] addSubview:tabStripView];
 
   [sourceWindow setWindowController:nil];
   [self setWindow:destWindow];
   [destWindow setWindowController:self];
@@ skipping 430 matching lines @@
     return browser_window_controller::kCoreAnimationEnabledLazy;
   }
   if (CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
           switches::kUseCoreAnimation) == "disabled") {
     return browser_window_controller::kCoreAnimationDisabled;
   }
   return browser_window_controller::kCoreAnimationEnabledAlways;
 }
 
 @end  // @implementation BrowserWindowController(Private)