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..484c6dc1d755b7e1b0906ee2911fc58bbf2287e3 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,62 @@ void OmniboxPopupViewMac::UpdatePopupAppearance() {
   // Load the results into the popup's matrix.
   DCHECK_GT(rows, 0U);
   [matrix_ renewRows:rows columns:1];
+
+  CGFloat max_contents_width = 0;
+  CGFloat max_required_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);
+    [cell setRequiredWidth:0.0f];
+    [cell setContentsWidth:0.0f];
+
+    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;
+      size_t contents_index =
+          match.fill_into_edit.length() - match.contents.length();
+
+      string16 ignored_prefix = match.fill_into_edit.substr(input_start,
+           contents_index - input_start);
+      NSMutableAttributedString* ignored_prefix_as = CreateAttributedString(
+          ignored_prefix, ContentTextColor(), result_font);
+      [ignored_prefix_as appendAttributedString:match_text];
+      CGFloat required_width = [ignored_prefix_as size].width;
+      [cell setRequiredWidth:required_width];
+      if (required_width > max_required_width)
+        max_required_width = required_width;
+
+      // Use ellipsis prefix to compute the contents_width and
+      // max_contents_width.
+      NSMutableAttributedString* ellipsis_prefix = CreateAttributedString(
+          base::string16(gfx::kEllipsisUTF16).append(UTF8ToUTF16(" ")),
+          ContentTextColor(), result_font);
+      [ellipsis_prefix appendAttributedString:match_text];
+      CGFloat contents_width = [ellipsis_prefix size].width;
+      [cell setContentsWidth:contents_width];
+      if (contents_width > max_contents_width)
+        max_contents_width = contents_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 setMaxRequiredWidth:0];
+    [cell setMaxContentsWidth:0];
+    if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_INFINITE) {
+      [cell setMaxRequiredWidth:max_required_width];
+      [cell setMaxContentsWidth:max_contents_width];
+    }

[Scott Hess - ex-Googler, 2013/12/16 22:22:05]

I feel like we're missing each other, here.  I'm concerned about the extra 30
lines of opaque per-cell setup landed in the loop above, but you've added
additional state to the cell where all of the cells are going to replicate some
calculations.  I'm fine with the alignment code being here - this is the level
which needs to calculate the alignment, after all, the cell just needs to know
where the offset is, it doesn't have to know everything that goes into
calculating the offset.

Could you perhaps review my previous comment in this area and let me know which
parts you disagreed with?  I thought I had figured things out well enough to
suggest ways to avoid having to have a {required, content} * {desired, max} for
each.  Maybe I missed something subtle?

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

My problem is that the implementations of omnibox dropdown in views vs mac vs
android differ a lot. This model where there is a cell/match-level class which
captures the width information seems to work well across the three
implementations. 
For better maintainability, I will prefer a solution which looks similar on all
platforms.

As such, how a cell is rendered should be the responsibility of the cell-level
class, and I can move a lot of MatchText/NSAttributedString related code to
OmniboxPopupCell too. But that will be too much of a change for this CL.

Given the current solution looks uniform on all platforms, and satisfies the
use-cases correctly,  I will prefer not to modify this further at this point of
time. 

I will revisit the three implementations again and see if I can move the offset
calculation to the popup level class.

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

That helps maintainability of your feature, but doesn't necessarily help
maintainability of the per-platform classes.  Views controls are not Mac
controls are not Android controls, it's just the way it is.  If you want to
share calculators, abstract it out into a helper so that it _looks_ similar (I'm
pretty sure [cell setMaxRequiredWidth:0] doesn't look like anything on Views
...).

My issue about having the cell understand the min/max/etc constraints is that
the cell cannot calculate the results in isolation.  The matrix must be involved
to propagate things between neighboring cells.  But once the matrix is involved,
then you might as well have the matrix do the calculation directly, rather than
distribute it, since they all have to make it exactly the same.  I'm not worried
about the duplication of CPU effort, I'm worried about whether thee moving parts
can get out of sync with each other, having a single point calculating it helps
prevent that.

   }
 
   // Set the cell size to fit a line of text in the cell's font.  All
@@ -245,14 +296,10 @@ NSAttributedString* OmniboxPopupViewMac::MatchText(
 }
 
 // static
-NSMutableAttributedString* OmniboxPopupViewMac::DecorateMatchedString(
+NSMutableAttributedString* OmniboxPopupViewMac::CreateAttributedString(

[Scott Hess - ex-Googler, 2013/12/16 22:22:05]

I think DecorateMatchedString() was a static member so that it could be tested. 
So either test this, or move it to the anonymous namespace.  Looks to me like
the test for DecorateMatchedString() is pretty basic, so I'm not gung-ho on
writing a test for this, but OTOH you could basically copy/paste
DecorateMatchedStringNoMatch and be done, so your call as to which path to take.

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

Moved to the anonymous namespace.

     const base::string16& match_string,
-    const AutocompleteMatch::ACMatchClassifications& classifications,
     NSColor* text_color,
-    NSColor* dim_text_color,
     gfx::Font& font) {
-  // Cache for on-demand computation of the bold version of |font|.
-  NSFont* bold_font = nil;
 
   // Start out with a string using the default style info.
   NSString* s = base::SysUTF16ToNSString(match_string);
@@ -264,25 +311,42 @@ NSMutableAttributedString* OmniboxPopupViewMac::DecorateMatchedString(
       [[[NSMutableAttributedString alloc] initWithString:s
                                               attributes:attributes]
         autorelease];
+  return as;
+}
+
+// static
+NSMutableAttributedString* OmniboxPopupViewMac::DecorateMatchedString(
+    const base::string16& match_string,
+    const AutocompleteMatch::ACMatchClassifications& classifications,
+    NSColor* text_color,
+    NSColor* dim_text_color,
+    gfx::Font& font) {
+
+  NSMutableAttributedString* as =
+      CreateAttributedString(match_string, text_color, font);
+
+  size_t match_length = match_string.length();
 
   // As a protective measure, bail if the length of the match string is not
   // the same as the length of the converted NSString. http://crbug.com/121703
-  if ([s length] != match_string.size())
+  if ([as length] != match_length)
     return as;
 
+  // Cache for on-demand computation of the bold version of |font|.
+  NSFont* bold_font = nil;

[Scott Hess - ex-Googler, 2013/12/16 22:22:05]

The next comment should come after a whitespace line.

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

Done.

   // Mark up the runs which differ from the default.
   for (ACMatchClassifications::const_iterator i = classifications.begin();
        i != classifications.end(); ++i) {
     const BOOL is_last = (i+1) == classifications.end();
     const NSInteger next_offset =
-        (is_last ? [s length] : static_cast<NSInteger>((i + 1)->offset));
+        (is_last ? match_length : static_cast<NSInteger>((i + 1)->offset));
     const NSInteger location = static_cast<NSInteger>(i->offset);
     const NSInteger length = next_offset - static_cast<NSInteger>(i->offset);
     // Guard against bad, off-the-end classification ranges.
-    if (i->offset >= [s length] || length <= 0)
+    if (i->offset >= match_length || length <= 0)
       break;
     const NSRange range = NSMakeRange(location,
-        MIN(length, static_cast<NSInteger>([s length]) - location));
+        MIN(length, static_cast<NSInteger>(match_length) - location));
 
     if (0 != (i->style & ACMatchClassification::URL)) {
       [as addAttribute:NSForegroundColorAttributeName