[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..3b0dcc51d2569ebeaab27cbae5b6634899547c96 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,136 @@ const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@implementation OmniboxPopupTableController
+
+- (instancetype)init {
+  if ((self = [super init])) {
+    hoveredIndex_ = -1;
+  }
+  return self;
+}
+
+- (instancetype)initWithMatchResults:(const AutocompleteResult&)result
+                           popupView:(const OmniboxPopupViewMac&)popupView
+                         answerImage:(NSImage*)answerImage {
+  if ((self = [super init])) {
+    hoveredIndex_ = -1;
+    array_.reset([[NSMutableArray alloc] init]);
+    CGFloat max_match_contents_width = 0.0f;
+    CGFloat contents_offset = -1.0f;
+    for (const AutocompleteMatch& match : result) {
+      base::scoped_nsobject<OmniboxPopupCellData> cellData(
+          [[OmniboxPopupCellData alloc]
+              initWithMatch:match
+                      image:popupView.ImageForMatch(match)]);
+      if (match.answer && answerImage)

[groby-ooo-7-16, 2015/06/11 01:22:18]

Again, this modifies something that's technically immutable - I'd rather you
always pass answerImage into the initWithMatch and make this call there.

[dschuyler, 2015/06/11 22:34:22]

Done.

+        [cellData setAnswerImage:answerImage];
+      [array_ addObject:cellData];
+      if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
+        max_match_contents_width = std::max(max_match_contents_width,
+                                            [cellData getMatchContentsWidth]);
+        if (contents_offset < 0.0f)
+          contents_offset = [OmniboxPopupCell computeContentsOffset:match];
+        [cellData setContentsOffset:contents_offset];
+      }
+    }
+
+    for (const OmniboxPopupCellData* cellData : array_.get()) {
+      [cellData setMaxMatchContentsWidth:max_match_contents_width];

[groby-ooo-7-16, 2015/06/11 01:22:18]

Grumblemumble immutable grumblemumble.

(We can get away with it because we know it won't be used by anybody else before
this, but... yuck)

Since the MaxMatchContentsWidth is the same for all cells, can we move that
elsewhere?

[dschuyler, 2015/06/11 22:34:23]

Done.

+    }
+  }
+  return self;
+}
+
+- (instancetype)initWithArray:(NSMutableArray*)array {
+  if ((self = [super init])) {
+    hoveredIndex_ = -1;
+    array_.reset([array retain]);

[groby-ooo-7-16, 2015/06/11 01:22:17]

Usually, if you init with a mutable array, you copy/retain (because you don't
want it mutated behind your back)

[Scott Hess - ex-Googler, 2015/06/11 21:52:02]

I think -copy returns a reference, so you just do:
  array_.reset([array copy]);
BUT, |array| should be mutable in the first place, so really the caller should
be passing an NSArray, and will either need to put it in a scoper or use
-autorelease (usually a method would be expected to add a reference, not keep
the reference).

[dschuyler, 2015/06/11 22:34:22]

I'm having trouble following this, is this a mistype: "should be [im]mutable in
the first place" or similar?

[dschuyler, 2015/06/11 22:34:23]

Done.

[Scott Hess - ex-Googler, 2015/06/11 22:36:19]

Yeah, I think there was a missing n't or im in there.  If you don't need
something mutable, don't ask for something mutable, I think.

[dschuyler, 2015/06/12 00:43:26]

I think we still want the data member to be a mutable array (from prior review
comments in this CL).  Is it correct to make the arg non mutable and copy the
array?

Done.

[Scott Hess - ex-Googler, 2015/06/12 20:21:32]

I think maybe you're wavering back and forth based on implementation details,
not abstraction boundaries.

AFAICT, after initialization, this class never needs to change the contents of
array_, so array_ should be an immutable NSArray instance.

-initWithMatchResults:* is using array_ as an NSMutableArray for construction
purposes only, so it should be using a local scoped_nsobject<NSMutableArray> for
that, which then either has its only reference put in array_ (so it is never
mutated again), or a -copy put into array_ (which guarantees that it can't be
mutated again).  Either way the implementation details of initialization should
not be leaking into the class' overall contract.

The prior comment (at least the one I'm thinking of) wasn't about whether the
data member should be a mutable array, it was about whether it's worthwhile to
make an immutable copy, versus simply using the mutable array you already have. 
NSMutableArray is an NSArray subclass, so you can do it either way.  But -copy
has a non-zero cost, and if nobody ever changes the NSMutableArray (because you
only have an NSArray reference to it), then the -copy cost doesn't gain any
advantages.

The common case where this kind of thing goes awry is where you have an
NSMutableArray as part of your implementation, and implement something like
-(NSArray*)array { return mutable_array_; }.  If the caller then decides to
iterate over that array calling into your API to remove selected items, then the
array being iterated unexpectedly changes contents while being iterated, which
is obviously going to be buggy.  That's why I wrote the "only reference" bit
above, because that is only possible if your code keeps both mutable and
immutable references to the same object.

[I think groby is leaning towards calling -copy and making it immutable, and the
cost of that is low enough that I'm merely neutral, so I guess that leans
towards make the copy.]

[dschuyler, 2015/06/12 21:44:30]

Thanks.  I'm hoping I'm closer now.  The array is only mutable during
construction.  It's still doing the copy, but the array is short.  I can see
where this is a trade off between a robust api and performance.  If I'm
following along correctly, we are currently on the robust api side (immutable
array and copying).

Done.

[groby-ooo-7-16, 2015/06/12 22:24:30]

Yes, please. I now my future self, and she'll make horrible mistakes when the
API is fragile. And since I like her, let's protect her ;)

Along those same lines, let's also only accept an NSArray, not an
NSMutableArray. (It'll still accept NSMutableArray, but it won't invite you to
use one)

in general, my API preference is to use the least specialized type possible,
unless there are very specific performance constraints. 

+  }
+  return self;
+}
+
+- (NSInteger)numberOfRowsInTableView:(NSTableView*)tableView {
+  return [array_ count];
+}
+
+- (id)tableView:(NSTableView*)tableView
+    objectValueForTableColumn:(NSTableColumn*)tableColumn
+                          row:(NSInteger)rowIndex {
+  return nil;

[groby-ooo-7-16, 2015/06/11 01:22:18]

As discussed in earlier CLs: This should return the actual objectValue that is
given to the cell before being displayed. I.e. the CellData associated with this
rowIndex.

That's the _job_ of this API. 

[dschuyler, 2015/06/11 22:34:23]

Done.

+}
+
+- (void)tableView:(NSTableView*)aTableView
+    setObjectValue:(id)anObject
+    forTableColumn:(NSTableColumn*)aTableColumn
+               row:(NSInteger)rowIndex {
+}

[groby-ooo-7-16, 2015/06/11 01:22:17]

NOTREACHED, please, since we'll never modify values.

[dschuyler, 2015/06/11 22:34:22]

Done.

+
+- (void)tableView:(NSTableView*)tableView
+    willDisplayCell:(id)cell
+     forTableColumn:(NSTableColumn*)tableColumn
+                row:(NSInteger)rowIndex {
+  OmniboxPopupCell* popupCell =
+      base::mac::ObjCCastStrict<OmniboxPopupCell>(cell);
+  [popupCell setObjectValue:[array_ objectAtIndex:rowIndex]];

[groby-ooo-7-16, 2015/06/11 01:22:17]

  [popupCell setObjectValue:[array_ objectAtIndex:rowIndex]];
is handled if tableView:objectValueForTableColumn: returns the proper value. 

[dschuyler, 2015/06/11 22:34:22]

Done.

+  [popupCell
+      setState:([tableView selectedRow] == rowIndex) ? NSOnState : NSOffState];
+  [popupCell highlight:(hoveredIndex_ == rowIndex)
+             withFrame:[tableView bounds]
+                inView:tableView];
+}
+
+- (NSInteger)highlightedRow {
+  return hoveredIndex_;
+}
+
+- (void)highlightRowAt:(NSInteger)rowIndex {

[groby-ooo-7-16, 2015/06/11 01:22:18]

- (void)setHighlightedRow:(NSInteger)rowIndex

setters are always marked with a beginning "set"

[dschuyler, 2015/06/11 22:34:23]

Done.

+  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
 
-- (id)initWithObserver:(OmniboxPopupMatrixObserver*)observer {
+- (instancetype)initWithObserver:(OmniboxPopupMatrixObserver*)observer {
   if ((self = [super initWithFrame:NSZeroRect])) {
     observer_ = observer;
-    [self setCellClass:[OmniboxPopupCell class]];
+
+    base::scoped_nsobject<NSTableColumn> column(
+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"]);

[groby-ooo-7-16, 2015/06/11 01:22:17]

Well, it'd be the "MainColumn" if that's the ID we want to use.

[dschuyler, 2015/06/11 22:34:22]

Done.

+    [column setDataCell:[[OmniboxPopupCell alloc] init]];
+    [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 +164,7 @@ const NSInteger kMiddleButtonNumber = 2;
 }
 
 - (void)mouseExited:(NSEvent*)theEvent {
-  [self highlightRowAt:-1];
+  [[self controller] highlightRowAt:-1];
 }
 
 // The tracking area events aren't forwarded during a drag, so handle
@@ -103,7 +194,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 +208,7 @@ const NSInteger kMiddleButtonNumber = 2;
   NSCell* selectedCell = [self selectedCell];
 
   // Clear any existing highlight.
-  [self highlightRowAt:-1];
+  [[self controller] highlightRowAt:-1];
 
   do {
     if (![self selectCellForEvent:theEvent]) {
@@ -145,6 +236,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 +267,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] highlightRowAt: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;