[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
index 9000941c26cea3536cdcc0166a58e9597d9a4aa9..da4479868f150c3f991c5bdb072558517fe9d2ea 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
@@ -14,9 +14,67 @@ const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@implementation OmniboxPopupTableController
+
+- (id)init {
+  if ((self = [super init])) {
+    array_.reset([[NSMutableArray alloc] init]);
+    hovered_cell_ = -1;
+  }
+  return self;
+}
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)tableView {
+  return [array_ count];
+}
+
+- (id)tableView:(NSTableView*)aTableView
+    objectValueForTableColumn:(NSTableColumn*)aTableColumn
+                          row:(NSInteger)rowIndex {
+  // Returning the cell allows selection highlighting to work properly.
+  return [array_ objectAtIndex:rowIndex];

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

This isn't a cell, though, it's a dictionary.

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

Whoops, old comment.  Removed now.

Done.

+}
+
+- (void)tableView:(NSTableView*)aTableView
+    willDisplayCell:(id)aCell

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

You should use ObjCCast<> or ObjCCastStrict<> to make this something more
specific than id before otherwise messaging it.  One is "Give me one of these or
nil", the other is "Give me one of these or die", probably the latter is in
order here.

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

Done.

+     forTableColumn:(NSTableColumn*)aTableColumn
+                row:(NSInteger)rowIndex {
+  NSDictionary* dictionary = [array_ objectAtIndex:rowIndex];
+
+  NSImage* image = [dictionary objectForKey:@"image"];
+  [aCell setImage:image];
+
+  AutocompleteMatchWrapper* acm = [dictionary objectForKey:@"match"];
+  [aCell setMatch:acm->match_];

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

Since you manufacture the various internal info in -setMatch:, I don't think
setting all that information on every display is necessarily great.  I'd
definitely drop a LOG(ERROR) somewhere and track how often more-expensive things
are being called when you do things like hover around.

You might also want to find someone else to rope in who is more current on
NSTableView.  It's possible my memories of which cases want to cache a bunch of
cells versus which cases want to just re-build the cell on the fly are wrong.

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

It does get called during hover/highlight changes.

+
+  [aCell
+      setState:([aTableView selectedRow] == rowIndex) ? NSOnState : NSOffState];
+  [aCell highlight:(hovered_cell_ == rowIndex)
+         withFrame:[aTableView bounds]
+            inView:aTableView];
+}
+
+- (NSInteger)highlightedRow {
+  return hovered_cell_;
+}
+
+- (void)highlightRowAt:(NSInteger)rowIndex withView:(NSView*)view {
+  hovered_cell_ = rowIndex;
+}
+
+- (void)setDataArray:(NSArray*)array {
+  hovered_cell_ = -1;
+  array_.reset([array retain]);
+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  return [[[array_ objectAtIndex:row] objectForKey:@"height"] floatValue];
+}
+
+@end
+
+@interface OmniboxPopupMatrix ()
 - (void)resetTrackingArea;
