[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
index 577a9392f2401cb7de20a158dffe14aa1ebf001f..6b57f4c5f9f255513c0269fcbc7ea78d1d927337 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
@@ -65,7 +65,7 @@ OmniboxPopupViewMac::~OmniboxPopupViewMac() {
 
   // Break references to |this| because the popup may not be
   // deallocated immediately.
-  [matrix_ setDelegate:NULL];
+  [matrix_ setTheDelegate:NULL];
 }
 
 bool OmniboxPopupViewMac::IsOpen() const {
@@ -83,7 +83,7 @@ void OmniboxPopupViewMac::UpdatePopupAppearance() {
 
     // Break references to |this| because the popup may not be
     // deallocated immediately.
-    [matrix_ setDelegate:nil];
+    [matrix_ setTheDelegate:NULL];
     matrix_.reset();
 
     popup_.reset(nil);
@@ -95,43 +95,32 @@ void OmniboxPopupViewMac::UpdatePopupAppearance() {
 
   CreatePopupIfNeeded();
 
-  // Calculate the width of the matrix based on backing out the popup's border
-  // from the width of the field.
-  const CGFloat matrix_width = NSWidth([field_ bounds]);
-  DCHECK_GT(matrix_width, 0.0);
-
   // Load the results into the popup's matrix.
   DCHECK_GT(rows, 0U);
-  [matrix_ renewRows:rows columns:1];
-  CGFloat max_match_contents_width = 0.0f;
-  CGFloat contents_offset = -1.0f;
+  NSMutableArray* array = [NSMutableArray array];
+  NSInteger popupHeight = 0;
+  NSRect cellRect = NSZeroRect;
+  cellRect.size.width = NSWidth([field_ bounds]);
   for (size_t ii = 0; ii < rows; ++ii) {
-    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
     const AutocompleteMatch& match = GetResult().match_at(ii + start_match);
-    [cell setImage:ImageForMatch(match)];
-    [cell setMatch:match];
-    if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
-      max_match_contents_width = std::max(max_match_contents_width,
-                                           [cell getMatchContentsWidth]);
-      if (contents_offset < 0.0f) {
-        contents_offset = [OmniboxPopupCell computeContentsOffset:match];
-      }
-      [cell setContentsOffset:contents_offset];
-    }
-  }
-
-  for (size_t ii = 0; ii < rows; ++ii) {
-    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
-    [cell setMaxMatchContentsWidth:max_match_contents_width];
+    NSMutableDictionary* dictionary = [NSMutableDictionary dictionary];
+    AutocompleteMatchWrapper* acm =
+        [[[AutocompleteMatchWrapper alloc] init] autorelease];
+    acm->match_ = match;

[groby-ooo-7-16, 2015/05/07 02:39:59]

Why not keep match and image in the wrapped match? (BTW: wrappedMatch is a more
readable name than acm)

[dschuyler, 2015/05/07 20:53:10]

It feels like there are two opposing options to choose from.  One is to go with
a struct/class style to hold the data and another is to go with a dictionary
style.  I'd like to go one direction or the other, by either removing the
dictionary local or breaking pieces out as separate key/value pairs in the
dictionary.

There's a higher level choice that can be made here.  That is whether we should
keep an array of NSCell objects or whether to keep an array of either structs or
dictionaries.

I'd like to chat about the options if we get a chance :)

Done.

[Scott Hess - ex-Googler, 2015/05/07 22:35:42]

Keep in mind that we're an Objective-C++ project, and C++ is really common. 
Boxing C structures in Objective-C objects is a very FoundationKit thing to do,
but when you can just as easily use an STL container without all the boxing, I'd
say go with that.  The results sometimes look funky, but IMHO things like
-valueWithRect: are already pretty funky.

+    [dictionary setObject:acm forKey:@"match"];
+    NSImage* image = ImageForMatch(match);
+    [dictionary setObject:image forKey:@"image"];
+    cellRect.origin.y = popupHeight;
+    CGFloat height = [image size].height + kCellHeightAdjust;
+    cellRect.size.height = height;
+    [dictionary setObject:[NSValue valueWithRect:cellRect] forKey:@"rect"];
+    [array addObject:dictionary];
+    popupHeight += height;
   }
-
-  // Set the cell size to fit a line of text in the cell's font.  All
-  // cells should use the same font and each should layout in one
-  // line, so they should all be about the same height.
-  const NSSize cell_size = [[matrix_ cellAtRow:0 column:0] cellSize];
-  DCHECK_GT(cell_size.height, 0.0);
-  const CGFloat cell_height = cell_size.height + kCellHeightAdjust;
-  [matrix_ setCellSize:NSMakeSize(matrix_width, cell_height)];
+  [matrix_ setDataArray:array];
+  [matrix_ reloadData];
+  if (rows)
+    popupHeight += [matrix_ intercellSpacing].height * (rows - 1);
 
   // Update the selection before placing (and displaying) the window.
   PaintUpdatesNow();
@@ -140,7 +129,7 @@ void OmniboxPopupViewMac::UpdatePopupAppearance() {
   // because actually resizing the matrix messed up the popup size
   // animation.
   DCHECK_EQ([matrix_ intercellSpacing].height, 0.0);
-  PositionPopup(rows * cell_height);
+  PositionPopup(popupHeight);
 }
 
 gfx::Rect OmniboxPopupViewMac::GetTargetBounds() {
@@ -156,12 +145,12 @@ gfx::Rect OmniboxPopupViewMac::GetTargetBounds() {
 // everything.  Popup should already be visible.
 void OmniboxPopupViewMac::PaintUpdatesNow() {
   size_t start_match = model_->result().ShouldHideTopMatch() ? 1 : 0;
-  if (start_match > model_->selected_line()) {
-    [matrix_ deselectAllCells];
-  } else {
-    [matrix_ selectCellAtRow:model_->selected_line() - start_match column:0];
+  NSIndexSet* indexSet = [NSIndexSet indexSet];
+  if (start_match <= model_->selected_line()) {
+    indexSet =
+        [NSIndexSet indexSetWithIndex:model_->selected_line() - start_match];
   }
-
+  [matrix_ selectRowIndexes:indexSet byExtendingSelection:NO];
 }
 
 void OmniboxPopupViewMac::OnMatrixRowSelected(OmniboxPopupMatrix* matrix,
@@ -242,10 +231,6 @@ void OmniboxPopupViewMac::PositionPopup(const CGFloat matrixHeight) {
   popup_frame.origin =
       [[controller window] convertBaseToScreen:popup_frame.origin];
 
-  // Do nothing if the popup is already animating to the given |frame|.
-  if (NSEqualRects(popup_frame, target_popup_frame_))
-    return;
-
   // Top separator.
   NSRect top_separator_frame = NSZeroRect;
   top_separator_frame.size.width = NSWidth(popup_frame);
@@ -270,13 +255,18 @@ void OmniboxPopupViewMac::PositionPopup(const CGFloat matrixHeight) {
   background_rect.origin.y = NSMaxY(top_separator_frame);
   [background_view_ setFrame:background_rect];
 
+  // Calculate the width of the table based on backing out the popup's border
+  // from the width of the field.
+  const CGFloat tableWidth = NSWidth([field_ bounds]);
+  DCHECK_GT(tableWidth, 0.0);
+
   // Matrix.
   NSPoint field_origin_base =
       [field_ convertPoint:[field_ bounds].origin toView:nil];
   NSRect matrix_frame = NSZeroRect;
   matrix_frame.origin.x = field_origin_base.x - NSMinX(anchor_rect_base);
   matrix_frame.origin.y = kPopupPaddingVertical;
-  matrix_frame.size.width = [matrix_ cellSize].width;
+  matrix_frame.size.width = tableWidth;
   matrix_frame.size.height = matrixHeight;
   [matrix_ setFrame:matrix_frame];