[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..30135da7c7aa02a8d108b3b62377bb49f6eab399 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
@@ -29,11 +29,7 @@
 
 namespace {
 
-// How much to adjust the cell sizing up from the default determined
-// by the font.
-const CGFloat kCellHeightAdjust = 6.0;
-
-// Padding between matrix and the top and bottom of the popup window.
+// Padding between table and the top and bottom of the popup window.
 const CGFloat kPopupPaddingVertical = 5.0;
 
 // Animation duration when animating the popup window smaller.
@@ -65,13 +61,19 @@ OmniboxPopupViewMac::~OmniboxPopupViewMac() {
 
   // Break references to |this| because the popup may not be
   // deallocated immediately.
-  [matrix_ setDelegate:NULL];
+  [table_view_ setCppDelegate:NULL];
 }
 
 bool OmniboxPopupViewMac::IsOpen() const {
   return popup_ != nil;
 }
 
+void OmniboxPopupViewMac::InvalidateLine(size_t line) {
+  // Update the selection state so that it's drawn properly later.
+  [[table_view_ cellAtRow:line]
+      setState:(line == model_->selected_line() ? NSOnState : NSOffState)];

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

Does this invalidate if that line is already set to that state?

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

Not that I'm aware of.  In my testing it is called twice per line change (once
for the old line and once for the new line.

+}
+
 void OmniboxPopupViewMac::UpdatePopupAppearance() {
   DCHECK([NSThread isMainThread]);
   const AutocompleteResult& result = GetResult();
@@ -83,8 +85,8 @@ void OmniboxPopupViewMac::UpdatePopupAppearance() {
 
     // Break references to |this| because the popup may not be
     // deallocated immediately.
-    [matrix_ setDelegate:nil];
-    matrix_.reset();
+    [table_view_ setCppDelegate:NULL];
+    table_view_.reset();
 
     popup_.reset(nil);
 
@@ -95,18 +97,14 @@ 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);
+  [table_view_ setCellCount:rows];
 
-  // Load the results into the popup's matrix.
+  // Load the results into the popup's table.
   DCHECK_GT(rows, 0U);
-  [matrix_ renewRows:rows columns:1];
   CGFloat max_match_contents_width = 0.0f;
   CGFloat contents_offset = -1.0f;
   for (size_t ii = 0; ii < rows; ++ii) {

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

|ii|?  Criminy, when did I write this?

-    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
+    OmniboxPopupCell* cell = [table_view_ cellAtRow:ii];
     const AutocompleteMatch& match = GetResult().match_at(ii + start_match);
     [cell setImage:ImageForMatch(match)];
     [cell setMatch:match];
@@ -119,28 +117,26 @@ void OmniboxPopupViewMac::UpdatePopupAppearance() {
       [cell setContentsOffset:contents_offset];
     }
   }
+  [table_view_ noteNumberOfRowsChanged];
 
+  NSInteger popupHeight = 0;
   for (size_t ii = 0; ii < rows; ++ii) {
-    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
+    OmniboxPopupCell* cell = [table_view_ cellAtRow:ii];
     [cell setMaxMatchContentsWidth:max_match_contents_width];
+    popupHeight += [cell cellSize].height;
+  }
+  if (rows) {
+    popupHeight += [table_view_ intercellSpacing].height * (rows - 1);
   }
-
-  // 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)];
 
   // 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
+  // Calculate the table size manually rather than using -sizeToCells
+  // because actually resizing the table messed up the popup size
   // animation.
-  DCHECK_EQ([matrix_ intercellSpacing].height, 0.0);
-  PositionPopup(rows * cell_height);
+  DCHECK_EQ([table_view_ intercellSpacing].height, 0.0);
+  PositionPopup(popupHeight);
 }
 
 gfx::Rect OmniboxPopupViewMac::GetTargetBounds() {
@@ -156,28 +152,29 @@ 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];
   }
-
+  [table_view_ selectRowIndexes:indexSet byExtendingSelection:NO];
 }
 
