[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h
index 486df2c83f31c1f571e2396ef7c972ab2c5a1880..9722efd258179a940d64199662864b6b639e18bf 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h
@@ -11,19 +11,12 @@
 #include "components/omnibox/autocomplete_match.h"
 
 class OmniboxPopupViewMac;
+@class OmniboxPopupCell;
 
-// OmniboxPopupCell overrides how backgrounds are displayed to
-// handle hover versus selected.  So long as we're in there, it also
-// provides some default initialization.
-@interface OmniboxPopupCell : NSButtonCell {
+@interface OmniboxPopupCellData : NSObject {
  @private
-  // The popup view parent of this cell.
-  OmniboxPopupViewMac* parent_;
-
-  // The match which will be rendered for this row in omnibox dropdown.
-  AutocompleteMatch match_;
-
   // NSAttributedString instances for various match components.
+  base::scoped_nsobject<NSAttributedString> contents_;
   base::scoped_nsobject<NSAttributedString> separator_;
   base::scoped_nsobject<NSAttributedString> description_;
 
@@ -33,14 +26,32 @@ class OmniboxPopupViewMac;
   // which accesses the |NSAttributedString| instance.
   base::scoped_nsobject<NSAttributedString> prefix_;
 
+  base::scoped_nsobject<NSImage> image_;
+
   // The width of widest match contents in a set of infinite suggestions.
   CGFloat maxMatchContentsWidth_;
 
   // The offset at which the infinite suggestion contents should be displayed.
   CGFloat contentsOffset_;
+
+  bool isContentsRTL_;

[shrike, 2015/05/14 18:02:38]

Should this be C++ bool or ObjC BOOL? For ObjC classes I've made it BOOL.

[dschuyler, 2015/05/14 22:28:19]

Done.

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

I think that in general Objective-C should use BOOL/YES/NO, C++ bool/true/false,
except at the intersection where you make things up.  I think that you can also
just let things leak across without having to write things like "cpp_bool =
objc_BOOL ? true : false;".  Basically aim for consistency, I guess, like if
some code would look the same for the OSX code as the Views code, then better to
be the same rather than egregiously convert things.

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

The "accepted way" to go from BOOL to bool is the '!!' trick. I.e
  BOOL cocoaVar = YES;
  bool cppVar = !!cocoaVar;

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

I tried this: changed it to BOOL as well as changing the isRTL elsewhere that it
gets compared to.  Then used !! to convert to BOOL.

Then I reasoned: this is only used in a cpp domain (with values to/from cpp) and
the code is simpler to keep in cpp.  So, I changed it back to bool.

I'm trying to read into the spirit of the suggestions being made, to keep the
code easy to read and follow.  For this particular case I *think* that bool may
be the right way to go.

+  AutocompleteMatch::Type matchType_;
+  NSRect rect_;
 }
 
-- (void)setMatch:(const AutocompleteMatch&)match;
+- (id)initWithMatch:(const AutocompleteMatch&)match;
+
+- (void)drawMatchWithFrame:(NSRect)cellFrame
+                    inCell:(OmniboxPopupCell*)cell
+                    inView:(NSView*)controlView;
+
+- (void)setContents:(NSAttributedString*)contents;
+
+- (void)setImage:(NSImage*)image;
+
+- (void)setRect:(NSRect)rect;

[shrike, 2015/05/14 18:02:39]

Should be able to get rid of setRect: and rect declarations and definitions in
the .mm, and replace with a property:

@property (assign, nonatomic) NSRect rect;

and in the .mm

@synthesize rect = rect_;

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

Thanks, I had wondered which way would be preferred.

Done.

+
+- (NSRect)rect;
 
 - (void)setMaxMatchContentsWidth:(CGFloat)maxMatchContentsWidth;
 
@@ -49,6 +60,24 @@ class OmniboxPopupViewMac;
 // Returns the width of the match contents.
 - (CGFloat)getMatchContentsWidth;
 
+@end
+
+// OmniboxPopupCell overrides how backgrounds are displayed to
+// handle hover versus selected.  So long as we're in there, it also
+// provides some default initialization.
+@interface OmniboxPopupCell : NSCell {
+ @private
+  base::scoped_nsobject<OmniboxPopupCellData> cellData_;
+}
+
+- (void)setCellData:(OmniboxPopupCellData*)cellData;
+
+- (CGFloat)drawMatchPart:(NSAttributedString*)attributedString
+               withFrame:(NSRect)cellFrame
+                atOffset:(CGFloat)offset
+            withMaxWidth:(int)maxWidth
+                  inView:(NSView*)controlView;
+
 // Returns the offset of the start of the contents in the input text for the
 // given match. It is costly to compute this offset, so it is computed once and
 // shared by all OmniboxPopupCell instances through OmniboxPopupViewMac parent.