[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..ef97f09abe37e4b54dc43ff24d5eede131ce1699 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
@@ -14,41 +14,97 @@ const NSInteger kMiddleButtonNumber = 2;
 
 }  // namespace
 
-@interface OmniboxPopupMatrix()
+@implementation OmniboxPopupTableController
+
+- (id)init {
+  if ((self = [super init])) {
+    array_ = [[NSMutableArray alloc] init];
+  }
+  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];
+}
+
+- (NSCell*)tableView:(NSTableView*)tableView
+    dataCellForTableColumn:(NSTableColumn*)tableColumn
+                       row:(NSInteger)row {
+  // Returning the cell populates data in the table.
+  return [array_ objectAtIndex:row];
+}
+
+- (void)setCellCount:(NSUInteger)count {
+  // Add more.
+  while (count >= [array_ count]) {

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

Suggest while ([array_ count] < count)...  Might be just me, but I find >= and
<= to be signals for "Go back and review this code a couple times, because it's
probably incorrect."

Also, I might be confused, but I figured you defined the cell copy code to allow
NSTableView to populate things, but here you're creating fresh cells and
returning them above.  So does the copy code ever actually kick into action?

[dschuyler, 2015/05/01 21:50:54]

I'm sure that in previous test I've done, it was being called.  Even though I'm
not seeing it called today, it seems correct to implement copyWithZone.  Would
it be better without the copyWithZone?

Done.

+    [array_ addObject:[[[OmniboxPopupCell alloc] init] autorelease]];
+  }
+  // Remove excess.
+  if (count < [array_ count])
+    [array_ removeObjectsInRange:NSMakeRange(count, [array_ count] - count)];

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

This is documented as using -removeObjectsAtIndex: as the implementation.  I
wonder if it's smart relative to what you'd get with repeated application of
-removeLastObject.  It _probably_ is.

[dschuyler, 2015/05/01 21:50:54]

Previously in this CL, it was calling removeLastObject.  Should I consider
changing it back or should I leave it as is?

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

I'm mixed.  Rachel suggested that change, and I can see why, but until I noticed
she had suggested that I was going to suggest:
  while (count < [array_ count]) {
    [array_ removeLastObject];
  }
because it reads simpler.  And now I'm thinking "NSArray is annoying" followed
closely by "I wonder if std::vector<scoped_nsobject<>> works like I think it
would?"

I suggest you leave it as-is and ignore me :-).

+  DCHECK_EQ(count, [array_ count]);
+  // Set selection to be the first cell.
+  if (count)
+    [[array_ objectAtIndex:0] setState:NSOnState];

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

Since you may have re-used existing cells in array_, how do you know no other
cells have NSOnState set?

[dschuyler, 2015/05/01 21:50:54]

Good catch.

Done.

+}
+
+- (NSArray*)getCellArray {
+  return array_;

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

You document this as an immutable copy, but it can change after you return it if
the caller makes calls which change array_.  Consider [[array_ copy]
autorelease].

[groby-ooo-7-16, 2015/04/25 19:01:12]

Technically, if the controller managed highlight/returning cell for a row, we
don't need to expose the array at all. I'm all in favor of hiding implementation
details :)

[dschuyler, 2015/05/01 21:50:53]

Done.

[dschuyler, 2015/05/01 21:50:54]

That comment was odd - the array itself would not change, though the contents
would.  (Some math books would say the array would not 'change shape').  As
Groby points out, it's reasonable to simply remove that function and its comment
:)

+}
+
+- (CGFloat)tableView:(NSTableView*)tableView heightOfRow:(NSInteger)row {
+  return [[array_ objectAtIndex:row] cellSize].height;
+}
+
+@end
+
+@interface OmniboxPopupTableView ()
 - (void)resetTrackingArea;
 - (void)highlightRowAt:(NSInteger)rowIndex;
 - (void)highlightRowUnder:(NSEvent*)theEvent;
 - (BOOL)selectCellForEvent:(NSEvent*)theEvent;
 @end
 
-@implementation OmniboxPopupMatrix
+@implementation OmniboxPopupTableView
 
