[Mac] Bookmark button cell updates

chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm

Index: chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
index 7bf527aca6a4f3297135523f07bbc6eb7b8cb11c..9335cf71edeb039205017ef309af3e99520fc650 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm
@@ -35,6 +35,9 @@ const int kIconLeftPadding = 4;
 
 const int kDefaultFontSize = 12;
 
+// Kerning value for the title text.
+const CGFloat kKernAmount = 0.2;

[Elly Fong-Jones, 2017/04/04 15:04:36]

it seems weird that this has to be specified manually - is there a systemwide
default we should use?

[lgrey, 2017/04/04 15:12:27]

I wondered what was going on here too. My understanding is:
|titleTextAttributes| (which is an instance method that previously had this as a
magic number) is used
to create an attributed string, which is what ends up getting drawn. To put it
another way, this value
determines the kerning, vs. just being used for size lookup.

Re: a systemwide default, my assumption is that bookmark buttons having a
specific value is
intentional. Is it worth checking in with UX?

[Elly Fong-Jones, 2017/04/05 11:26:37]

Well, what's the systemwide default, and how do they look with the systemwide
default vs the current value? We can check with UX, but they will ask us for
side-by-side screenshots.

[lgrey, 2017/04/05 15:21:56]

The docs say the default it "kerning is disabled", by which I'm guessing they
mean "we use the font's default kern tables without further adjustment"

Kern = 0.2 (current behavior) https://screenshot.googleplex.com/WXXF3iC6SoT
Kerning unspecified https://screenshot.googleplex.com/4qBEEmG8mn0

+
 };  // namespace
 
 // TODO(lgrey): Bake setting the chevron image into this
@@ -65,7 +68,7 @@ const int kDefaultFontSize = 12;
 
 @end
 
-@interface BookmarkButtonCell(Private)
+@interface BookmarkButtonCell (Private)
 // Returns YES if the cell is the offTheSide button cell.
 - (BOOL)isOffTheSideButtonCell;
 - (void)configureBookmarkButtonCell;
@@ -112,6 +115,24 @@ const int kDefaultFontSize = 12;
   return [[[OffTheSideButtonCell alloc] init] autorelease];
 }
 
++ (CGFloat)cellWidthForNode:(const bookmarks::BookmarkNode*)node
+                      image:(NSImage*)image {
+  NSString* title =
+      [self cleanTitle:base::SysUTF16ToNSString(node->GetTitle())];
+  CGFloat width = kIconLeftPadding + [image size].width;
+  if ([title length] > 0) {
+    CGSize titleSize = [title sizeWithAttributes:@{
+      NSParagraphStyleAttributeName : [self paragraphStyleForBookmarkBarCell],
+      NSKernAttributeName : @(kKernAmount),
+      NSFontAttributeName : [self fontForBookmarkBarCell],
+    }];
+    width += kIconTextSpacer + std::ceil(titleSize.width) + kTextRightPadding;
+  } else {
+    width += kIconLeftPadding;
+  }
+  return width;
+}
+
 - (id)initForNode:(const BookmarkNode*)node
              text:(NSString*)text
             image:(NSImage*)image
@@ -120,7 +141,7 @@ const int kDefaultFontSize = 12;
     menuController_ = menuController;
     [self configureBookmarkButtonCell];
     [self setTextColor:[NSColor blackColor]];
-    [self setBookmarkNode:node];
+    [self setBookmarkNode:node image:image];
     // When opening a bookmark folder, the default behavior is that the
     // favicon is greyed when menu item is hovered with the mouse cursor.
     // When using NSNoCellMask, the favicon won't be greyed when menu item
@@ -130,15 +151,6 @@ const int kDefaultFontSize = 12;
     // It makes the behavior of the bookmark folder consistent with hovering
     // on the bookmark bar.
     [self setHighlightsBy:NSNoCellMask];
-
-    if (node) {
-      NSString* title = base::SysUTF16ToNSString(node->GetTitle());
-      [self setBookmarkCellText:title image:image];
-    } else {
-      [self setEmpty:YES];
-      [self setBookmarkCellText:l10n_util::GetNSString(IDS_MENU_EMPTY_SUBMENU)
-                          image:nil];
-    }
   }
 
   return self;
