[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.h"
 
 #include <cmath>
 
 #include "base/stl_util.h"
 #include "base/strings/sys_string_conversions.h"
@@ skipping 47 matching lines @@
   DCHECK(edit_model);
 }
 
 OmniboxPopupViewMac::~OmniboxPopupViewMac() {
   // Destroy the popup model before this object is destroyed, because
   // it can call back to us in the destructor.
   model_.reset();
 
   // Break references to |this| because the popup may not be
   // deallocated immediately.
-  [matrix_ setDelegate:NULL];
+  [matrix_ setTheDelegate:NULL];
 }
 
 bool OmniboxPopupViewMac::IsOpen() const {
   return popup_ != nil;
 }
 
 void OmniboxPopupViewMac::UpdatePopupAppearance() {
   DCHECK([NSThread isMainThread]);
   const AutocompleteResult& result = GetResult();
   const size_t start_match = result.ShouldHideTopMatch() ? 1 : 0;
   const size_t rows = result.size() - start_match;
   if (rows == 0) {
     [[popup_ parentWindow] removeChildWindow:popup_];
     [popup_ orderOut:nil];
 
     // Break references to |this| because the popup may not be
     // deallocated immediately.
-    [matrix_ setDelegate:nil];
+    [matrix_ setTheDelegate:NULL];
     matrix_.reset();
 
     popup_.reset(nil);
 
     target_popup_frame_ = NSZeroRect;
 
     return;
   }
 
   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;

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

CGFloat.

[dschuyler, 2015/05/13 01:41:11]

Done.

+  NSRect cellRect = NSZeroRect;
+  cellRect.size.width = NSWidth([field_ bounds]);

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

Actually, just set it to [field_ bounds], so that origin.x is also initialized. 
Almost always bounds origin is at {0,0}, so it's equivalent, but this way you
can combine these two lines.

   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];
-    }
+    NSMutableDictionary* dictionary = [NSMutableDictionary dictionary];
+    AutocompleteMatchWrapper* acm =
+        [[[AutocompleteMatchWrapper alloc] init] autorelease];
+    acm->match_ = match;

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

Starting to wonder if you shouldn't rather implement [AutocompleteMatchWrapper
wrapperWithMatch:match], since every use does this.  Though my other comment
suggested that you might just consider STL structures rather than NSArray of
NSDictionary of boxed values.

[dschuyler, 2015/05/13 01:41:11]

Yeah, that makes sense.  The wrapper class is gone now.  There's a new class
that tracks the digested info from the match (without storing the match itself).
 I put the init with match on that class.

+    [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;

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

I'd set cellRect.size.height directly, then use NSHeight(cellRect) here (or
cellRect.size.height if you think it makes more sense).  My reasoning is that
the various aliases may-or-may-not maintain their equivalence as people make
changes to the code, so only have one value.

[dschuyler, 2015/05/13 01:41:11]

I used cellRect.size.height because for readability I think it's clearer that
it's using the value calculated above.  It's easier to see that
cellRect.size.height is the same.

Done.

   }
-
-  for (size_t ii = 0; ii < rows; ++ii) {
-    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
-    [cell setMaxMatchContentsWidth:max_match_contents_width];
-  }
-
-  // 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);

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

WDYT of having the entire loop in this if()?

That said, some people would argue that the DCHECK_GT() above means you
shouldn't bother with this if().

[groby-ooo-7-16, 2015/05/08 03:24:08]

+1 on "don't bother". Either we assert rows is *always* >0, or we check rows. In
fact, I'd even kill the DCHECK - the code is essentially

    if (rows == 0) {
      ...
      return;
    } 
    DCHECK_GT(rows, 0)

It's an odd assertion to make. (It's trivially true, and a decent compiler will
probably eliminate it completely anyways)

What I _would_ like to see DCHECK'd is (start_match >= result.size())

[dschuyler, 2015/05/13 01:41:11]

I went with the DCHECK_GT option :)

 
   // Update the selection before placing (and displaying) the window.
   PaintUpdatesNow();
 
   // Calculate the matrix size manually rather than using -sizeToCells
   // 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() {
   // Flip the coordinate system before returning.
   NSScreen* screen = [[NSScreen screens] objectAtIndex:0];
   NSRect monitor_frame = [screen frame];
   gfx::Rect bounds(NSRectToCGRect(target_popup_frame_));
   bounds.set_y(monitor_frame.size.height - bounds.y() - bounds.height());
   return bounds;
 }
 
 // This is only called by model in SetSelectedLine() after updating
 // 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,
                                               size_t row) {
   size_t start_match = model_->result().ShouldHideTopMatch() ? 1 : 0;
   model_->SetSelectedLine(row + start_match, false, false);
 }
 
 void OmniboxPopupViewMac::OnMatrixRowClicked(OmniboxPopupMatrix* matrix,
                                              size_t row) {
@@ skipping 60 matching lines @@
   NSRect popup_frame = anchor_rect_base;
   // Size to fit the matrix and shift down by the size.
   popup_frame.size.height = matrixHeight + kPopupPaddingVertical * 2.0;
   popup_frame.size.height += [OmniboxPopupTopSeparatorView preferredHeight];
   popup_frame.size.height += [OmniboxPopupBottomSeparatorView preferredHeight];
   popup_frame.origin.y -= NSHeight(popup_frame);
   // Shift to screen coordinates.
   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);
   top_separator_frame.size.height =
       [OmniboxPopupTopSeparatorView preferredHeight];
   [top_separator_view_ setFrame:top_separator_frame];
 
   // Bottom separator.
   NSRect bottom_separator_frame = NSZeroRect;
   bottom_separator_frame.size.width = NSWidth(popup_frame);
   bottom_separator_frame.size.height =
       [OmniboxPopupBottomSeparatorView preferredHeight];
   bottom_separator_frame.origin.y =
       NSHeight(popup_frame) - NSHeight(bottom_separator_frame);
   [bottom_separator_view_ setFrame:bottom_separator_frame];
 
   // Background view.
   NSRect background_rect = NSZeroRect;
   background_rect.size.width = NSWidth(popup_frame);
   background_rect.size.height = NSHeight(popup_frame) -
       NSHeight(top_separator_frame) - NSHeight(bottom_separator_frame);
   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];
 
   NSRect current_poup_frame = [popup_ frame];
   target_popup_frame_ = popup_frame;
 
   // Animate the frame change if the only change is that the height got smaller.
   // Otherwise, resize immediately.
   bool animate = (NSHeight(popup_frame) < NSHeight(current_poup_frame) &&
                   NSWidth(popup_frame) == NSWidth(current_poup_frame));
@@ skipping 39 matching lines @@
 }
 
 void OmniboxPopupViewMac::OpenURLForRow(size_t row,
                                         WindowOpenDisposition disposition) {
   size_t start_match = model_->result().ShouldHideTopMatch() ? 1 : 0;
   row += start_match;
   DCHECK_LT(row, GetResult().size());
   omnibox_view_->OpenMatch(GetResult().match_at(row), disposition, GURL(),
                            base::string16(), row);
 }