[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 3548b9f9881a2f951b9912894c79b92dfc3d4e94..2aab95e0289e6acbe1dd2635520aec762fbe6ee6 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
@@ -5,6 +5,7 @@
 #import "chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h"
 
 #include "base/logging.h"
+#include "base/mac/foundation_util.h"
 #import "chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h"
 
 namespace {
@@ -14,9 +15,77 @@ const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@implementation OmniboxPopupTableController
+
+- (id)init {
+  if ((self = [super init])) {
+    array_.reset([[NSMutableArray alloc] init]);
+    hovered_index_ = -1;
+  }
+  return self;
+}
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)tableView {
+  return [array_ count];
+}
+
+- (id)tableView:(NSTableView*)tableView
+    objectValueForTableColumn:(NSTableColumn*)tableColumn
+                          row:(NSInteger)rowIndex {
+  return nil;
+}
+
+- (void)tableView:(NSTableView*)aTableView
+    setObjectValue:(id)anObject
+    forTableColumn:(NSTableColumn*)aTableColumn
+               row:(NSInteger)rowIndex {
+}
+
+- (void)tableView:(NSTableView*)tableView
+    willDisplayCell:(id)cell
+     forTableColumn:(NSTableColumn*)tableColumn
+                row:(NSInteger)rowIndex {
+  OmniboxPopupCell* popupCell =
+      base::mac::ObjCCastStrict<OmniboxPopupCell>(cell);
+  base::scoped_nsobject<OmniboxPopupCellData> cellData(
+      [[array_ objectAtIndex:rowIndex] retain]);
+  [popupCell setCellData:cellData];

[shrike, 2015/05/14 18:02:39]

Is it OK to just say

    [popupCell setCellData:[array_ objectAtIndex:rowIndex]];

It doesn't look like cellData is used anywhere except this one spot.

[dschuyler, 2015/05/14 22:28:20]

From an obj-C point of view, it seems fine to do so.
From a coding style point of view, I'm unsure of the answer.

[Scott Hess - ex-Googler, 2015/05/15 00:09:38]

I think it's OK, or to have cellData not be a scoped_nsobject<>().  The
difference from my earlier point about using a scoper was that you should prefer
a scoper to using an -autorelease, but in this case the -autorelease is already
implied, so taking advantage of it to simplify things is fine.

Actually, since -retain also has a cost, taking advantage of the known-good
reference is probably preferred to nailing it down with another reference. 
Light preference, I wouldn't contort the code for it, but in this case you
wouldn't contort the code.

Doing it all on one line as Jayson suggests is good, because it minimizes the
span of code where you have to rely on convention for correctness.

+  [popupCell
+      setState:([tableView selectedRow] == rowIndex) ? NSOnState : NSOffState];
+  [popupCell highlight:(hovered_index_ == rowIndex)
+             withFrame:[tableView bounds]
+                inView:tableView];
+}
+
+- (NSInteger)highlightedRow {
+  return hovered_index_;
+}
+
+- (NSRect)rectForRow:(NSInteger)rowIndex {
+  if (rowIndex == -1)
+    return NSZeroRect;
+  OmniboxPopupCellData* cellData = [array_ objectAtIndex:rowIndex];
+  return [cellData rect];
+}
+
+- (void)highlightRowAt:(NSInteger)rowIndex withView:(NSView*)view {
+  hovered_index_ = rowIndex;
+}
+
+- (void)setDataArray:(NSArray*)array {
+  hovered_index_ = -1;
+  array_.reset([array retain]);
+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  OmniboxPopupCellData* cellData = [array_ objectAtIndex:row];
+  return [cellData rect].size.height;
+}
+
+@end
+
+@interface OmniboxPopupMatrix ()
 - (void)resetTrackingArea;
-- (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
@@ -26,17 +95,23 @@ const NSInteger kMiddleButtonNumber = 2;
 - (id)initWithObserver:(OmniboxPopupMatrixObserver*)observer {
   if ((self = [super initWithFrame:NSZeroRect])) {
     observer_ = observer;
-    [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];
   }
@@ -47,17 +122,6 @@ const NSInteger kMiddleButtonNumber = 2;
   observer_ = observer;
 }
 
-- (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 +137,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 +167,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(observer_);
     observer_->OnMatrixRowMiddleClicked(self, highlightedRow);
@@ -117,7 +181,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 +209,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 +237,23 @@ 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];
+  NSInteger oldRow = [controller_ highlightedRow];
+  NSInteger newRow = [self rowAtPoint:point];
+  if (oldRow != newRow) {
+    [controller_ highlightRowAt:newRow withView:self];
+    [self setNeedsDisplayInRect:[controller_ rectForRow:oldRow]];
+    [self setNeedsDisplayInRect:[controller_ rectForRow:newRow]];
   }
 }
 
-// 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(observer_);
     observer_->OnMatrixRowSelected(self, row);
     return YES;