-- (id)initWithDelegate:(OmniboxPopupMatrixDelegate*)delegate {
+- (id)initWithDelegate:(OmniboxPopupTableDelegate*)delegate {
   if ((self = [super initWithFrame:NSZeroRect])) {
     delegate_ = delegate;
-    [self setCellClass:[OmniboxPopupCell class]];
+    controller_ = [[OmniboxPopupTableController alloc] init];
+
+    NSTableColumn* column =

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

scoped_nsobject<>.

[dschuyler, 2015/05/01 21:50:54]

Could I have done autorelease here instead of the scoped_nsobject?

Done.

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

scoped_nsobject<> means "Add a reference here, drop the reference when going out
of scope."  -autorelease means "Add this reference to a structure somewhere on
the stack, which will be cleaned up in the future."  Apple has done a ton to
make -autorelease efficient, so performance-wise it's not super compelling, but
having the object released in the scope of use rather than after the code in
question runs is almost always a benefit, because if something goes wrong it's
much easier to backtrack to the initiating code.

+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"];
+    [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 setBackgroundColor:[NSColor controlBackgroundColor]];
-    [self renewRows:0 columns:1];
     [self setAllowsEmptySelection:YES];
-    [self setMode:NSRadioModeMatrix];
-    [self deselectAllCells];
+    [self deselectAll:self];
+    [self setSelectionHighlightStyle:NSTableViewSelectionHighlightStyleNone];
 
     [self resetTrackingArea];
   }
   return self;
 }
 
-- (void)setDelegate:(OmniboxPopupMatrixDelegate*)delegate {
+- (void)setCppDelegate:(OmniboxPopupTableDelegate*)delegate {
   delegate_ = delegate;
 }
 
 - (NSInteger)highlightedRow {
-  NSArray* cells = [self cells];
+  NSArray* cells = [controller_ getCellArray];
   const NSUInteger count = [cells count];
   for(NSUInteger i = 0; i < count; ++i) {
     if ([[cells objectAtIndex:i] isHighlighted]) {
@@ -106,7 +162,7 @@ const NSInteger kMiddleButtonNumber = 2;
   const NSInteger highlightedRow = [self highlightedRow];
   if (highlightedRow != -1) {
     DCHECK(delegate_);
-    delegate_->OnMatrixRowMiddleClicked(self, highlightedRow);
+    delegate_->OnTableRowMiddleClicked(self, highlightedRow);
   }
 }
 
@@ -141,10 +197,21 @@ const NSInteger kMiddleButtonNumber = 2;
     }
 
     DCHECK(delegate_);
-    delegate_->OnMatrixRowClicked(self, selectedRow);
+    delegate_->OnTableRowClicked(self, selectedRow);
   }
 }
 
+- (void)setCellCount:(NSUInteger)count {
+  return [controller_ setCellCount:count];
+}
+
+- (OmniboxPopupCell*)cellAtRow:(NSUInteger)row {
+  NSArray* array = [controller_ getCellArray];
+  if (row >= [array count])
+    return nil;
+  return [array objectAtIndex:row];
+}
+
 - (void)resetTrackingArea {
   if (trackingArea_.get())
     [self removeTrackingArea:trackingArea_.get()];
@@ -163,32 +230,30 @@ const NSInteger kMiddleButtonNumber = 2;
 - (void)highlightRowAt:(NSInteger)rowIndex {
   // highlightCell will be nil if rowIndex is out of range, so no cell will be
   // highlighted.

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

I think the out-of-range at this point is if the mouse is in the padding above
or below the view.  I don't recall the exact details, I think at one time this
view acted as the backing for the popup window, but now I think there may be a
superview doing that job, in which case it may no longer be possible to be above
or below the valid rows.  In any case, the use of -cellAtRow:column: wasn't
because that's right, but because that worked, so I don't think you need to
emulate that here for any particular reason.

Also, I think the comment is out-of-place now.  It looks to me like it applies
to the if() just below, but really it applies to the if() in the for() loop.

[dschuyler, 2015/05/01 21:50:53]

This code has been reworked in the new patch.

-  NSCell* highlightCell = [self cellAtRow:rowIndex column:0];
+  OmniboxPopupCell* highlightCell = [self cellAtRow:rowIndex];
+  if ([highlightCell isHighlighted])
+    return;
 
-  for (NSCell* cell in [self cells]) {
-    [cell setHighlighted:(cell == highlightCell)];
+  for (OmniboxPopupCell* cell in controller_.getCellArray) {

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

I have no particular preference WRT controller_.getCellArray (well, I do, but my
preference is ancient), but this code generally uses [controller_ getCellArray]
style, so I think stick with that.

+    if ([cell isHighlighted] || cell == highlightCell) {
+      [cell highlight:(cell == highlightCell) withFrame:_bounds inView:self];
+    }

[Scott Hess - ex-Googler, 2015/04/25 06:04:27]

I'm not really sure what this is saying.  The previous code set the highlight
state of all the cells, exactly one of them would get true, the rest false.  I
think this maybe un-highlights the one which is highlighted, and highlights the
one which is not?  I think something like this would be clearer:

  if (cell == highlightCell) {
    [cell highlight:YES ...];
  } else if ([cell isHighlighted]) {
    [cell highlight:NO ...];
  }

That said, the internal implementation of -highlight:* should short-circuit if
the highlight state is the same, which means that calling -isHighlighted to
check whether it is highlighted is approximately the same cost as just calling
-highlight:* in the first place.

[groby-ooo-7-16, 2015/04/25 19:01:12]

I'll re-ask the question I asked in my last comments: Is there a particular
reason the code doesn't just track the currently highlighted cell? Why always
iterate over everything? Am I missing a Cocoa subtlety here?

[dschuyler, 2015/05/01 21:50:54]

Done.

[dschuyler, 2015/05/01 21:50:54]

This code has been reworked in the new patch.  It seems to work with tracking
the values independently so far.

   }
 }
 
 - (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];
-  }
+  [self highlightRowAt:[self rowAtPoint:point]];
 }
 
 // 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];
+  if (row != -1) {
     DCHECK(delegate_);
-    delegate_->OnMatrixRowSelected(self, row);
+    delegate_->OnTableRowSelected(self, row);
     return YES;
   }
   return NO;