Infinite Suggest for mac

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
index 17678d2c9d15f74f20e18e8a2c04200f71bd9bfd..f03ca069051823acdce27429d10e5e44e2a6d501 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
@@ -47,6 +47,8 @@ const NSTimeInterval kShrinkAnimationDuration = 0.1;
 // contents.
 const CGFloat kMaxContentsFraction = 0.7;
 
+const char16 kEllipsis[] = { 0x2026, 0x20, 0x0 };

[Scott Hess - ex-Googler, 2013/12/12 19:11:15]

Don't define your own constant.  ElideString() uses gfx::ElideText(), which uses
gfx::kEllipsisUTF16.  Doesn't have the space, but that's easy enough to
construct.

[Anuj, 2013/12/12 23:56:22]

Done.

+
 // Background colors for different states of the popup elements.
 NSColor* BackgroundColor() {
   return [NSColor controlBackgroundColor];
@@ -123,11 +125,58 @@ void OmniboxPopupViewMac::UpdatePopupAppearance() {
   // Load the results into the popup's matrix.
   DCHECK_GT(rows, 0U);
   [matrix_ renewRows:rows columns:1];
+
+  CGFloat max_match_width = 0;
+  CGFloat max_required_width = 0;
+  CGFloat ignored_prefix_width = 0;
+  CGFloat ellipsis_prefix_width = 0;
   for (size_t ii = 0; ii < rows; ++ii) {
     OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
     const AutocompleteMatch& match = GetResult().match_at(ii + start_match);
     [cell setImage:ImageForMatch(match)];
-    [cell setAttributedTitle:MatchText(match, result_font, matrix_width)];
+    NSAttributedString* match_text =
+        MatchText(match, result_font, matrix_width);

[Scott Hess - ex-Googler, 2013/12/12 19:11:15]

This is going to construct a result which you are then going to throw away in
the SEARCH_SUGGEST_INFINITE case and construct all over again using very similar
code.  So it seems like it might make sense to abstract it out into a similar
helper, rather than having it inline, obscuring the overall matrix setup.

I'm still trying to detangle how one might handle ignored_prefix_width and
ellipsis_prefix_width, which seem constant for all SEARCH_SUGGEST_INFINITE,
versus the max_ values which aren't.

Your screen shots show the offsets as being the same for all values, so the
per-iteration calculations in the second loop seem suspect, to me.  I'm
wondering if it would be more appropriate to tell the cell itself whether it's
one of these cells, and then calculate a constant offset and set it for _all_ of
the cells, with only the cells set to that type actually adjusting by it.

[Anuj, 2013/12/12 19:40:55]

1. I can change the code as follows
if (ignored_prefix_width == 0)
  ignored_prefix_width = [ignored_prefix_as size].width;

Same for ellipsis_prefix_width.

2. max_match_width and max_required_width still need to be computed.

3. You are right - The additional_offset calculation can be moved out of the
second loop.

4. I should probably use scoped_ptr<NSMutableAttributedString> inside the loop -
please advice.

+
+    if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_INFINITE) {
+

[Scott Hess - ex-Googler, 2013/12/12 19:11:15]

No empty line.

[Anuj, 2013/12/12 23:56:22]

Done.

+      string16 ignored_prefix = match.fill_into_edit.substr(
+          0, match.fill_into_edit.rfind(match.contents));
+      NSMutableAttributedString* ignored_prefix_as = DecorateMatchedString(
+        ignored_prefix, ACMatchClassifications(), ContentTextColor(),
+        DimContentTextColor(), result_font);
+      ignored_prefix_width = [ignored_prefix_as size].width;
+      [ignored_prefix_as appendAttributedString:match_text];
+      CGFloat required_width = [ignored_prefix_as size].width;
+      if (required_width > max_required_width)
+        max_required_width = required_width;
+
+      NSMutableAttributedString* ellipsis_prefix = DecorateMatchedString(
+          kEllipsis, ACMatchClassifications(), ContentTextColor(),
+          DimContentTextColor(), result_font);
+      ellipsis_prefix_width = [ellipsis_prefix size].width;
+      [ellipsis_prefix appendAttributedString:match_text];
+      CGFloat match_width = [ellipsis_prefix size].width;
+      if (match_width > max_match_width)
+        max_match_width = match_width;
+      match_text = ellipsis_prefix;
+    }
+    [cell setAttributedTitle:match_text];
+  }
+
+  for (size_t ii = 0; ii < rows; ++ii) {
+    const AutocompleteMatch& match = GetResult().match_at(ii + start_match);
+    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
+    [cell setAdditionalOffset:0];
+    if (match.type != AutocompleteMatchType::SEARCH_SUGGEST_INFINITE) {
+      continue;
+    }
+
+    const float text_width = matrix_width - kTextXOffset;
+    CGFloat additional_offset = (max_required_width < text_width) ?
+        (ignored_prefix_width - ellipsis_prefix_width) :
+        (text_width - max_match_width);
+    if (additional_offset > 0)
+      [cell setAdditionalOffset:additional_offset];
   }
 
   // Set the cell size to fit a line of text in the cell's font.  All