-- (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
@@ -26,38 +84,33 @@ const NSInteger kMiddleButtonNumber = 2;
 - (id)initWithDelegate:(OmniboxPopupMatrixDelegate*)delegate {
   if ((self = [super initWithFrame:NSZeroRect])) {
     delegate_ = delegate;
-    [self setCellClass:[OmniboxPopupCell class]];
+    controller_.reset([[OmniboxPopupTableController alloc] init]);
+
+    base::scoped_nsobject<NSTableColumn> column(
+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"]);
+    [column setDataCell:[[[OmniboxPopupCell alloc] init] autorelease]];
+    [self addTableColumn:column];
+
+    [self setDelegate:controller_];
+    [self setDataSource:controller_];
 
     // Cells pack with no spacing.
     [self setIntercellSpacing:NSMakeSize(0.0, 0.0)];
 
-    [self setDrawsBackground:YES];
+    [self setSelectionHighlightStyle:NSTableViewSelectionHighlightStyleNone];
     [self setBackgroundColor:[NSColor controlBackgroundColor]];
-    [self renewRows:0 columns:1];
     [self setAllowsEmptySelection:YES];
-    [self setMode:NSRadioModeMatrix];
-    [self deselectAllCells];
+    [self deselectAll:self];
 
     [self resetTrackingArea];
   }
   return self;
 }
 
-- (void)setDelegate:(OmniboxPopupMatrixDelegate*)delegate {
+- (void)setTheDelegate:(OmniboxPopupMatrixDelegate*)delegate {

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

WRT our discussing in the CL, I worry that -setTheDelegate: is even worse than
-setCppDelegate:.

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

Let's do the name change in another CL.

   delegate_ = delegate;
 }
 
-- (NSInteger)highlightedRow {
-  NSArray* cells = [self cells];
-  const NSUInteger count = [cells count];
-  for(NSUInteger i = 0; i < count; ++i) {
-    if ([[cells objectAtIndex:i] isHighlighted]) {
-      return i;
-    }
-  }
-  return -1;
-}
-
 - (void)updateTrackingAreas {
   [self resetTrackingArea];
   [super updateTrackingAreas];
@@ -73,7 +126,7 @@ const NSInteger kMiddleButtonNumber = 2;
 }
 
 - (void)mouseExited:(NSEvent*)theEvent {
-  [self highlightRowAt:-1];
+  [controller_ highlightRowAt:-1 withView:self];
 }
 
 // The tracking area events aren't forwarded during a drag, so handle
@@ -103,7 +156,7 @@ const NSInteger kMiddleButtonNumber = 2;
   // make sure the user is getting the right feedback.
   [self highlightRowUnder:theEvent];
 
-  const NSInteger highlightedRow = [self highlightedRow];
+  const NSInteger highlightedRow = [controller_ highlightedRow];
   if (highlightedRow != -1) {
     DCHECK(delegate_);
     delegate_->OnMatrixRowMiddleClicked(self, highlightedRow);
@@ -117,7 +170,7 @@ const NSInteger kMiddleButtonNumber = 2;
   NSCell* selectedCell = [self selectedCell];
 
   // Clear any existing highlight.
-  [self highlightRowAt:-1];
+  [controller_ highlightRowAt:-1 withView:self];
 
   do {
     if (![self selectCellForEvent:theEvent]) {
@@ -145,6 +198,19 @@ const NSInteger kMiddleButtonNumber = 2;
   }
 }
 
+- (void)selectRowAt:(NSInteger)rowIndex {
+  NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:rowIndex];
+  [self selectRowIndexes:indexSet byExtendingSelection:NO];
+}
+
+- (NSInteger)highlightedRow {
+  return [controller_ highlightedRow];
+}
+
+- (void)setDataArray:(NSArray*)array {
+  [controller_ setDataArray:array];
+}
+
 - (void)resetTrackingArea {
   if (trackingArea_.get())
     [self removeTrackingArea:trackingArea_.get()];
@@ -160,33 +226,19 @@ const NSInteger kMiddleButtonNumber = 2;
   [self addTrackingArea:trackingArea_.get()];
 }
 
-- (void)highlightRowAt:(NSInteger)rowIndex {
-  // highlightCell will be nil if rowIndex is out of range, so no cell will be
-  // highlighted.
-  NSCell* highlightCell = [self cellAtRow:rowIndex column:0];
-
-  for (NSCell* cell in [self cells]) {
-    [cell setHighlighted:(cell == highlightCell)];
-  }
-}
-
 - (void)highlightRowUnder:(NSEvent*)theEvent {
   NSPoint point = [self convertPoint:[theEvent locationInWindow] fromView:nil];
-  NSInteger row, column;
-  if ([self getRow:&row column:&column forPoint:point]) {
-    [self highlightRowAt:row];
-  } else {
-    [self highlightRowAt:-1];
-  }
+  [controller_ highlightRowAt:[self rowAtPoint:point] withView:self];
+  [self setNeedsDisplay:YES];
 }
 
 // Select cell under |theEvent|, returning YES if a selection is made.
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent {
   NSPoint point = [self convertPoint:[theEvent locationInWindow] fromView:nil];
 
-  NSInteger row, column;
-  if ([self getRow:&row column:&column forPoint:point]) {
-    DCHECK_EQ(column, 0);
+  NSInteger row = [self rowAtPoint:point];
+  [self selectRowAt:row];
+  if (row != -1) {
     DCHECK(delegate_);
     delegate_->OnMatrixRowSelected(self, row);
     return YES;