Mac: Correctly clip tab title...

chrome/browser/ui/cocoa/tab_controller.mm

 // Copyright (c) 2010 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.
 
 #include "app/l10n_util_mac.h"
 #include "base/mac_util.h"
 #import "chrome/browser/themes/browser_theme_provider.h"
 #import "chrome/browser/ui/cocoa/menu_controller.h"
 #import "chrome/browser/ui/cocoa/tab_controller.h"
 #import "chrome/browser/ui/cocoa/tab_controller_target.h"
@@ skipping 96 matching lines @@
 
 // Called when the tab's nib is done loading and all outlets are hooked up.
 - (void)awakeFromNib {
   // Remember the icon's frame, so that if the icon is ever removed, a new
   // one can later replace it in the proper location.
   originalIconFrame_ = [iconView_ frame];
 
   // When the icon is removed, the title expands to the left to fill the space
   // left by the icon.  When the close button is removed, the title expands to
   // the right to fill its space.  These are the amounts to expand and contract
-  // titleView_ under those conditions.
+  // titleView_ under those conditions. We don't have to explicilty save the
+  // offset between the title and the close button since we can just get that
+  // value for the close button's frame.
   NSRect titleFrame = [titleView_ frame];
   iconTitleXOffset_ = NSMinX(titleFrame) - NSMinX(originalIconFrame_);
-  titleCloseWidthOffset_ = NSMaxX([closeButton_ frame]) - NSMaxX(titleFrame);
 
   [self internalSetSelected:selected_];
 }
 
 // Called when Cocoa wants to display the context menu. Lazily instantiate
 // the menu based off of the cross-platform model. Re-create the menu and
 // model every time to get the correct labels and enabling.
 - (NSMenu*)menu {
   contextMenuDelegate_.reset(
       new TabControllerInternal::MenuDelegate(target_, self));
@@ skipping 88 matching lines @@
 }
 
 // Returns YES if we should be showing the close button. The selected tab
 // always shows the close button.
 - (BOOL)shouldShowCloseButton {
   if ([self mini])
     return NO;
   return ([self selected] || [self iconCapacity] >= 3);
 }
 
+- (NSTextField*)titleView {
+  return titleView_;
+}
+
+- (HoverCloseButton*)closeButton {
+  return closeButton_;
+}
+
 - (void)updateVisibility {
   // iconView_ may have been replaced or it may be nil, so [iconView_ isHidden]
   // won't work.  Instead, the state of the icon is tracked separately in
   // isIconShowing_.
-  BOOL oldShowIcon = isIconShowing_ ? YES : NO;
   BOOL newShowIcon = [self shouldShowIcon] ? YES : NO;

[viettrungluu, 2010/12/14 00:19:50]

The |? YES : NO| here is just strange. Could you get rid of it as a drive-by?

[sail, 2010/12/14 01:59:53]

Done.

 
   [iconView_ setHidden:newShowIcon ? NO : YES];

[viettrungluu, 2010/12/14 00:19:50]

Ditto. Should just be |!newShowIcon|.

[sail, 2010/12/14 01:59:53]

Done.

   isIconShowing_ = newShowIcon;
 
   // If the tab is a mini-tab, hide the title.
   [titleView_ setHidden:[self mini]];
 
-  BOOL oldShowCloseButton = [closeButton_ isHidden] ? NO : YES;
   BOOL newShowCloseButton = [self shouldShowCloseButton] ? YES : NO;

[viettrungluu, 2010/12/14 00:19:50]

Etc.

[sail, 2010/12/14 01:59:53]

Done.

 
   [closeButton_ setHidden:newShowCloseButton ? NO : YES];
 
   // Adjust the title view based on changes to the icon's and close button's
   // visibility.
-  NSRect titleFrame = [titleView_ frame];
+  NSRect oldTitleFrame = [titleView_ frame];
+  NSRect newTitleFrame;
+  newTitleFrame.size.height = oldTitleFrame.size.height;

[viettrungluu, 2010/12/14 00:19:50]

NSMakeRect and a couple of uses of ?: would be more concise.

[sail, 2010/12/14 01:59:53]

Unfortunately I need a temporary variable here since the width depends on the
origin.

+  newTitleFrame.origin.y = oldTitleFrame.origin.y;
 
-  if (oldShowIcon != newShowIcon) {
-    // Adjust the left edge of the title view according to the presence or
-    // absence of the icon view.
-
-    if (newShowIcon) {
-      titleFrame.origin.x += iconTitleXOffset_;
-      titleFrame.size.width -= iconTitleXOffset_;
-    } else {
-      titleFrame.origin.x -= iconTitleXOffset_;
-      titleFrame.size.width += iconTitleXOffset_;
-    }
+  if (newShowIcon) {
+    newTitleFrame.origin.x = originalIconFrame_.origin.x + iconTitleXOffset_;
+  } else {
+    newTitleFrame.origin.x = originalIconFrame_.origin.x;
   }
 
-  if (oldShowCloseButton != newShowCloseButton) {
-    // Adjust the right edge of the title view according to the presence or
-    // absence of the close button.
-    if (newShowCloseButton)
-      titleFrame.size.width -= titleCloseWidthOffset_;
-    else
-      titleFrame.size.width += titleCloseWidthOffset_;
+  if (newShowCloseButton) {
+    newTitleFrame.size.width = NSMinX([closeButton_ frame]) -
+                               NSMinX(newTitleFrame);

[viettrungluu, 2010/12/14 00:19:50]

Presumably you may want to calculate the x-coordinate of newTitleFrame('s
origin) before the NSMakeRect. In any case, you shouldn't pass the
partially-defined newTitleFrame into NSMinX....

[sail, 2010/12/14 01:59:53]

Done.

+  } else {
+    newTitleFrame.size.width = NSMaxX([closeButton_ frame]) -
+                               NSMinX(newTitleFrame);

[viettrungluu, 2010/12/14 00:19:50]

"

[sail, 2010/12/14 01:59:53]

Done.

   }
 
-  [titleView_ setFrame:titleFrame];
+  [titleView_ setFrame:newTitleFrame];
 }
 
 - (void)updateTitleColor {
   NSColor* titleColor = nil;
   ThemeProvider* theme = [[[self view] window] themeProvider];
   if (theme && ![self selected]) {
     titleColor =
         theme->GetNSColor(BrowserThemeProvider::COLOR_BACKGROUND_TAB_TEXT,
                           true);
   }
@@ skipping 20 matching lines @@
 // Called by the tabs to determine whether we are in rapid (tab) closure mode.
 - (BOOL)inRapidClosureMode {
   if ([[self target] respondsToSelector:@selector(inRapidClosureMode)]) {
     return [[self target] performSelector:@selector(inRapidClosureMode)] ?
         YES : NO;
   }
   return NO;
 }
 
 @end