[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..788c72d7a65041790efbef1c743d4d55ebcd71ae 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,9 +17,106 @@ const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@implementation OmniboxPopupTableController
+
+- (id)init {
+  if ((self = [super init])) {
+    hoveredIndex_ = -1;
+  }
+  return self;
+}
+
+- (id)initWithMatchResults:(const AutocompleteResult&)result
+                 popupView:(const OmniboxPopupViewMac&)popupView {
+  if ((self = [super init])) {
+    hoveredIndex_ = -1;
+    // Load the results into the popup's matrix.
+    const size_t start_match = result.ShouldHideTopMatch() ? 1 : 0;
+    const size_t rows = result.size() - start_match;
+    DCHECK_GT(rows, 0U);

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

IMHO this would be better as:
   DCHECK_GT(result.size(), start_match);

[dschuyler, 2015/05/26 18:40:20]

Done.

+    base::scoped_nsobject<NSMutableArray> array([[NSMutableArray alloc] init]);

[groby-ooo-7-16, 2015/05/21 03:01:48]

Why not keep it directly in array_?

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

array_ is NSArray.  Which makes me wonder if array_.reset([array copy]).  I
can't think of any strong reason against array_ being NSMutableArray.  It's less
correct in a dimension which is fairly irrelevant.  Then again this is kind of a
rathole to go down.

[dschuyler, 2015/05/26 18:40:20]

If there are good reasons to choose style A or B without a clear winner, I'm
inclined to stay with whichever way is currently written.  Of course, it's up to
you two :)  [to paraphrase a friend of mine]: Is one way more betterer than the
other?

[dschuyler, 2015/05/26 18:40:20]

For reasons Scott gives below. :)

[groby-ooo-7-16, 2015/05/26 21:23:22]

Acknowledged.

[Scott Hess - ex-Googler, 2015/05/29 18:00:26]

This is like const functions in C++, where there's right, and there's sanity. 
NSArray should be immutable, so technically array_.reset([array copy]) is more
correcter.  But so long as you pass off the only copy and never downcast to
modify things, it doesn't really matter if the actual array_ value is an NSArray
or NSMutableArray, and just using the array you're constructing things in is
cheaper.

IMHO since this is a transient UI element, storing the construction array is
just fine, there's not a lot of scope for things to go awry.  If it was
calculating this once and then storing it for long-term use, I'd flip to wanting
an immutable copy.  Style-wise, another place I get uncomfortable is if someone
has an internal mutable array and returns an NSArray to some caller, since that
can allow for subtle bugs if the caller expects that to be an immutable copy,
which isn't going to come up in this case.

[dschuyler, 2015/06/09 01:30:46]

Thanks

Done.

+    CGFloat max_match_contents_width = 0.0f;
+    CGFloat contents_offset = -1.0f;
+    for (size_t ii = 0; ii < rows; ++ii) {

[groby-ooo-7-16, 2015/05/21 03:01:48]

Please don't perpetuate ii :) Best, don't even loop manually.

AutocompleteResult conforms to STL's iterator conventions, so
for (const AutocompleteMatch& match : result) {

will do the trick. Plus, potentially a removeObjectAtIndex:0 at the end if we
hide the top match. Yes, we pay one extra conversion, but it makes the code more
readable.

[Scott Hess - ex-Googler, 2015/05/21 04:46:36]

Yeah, that's some ancient habit of mine which has since been ground away.  ii
and jj are way easier to do search/replace on, but it's the kind of thing you
need in the style guide to make sense, and most people just think it's weird.

[dschuyler, 2015/05/26 18:40:20]

Acknowledged.

[dschuyler, 2015/05/26 18:40:20]

The <should hide top match> stuff is a discontinued experiment afaik.  Peter had
asked if I wanted to remove it, but I was planning to minimize this CL and do it
later.  It can be done now if it makes this CL simpler.

Done.

[groby-ooo-7-16, 2015/05/26 21:23:22]

If you can do that as a prior CL and rebase this CL onto that cleanup, much
appreciated. 

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

That CL is now started.

[dschuyler, 2015/06/10 20:48:31]

Done.

+      const AutocompleteMatch& match = result.match_at(ii + start_match);
+      NSImage* image = popupView.ImageForMatch(match);

[groby-ooo-7-16, 2015/05/21 03:01:48]

Bleh. I hate we have to pass the popupView. I wonder if we need to hoist the
datasource out.

[dschuyler, 2015/05/26 18:40:21]

Which direction is the right way to go for this particular CL?
Could we alter that in a later CL?

[groby-ooo-7-16, 2015/05/26 21:23:22]

Yes, I think that's fine. It's not perfect, but it's OK (IMHO). Please do add a
TODO/follow-up bug, though.

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

Bug entered 492896

+      base::scoped_nsobject<OmniboxPopupCellData> cellData(
+          [[OmniboxPopupCellData alloc] initWithMatch:match image:image]);
+      [array addObject:cellData];

[groby-ooo-7-16, 2015/05/21 03:01:48]

You _could_ just move the alloc/initWithMatch call in here and skip the scoped
temporary. (Yes, deeply nested message chains are common in Cocoa :)

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

Someone needs to own the +alloc ref, though.

[groby-ooo-7-16, 2015/05/22 05:03:07]

Yep:

[array addObject:[[[OmniboxPopupCellData alloc] initWithMatch: match
image:image] autorelease]];

Ultimately, I'm fine either way with a _slight_ preference to avoiding temp
objects.

[Scott Hess - ex-Googler, 2015/05/22 05:17:58]

I think that means you can do what you wish, either use a scoped_nsobject<> or
autorelease.  I think it's six of one and half-a-dozen of the other, in the end,
where my earlier "tiny bit of performance" argument gets offset by "looks more
like Cocoa" - so I guess experiment then decide, so that you can be consistent
in your code so that readers aren't getting confused.

[dschuyler, 2015/05/26 18:40:21]

Acknowledged.

+      if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
+        max_match_contents_width = std::max(max_match_contents_width,

[groby-ooo-7-16, 2015/05/21 03:01:48]

So max_match_contents_width stays 0 unless you have a SEARCH_SUGGEST_TAIL?

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

Yeah, this is a thing we ground on about in the original CL.  You can either
have zero or one set of results like this.  When you have one, they are in a
block, and the prefix is elided so that you can see the grouping visually.  I'm
still not entirely convinced it wouldn't be better as a distinct loop, but I
suspect it's messy either way.

[I think that was accurate.  Multiple cells needed to align something.]

[dschuyler, 2015/05/26 18:40:20]

Acknowledged.

[dschuyler, 2015/05/26 18:40:20]

Acknowledged.

[dschuyler, 2015/05/26 18:40:21]

I'd like to leave the TAIL stuff unchanged until Anuj can get involved.

[groby-ooo-7-16, 2015/05/26 21:23:22]

Acknowledged.

+                                            [cellData getMatchContentsWidth]);
+        if (contents_offset < 0.0f) {

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

Drop the {} here.

[dschuyler, 2015/05/26 18:40:20]

Done.

+          contents_offset = [OmniboxPopupCell computeContentsOffset:match];
+        }
+        [cellData setContentsOffset:contents_offset];
+      }
+    }
+
+    for (size_t ii = 0; ii < rows; ++ii) {

[groby-ooo-7-16, 2015/05/21 03:01:48]

for (OmniboxPopupCellData* cellData : array)
  [cellData setMax....

[dschuyler, 2015/05/26 18:40:20]

Done.

+      OmniboxPopupCellData* cellData = [array objectAtIndex:ii];
+      [cellData setMaxMatchContentsWidth:max_match_contents_width];
+    }
+    array_.reset([array retain]);
+  }
+  return self;
+}
+
+- (id)initWithArray:(NSArray*)array {

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

instancetype.

[dschuyler, 2015/05/26 18:40:20]

Done.

+  if ((self = [super init])) {
+    hoveredIndex_ = -1;
+    array_.reset([array retain]);
+  }
+  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);
+  [popupCell setRepresentedObject:[[array_ objectAtIndex:rowIndex] retain]];
+  [popupCell
+      setState:([tableView selectedRow] == rowIndex) ? NSOnState : NSOffState];
+  [popupCell highlight:(hoveredIndex_ == rowIndex)
+             withFrame:[tableView bounds]
+                inView:tableView];
+}
+
+- (NSInteger)highlightedRow {
+  return hoveredIndex_;
+}
+
+- (void)highlightRowAt:(NSInteger)rowIndex {
+  hoveredIndex_ = rowIndex;
+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  OmniboxPopupCellData* cellData = [array_ objectAtIndex:row];

[groby-ooo-7-16, 2015/05/21 03:01:48]

chaining methods would be preferred - no need to have temps.
return [[array_ objectAtIndex:row] rowHeight];

[dschuyler, 2015/05/26 18:40:20]

Done.

+  return [cellData rowHeight];

[groby-ooo-7-16, 2015/05/21 03:01:48]

Just a general note on tables, no need to implement:

If the data source were a separate class, you'd invoke
-preparedCellAtColumn:row: to get a cell populated with the appropriate data.

That method, by the way, is where the cell's objectValue is set from the
datasource.

[dschuyler, 2015/05/26 18:40:21]

Acknowledged.

+}
+
+@end
+
+@interface OmniboxPopupMatrix ()
 - (void)resetTrackingArea;
