[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 65 matching lines @@
   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_ setObserver: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];
+  NSMutableArray* array = [NSMutableArray array];

[Scott Hess - ex-Googler, 2015/05/15 00:09:38]

I'd also do scoped_nsobject<> of [[NSMutableArray alloc] init] here.

[dschuyler, 2015/05/15 21:52:04]

Done.

+  CGFloat popupHeight = 0;
+  NSRect cellRect = NSZeroRect;
+  cellRect.size.width = NSWidth([field_ bounds]);
   CGFloat max_match_contents_width = 0.0f;
   CGFloat contents_offset = -1.0f;
   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];
+    base::scoped_nsobject<OmniboxPopupCellData> cellData(
+        [[OmniboxPopupCellData alloc] initWithMatch:match]);
+    cellRect.origin.y = popupHeight;
+    NSImage* image = ImageForMatch(match);
+    cellRect.size.height = [image size].height + kCellHeightAdjust;
+    [cellData setRect:cellRect];
+    [cellData setImage:image];

[Scott Hess - ex-Googler, 2015/05/15 00:09:38]

Should these be individual setters, or should the designated initializer be
something like -initWithMatch:rect:image:?

Also, should the image be calculated here, or should the cell data calculate it
from the match?

Hmm, and should the cell data even have a rect?  Isn't the rect defined by the
cell which the cellData is put into?  What would happen if the rect set here
conflicted with the rect the cell is being drawn into?

[groby-ooo-7-16, 2015/05/15 16:53:56]

I'd prefer -initWithMatch:image - because that way, we can remove the setter and
make clear that yes, cellData is immutable. (See previous comment on
copyWithZone)

And yes, the rect should be on the cell. 

[Scott Hess - ex-Googler, 2015/05/15 17:28:59]

I don't think it can get to zero setters, because -setMaxMatchContentsWidth:
needs to receive the accumulated-across-the-loop data, and I'm not entirely sure
having separate initializers for with/without-contentsOffset: (plus associated
if/else up here) is worth it.

AFAICT, since maxMatchContentsWidth_ is only used at display time, it could be a
single attribute of the display cell rather than a duplicated attribute of the
displayed data.  But that would just require adding parameters to pass it around
in the display code, so practically I'm not sure it's worthwhile.

[dschuyler, 2015/05/15 21:52:04]

Done.

+    [array addObject:cellData];
+    popupHeight += cellRect.size.height;
     if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
-      max_match_contents_width = std::max(max_match_contents_width,
-                                           [cell getMatchContentsWidth]);
+      max_match_contents_width =
+          std::max(max_match_contents_width, [cellData getMatchContentsWidth]);
       if (contents_offset < 0.0f) {
         contents_offset = [OmniboxPopupCell computeContentsOffset:match];

[Scott Hess - ex-Googler, 2015/05/15 17:28:59]

Does this helper still belong in the cell class, or in the data class?  Not
really arguing for either way, but having the width calculation just above move
to the data object is suggestive.  Making it an instance variable might also
make sense, like:
  [cellData computeContentsOffset];

[dschuyler, 2015/05/15 21:52:04]

It sounds like this search suggest tail feature is still a work in progress. 
I'd like to minimize changes to it until Anuj can get further along with it and
we can test the changes.

[Scott Hess - ex-Googler, 2015/05/15 22:02:08]

In that case I can definitely see not changing from a class method to an
instance method, your call as to whether both methods should travel together
(both are in the _TAIL case).

[dschuyler, 2015/05/18 23:38:44]

I've contacted Anuj about the fact that I'm making changes in this area.  Let's
keep this as it is for this CL.

       }
-      [cell setContentsOffset:contents_offset];
+      [cellData setContentsOffset:contents_offset];
     }
   }
 
   for (size_t ii = 0; ii < rows; ++ii) {
-    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
-    [cell setMaxMatchContentsWidth:max_match_contents_width];
+    OmniboxPopupCellData* cellData = [array objectAtIndex:ii];
+    [cellData 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];
+  popupHeight += [matrix_ intercellSpacing].height * (rows - 1);
 
   // 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];
+  NSIndexSet* indexSet = nil;
+  if (start_match <= model_->selected_line()) {
+    indexSet =
+        [NSIndexSet indexSetWithIndex:model_->selected_line() - start_match];
   } else {
-    [matrix_ selectCellAtRow:model_->selected_line() - start_match column:0];
+    indexSet = [NSIndexSet indexSet];
   }

[Scott Hess - ex-Googler, 2015/05/15 00:09:38]

Nevermind my earlier (or later?) comment, this is right.  I misunderstood the
suggestion, and am too tired to find my other comment.

[dschuyler, 2015/05/15 21:52:04]

Acknowledged.

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