[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

 // Copyright 2013 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/omnibox/omnibox_popup_cell.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/i18n/rtl.h"
@@ skipping 137 matching lines @@
                  value:DimTextColor()
                  range:range];
     }
   }
 
   return as;
 }
 
 }  // 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(
         [CreateAttributedString(raw_separator, DimTextColor()) retain]);
   }
   return self;
 }
 
 - (void)setMatch:(const AutocompleteMatch&)match {
   match_ = match;
@@ skipping 18 matching lines @@
     description_.reset([CreateClassifiedAttributedString(
         answerString, DimTextColor(), match_.description_class) retain]);
   } else if (match_.description.empty()) {
     description_.reset();
   } else {
     description_.reset([CreateClassifiedAttributedString(
         match_.description, DimTextColor(), match_.description_class) retain]);
   }
 }
 
+- (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;
 }
 
 - (void)setContentsOffset:(CGFloat)contentsOffset {
   contentsOffset_ = contentsOffset;
 }
 
 // The default NSButtonCell drawing leaves the image flush left and
 // the title next to the image.  This spaces things out to line up
@@ skipping 128 matching lines @@
 - (CGFloat)drawMatchPart:(NSAttributedString*)as
                withFrame:(NSRect)cellFrame
                 atOffset:(CGFloat)offset
             withMaxWidth:(int)maxWidth
                   inView:(NSView*)controlView {
   if (offset > NSWidth(cellFrame))
     return 0.0f;
   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);
 }
 
 - (CGFloat)getMatchContentsWidth {
   NSAttributedString* contents = [self attributedTitle];
   return contents ? [contents size].width : 0;
 }
 
 
@@ skipping 57 matching lines @@
   }
   glyphOffset -= minOffset;
   if (glyphWidth == 0)
     glyphWidth = inputWidth - glyphOffset;
   if (isContentsRTL)
     glyphOffset += glyphWidth;
   return isRTL ? (inputWidth - glyphOffset) : glyphOffset;
 }
 
 @end