@@ -185,7 +197,7 @@ const int kDefaultFontSize = 12;
   [self setShowsBorderOnlyWhileMouseInside:YES];
   [self setControlSize:NSSmallControlSize];
   [self setAlignment:NSLeftTextAlignment];
-  [self setFont:[NSFont systemFontOfSize:kDefaultFontSize]];
+  [self setFont:[[self class] fontForBookmarkBarCell]];
   [self setBordered:NO];
   [self setBezeled:NO];
   [self setWraps:NO];
@@ -220,12 +232,8 @@ const int kDefaultFontSize = 12;
 
 - (void)setBookmarkCellText:(NSString*)title
                       image:(NSImage*)image {
-  title = [title stringByReplacingOccurrencesOfString:@"\n"
-                                           withString:@" "];
-  title = [title stringByReplacingOccurrencesOfString:@"\r"
-                                           withString:@" "];
-
-  if ([title length]) {
+  title = [[self class] cleanTitle:title];
+  if ([title length] && ![self isOffTheSideButtonCell]) {
     [self setImagePosition:NSImageLeft];
     [self setTitle:title];
   } else if ([self isFolderButtonCell]) {
@@ -245,7 +253,19 @@ const int kDefaultFontSize = 12;
 }
 
 - (void)setBookmarkNode:(const BookmarkNode*)node {
+  [self setBookmarkNode:node image:nil];
+}
+
+- (void)setBookmarkNode:(const BookmarkNode*)node image:(NSImage*)image {
   [self setRepresentedObject:[NSValue valueWithPointer:node]];
+  if (node) {
+    NSString* title = base::SysUTF16ToNSString(node->GetTitle());
+    [self setBookmarkCellText:title image:image];
+  } else {
+    [self setEmpty:YES];
+    [self setBookmarkCellText:l10n_util::GetNSString(IDS_MENU_EMPTY_SUBMENU)
+                        image:nil];
+  }
 }
 
 - (const BookmarkNode*)bookmarkNode {
@@ -315,10 +335,7 @@ const int kDefaultFontSize = 12;
 }
 
 - (NSDictionary*)titleTextAttributes {
-  base::scoped_nsobject<NSMutableParagraphStyle> style(
-      [NSMutableParagraphStyle new]);
-  [style setAlignment:NSNaturalTextAlignment];
-  [style setLineBreakMode:NSLineBreakByTruncatingTail];
+  NSParagraphStyle* style = [[self class] paragraphStyleForBookmarkBarCell];
   NSColor* textColor = textColor_.get();
   if (!textColor) {
     textColor = [NSColor blackColor];
@@ -328,14 +345,14 @@ const int kDefaultFontSize = 12;
   }
   NSFont* theFont = [self font];
   if (!theFont) {
-    theFont = [NSFont systemFontOfSize:kDefaultFontSize];
+    theFont = [[self class] fontForBookmarkBarCell];
   }
 
   return @{
-     NSFontAttributeName : theFont,
-     NSForegroundColorAttributeName : textColor,
-     NSParagraphStyleAttributeName : style.get(),
-     NSKernAttributeName : [NSNumber numberWithFloat:0.2]
+    NSFontAttributeName : theFont,
+    NSForegroundColorAttributeName : textColor,
+    NSParagraphStyleAttributeName : style,
+    NSKernAttributeName : @(kKernAmount),
   };
 }
 
@@ -441,5 +458,23 @@ const int kDefaultFontSize = 12;
   return 0.0;
 }
 
++ (NSFont*)fontForBookmarkBarCell {
+  return [NSFont systemFontOfSize:kDefaultFontSize];
+}
+
++ (NSParagraphStyle*)paragraphStyleForBookmarkBarCell {
+  NSMutableParagraphStyle* style = [[NSMutableParagraphStyle alloc] init];
+  [style setAlignment:NSNaturalTextAlignment];
+  [style setLineBreakMode:NSLineBreakByTruncatingTail];
+  return [style autorelease];
+}
+
+// Returns |title| with newlines and line feeds replaced with
+// spaces.
++ (NSString*)cleanTitle:(NSString*)title {
+  title = [title stringByReplacingOccurrencesOfString:@"\n" withString:@" "];
+  title = [title stringByReplacingOccurrencesOfString:@"\r" withString:@" "];
+  return title;
+}
 
 @end