[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix_unittest.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix_unittest.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix_unittest.mm
index 191d5906b190ae76446e3fcff0d53488f5c16d6b..f7ccad4f79c64cf08c14c64b27529cf7377c982e 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix_unittest.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix_unittest.mm
@@ -2,15 +2,23 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#import "chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h"
 #import "chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.h"
 
 #import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
 #import "ui/events/test/cocoa_test_event_utils.h"
 
+@interface OmniboxPopupTableController ()
+// Setup the information used by the NSTableView data source.
+- (id)initWithArray:(NSArray*)array;
+@end

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

Same as my other comment, test-only API in the header, with the interface
marked.  And instancetype.

[dschuyler, 2015/05/26 18:40:21]

I made the change for instancetype.

+
 namespace {
 
-NSEvent* MouseEventInRow(NSMatrix* matrix, NSEventType type, NSInteger row) {
-  NSRect cell_rect = [matrix cellFrameAtRow:row column:0];
+NSEvent* MouseEventInRow(OmniboxPopupMatrix* matrix,
+                         NSEventType type,
+                         NSInteger row) {
+  NSRect cell_rect = [matrix rectOfRow:row];
   NSPoint point_in_view = NSMakePoint(NSMidX(cell_rect), NSMidY(cell_rect));
   NSPoint point_in_window = [matrix convertPoint:point_in_view toView:nil];
   return cocoa_test_event_utils::MouseEventAtPoint(
@@ -21,20 +29,36 @@ class OmniboxPopupMatrixTest : public CocoaTest,
                                public OmniboxPopupMatrixObserver {
  public:
   OmniboxPopupMatrixTest()
-      : selected_row_(0),
-        clicked_row_(0),
-        middle_clicked_row_(0) {
-  }
+      : selected_row_(0), clicked_row_(0), middle_clicked_row_(0) {}
 
   void SetUp() override {
     CocoaTest::SetUp();
     matrix_.reset([[OmniboxPopupMatrix alloc] initWithObserver:this]);
     [[test_window() contentView] addSubview:matrix_];
+
+    base::scoped_nsobject<NSTableColumn> column(
+        [[NSTableColumn alloc] initWithIdentifier:@"MainCell"]);
+    [column setDataCell:[[[OmniboxPopupCell alloc] init] autorelease]];
+    [matrix_ addTableColumn:column];

[groby-ooo-7-16, 2015/05/21 03:01:48]

Why add another table column? Didn't -initWithObserver already do that?

[dschuyler, 2015/05/26 18:40:21]

Done.

+
+    NSMutableArray* array = [NSMutableArray array];
+    for (size_t ii = 0; ii < 3; ++ii) {

[groby-ooo-7-16, 2015/05/21 03:01:48]

Please, please, please: No ii :)

[dschuyler, 2015/05/26 18:40:21]

Done.

+      base::scoped_nsobject<OmniboxPopupCellData> cellData(
+          [[OmniboxPopupCellData alloc] init]);

[groby-ooo-7-16, 2015/05/21 03:01:48]

chain methods :)

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

For tests, I'm more lenient about -autorelease, as the -setDataCell: call above.
 Little bit more towards the easy-to-read side of the balance versus the
basically-free-performance side.

[dschuyler, 2015/05/26 18:40:21]

I'd like to keep the reference counting correct.
Is it better to have the scoped object or the autorelease? 

[groby-ooo-7-16, 2015/05/26 21:23:22]

It's a minimal cost, so I'm not too worried. I lean slightly towards the chained
methods (because Cocoa), but if you prefer scoped_nsobject, there's enough code
that does the same. Dealer's choice :)

[dschuyler, 2015/05/28 20:34:22]

I also vote for the easy-to-read, but it sounds like my easy-to-read perspective
is different.  Being more of a CPP programmer than OBJC, it's easier to
standardize on using ~either~ autorelease or scoped_nsobject.  It's harder to
read (and write) ~sometimes~ use autorelease and sometimes use scoped_object.

Acknowledged.

[Scott Hess - ex-Googler, 2015/05/29 18:00:26]

I actually gave up on CPP vs OBJC long ago, and mostly just restrict myself to
"Be as consistent as possible for the scope".  It's the switching back and forth
between styles which generates bugs.

That said, IMHO test code in Objective-C is very likely to have autoreleased
objects, such as from +[NSArray arrayWithObjects:], so it's harder to take a
firm line.

[dschuyler, 2015/06/09 01:30:46]

Done.

+      [array addObject:cellData];
+    }
+    [matrix_ setController:[[OmniboxPopupTableController alloc]
+                               initWithArray:array]];

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

I believe the only refs to OPTController after this line are weak references. 
You need someone to own it.

[dschuyler, 2015/05/26 18:40:21]

Thanks.  I totally missed that.

+
+    NSRect table_frame = NSZeroRect;

[groby-ooo-7-16, 2015/05/21 03:01:48]

NSMakeRect(0, 0, testWidth, NSHeight([matrix_ frame]);

In general: Do we need a setFrame here?

[Scott Hess - ex-Googler, 2015/05/21 20:40:27]

I'd not bother defining testWidth unless you're using it multiple times, since I
think its meaning is obvious from context.  And if you are defining it, it
should be const CGFloat kTestWidth, probably.

[dschuyler, 2015/05/26 18:40:21]

Done.

[dschuyler, 2015/05/26 18:40:21]

Not needed any longer. 
Done.

+    CGFloat testWidth = 400;
+    table_frame.size.width = testWidth;
+    table_frame.size.height = NSHeight([matrix_ frame]);
+    [matrix_ setFrame:table_frame];
   };
 
   void OnMatrixRowSelected(OmniboxPopupMatrix* matrix, size_t row) override {
     selected_row_ = row;
-    [matrix_ selectCellAtRow:row column:0];
   }
 
   void OnMatrixRowClicked(OmniboxPopupMatrix* matrix, size_t row) override {
@@ -59,7 +83,6 @@ class OmniboxPopupMatrixTest : public CocoaTest,
 TEST_VIEW(OmniboxPopupMatrixTest, matrix_);
 
 TEST_F(OmniboxPopupMatrixTest, HighlightedRow) {
-  [matrix_ renewRows:3 columns:1];
   EXPECT_EQ(-1, [matrix_ highlightedRow]);
 
   [matrix_ mouseEntered:MouseEventInRow(matrix_, NSMouseMoved, 0)];
@@ -72,8 +95,6 @@ TEST_F(OmniboxPopupMatrixTest, HighlightedRow) {
 }
 
 TEST_F(OmniboxPopupMatrixTest, SelectedRow) {
-  [matrix_ renewRows:3 columns:1];
-
   [NSApp postEvent:MouseEventInRow(matrix_, NSLeftMouseUp, 2) atStart:YES];
   [matrix_ mouseDown:MouseEventInRow(matrix_, NSLeftMouseDown, 2)];