-- (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
@@ -26,36 +126,34 @@ const NSInteger kMiddleButtonNumber = 2;
 - (id)initWithObserver:(OmniboxPopupMatrixObserver*)observer {
   if ((self = [super initWithFrame:NSZeroRect])) {
     observer_ = observer;
-    [self setCellClass:[OmniboxPopupCell class]];
+
+    base::scoped_nsobject<NSTableColumn> column(
+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"]);
+    base::scoped_nsobject<OmniboxPopupCell> columnCell(
+        [[OmniboxPopupCell alloc] init]);

[groby-ooo-7-16, 2015/05/21 03:01:48]

Again, skip the scoped temp and chain methods. 

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

I think this one also leaves an additional ref unless you have the scoper or
autorelease.

[groby-ooo-7-16, 2015/05/22 05:03:07]

Acknowledged.

[dschuyler, 2015/05/26 18:40:20]

Acknowledged.

[dschuyler, 2015/05/26 18:40:21]

Done.

+    [column setDataCell:columnCell];
+    [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 +171,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 +201,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 +215,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 +243,21 @@ 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 {
+  [self setDelegate:controller];
+  [self setDataSource:controller];
+  [self reloadData];
+}
+
 - (void)resetTrackingArea {
   if (trackingArea_.get())
     [self removeTrackingArea:trackingArea_.get()];
@@ -160,33 +273,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;