[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 072547efa69fec897c846bd4ede4bd019be16bc6..0658faace81b8eafc223575631488c3db4e97ed2 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
@@ -5,7 +5,10 @@
 #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"
+#include "chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.h"
+#include "components/omnibox/autocomplete_result.h"
 
 namespace {
 
@@ -14,48 +17,132 @@ const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@interface OmniboxPopupTableController ()
+- (instancetype)initWithArray:(NSArray*)array;
+@end
+
+@implementation OmniboxPopupTableController
+
+- (instancetype)initWithMatchResults:(const AutocompleteResult&)result
+                           tableView:(OmniboxPopupMatrix*)tableView
+                           popupView:(const OmniboxPopupViewMac&)popupView
+                         answerImage:(NSImage*)answerImage {
+  base::scoped_nsobject<NSMutableArray> array([[NSMutableArray alloc] init]);
+  CGFloat max_match_contents_width = 0.0f;
+  CGFloat contentsOffset = -1.0f;
+  for (const AutocompleteMatch& match : result) {
+    if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL &&
+        contentsOffset < 0.0f)
+      contentsOffset = [OmniboxPopupCell computeContentsOffset:match];
+    base::scoped_nsobject<OmniboxPopupCellData> cellData(
+        [[OmniboxPopupCellData alloc]
+             initWithMatch:match
+            contentsOffset:contentsOffset
+                     image:popupView.ImageForMatch(match)
+               answerImage:(match.answer ? answerImage : nil)]);
+    [array addObject:cellData];
+    if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
+      max_match_contents_width =
+          std::max(max_match_contents_width, [cellData getMatchContentsWidth]);
+    }
+  }
+
+  [tableView setMaxMatchContentsWidth:max_match_contents_width];
+  return [self initWithArray:array];
+}
+
+- (instancetype)initWithArray:(NSArray*)array {
+  if ((self = [super init])) {
+    hoveredIndex_ = -1;
+    array_.reset([array copy]);
+  }
+  return self;
+}
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)tableView {
+  return [array_ count];
+}
+
+- (id)tableView:(NSTableView*)tableView
+    objectValueForTableColumn:(NSTableColumn*)tableColumn
+                          row:(NSInteger)rowIndex {
+  return [array_ objectAtIndex:rowIndex];
+}
+
+- (void)tableView:(NSTableView*)aTableView
+    setObjectValue:(id)anObject
+    forTableColumn:(NSTableColumn*)aTableColumn
+               row:(NSInteger)rowIndex {
+  NOTREACHED();
+}
+
+- (void)tableView:(NSTableView*)tableView
+    willDisplayCell:(id)cell
+     forTableColumn:(NSTableColumn*)tableColumn
+                row:(NSInteger)rowIndex {

[Scott Hess - ex-Googler, 2015/06/12 22:21:59]

I think if they fit, these should align the colon, and only do the four-space
like this if it doesn't fit.  If you consult the style guide and it seems
ambiguous, ignore me.

[dschuyler, 2015/06/12 23:07:54]

This is formatted the way cl format wants to do it.  I realize that is not
really the authority of the style guide, but I'm willing to rely on it for this
case if that's cool.

[Scott Hess - ex-Googler, 2015/06/12 23:18:13]

Acknowledged.

+  OmniboxPopupCell* popupCell =
+      base::mac::ObjCCastStrict<OmniboxPopupCell>(cell);
+  [popupCell
+      setState:([tableView selectedRow] == rowIndex) ? NSOnState : NSOffState];
+  [popupCell highlight:(hoveredIndex_ == rowIndex)
+             withFrame:[tableView bounds]
+                inView:tableView];
+}
+
+- (NSInteger)highlightedRow {
+  return hoveredIndex_;
+}
+
+- (void)setHighlightedRow:(NSInteger)rowIndex {
+  hoveredIndex_ = rowIndex;
+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  return [[array_ objectAtIndex:row] rowHeight];
+}
+
+@end
+
+@interface OmniboxPopupMatrix ()
 - (void)resetTrackingArea;
