[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h
index 2ce61674c6b3890e773de072237ebebe06a5aae6..22e3996b54842bc7f1c8f8cea95493acc60adf72 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h
@@ -10,37 +10,62 @@
 #import "ui/base/cocoa/tracking_area.h"
 #include "ui/base/window_open_disposition.h"
 
-@class OmniboxPopupMatrix;
+@class OmniboxPopupCell;
 
-class OmniboxPopupMatrixDelegate {
+@interface OmniboxPopupTableController
+    : NSViewController<NSTableViewDelegate, NSTableViewDataSource> {
+ @private
+  NSMutableArray* array_;

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

Should this be a scoped_nsobject<NSMutableArray>?

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

Done.

+};
+
+// Allocate or free cells so that count are available.
+// Also sets up default settings such as selecting the first cell.
+- (void)setCellCount:(NSUInteger)count;
+
+// Get an immutable version of the array.
+- (NSArray*)getCellArray;
+
+@end
+
+@class OmniboxPopupTableView;
+
+class OmniboxPopupTableDelegate {
  public:
-  // Called when the selection in the matrix changes.
-  virtual void OnMatrixRowSelected(OmniboxPopupMatrix* matrix, size_t row) = 0;
+  // Called when the selection in the table changes.
+  virtual void OnTableRowSelected(OmniboxPopupTableView* table_view,
+                                  size_t row) = 0;
 
   // Called when the user clicks on a row.
-  virtual void OnMatrixRowClicked(OmniboxPopupMatrix* matrix, size_t row) = 0;
+  virtual void OnTableRowClicked(OmniboxPopupTableView* table_view,
+                                 size_t row) = 0;
 
   // Called when the user middle clicks on a row.
-  virtual void OnMatrixRowMiddleClicked(OmniboxPopupMatrix* matrix,
-                                        size_t row) = 0;
+  virtual void OnTableRowMiddleClicked(OmniboxPopupTableView* table_view,
+                                       size_t row) = 0;
 };
 
-// Sets up a tracking area to implement hover by highlighting the cell the mouse
-// is over.
-@interface OmniboxPopupMatrix : NSMatrix {
-  OmniboxPopupMatrixDelegate* delegate_;  // weak
+@interface OmniboxPopupTableView : NSTableView {
+  OmniboxPopupTableDelegate* delegate_;  // weak
   ui::ScopedCrTrackingArea trackingArea_;
+  OmniboxPopupTableController* controller_;

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

scoped_nsobject<>?  Someone needs to own it.

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

Would the super own it with the setDelegate or setDataSource?  Would it hurt to
to scope it to this derived class even if the super does so as well?

Done.

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

Usually, delegates are not owned pointers.  I believe -setDataSource: also does
not take ownership.  Even if they did, the code in your designated -init was
creating a strong reference and never releasing it, so someone needs to take
long-term ownership of that reference (or you could delete it, but in this case
I think that would just lead to use-after-free).

 }
 
-// Create a zero-size matrix.
-- (id)initWithDelegate:(OmniboxPopupMatrixDelegate*)delegate;
+// Create a zero-size table.
+- (id)initWithDelegate:(OmniboxPopupTableDelegate*)delegate;
 
 // Sets the delegate.
-- (void)setDelegate:(OmniboxPopupMatrixDelegate*)delegate;
+- (void)setCppDelegate:(OmniboxPopupTableDelegate*)delegate;

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

I think that's a reasonable change, if clunky (but C++ plus Objective-C can be
clunky), but if you make it you should be comprehensive about it. 
-initWithCppDelegate, cpp_delegate, etc.  Or figure out an alternative phrasing,
though that is prone to being even weirder when someone else starts using it on
either side of the divide.

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

Done.

 
 // Return the currently highlighted row.  Returns -1 if no row is highlighted.
 - (NSInteger)highlightedRow;
 
+// Allocate or free cells so that count are available.
+// Also sets up default settings such as selecting the first cell.
+- (void)setCellCount:(NSUInteger)count;
+
+// Fetch a cell with limit checks on the array.
+- (OmniboxPopupCell*)cellAtRow:(NSUInteger)row;

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

I can see why you did this for convenience sake, but generally Cocoa code
expects the caller to check limits.  The omnibox_popup_view_mac.mm code which
calls this really should fail hard if it passes an invalid row, so maybe just
fix the -highlightRowAt: code to make the check.

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

Done.

+
 @end
 
 #endif  // CHROME_BROWSER_UI_COCOA_OMNIBOX_OMNIBOX_POPUP_MATRIX_H_