[AiS] changing mac omnibox suggestions form NSMatrix to NSTableView

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // 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"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/i18n/rtl.h"
@@ skipping 137 matching lines @@
                  value:DimTextColor()
                  range:range];
     }
   }
 
   return as;
 }
 
 }  // namespace
 
-@implementation OmniboxPopupCell
+@implementation OmniboxPopupCellData
 
-- (id)init {
+@synthesize rect = rect_;
+
+- (id)initWithMatch:(const AutocompleteMatch&)match {
   self = [super init];
-  if (self) {
-    [self setImagePosition:NSImageLeft];
-    [self setBordered:NO];
-    [self setButtonType:NSRadioButton];
+  if (!self)
+    return self;

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

Ack!  Chromium style conflicts with my years of Objective-C experience!

Hmm, I think go with it, this is too long otherwise, unless you add a helper.

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

This is really rubbing all ObjC senses the wrong way - I'd second the idea of a
helper. (Yes, this is purely stylistic - but every single cocoa/ reviewer will
double take on this :)

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

So I'm kinda guessing what may be so offensive.  At first I wondered if you'd
prefer a "return nil;" instead of "return self;".

I can still move the body to a separate function.  I wasn't sure what to name
that function, so I indented instead.  Maybe initHelperWithMatch?  Or is it fine
indented?

Done.

[Scott Hess - ex-Googler, 2015/05/15 22:02:07]

Ha, no, there's this whole genre of lore that has grown up over the years around
chaining to your superclass' designated initializer.  The gist of it WRT
Chromium Objective-C is that things should look like:

- (instancetype)initWithMyStuff:(MyType)my_stuff {
  if ((self = [super init])) {
    // Additional work.
  }
  return self;
}

The previous idiom of separate lines for the [super init] is also fine.  But the
Chromium style of "Exit early rather than nesting deeply" is directly in
conflict with this.

I think Rachel's probably correct that this is a case where doing it the
old-school way will just keep more Objective-C coder's from having
head-exploding issues.

What is super amusing about all of this is that the original reasons for
+alloc/-init have pretty much gone by the wayside, and the set of classes for
which the super call changes self are vanishingly small (and usually doing it
for borderline reasons).  These days I think most people agree that you should
make your special case obvious, like +createMyOptionallySharedInstance rather
than try to bury it in -init magic.

Also ... nononono to naming the helper -initHelperAnything.  -init* is always an
initializer.

[groby-ooo-7-16, 2015/05/15 22:41:33]

Yup, what Scott said. What you do in init is a religious debate. The rebellious
adherents of the school of NextStep have always written it as if (self = [super
init]) and shall forever continue to do so, until the day the earth ends, while
the upstarts of the empire of OSX and iOS find little use in it. 

And in the case of Chromium, we decided to align with the Rebel Alliance instead
of the Empire. 

So if you write it differently, people scanning over the code will simply have
their pattern recognizer throw an exception, alerting them to the possible
presence of an imperial Star Destroyer. Or something. For a good in-depth
reading on this:
http://www.cocoawithlove.com/2009/04/what-does-it-mean-when-you-assign-super....

[dschuyler, 2015/05/18 23:38:44]

Acknowledged.

[dschuyler, 2015/05/18 23:38:44]

Acknowledged.

 
-    // 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(
+      [CreateAttributedString(raw_separator, DimTextColor()) retain]);

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

Note that scoped_nsobject<> implements operator= to include the retain.  So this
could be:
  separator_ = [...];
I am not suggesting that you make that change, just pointing it out.  I'm kind
of feeling that a retain() function would make sense, really, to make it clear
that code is saying "Take a reference to this object and keep it."

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

Acknowledged.

 
-    const base::string16& raw_separator =
-        l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
-    separator_.reset(
-        [CreateAttributedString(raw_separator, DimTextColor()) retain]);
+  isContentsRTL_ =
+      (base::i18n::RIGHT_TO_LEFT ==
+       base::i18n::GetFirstStrongCharacterDirection(match.contents));
+  matchType_ = match.type;
+
+  // Prefix may not have any characters with strong directionality, and may take
+  // the UI directionality. But prefix needs to appear in continuation of the
+  // contents so we force the directionality.
+  NSTextAlignment textAlignment =
+      isContentsRTL_ ? NSRightTextAlignment : NSLeftTextAlignment;
+  prefix_.reset(
+      [CreateAttributedString(base::UTF8ToUTF16(match.GetAdditionalInfo(
+                                  kACMatchPropertyContentsPrefix)),
+                              ContentTextColor(), textAlignment) retain]);
+
+  contents_.reset([CreateClassifiedAttributedString(
+      match.contents, ContentTextColor(), match.contents_class) retain]);
+
+  if (match.answer) {
+    base::string16 answerString;
+    DCHECK(!match.answer->second_line().text_fields().empty());
+    for (const SuggestionAnswer::TextField& textField :
+         match.answer->second_line().text_fields())
+      answerString += textField.text();
+    const base::char16 space(' ');
+    const SuggestionAnswer::TextField* textField =
+        match.answer->second_line().additional_text();
+    if (textField)
+      answerString += space + textField->text();

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

I find my brain really questioning this.  I'd guess it implies that text() is
returning an std::string or a base::string16?  If it was:
   answerString.append(space).append(text());
I wouldn't have to think that through, and it would work even if someone changes
text() return, and it also wouldn't create the intermediate object.

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

Done.

+    textField = match.answer->second_line().status_text();
+    if (textField)
+      answerString += space + textField->text();
+    description_.reset([CreateClassifiedAttributedString(
+        answerString, DimTextColor(), match.description_class) retain]);
+  } else if (match.description.empty()) {
+    description_.reset();
+  } else {
+    description_.reset([CreateClassifiedAttributedString(
+        match.description, DimTextColor(), match.description_class) retain]);
   }
   return self;
 }
 
-- (void)setMatch:(const AutocompleteMatch&)match {
-  match_ = match;
-  NSAttributedString *contents = CreateClassifiedAttributedString(
-      match_.contents, ContentTextColor(), match_.contents_class);
-  [self setAttributedTitle:contents];
+- (void)setContents:(NSAttributedString*)contents {
+  contents_.reset([contents retain]);
+}
 
-  if (match_.answer) {
-    base::string16 answerString;
-    DCHECK(!match_.answer->second_line().text_fields().empty());
-    for (const SuggestionAnswer::TextField& textField :
-         match_.answer->second_line().text_fields())
-      answerString += textField.text();
-    const base::char16 space(' ');
-    const SuggestionAnswer::TextField* textField =
-        match_.answer->second_line().additional_text();
-    if (textField)
-      answerString += space + textField->text();
-    textField = match_.answer->second_line().status_text();
-    if (textField)
-      answerString += space + textField->text();
-    description_.reset([CreateClassifiedAttributedString(
-        answerString, DimTextColor(), match_.description_class) retain]);
-  } else if (match_.description.empty()) {
-    description_.reset();
-  } else {
-    description_.reset([CreateClassifiedAttributedString(
-        match_.description, DimTextColor(), match_.description_class) retain]);
-  }
+- (void)setImage:(NSImage*)image {
+  image_.reset([image retain]);
 }
 
 - (void)setMaxMatchContentsWidth:(CGFloat)maxMatchContentsWidth {
   maxMatchContentsWidth_ = maxMatchContentsWidth;
 }
 
 - (void)setContentsOffset:(CGFloat)contentsOffset {
   contentsOffset_ = contentsOffset;
 }
 
-// The default NSButtonCell drawing leaves the image flush left and
-// the title next to the image.  This spaces things out to line up
-// with the star button and autocomplete field.
-- (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView *)controlView {
-  if ([self state] == NSOnState || [self isHighlighted]) {
-    if ([self state] == NSOnState)
-      [SelectedBackgroundColor() set];
-    else
-      [HoveredBackgroundColor() set];
-    NSBezierPath* path =
-        [NSBezierPath bezierPathWithRoundedRect:cellFrame
-                                        xRadius:kCellRoundingRadius
-                                        yRadius:kCellRoundingRadius];
-    [path fill];
-  }
+- (CGFloat)getMatchContentsWidth {
+  return contents_ ? [contents_ size].width : 0;
+}
+
+- (id)copyWithZone:(NSZone*)zone {
+  // Note: Before changing this code, please check with groby, shess, or
+  // dschuyler about why it's written with swaps and local data.

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

Consult with Rachel, first, but ... in this case, we control the horizontal and
the vertical, so we don't have to live with NSCell's mistakes.  Since you're
creating a new object below, not calling [super copyWithZone:zone], that new
object has no references to self's instance variables the screw up, so you can
either call accessors to set things, or copy->instance_var_ directly.

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

Rachel is fully supportive of that argument :) 

That is, if we need copyWithZone at all. I don't think we do. As far as I can
tell, CellData is an immutable object _as far as the cell is concerned_.

So kill all this, and in the cell's -copyWithZone just do a copy->cellData_ =
[cellData_ retain];


(If I miss something and you _do_ need to copy this, you should mark the
CellData class as conforming to NSCopying)

[Scott Hess - ex-Googler, 2015/05/15 17:28:59]

operator= already implies retain, so either copy->cellData_ = cellData_, or
copy->cellData_.reset([celldata_ retain]).

The cell's -copyWithZone: actually already does the operator= version, so AFAICT
this method is never called.

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

Done.

+  base::scoped_nsobject<NSAttributedString> contents;
+  base::scoped_nsobject<NSAttributedString> separator;
+  base::scoped_nsobject<NSAttributedString> description;
+  base::scoped_nsobject<NSAttributedString> prefix;
+  base::scoped_nsobject<NSImage> image;
+  contents_.swap(contents);
+  separator_.swap(separator);
+  description_.swap(description);
+  prefix_.swap(prefix);
+  image_.swap(image);
+
+  OmniboxPopupCellData* copy = [[OmniboxPopupCellData alloc] init];
+  copy->contents_.reset(contents);
+  copy->separator_.reset(separator);
+  copy->description_.reset(description);
+  copy->prefix_.reset(prefix);
+  copy->image_.reset(image);
+  copy->maxMatchContentsWidth_ = maxMatchContentsWidth_;
+  copy->contentsOffset_ = contentsOffset_;
+  copy->isContentsRTL_ = isContentsRTL_;
+  copy->matchType_ = matchType_;
+  copy->rect_ = rect_;
+
+  contents_.swap(contents);
+  separator_.swap(separator);
+  description_.swap(description);
+  prefix_.swap(prefix);
+  image_.swap(image);
+  return copy;
+}
+
+- (void)drawMatchWithFrame:(NSRect)cellFrame
+                    inCell:(OmniboxPopupCell*)cell
+                    inView:(NSView*)controlView {
+  CGFloat remainingWidth = GetContentAreaWidth(cellFrame);
+  CGFloat contentsWidth = [self getMatchContentsWidth];
+  CGFloat separatorWidth = [separator_ size].width;
+  CGFloat descriptionWidth = description_ ? [description_ size].width : 0;
+  int contentsMaxWidth, descriptionMaxWidth;
+  OmniboxPopupModel::ComputeMatchMaxWidths(
+      ceilf(contentsWidth), ceilf(separatorWidth), ceilf(descriptionWidth),
+      ceilf(remainingWidth), !AutocompleteMatch::IsSearchType(matchType_),
+      &contentsMaxWidth, &descriptionMaxWidth);
 
   // Put the image centered vertically but in a fixed column.
-  NSImage* image = [self image];
-  if (image) {
+  if (image_) {
     NSRect imageRect = cellFrame;
-    imageRect.size = [image size];
+    imageRect.size = [image_ size];
     imageRect.origin.y +=
         std::floor((NSHeight(cellFrame) - NSHeight(imageRect)) / 2.0);
     imageRect.origin.x += kImageXOffset;
-    [image drawInRect:FlipIfRTL(imageRect, cellFrame)
-             fromRect:NSZeroRect  // Entire image
-            operation:NSCompositeSourceOver
-             fraction:1.0
-       respectFlipped:YES
-                hints:nil];
+    [image_ drawInRect:FlipIfRTL(imageRect, cellFrame)
+              fromRect:NSZeroRect
+             operation:NSCompositeSourceOver
+              fraction:1.0
+        respectFlipped:YES
+                 hints:nil];
   }
 
-  [self drawMatchWithFrame:cellFrame inView:controlView];
-}
-
-- (void)drawMatchWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
-  NSAttributedString* contents = [self attributedTitle];
-
-  CGFloat remainingWidth = GetContentAreaWidth(cellFrame);
-  CGFloat contentsWidth = [self getMatchContentsWidth];
-  CGFloat separatorWidth = [separator_ size].width;
-  CGFloat descriptionWidth = description_.get() ? [description_ size].width : 0;
-  int contentsMaxWidth, descriptionMaxWidth;
-  OmniboxPopupModel::ComputeMatchMaxWidths(
-      ceilf(contentsWidth),
-      ceilf(separatorWidth),
-      ceilf(descriptionWidth),
-      ceilf(remainingWidth),
-      !AutocompleteMatch::IsSearchType(match_.type),
-      &contentsMaxWidth,
-      &descriptionMaxWidth);
-
   CGFloat offset = kTextStartOffset;
-  if (match_.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
+  if (matchType_ == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
     // Infinite suggestions are rendered with a prefix (usually ellipsis), which
     // appear vertically stacked.
     offset += [self drawMatchPrefixWithFrame:cellFrame
+                                      inCell:cell
                                       inView:controlView
                         withContentsMaxWidth:&contentsMaxWidth];
   }
-  offset += [self drawMatchPart:contents
+  offset += [cell drawMatchPart:contents_
                       withFrame:cellFrame
                        atOffset:offset
                    withMaxWidth:contentsMaxWidth
                          inView:controlView];
 
   if (descriptionMaxWidth != 0) {
-    offset += [self drawMatchPart:separator_
+    offset += [cell drawMatchPart:separator_
                         withFrame:cellFrame
                          atOffset:offset
                      withMaxWidth:separatorWidth
                            inView:controlView];
-    offset += [self drawMatchPart:description_
+    offset += [cell drawMatchPart:description_
                         withFrame:cellFrame
                          atOffset:offset
                      withMaxWidth:descriptionMaxWidth
                            inView:controlView];
   }
 }
 
 - (CGFloat)drawMatchPrefixWithFrame:(NSRect)cellFrame
+                             inCell:(OmniboxPopupCell*)cell
                              inView:(NSView*)controlView
                withContentsMaxWidth:(int*)contentsMaxWidth {
   CGFloat offset = 0.0f;
   CGFloat remainingWidth = GetContentAreaWidth(cellFrame);
   bool isRTL = base::i18n::IsRTL();
-  bool isContentsRTL = (base::i18n::RIGHT_TO_LEFT ==
-      base::i18n::GetFirstStrongCharacterDirection(match_.contents));
-  // Prefix may not have any characters with strong directionality, and may take
-  // the UI directionality. But prefix needs to appear in continuation of the
-  // contents so we force the directionality.
-  NSTextAlignment textAlignment = isContentsRTL ?
-      NSRightTextAlignment : NSLeftTextAlignment;
-  prefix_.reset([CreateAttributedString(base::UTF8ToUTF16(
-      match_.GetAdditionalInfo(kACMatchPropertyContentsPrefix)),
-      ContentTextColor(), textAlignment) retain]);
   CGFloat prefixWidth = [prefix_ size].width;
 
   CGFloat prefixOffset = 0.0f;
-  if (isRTL != isContentsRTL) {
+  if (isRTL != isContentsRTL_) {
     // The contents is rendered between the contents offset extending towards
     // the start edge, while prefix is rendered in opposite direction. Ideally
     // the prefix should be rendered at |contentsOffset_|. If that is not
     // sufficient to render the widest suggestion, we increase it to
     // |maxMatchContentsWidth_|.  If |remainingWidth| is not sufficient to
     // accommodate that, we reduce the offset so that the prefix gets rendered.
     prefixOffset = std::min(
         remainingWidth - prefixWidth, std::max(contentsOffset_,
                                                maxMatchContentsWidth_));
     offset = std::max<CGFloat>(0.0, prefixOffset - *contentsMaxWidth);
   } else { // The direction of contents is same as UI direction.
     // Ideally the offset should be |contentsOffset_|. If the max total width
     // (|prefixWidth| + |maxMatchContentsWidth_|) from offset will exceed the
     // |remainingWidth|, then we shift the offset to the left , so that all
     // postfix suggestions are visible.
     // We have to render the prefix, so offset has to be at least |prefixWidth|.
-    offset = std::max(prefixWidth,
+    offset = std::max(
+        prefixWidth,
         std::min(remainingWidth - maxMatchContentsWidth_, contentsOffset_));
     prefixOffset = offset - prefixWidth;
   }
   *contentsMaxWidth = std::min((int)ceilf(remainingWidth - prefixWidth),

[Scott Hess - ex-Googler, 2015/05/15 17:28:59]

Oooh, maybe give this code a once-over for inappropriate casting.  This looks to
me like *contentsMaxWidth should really be a CGFloat, since -drawMatchPart:* is
just casting it back.  Apologies that this got to here, I seem to remember the
original review of this code was pretty involved.

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

May we do a separate CL for this?

[Scott Hess - ex-Googler, 2015/05/15 22:02:07]

If you prefer.

[dschuyler, 2015/05/18 23:38:44]

I made bug 489478 to track this.

                                *contentsMaxWidth);
-  [self drawMatchPart:prefix_
+  [cell drawMatchPart:prefix_
             withFrame:cellFrame
              atOffset:prefixOffset + kTextStartOffset
          withMaxWidth:prefixWidth
                inView:controlView];
   return offset;
 }
 
-- (CGFloat)drawMatchPart:(NSAttributedString*)as
+@end
+
+@implementation OmniboxPopupCell
+
+- (void)setCellData:(OmniboxPopupCellData*)cellData {
+  cellData_.reset([cellData retain]);
+}
+
+- (id)copyWithZone:(NSZone*)zone {
+  // Note: Before changing this code, please check with groby, shess, or
+  // dschuyler about why it's written with swaps and local data.

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

Now _this_ code has to keep the crazy.

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

Acknowledged.

+  base::scoped_nsobject<OmniboxPopupCellData> cellData(nil);

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

Though I think the (nil) can be disposed of.  It's default to nil.

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

Done.

+  cellData_.swap(cellData);

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

And I would say cellData.swap(cellData_), to make it clear where the reference
ends up.  It's exactly the same code in the end, but we (at least I) are tuned
to expect things to end up in the LHS.

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

I wondered about that.  One one hand there's seeing that the same operation is
done again (argument for keeping them the same).  On the other hand there's the
LHS consistency.

Done.

+  OmniboxPopupCell* copy = [super copyWithZone:zone];
+  copy->cellData_ = cellData;
+  cellData_.swap(cellData);
+  return copy;
+}
+
+// The default NSButtonCell drawing leaves the image flush left and
+// the title next to the image.  This spaces things out to line up
+// with the star button and autocomplete field.
+- (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
+  if ([self state] == NSOnState || [self isHighlighted]) {
+    if ([self state] == NSOnState)
+      [SelectedBackgroundColor() set];
+    else
+      [HoveredBackgroundColor() set];
+    NSBezierPath* path =
+        [NSBezierPath bezierPathWithRoundedRect:cellFrame
+                                        xRadius:kCellRoundingRadius
+                                        yRadius:kCellRoundingRadius];
+    [path fill];
+  }
+
+  [cellData_ drawMatchWithFrame:cellFrame inCell:self inView:controlView];
+}
+
+- (CGFloat)drawMatchPart:(NSAttributedString*)attributedString
                withFrame:(NSRect)cellFrame
                 atOffset:(CGFloat)offset
             withMaxWidth:(int)maxWidth
                   inView:(NSView*)controlView {
   if (offset > NSWidth(cellFrame))
     return 0.0f;
   NSRect renderRect = ShiftRect(cellFrame, offset);
   renderRect.size.width =
       std::min(NSWidth(renderRect), static_cast<CGFloat>(maxWidth));
-  if (renderRect.size.width != 0) {
-    [self drawTitle:as
-          withFrame:FlipIfRTL(renderRect, cellFrame)
-             inView:controlView];
-  }
+  NSRect textRect =
+      [attributedString boundingRectWithSize:renderRect.size options:nil];
+  renderRect.origin.y +=
+      std::floor((NSHeight(cellFrame) - NSHeight(textRect)) / 2.0);
+  if (renderRect.size.width != 0)
+    [attributedString drawInRect:FlipIfRTL(renderRect, cellFrame)];
   return NSWidth(renderRect);
 }
 
-- (CGFloat)getMatchContentsWidth {
-  NSAttributedString* contents = [self attributedTitle];
-  return contents ? [contents size].width : 0;
-}
-
-
 + (CGFloat)computeContentsOffset:(const AutocompleteMatch&)match {
   const base::string16& inputText = base::UTF8ToUTF16(
       match.GetAdditionalInfo(kACMatchPropertyInputText));
   int contentsStartIndex = 0;
   base::StringToInt(
       match.GetAdditionalInfo(kACMatchPropertyContentsStartIndex),
       &contentsStartIndex);
   // Ignore invalid state.
   if (!StartsWith(match.fill_into_edit, inputText, true)
       || !EndsWith(match.fill_into_edit, match.contents, true)
       || ((size_t)contentsStartIndex >= inputText.length())) {
     return 0;
   }
   bool isRTL = base::i18n::IsRTL();
   bool isContentsRTL = (base::i18n::RIGHT_TO_LEFT ==
       base::i18n::GetFirstStrongCharacterDirection(match.contents));
 
   // Color does not matter.
-  NSAttributedString* as = CreateAttributedString(inputText, DimTextColor());
-  base::scoped_nsobject<NSTextStorage> textStorage([[NSTextStorage alloc]
-      initWithAttributedString:as]);
+  NSAttributedString* attributedString =
+      CreateAttributedString(inputText, DimTextColor());
+  base::scoped_nsobject<NSTextStorage> textStorage(
+      [[NSTextStorage alloc] initWithAttributedString:attributedString]);
   base::scoped_nsobject<NSLayoutManager> layoutManager(
       [[NSLayoutManager alloc] init]);
   base::scoped_nsobject<NSTextContainer> textContainer(
       [[NSTextContainer alloc] init]);
   [layoutManager addTextContainer:textContainer];
   [textStorage addLayoutManager:layoutManager];
 
   NSUInteger charIndex = static_cast<NSUInteger>(contentsStartIndex);
   NSUInteger glyphIndex =
       [layoutManager glyphIndexForCharacterAtIndex:charIndex];
 
   // This offset is computed from the left edge of the glyph always from the
   // left edge of the string, irrespective of the directionality of UI or text.
   CGFloat glyphOffset = [layoutManager locationForGlyphAtIndex:glyphIndex].x;
 
-  CGFloat inputWidth = [as size].width;
+  CGFloat inputWidth = [attributedString size].width;
 
   // The offset obtained above may need to be corrected because the left-most
   // glyph may not have 0 offset. So we find the offset of left-most glyph, and
   // subtract it from the offset of the glyph we obtained above.
   CGFloat minOffset = glyphOffset;
 
   // If content is RTL, we are interested in the right-edge of the glyph.
   // Unfortunately the bounding rect computation methods from NSLayoutManager or
   // NSFont don't work correctly with bidirectional text. So we compute the
   // glyph width by finding the closest glyph offset to the right of the glyph
   // we are looking for.
   CGFloat glyphWidth = inputWidth;
 
-  for (NSUInteger i = 0; i < [as length]; i++) {
+  for (NSUInteger i = 0; i < [attributedString length]; i++) {
     if (i == charIndex) continue;
     glyphIndex = [layoutManager glyphIndexForCharacterAtIndex:i];
     CGFloat offset = [layoutManager locationForGlyphAtIndex:glyphIndex].x;
     minOffset = std::min(minOffset, offset);
     if (offset > glyphOffset)
       glyphWidth = std::min(glyphWidth, offset - glyphOffset);
   }
   glyphOffset -= minOffset;
   if (glyphWidth == 0)
     glyphWidth = inputWidth - glyphOffset;
   if (isContentsRTL)
     glyphOffset += glyphWidth;
   return isRTL ? (inputWidth - glyphOffset) : glyphOffset;
 }
 
 @end