[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
index df7b00d3bc6b1c315e1189e80962b1cd23eb322c..ce3701117f6847949c294c20d1f6f5e1c45bbf3c 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
@@ -155,18 +155,16 @@ NSAttributedString* CreateClassifiedAttributedString(
 
 }  // namespace
 
+@implementation AutocompleteMatchWrapper
+@end
+
 @implementation OmniboxPopupCell
 
+@synthesize attributedTitle;
+
 - (id)init {
   self = [super init];
   if (self) {
-    [self setImagePosition:NSImageLeft];
-    [self setBordered:NO];
-    [self setButtonType:NSRadioButton];
-
-    // Without this highlighting messes up white areas of images.
-    [self setHighlightsBy:NSNoCellMask];
-
     const base::string16& raw_separator =
         l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
     separator_.reset(
@@ -205,6 +203,23 @@ NSAttributedString* CreateClassifiedAttributedString(
   }
 }
 
+- (id)copyWithZone:(NSZone*)zone {
+  NSAttributedString* separator = separator_.release();
+  NSAttributedString* description = description_.release();
+  NSAttributedString* prefix = prefix_.release();

[Scott Hess - ex-Googler, 2015/05/07 22:35:42]

This is an unexpected approach.  Why do you need the above three lines and the
later lines which call reset() on self's instance variables?

The copy should have nil in each of these slots.  If it's super C++-aware, it
should have an additional reference in each of these slots, which gets replaced
with the copy below.  I don't expect it's super C++-aware, though.

[groby-ooo-7-16, 2015/05/08 03:24:08]

This is here due to NSCopyObject. NSCell uses that in its copyWithZone (or so
lore has it, I haven't disassembled the SDK recently), and so [super
copyWithZone:] makes a bitwise copy of this object. Which means not only that
the slots aren't nil, but that the retain counts are messed up.

And so, we need to 
* Transfer scoped objects to  temp vars (so we don't lose them) and nil out
these slots. That's what release() is for
* Create an actual copy of those objects and retain it on the copied cell
* Store the old objects in the slots in the current object.

Mike Ash has a nice article on this[1]. He also has a slightly more elegant way,
but that assumes that scoped_nsobject is an NSObject*. Which it is, but we
probably don't want to rely on this or leak the implementation detail here. 

[1]
https://www.mikeash.com/pyblog/friday-qa-2010-08-27-defensive-programming-in-...

[Scott Hess - ex-Googler, 2015/05/08 04:11:48]

W.  T.  F.

In that case, might I suggest:
  scoped_nsobject<NSAttributedString> separator(separator_.release());
  //...
  copy->separator_.reset([separator copy]);
  //...
  separator_.swap(separator);
As the base reference gives me the willies.  Also, maybe a comment on the order
of "This is crazy because NSCell's -copyFromZone: is crazy", so that some later
Good Samaritan doesn't clean it up for you.

[groby-ooo-7-16, 2015/05/08 05:08:29]

I hate to be the contrarian, but... 

Assume a separator_ with refcount 1. The release nils separator_,  *without*
affecting refcount. (release doesn't release, because... scoped_ptr::release
doesn't deref either. No, I don't think that's a good enough reason, but hey.)

But scoped_nsobject's constructor _does_ retain, and so we now have a refcount
of 2. 

So at that point, swap() won't do the trick since separator_ has an increased
refcount. The last line would need to be separator_.reset(separator.get()); 
reset() doesn't retain, so both separator_ and separator now hold the same
object, with refcount 2. When separator goes out of scope, the object still has
a refcount of 1.

Oh yes, it's _that_ awesome. (Or I should be home instead of thinking about
refcounts. One of the two)


Alternative:
scoped_nsobject<NSAttributedString> separator;
separator.reset(separator_.release());
copy->separator_.reset([separator copy]);
separator_.swap(separator);

Alternative 2:
scoped_nsobject<NSAttributedString> separator(separator_);
separator_.reset();
copy->separator_.reset([separator copy]);
separator_ = separator();

The last one is the cleanest one to me: Take a local reference, release the ivar
reference and nil it, make a copy of the local object, ivar references local
object, scoped_nsobject dtor releases local reference

Any of these versions probably should come with a "call me crazy, maybe" comment
so the next person won't go through this entire dance again.

To cheer all of you up: 10.10 is moving along with deprecating NSCell. Another
10 years or so, and we'll never have to think about this :)

[Scott Hess - ex-Googler, 2015/05/08 06:00:13]

Alternative 3:
  scoped_nsobject<NSAttributedString> separator;
  separator.swap(separator_);
  copy->separator_.reset([separator copy]);
  separator_.swap(separator);
I can never quite wrap my head around whether it would make sense to have
Pass(), so you could wrote:
  scoped_nsobject<NSAttributedString> separator(separator_.Pass());
  copy->separator_.reset([separator copy]);
  separator_.swap(separator);
Probably too dangerous to exist.
Eh, I remember figuring out NSControl/NSCell and thinking "Yeah, that makes
sense given implementation issues" and moving on.  But then again I thought
autorelease also made perfect sense when you thought about the implementation. 
I have recently come to suspect that many people manage to become professional
programmers without spending time thinking out how they'd implement the
libraries they use.  Fortunately in the future Apple will be free to use
memcpy()-style replication of objects in view-based widget hierarchies, so it's
all good.

[I kid, they'll be using Swift, so memcpy() won't be available.]

[groby-ooo-7-16, 2015/05/08 06:20:43]

I like the symmetry of #3. I'm reasonably certain it deals with the refcount
properly, but it's late. I'd suggest debug printing them to make sure.

[Scott Hess - ex-Googler, 2015/05/08 18:25:49]

Another option which just occurred to me would be to give up entirely and strip
your NSCell down to POD and weak pointers.  For instance, a pointer (not a
reference) to the data source plus the cell's index.  This is an atrocity, but
my assumption is that if NSCopyObject() isn't doing the right thing WRT
Objective-C ref-counting, then it is also unlikely to be doing the right thing
WRT C++ copying.  So if someone later changes AutocompleteMatch to internally
use something ref-counted, that's also potentially messed up.

This is going to bother me for weeks.

+  OmniboxPopupCell* copy = [super copyWithZone:zone];
+  copy->match_ = match_;
+  copy->separator_.reset([separator copy]);

[groby-ooo-7-16, 2015/05/08 03:24:08]

You technically don't need to copy the NSAttributedString - none of them are
mutable, and so you're free to simply retain the original one.

[Scott Hess - ex-Googler, 2015/05/08 04:11:48]

If they aren't mutable, then -copy should be implemented as -retain.

+  copy->description_.reset([description copy]);
+  copy->prefix_.reset([prefix copy]);
+  copy->maxMatchContentsWidth_ = maxMatchContentsWidth_;
+  copy->contentsOffset_ = contentsOffset_;
+  separator_.reset(separator);
+  description_.reset(description);
+  prefix_.reset(prefix);
+  return copy;
+}
+
 - (void)setMaxMatchContentsWidth:(CGFloat)maxMatchContentsWidth {
   maxMatchContentsWidth_ = maxMatchContentsWidth;
 }
@@ -353,10 +368,11 @@ NSAttributedString* CreateClassifiedAttributedString(
   NSRect renderRect = ShiftRect(cellFrame, offset);
   renderRect.size.width =
       std::min(NSWidth(renderRect), static_cast<CGFloat>(maxWidth));
+  NSRect textRect = [as boundingRectWithSize:renderRect.size options:nil];
+  renderRect.origin.y += std::floor((NSHeight(cellFrame) - NSHeight(textRect))
+      / 2.0);

[Scott Hess - ex-Googler, 2015/05/07 22:35:42]

As long as you're on two lines, just pull everything after the += on to this
line to make the parens clearer.

[dschuyler, 2015/05/13 01:41:11]

Done.

   if (renderRect.size.width != 0) {
-    [self drawTitle:as
-          withFrame:FlipIfRTL(renderRect, cellFrame)
-             inView:controlView];
+    [as drawInRect:FlipIfRTL(renderRect, cellFrame)];

[Scott Hess - ex-Googler, 2015/05/07 22:35:42]

I think the style in here is to drop the {} when it goes to single-line body.

[I don't like that style myself, but still have to follow the local style.]

[dschuyler, 2015/05/13 01:41:11]

Done.

   }
   return NSWidth(renderRect);
 }