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..4df87c00ceacf1ee9d5e96c369e5bcdb91ef4c38 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm
@@ -123,11 +123,64 @@ 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);
+
+    if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_INFINITE) {
+      // Use ignored_prefix from the fill_into_edit to compute the
+      // required_width and max_required_width.
+      size_t input_start =
+          (match.transition == content::PAGE_TRANSITION_KEYWORD) ?
+              match.keyword.length() + 1 : 0;
+      string16 ignored_prefix = match.fill_into_edit.substr(input_start,
+          match.fill_into_edit.rfind(match.contents) - input_start);
+      NSMutableAttributedString* ignored_prefix_as = DecorateMatchedString(
+          ignored_prefix, ACMatchClassifications(), ContentTextColor(),
+          DimContentTextColor(), result_font);

[Scott Hess - ex-Googler, 2013/12/14 00:40:57]

This is confusing.  Since you're passing empty classification, it only exists to
snag the attributes?  Abstract out the big of DecorateMatchedString() which
converts a base::string16 to an attributed string and share the attributes that
way.

[Anuj, 2013/12/16 02:53:19]

Done.

+      if (ignored_prefix_width == 0)
+        ignored_prefix_width = [ignored_prefix_as size].width;

[Scott Hess - ex-Googler, 2013/12/14 00:40:57]

My concern about this (and others) isn't actually whether you set them over and
over, it's whether they're always the same.  Just looking at the code doesn't
tell me whether they're always the same, unless I dig down into how the matches
themselves work.  You could also just add a comment saying "These are always the
same", or a DCHECK.

It is slightly reasonable to check-and-skip in a case like this one where you
otherwise make a method call.  But this function runs in response to user
actions, so it's not essential to make that kinds of savings.

[Anuj, 2013/12/16 02:53:19]

Done.

+      [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;
+
+      // Use ellipsis prefix to compute the match_width and max_match_width.
+      NSMutableAttributedString* ellipsis_prefix = DecorateMatchedString(
+          base::string16(gfx::kEllipsisUTF16).append(UTF8ToUTF16(" ")),
+          ACMatchClassifications(), ContentTextColor(), DimContentTextColor(),
+          result_font);

[Scott Hess - ex-Googler, 2013/12/14 00:40:57]

Use the abstracted helper here, too.

[Anuj, 2013/12/16 02:53:19]

Done.

+      if (ellipsis_prefix_width == 0)
+        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];
+  }
+
+  const float text_width = matrix_width - kTextXOffset;
+  CGFloat infinite_suggest_offset = (max_required_width < text_width) ?
+    std::max(ignored_prefix_width - ellipsis_prefix_width, 0.0f) :
+    std::max(text_width - max_match_width, 0.0f);
+
+  for (size_t ii = 0; ii < rows; ++ii) {
+    const AutocompleteMatch& match = GetResult().match_at(ii + start_match);
+    OmniboxPopupCell* cell = [matrix_ cellAtRow:ii column:0];
+    CGFloat additional_offset =
+        (match.type != AutocompleteMatchType::SEARCH_SUGGEST_INFINITE) ?
+            0 : infinite_suggest_offset;
+    [cell setAdditionalOffset:additional_offset];
   }

[Scott Hess - ex-Googler, 2013/12/14 00:40:57]

This is all very complicated.  I honestly have no idea what it's doing, because
you're calculating three things at once.  I suspect it could be simpler, but I'm
not sure it's easy to simplify on a stepwise basis.

My suggestion is to first loop through filling the cells in with "... <part we
keep>", setting the offset to the ignored_prefix_width.  Don't track required
width at all.  Put the cells which were from SEARCH_SUGGEST_INFINITE in an
NSMutableArray, like:
  NSMutableArray* infinite_cells = [NSMutableArray arrayWithCapacity:rows];
...
  [infinite_cells addObject:cell];

At this point, some cells maybe extend beyond the RHS.  So add a loop over
infinite_cells to shift things left as needed:
  CGFloat final_offset = ignored_prefix_width;
  for (OmniboxPopupCell* cell in infinite_cells) {
    CGFloat x = NSMaxX(matrix_width) - [cell attributedTitle size].width -
other_stuff;
    final_offset = std::min(final_offset, x);
  }
  final_offset = std::max(final_offset, 0.0);
  for (OmniboxPopupCell* cell in infinite_cells) {
    [cell setAdditionalOffset:final_offset];
  }

When I first started writing that, I wanted to check whether final_offset
differed from ignored_prefix_width, I'm not sure it matters.  Just run through
finding the min() x-coord which works (possibly ignored_prefix_width), then
reset them all.

[Anuj, 2013/12/16 02:53:19]

I have modified the implementation. Note that the width values passed to the
cell should be reset in case the older values were from an infinite suggestion,
but not the new one.

[Anuj, 2013/12/19 05:36:13]

Since cells get reused, the offset needs to be reset on all cells. So there is
no specific advantage of tracking the infinite_cells.
This is not sufficient. We need the max required width, because we want the
suggestions to be vertically aligned on the ellipsis.

[Scott Hess - ex-Googler, 2013/12/19 20:12:50]

The point to tracking the infinite cells is because they are the cells which
need to be considered to calculate the required offset to be shared between
those cells.  _All_ of the cells would get their default offset (based on the
stripped prefix or not-applicable) on the first loop
I'm sure I'm completely misunderstanding something, then, because the point of
the above code is to find the cell with the max required width, and adjust all
of them to be aligned with that one.  The difference is that it doesn't find the
max required width, if finds the min required x-pos, which is (AFAICT) the point
of finding the max required width.

 
   // Set the cell size to fit a line of text in the cell's font.  All