[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
index 936ce5751103fb1b3bf15a41aa3f1b6258b4b182..f95731b400c225ee53a5bebfd7d1d95ed7a7004b 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
@@ -8,6 +8,7 @@
 #include <cmath>
 
 #include "base/i18n/rtl.h"
+#include "base/mac/foundation_util.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -24,6 +25,13 @@
 
 namespace {
 
+// How much to adjust the cell sizing up from the default determined
+// by the font.
+const CGFloat kCellHeightAdjust = 6.0;
+
+// How large the icon should be when displayed.
+const CGFloat kImageSize = 19.0;
+
 // How far to offset image column from the left.
 const CGFloat kImageXOffset = 5.0;
 
@@ -279,85 +287,102 @@ NSAttributedString* CreateClassifiedAttributedString(
 
 }  // namespace
 
-@implementation OmniboxPopupCell
-
-- (instancetype)init {
-  self = [super init];
-  if (self) {
-    [self setImagePosition:NSImageLeft];
-    [self setBordered:NO];
-    [self setButtonType:NSRadioButton];
-
-    // Without this highlighting messes up white areas of images.
-    [self setHighlightsBy:NSNoCellMask];
+@interface OmniboxPopupCell ()
+- (CGFloat)drawMatchPart:(NSAttributedString*)attributedString
+               withFrame:(NSRect)cellFrame
+                atOffset:(CGFloat)offset
+            withMaxWidth:(int)maxWidth;
+@end
 
-    const base::string16& raw_separator =
-        l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
-    separator_.reset(
-        [CreateAttributedString(raw_separator, DimTextColor()) retain]);
+@implementation OmniboxPopupCellData
+
+@synthesize contents = contents_;

[groby-ooo-7-16, 2015/06/12 18:51:07]

This is the "default" mode for @synthesize. Since XCode 4.4/clang 4, the
compiler will autosynthesize for you - so all these lines can go! Yay!

[dschuyler, 2015/06/12 21:39:14]

It seems that there is a warning that requires it.

+@synthesize description = description_;
+@synthesize prefix = prefix_;
+@synthesize image = image_;
+@synthesize answerImage = answerImage_;
+@synthesize contentsOffset = contentsOffset_;
+@synthesize isContentsRTL = isContentsRTL_;
+@synthesize matchType = matchType_;
+
+- (instancetype)initWithMatch:(const AutocompleteMatch&)match
+                        image:(NSImage*)image
+                  answerImage:(NSImage*)answerImage {
+  if ((self = [super init])) {
+    image_ = [image retain];
+    answerImage_ = [answerImage retain];
+
+    isContentsRTL_ =
+        (base::i18n::RIGHT_TO_LEFT ==
+         base::i18n::GetFirstStrongCharacterDirection(match.contents));
+    matchType_ = match.type;
+
+    // Prefix may not have any characters with strong directionality, and may
+    // take the UI directionality. But prefix needs to appear in continuation
+    // of the contents so we force the directionality.
+    NSTextAlignment textAlignment =
+        isContentsRTL_ ? NSRightTextAlignment : NSLeftTextAlignment;
+    prefix_ =
+        [CreateAttributedString(base::UTF8ToUTF16(match.GetAdditionalInfo(
+                                    kACMatchPropertyContentsPrefix)),
+                                ContentTextColor(), textAlignment) retain];
+
+    contents_ = [CreateClassifiedAttributedString(
+        match.contents, ContentTextColor(), match.contents_class) retain];
+
+    if (match.answer) {
+      base::scoped_nsobject<NSMutableAttributedString> answerString(
+          [[NSMutableAttributedString alloc] init]);
+      DCHECK(!match.answer->second_line().text_fields().empty());
+      for (const SuggestionAnswer::TextField& textField :
+           match.answer->second_line().text_fields()) {
+        [answerString
+            appendAttributedString:CreateAnswerString(textField.text(),
+                                                      textField.type())];
+      }
+      const base::string16 space(base::ASCIIToUTF16(" "));
+      // const base::char16 space(' ');

[groby-ooo-7-16, 2015/06/12 18:51:07]

Kill comment

[dschuyler, 2015/06/12 21:39:14]

Done.

+      const SuggestionAnswer::TextField* textField =
+          match.answer->second_line().additional_text();
+      if (textField) {
+        [answerString
+            appendAttributedString:CreateAnswerString(space + textField->text(),
+                                                      textField->type())];
+      }
+      textField = match.answer->second_line().status_text();
+      if (textField) {
+        [answerString
+            appendAttributedString:CreateAnswerString(space + textField->text(),
+                                                      textField->type())];
+      }
+      description_ = [answerString.release() retain];

[groby-ooo-7-16, 2015/06/12 18:51:06]

scoped_nsobject's release is not ObjC's release - it just means scoped_nsobject
doesn't track any more. (See [1]) Since it was started with a refCount of 1,
this would now create an object with a refCount of 2, and we'd leak. You want
autorelease() instead.

(Technically, you could just assign to |description_|, but that muddies
readability for refcounts)

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/scoped_ns...

[Scott Hess - ex-Googler, 2015/06/12 20:21:32]

IMHO this would be pretty clear to a C++ programmer:
  description_.swap(answerString);

[groby-ooo-7-16, 2015/06/12 21:01:54]

swap won't work, since description_ isn't scoped_nsobject. It's a retained
property, which saves a boatload of boilerplate getters/setters.

[dschuyler, 2015/06/12 21:39:14]

Done.

+    } else if (match.description.empty()) {
+      description_ = nil;

[groby-ooo-7-16, 2015/06/12 18:51:07]

Not necessary - all ivars are initialized to nil in ObjC. See
https://developer.apple.com/library/mac/documentation/General/Conceptual/Coco...

[dschuyler, 2015/06/12 21:39:14]

Done.

+    } else {
+      description_ = [CreateClassifiedAttributedString(
+          match.description, DimTextColor(), match.description_class) retain];
+    }
   }
   return self;
 }
 
-- (void)setAnswerImage:(NSImage*)image {
-  answerImage_.reset([image retain]);
+- (instancetype)copyWithZone:(NSZone*)zone {
+  return [self retain];
 }
 
-- (void)setMatch:(const AutocompleteMatch&)match {
-  match_ = match;
-  NSAttributedString *contents = CreateClassifiedAttributedString(
-      match_.contents, ContentTextColor(), match_.contents_class);
-  [self setAttributedTitle:contents];
-  [self setAnswerImage:nil];
-  if (match_.answer) {
-    base::scoped_nsobject<NSMutableAttributedString> answerString(
-        [[NSMutableAttributedString alloc] init]);
-    DCHECK(!match_.answer->second_line().text_fields().empty());
-    for (const SuggestionAnswer::TextField& textField :
-         match_.answer->second_line().text_fields()) {
-      NSAttributedString* attributedString =
-          CreateAnswerString(textField.text(), textField.type());
-      [answerString appendAttributedString:attributedString];
-    }
-    const base::char16 space(' ');
-    const SuggestionAnswer::TextField* textField =
-        match_.answer->second_line().additional_text();
-    if (textField) {
-      [answerString
-          appendAttributedString:CreateAnswerString(space + textField->text(),
-                                                    textField->type())];
-    }
-    textField = match_.answer->second_line().status_text();
-    if (textField) {
-      [answerString
-          appendAttributedString:CreateAnswerString(space + textField->text(),
-                                                    textField->type())];
-    }
-    description_.reset(answerString.release());
-  } else if (match_.description.empty()) {
-    description_.reset();
-  } else {
-    description_.reset([CreateClassifiedAttributedString(
-        match_.description, DimTextColor(), match_.description_class) retain]);
-  }
+- (CGFloat)getMatchContentsWidth {
+  return [contents_ size].width;
 }
 
-- (NSAttributedString*)description {
-  return description_;
+- (CGFloat)rowHeight {
+  return kImageSize + kCellHeightAdjust;
 }
 
-- (void)setMaxMatchContentsWidth:(CGFloat)maxMatchContentsWidth {
-  maxMatchContentsWidth_ = maxMatchContentsWidth;
-}
+@end
 
-- (void)setContentsOffset:(CGFloat)contentsOffset {
-  contentsOffset_ = contentsOffset;
-}
+@implementation OmniboxPopupCell
 
-// The default NSButtonCell drawing leaves the image flush left and
-// the title next to the image.  This spaces things out to line up
-// with the star button and autocomplete field.
-- (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView *)controlView {
+- (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   if ([self state] == NSOnState || [self isHighlighted]) {
     if ([self state] == NSOnState)
       [SelectedBackgroundColor() set];
@@ -370,154 +395,142 @@ NSAttributedString* CreateClassifiedAttributedString(
     [path fill];
   }
 
-  // Put the image centered vertically but in a fixed column.
-  NSImage* image = [self image];
-  if (image) {
-    NSRect imageRect = cellFrame;
-    imageRect.size = [image size];
-    imageRect.origin.y +=
-        std::floor((NSHeight(cellFrame) - NSHeight(imageRect)) / 2.0);
-    imageRect.origin.x += kImageXOffset;
-    [image drawInRect:FlipIfRTL(imageRect, cellFrame)
-             fromRect:NSZeroRect  // Entire image
-            operation:NSCompositeSourceOver
-             fraction:1.0
-       respectFlipped:YES
-                hints:nil];
-  }
-
-  [self drawMatchWithFrame:cellFrame inView:controlView];
+  [self drawMatchWithFrame:cellFrame
+              withCellData:[self objectValue]
+                    inView:controlView];
 }
 
-- (void)drawMatchWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
-  NSAttributedString* contents = [self attributedTitle];
-
+- (void)drawMatchWithFrame:(NSRect)cellFrame
+              withCellData:(OmniboxPopupCellData*)cellData
+                    inView:(NSView*)controlView {
+  OmniboxPopupMatrix* tableView =
+      base::mac::ObjCCastStrict<OmniboxPopupMatrix>(controlView);
   CGFloat remainingWidth = GetContentAreaWidth(cellFrame);
-  CGFloat contentsWidth = [self getMatchContentsWidth];
-  CGFloat separatorWidth = [separator_ size].width;
-  CGFloat descriptionWidth = description_.get() ? [description_ size].width : 0;
+  CGFloat contentsWidth = [cellData getMatchContentsWidth];
+  CGFloat separatorWidth = [[tableView separator] size].width;
+  CGFloat descriptionWidth =
+      [cellData description] ? [[cellData description] size].width : 0;
   int contentsMaxWidth, descriptionMaxWidth;
   OmniboxPopupModel::ComputeMatchMaxWidths(
-      ceilf(contentsWidth),
-      ceilf(separatorWidth),
-      ceilf(descriptionWidth),
+      ceilf(contentsWidth), ceilf(separatorWidth), ceilf(descriptionWidth),
       ceilf(remainingWidth),
-      !AutocompleteMatch::IsSearchType(match_.type),
-      &contentsMaxWidth,
+      !AutocompleteMatch::IsSearchType([cellData matchType]), &contentsMaxWidth,
       &descriptionMaxWidth);
 
+  // Put the image centered vertically but in a fixed column.
+  if ([cellData image]) {

[groby-ooo-7-16, 2015/06/12 18:51:07]

This check is not necessary. (A nil image would just compute an imageRect, and
then send the draw message to nil - which does nothing.

[dschuyler, 2015/06/12 21:39:14]

Done.

+    NSRect imageRect = cellFrame;
+    imageRect.size = [[cellData image] size];
+    imageRect.origin.y +=
+        std::floor((NSHeight(cellFrame) - NSHeight(imageRect)) / 2.0);
+    imageRect.origin.x += kImageXOffset;
+    [[cellData image] drawInRect:FlipIfRTL(imageRect, cellFrame)
+                        fromRect:NSZeroRect
+                       operation:NSCompositeSourceOver
+                        fraction:1.0
+                  respectFlipped:YES
+                           hints:nil];
+  }
+
   CGFloat offset = kTextStartOffset;
-  if (match_.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
+  if ([cellData matchType] == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
     // Infinite suggestions are rendered with a prefix (usually ellipsis), which
     // appear vertically stacked.
     offset += [self drawMatchPrefixWithFrame:cellFrame
+                                withCellData:cellData
                                       inView:controlView
                         withContentsMaxWidth:&contentsMaxWidth];
   }
-  offset += [self drawMatchPart:contents
+  offset += [self drawMatchPart:[cellData contents]
                       withFrame:cellFrame
                        atOffset:offset
-                   withMaxWidth:contentsMaxWidth
-                         inView:controlView];
+                   withMaxWidth:contentsMaxWidth];
 
   if (descriptionMaxWidth != 0) {
-    offset += [self drawMatchPart:separator_
+    offset += [self drawMatchPart:[tableView separator]
                         withFrame:cellFrame
                          atOffset:offset
-                     withMaxWidth:separatorWidth
-                           inView:controlView];
-    if (answerImage_) {
+                     withMaxWidth:separatorWidth];
+    if ([cellData answerImage]) {

[groby-ooo-7-16, 2015/06/12 18:51:07]

Unnecessary check

[dschuyler, 2015/06/12 21:39:14]

Done.

       NSRect imageRect = NSMakeRect(offset, NSMinY(cellFrame),
           NSHeight(cellFrame), NSHeight(cellFrame));
-      [answerImage_ drawInRect:FlipIfRTL(imageRect, cellFrame)
-                      fromRect:NSZeroRect
-                     operation:NSCompositeSourceOver
-                      fraction:1.0
-                respectFlipped:YES
-                         hints:nil];
+      [[cellData answerImage] drawInRect:FlipIfRTL(imageRect, cellFrame)
+                                fromRect:NSZeroRect
+                               operation:NSCompositeSourceOver
+                                fraction:1.0
+                          respectFlipped:YES
+                                   hints:nil];
       offset += NSWidth(imageRect);
     }
-    offset += [self drawMatchPart:description_
+    offset += [self drawMatchPart:[cellData description]
                         withFrame:cellFrame
                          atOffset:offset
-                     withMaxWidth:descriptionMaxWidth
-                           inView:controlView];
+                     withMaxWidth:descriptionMaxWidth];
   }
 }
 
 - (CGFloat)drawMatchPrefixWithFrame:(NSRect)cellFrame

[groby-ooo-7-16, 2015/06/12 18:51:06]

Didn't see a prototype for this?

[dschuyler, 2015/06/12 21:39:14]

Done.

+                       withCellData:(OmniboxPopupCellData*)cellData
                              inView:(NSView*)controlView
                withContentsMaxWidth:(int*)contentsMaxWidth {
   CGFloat offset = 0.0f;
   CGFloat remainingWidth = GetContentAreaWidth(cellFrame);
-  bool isContentsRTL = (base::i18n::RIGHT_TO_LEFT ==
-      base::i18n::GetFirstStrongCharacterDirection(match_.contents));
-  // Prefix may not have any characters with strong directionality, and may take
-  // the UI directionality. But prefix needs to appear in continuation of the
-  // contents so we force the directionality.
-  NSTextAlignment textAlignment = isContentsRTL ?
-      NSRightTextAlignment : NSLeftTextAlignment;
-  prefix_.reset([CreateAttributedString(base::UTF8ToUTF16(
-      match_.GetAdditionalInfo(kACMatchPropertyContentsPrefix)),
-      ContentTextColor(), textAlignment) retain]);
-  CGFloat prefixWidth = [prefix_ size].width;
+  CGFloat prefixWidth = [[cellData prefix] size].width;
+
+  OmniboxPopupMatrix* tableView =
+      base::mac::ObjCCastStrict<OmniboxPopupMatrix>(controlView);

[groby-ooo-7-16, 2015/06/12 18:51:06]

You could just pass in the OmniboxPopupMatrix directly - it seems this is only
called from places where we've done the cast already.

[dschuyler, 2015/06/12 21:39:14]

Done.

 
   CGFloat prefixOffset = 0.0f;
-  if (base::i18n::IsRTL() != isContentsRTL) {
+  if (base::i18n::IsRTL() != [cellData isContentsRTL]) {
     // The contents is rendered between the contents offset extending towards
     // the start edge, while prefix is rendered in opposite direction. Ideally
     // the prefix should be rendered at |contentsOffset_|. If that is not
     // sufficient to render the widest suggestion, we increase it to
-    // |maxMatchContentsWidth_|.  If |remainingWidth| is not sufficient to
+    // |maxMatchContentsWidth|.  If |remainingWidth| is not sufficient to
     // accommodate that, we reduce the offset so that the prefix gets rendered.
     prefixOffset = std::min(
-        remainingWidth - prefixWidth, std::max(contentsOffset_,
-                                               maxMatchContentsWidth_));
+        remainingWidth - prefixWidth,
+        std::max([cellData contentsOffset], [tableView maxMatchContentsWidth]));
     offset = std::max<CGFloat>(0.0, prefixOffset - *contentsMaxWidth);
   } else { // The direction of contents is same as UI direction.
     // Ideally the offset should be |contentsOffset_|. If the max total width
-    // (|prefixWidth| + |maxMatchContentsWidth_|) from offset will exceed the
+    // (|prefixWidth| + |maxMatchContentsWidth|) from offset will exceed the
     // |remainingWidth|, then we shift the offset to the left , so that all
     // postfix suggestions are visible.
     // We have to render the prefix, so offset has to be at least |prefixWidth|.
-    offset = std::max(prefixWidth,
-        std::min(remainingWidth - maxMatchContentsWidth_, contentsOffset_));
+    offset =
+        std::max(prefixWidth,
+                 std::min(remainingWidth - [tableView maxMatchContentsWidth],
+                          [cellData contentsOffset]));
     prefixOffset = offset - prefixWidth;
   }
   *contentsMaxWidth = std::min((int)ceilf(remainingWidth - prefixWidth),
                                *contentsMaxWidth);
-  [self drawMatchPart:prefix_
+  [self drawMatchPart:[cellData prefix]
             withFrame:cellFrame
              atOffset:prefixOffset + kTextStartOffset
-         withMaxWidth:prefixWidth
-               inView:controlView];
+         withMaxWidth:prefixWidth];
   return offset;
 }
 
 - (CGFloat)drawMatchPart:(NSAttributedString*)attributedString
                withFrame:(NSRect)cellFrame
                 atOffset:(CGFloat)offset
-            withMaxWidth:(int)maxWidth
-                  inView:(NSView*)controlView {
+            withMaxWidth:(int)maxWidth {
   if (offset > NSWidth(cellFrame))
     return 0.0f;
   NSRect renderRect = ShiftRect(cellFrame, offset);
   renderRect.size.width =
       std::min(NSWidth(renderRect), static_cast<CGFloat>(maxWidth));
-  if (renderRect.size.width != 0) {
-    [self drawTitle:attributedString
-          withFrame:FlipIfRTL(renderRect, cellFrame)
-             inView:controlView];
-  }
+  NSRect textRect =
+      [attributedString boundingRectWithSize:renderRect.size options:nil];
+  renderRect.origin.y +=
+      std::floor((NSHeight(cellFrame) - NSHeight(textRect)) / 2.0);

[groby-ooo-7-16, 2015/06/12 18:51:06]

Since this is only part of the match, are you sure we want to flip the rect in
the entire cellFrame? Or do we just want to draw the string RTL, but preserve
overall position?

[dschuyler, 2015/06/12 21:39:14]

I'm not 100% sure.  I do have a bug to look into the RTL further: 469362.
I'm adding a link to this comment to that bug.

+  if (NSWidth(renderRect) > 0.0)
+    [attributedString drawInRect:FlipIfRTL(renderRect, cellFrame)];
   return NSWidth(renderRect);
 }
 
-- (CGFloat)getMatchContentsWidth {
-  NSAttributedString* contents = [self attributedTitle];
-  return contents ? [contents size].width : 0;
-}
-
-
 + (CGFloat)computeContentsOffset:(const AutocompleteMatch&)match {
   const base::string16& inputText = base::UTF8ToUTF16(
       match.GetAdditionalInfo(kACMatchPropertyInputText));
@@ -584,4 +597,10 @@ NSAttributedString* CreateClassifiedAttributedString(
   return base::i18n::IsRTL() ? (inputWidth - glyphOffset) : glyphOffset;
 }
 
++ (NSAttributedString*)createSeparatorString {
+  base::string16 raw_separator =
+      l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
+  return CreateAttributedString(raw_separator, DimTextColor());
+}
+
 @end