-- (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
 
 @implementation OmniboxPopupMatrix
 
+@synthesize separator = separator_;
+@synthesize maxMatchContentsWidth = maxMatchContentsWidth_;
+
 - (instancetype)initWithObserver:(OmniboxPopupMatrixObserver*)observer {
   if ((self = [super initWithFrame:NSZeroRect])) {
     observer_ = observer;
-    [self setCellClass:[OmniboxPopupCell class]];
+
+    base::scoped_nsobject<NSTableColumn> column(
+        [[NSTableColumn alloc] initWithIdentifier:@"MainColumn"]);
+    [column setDataCell:[[OmniboxPopupCell alloc] init]];

[Scott Hess - ex-Googler, 2015/06/12 22:21:59]

I really really wish that the documentation was more clear about whether
-setDataCell: takes ownership, or adds a reference.  This is only correct if it
takes ownership.  You might want to test by having a temporary, and logging the
results of -retainCount before and after the -setDataCell: call.  That's not
100% certain, since there could be autorelease pools and the like involved, but
if the retain count does not change, that probably means that it takes ownership
rather than adding a ref.

[groby-ooo-7-16, 2015/06/12 22:43:01]

The docs say "@property (strong) id dataCell". It seems strong is the new fancy
way to say retain, so it takes ownership. You're safe.

[Scott Hess - ex-Googler, 2015/06/12 22:49:20]

I think that means it takes a reference under non-ARC, which implies that after
this statement runs there will be two outstanding references, one of which
nobody is tracking.

[groby-ooo-7-16, 2015/06/12 22:58:46]

Gah. Of course. Sorry, Friday-afternoon-CL-fatigue. 

[dschuyler, 2015/06/12 23:07:54]

Does that mean that this should be an autorelease?

[Scott Hess - ex-Googler, 2015/06/12 23:18:13]

Yep.

[dschuyler, 2015/06/13 00:11:04]

Done.

+    [self addTableColumn:column];
 
     // 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)setObserver:(OmniboxPopupMatrixObserver*)observer {
-  observer_ = observer;
+- (OmniboxPopupTableController*)controller {
+  return base::mac::ObjCCastStrict<OmniboxPopupTableController>(
+      [self 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)setObserver:(OmniboxPopupMatrixObserver*)observer {
+  observer_ = observer;
 }
 
 - (void)updateTrackingAreas {
@@ -73,7 +160,7 @@ const NSInteger kMiddleButtonNumber = 2;
 }
 
 - (void)mouseExited:(NSEvent*)theEvent {
-  [self highlightRowAt:-1];
+  [[self controller] setHighlightedRow:-1];
 }
 
 // The tracking area events aren't forwarded during a drag, so handle
@@ -103,7 +190,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 = [[self controller] highlightedRow];
   if (highlightedRow != -1) {
     DCHECK(observer_);
     observer_->OnMatrixRowMiddleClicked(self, highlightedRow);
@@ -117,7 +204,7 @@ const NSInteger kMiddleButtonNumber = 2;
   NSCell* selectedCell = [self selectedCell];
 
   // Clear any existing highlight.
-  [self highlightRowAt:-1];
+  [[self controller] setHighlightedRow:-1];
 
   do {
     if (![self selectCellForEvent:theEvent]) {
@@ -145,6 +232,22 @@ const NSInteger kMiddleButtonNumber = 2;
   }
 }
 
+- (void)selectRowIndex:(NSInteger)rowIndex {
+  NSIndexSet* indexSet = [NSIndexSet indexSetWithIndex:rowIndex];
+  [self selectRowIndexes:indexSet byExtendingSelection:NO];
+}
+
+- (NSInteger)highlightedRow {
+  return [[self controller] highlightedRow];
+}
+
+- (void)setController:(OmniboxPopupTableController*)controller {
+  matrixController_.reset([controller retain]);
+  [self setDelegate:controller];
+  [self setDataSource:controller];
+  [self reloadData];
+}
+
 - (void)resetTrackingArea {
   if (trackingArea_.get())
     [self removeTrackingArea:trackingArea_.get()];
@@ -160,33 +263,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 = [[self controller] highlightedRow];
+  NSInteger newRow = [self rowAtPoint:point];
+  if (oldRow != newRow) {
+    [[self controller] setHighlightedRow:newRow];
+    [self setNeedsDisplayInRect:[self rectOfRow:oldRow]];
+    [self setNeedsDisplayInRect:[self rectOfRow: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 selectRowIndex:row];
+  if (row != -1) {
     DCHECK(observer_);
     observer_->OnMatrixRowSelected(self, row);
     return YES;