[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 df7b00d3bc6b1c315e1189e80962b1cd23eb322c..2531a70157029943c978ab82a79b240ce05ab46e 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
@@ -155,18 +155,16 @@ NSAttributedString* CreateClassifiedAttributedString(
 
 }  // namespace
 
+@implementation AutocompleteMatchWrapper
+@end
+
 @implementation OmniboxPopupCell
 
+@synthesize attributedTitle;
+
 - (id)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];
-
     const base::string16& raw_separator =
         l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
     separator_.reset(
@@ -205,6 +203,23 @@ NSAttributedString* CreateClassifiedAttributedString(
   }
 }
 
+- (id)copyWithZone:(NSZone*)zone {
+  NSAttributedString* separator = separator_.release();
+  NSAttributedString* description = description_.release();
+  NSAttributedString* prefix = prefix_.release();
+  OmniboxPopupCell* copy = [super copyWithZone:zone];
+  copy->match_ = match_;
+  copy->separator_.reset([separator copy]);
+  copy->description_.reset([description copy]);
+  copy->prefix_.reset([prefix copy]);
+  copy->maxMatchContentsWidth_ = maxMatchContentsWidth_;
+  copy->contentsOffset_ = contentsOffset_;
+  separator_.reset(separator);
+  description_.reset(description);
+  prefix_.reset(prefix);
+  return copy;
+}

[Scott Hess - ex-Googler, 2015/05/04 19:40:20]

WRT your other question about whether -copyWithZone: is correct to implement,
technically it sure is, but practically it seldom is unless you're implementing
more at an infrastructure layer.  For instance, you might copy over background
color and highlight mode, because all cells in the control will share it.  But I
can't think of a case where you'd want to share match or description info
between cells.

That's the general case.  The specific case of NSTableView is that it sometimes
copies cells around internally, so for NSTableView cells using
-tableView:dataCellForTableColumn:row: style, I think you're supposed to
implement it.  But you're using -tableView:willDisplayCell:forTableColumn:row:,
which I think just re-uses the same cell over and over.

[dschuyler, 2015/05/04 23:51:40]

Yeah, it does seem to use the same cell over and over.

A prior patch of this CL kept an array of cells that were reused.  I could go
back to that way of doing it (using dataCellForTableColumn).  I could also do
more preprocessing of the match data so less work is done in the
willDisplayCell.  Does either direction sound preferable?

[Scott Hess - ex-Googler, 2015/05/07 22:35:41]

This is something I'm not entirely certain of.  It seems to me that
recalculating the NSAttributedString on every display may be somewhat poor - not
a huge horrible thing, but it does seem like excess work.  It's harder to avoid
recalculating the info involved with actually drawing the NSAttributedString. 
Of course if the entire thing is drawn into some interesting buffer-backed layer
thing, all of this hardly matters.

I would guess if you dropped some logging in, you'd find a burst of
set-the-cell-up for every cell on popup, then pairs for highlight/unhighlight as
you move around.  And hover/unhover?  If it's down to just that constant amount
of setup per user event, then using the same cell repeatedly is defensible, but
if it's "I'm going to redraw the entire thing" on every user event, then I'm
more inclined to think a bit of caching in an array of cells is in order.

+
 - (void)setMaxMatchContentsWidth:(CGFloat)maxMatchContentsWidth {
   maxMatchContentsWidth_ = maxMatchContentsWidth;
 }
@@ -353,10 +368,10 @@ NSAttributedString* CreateClassifiedAttributedString(
   NSRect renderRect = ShiftRect(cellFrame, offset);
   renderRect.size.width =
       std::min(NSWidth(renderRect), static_cast<CGFloat>(maxWidth));
+  NSRect textRect = [as boundingRectWithSize:renderRect.size options:nil];
+  renderRect.origin.y += (NSHeight(cellFrame) - NSHeight(textRect)) / 2;
   if (renderRect.size.width != 0) {
-    [self drawTitle:as
-          withFrame:FlipIfRTL(renderRect, cellFrame)
-             inView:controlView];
+    [as drawInRect:FlipIfRTL(renderRect, cellFrame)];
   }
   return NSWidth(renderRect);
 }