-void OmniboxPopupViewMac::OnMatrixRowSelected(OmniboxPopupMatrix* matrix,
-                                              size_t row) {
+void OmniboxPopupViewMac::OnTableRowSelected(OmniboxPopupTableView* table_view,
+                                             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) {
+void OmniboxPopupViewMac::OnTableRowClicked(OmniboxPopupTableView* table_view,
+                                            size_t row) {
   OpenURLForRow(row,
                 ui::WindowOpenDispositionFromNSEvent([NSApp currentEvent]));
 }
 
-void OmniboxPopupViewMac::OnMatrixRowMiddleClicked(OmniboxPopupMatrix* matrix,
-                                                   size_t row) {
+void OmniboxPopupViewMac::OnTableRowMiddleClicked(
+    OmniboxPopupTableView* table_view,
+    size_t row) {
   OpenURLForRow(row, NEW_BACKGROUND_TAB);
 }
 
@@ -195,13 +192,13 @@ void OmniboxPopupViewMac::CreatePopupIfNeeded() {
     [popup_ setBackgroundColor:[NSColor clearColor]];
     [popup_ setOpaque:NO];
 
-    // Use a flipped view to pin the matrix top the top left. This is needed
+    // Use a flipped view to pin the table view top the top left. This is needed
     // for animated resize.
     base::scoped_nsobject<FlippedView> contentView(
         [[FlippedView alloc] initWithFrame:NSZeroRect]);
     [popup_ setContentView:contentView];
 
-    // View to draw a background beneath the matrix.
+    // View to draw a background beneath the table.
     background_view_.reset([[NSBox alloc] initWithFrame:NSZeroRect]);
     [background_view_ setBoxType:NSBoxCustom];
     [background_view_ setBorderType:NSNoBorder];
@@ -209,8 +206,8 @@ void OmniboxPopupViewMac::CreatePopupIfNeeded() {
     [background_view_ setContentViewMargins:NSZeroSize];
     [contentView addSubview:background_view_];
 
-    matrix_.reset([[OmniboxPopupMatrix alloc] initWithDelegate:this]);
-    [background_view_ addSubview:matrix_];
+    table_view_.reset([[OmniboxPopupTableView alloc] initWithDelegate:this]);
+    [background_view_ addSubview:table_view_];
 
     top_separator_view_.reset(
         [[OmniboxPopupTopSeparatorView alloc] initWithFrame:NSZeroRect]);
@@ -226,15 +223,15 @@ void OmniboxPopupViewMac::CreatePopupIfNeeded() {
   }
 }
 
-void OmniboxPopupViewMac::PositionPopup(const CGFloat matrixHeight) {
+void OmniboxPopupViewMac::PositionPopup(const CGFloat tableHeight) {
   BrowserWindowController* controller =
       [BrowserWindowController browserWindowControllerForView:field_];
   NSRect anchor_rect_base = [controller omniboxPopupAnchorRect];
 
   // Calculate the popup's position on the screen.
   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;
+  // Size to fit the table view and shift down by the size.
+  popup_frame.size.height = tableHeight + kPopupPaddingVertical * 2.0;
   popup_frame.size.height += [OmniboxPopupTopSeparatorView preferredHeight];
   popup_frame.size.height += [OmniboxPopupBottomSeparatorView preferredHeight];
   popup_frame.origin.y -= NSHeight(popup_frame);
@@ -243,8 +240,8 @@ void OmniboxPopupViewMac::PositionPopup(const CGFloat matrixHeight) {
       [[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;
+  // if (NSEqualRects(popup_frame, target_popup_frame_))
+  //   return;

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

Pretty sure this was here for a reason.  Unfortunately I don't know what that
reason was any longer.  I'd verify that there aren't edge cases like
re-positioning while the animation is still running causing the animation to
restart or something.  Maybe there are old bugs to consult.

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

It's Saturday, I was bored, git blame smiled alluringly... There's no bug to
consult. The CL is https://codereview.chromium.org/2010002/

The animation code seems to have changed significantly since then, so I'd
recommend trying if this affects the repro case mentioned in that CL.

 
   // Top separator.
   NSRect top_separator_frame = NSZeroRect;
@@ -270,15 +267,19 @@ void OmniboxPopupViewMac::PositionPopup(const CGFloat matrixHeight) {
   background_rect.origin.y = NSMaxY(top_separator_frame);
   [background_view_ setFrame:background_rect];
 
-  // Matrix.
+  // 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);
+
   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.height = matrixHeight;
-  [matrix_ setFrame:matrix_frame];
+  NSRect table_frame = NSZeroRect;
+  table_frame.origin.x = field_origin_base.x - NSMinX(anchor_rect_base);
+  table_frame.origin.y = kPopupPaddingVertical;
+  table_frame.size.width = tableWidth;
+  table_frame.size.height = tableHeight;
+  [table_view_ setFrame:table_frame];
 
   NSRect current_poup_frame = [popup_ frame];
   target_popup_frame_ = popup_frame;