Alternate NTP: Add search token to omnibox

chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm

Index: chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
index 3078fe4ea9a4d556df291820a9cff393dde37d1f..556125bc75936c70cfe517cc6cc5329ee38dc3a4 100644
--- a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
+++ b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
@@ -27,6 +27,9 @@ const CGFloat kCornerRadius = 3.0;
 // How far to inset the left-hand decorations from the field's bounds.
 const CGFloat kLeftDecorationXOffset = 5.0;
 
+// Padding between the control's text and the collapsible decoration.
+const CGFloat kAutoCollapsePadding = 20.0;
+
 NSString* const kButtonDecorationKey = @"ButtonDecoration";
 
 // How far to inset the right-hand decorations from the field's bounds.
@@ -71,6 +74,7 @@ const NSTimeInterval kLocationIconDragTimeout = 0.25;
 // (|DecorationHorizontalPad()| is used between decorations).
 void CalculatePositionsHelper(
     NSRect frame,
+    CGFloat text_width,
     const std::vector<LocationBarDecoration*>& all_decorations,
     NSRectEdge x_edge,
     CGFloat regular_padding,
@@ -101,9 +105,15 @@ void CalculatePositionsHelper(
 
       // Find out how large the decoration will be in the remaining
       // space.
-      const CGFloat used_width =
+      CGFloat used_width =
           all_decorations[i]->GetWidthForSpace(NSWidth(available));
 
+      // Collapse if necessary.

[Scott Hess - ex-Googler, 2013/01/28 21:37:27]

Way Back When, I thought that this kind of auto-adjusting layout decision was
being handled in LocationBarViewMac::Layout(), because that's where it was being
handled for other platforms.  But I don't see obvious evidence of this, and
other platforms have changed a bunch in this area.  The main rationale for this
was so that in the future people wouldn't distribute things randomly across the
code and make it hard to understand.  Since you're already looking at the code
from other platforms, please give it a pass to see if this code will make sense
when someone from that platform sees it, in case it needs adjustment.

[sail, 2013/01/29 22:31:08]

I think moving this might be a good idea. In views all this logic is in a
special class (LocationBarLayout). We could do the same.
I've filed http://crbug.com/172949 to track this work.

+      CGFloat new_width = NSWidth(available) - used_width -
+                          kAutoCollapsePadding;
+      if (all_decorations[i]->ShouldAutoCollapse() && new_width < text_width)
+        used_width = LocationBarDecoration::kOmittedWidth;

[Scott Hess - ex-Googler, 2013/01/28 21:37:27]

I wonder if this wouldn't make more sense to fold into GetWidthForSpace(),
rather than having two places for the knowledge to live?  I think that if we
really want this (*), it would be reasonable to pass down both "total available
space" as NSWidth(available) and "desired decoration space" as
NSWidth(available) - text_width.  Then the decoration can make the decision
about collapsing and how much padding to consider.

(*) I think that there used to be more collapsing going on in the location-bar
code, but we moved away from it for some reason.

[sail, 2013/01/29 22:31:08]

Done.
Moved the collapse logic to SearchTokenDecoration::GetWidthForSpace(). Added
text_width as a parameter to GetWidthForSpace().

+
       if (used_width != LocationBarDecoration::kOmittedWidth) {
         DCHECK_GT(used_width, 0.0);
         NSRect decoration_frame;
@@ -127,6 +137,15 @@ void CalculatePositionsHelper(
   *remaining_frame = frame;
 }
 
+// Hide separators at the beginning and end of the decorator list.
+void HideUnneededSeparators(
+    std::vector<LocationBarDecoration*>* decorations) {
+  if (!decorations->empty() && decorations->front()->AsSeparatorDecoration())
+    decorations->erase(decorations->begin());
+  if (!decorations->empty() && decorations->back()->AsSeparatorDecoration())
+    decorations->pop_back();
+}
+
 // Helper function for calculating placement of decorations w/in the cell.
 // |frame| is the cell's boundary rectangle, |remaining_frame| will get any
 // space left after decorations are laid out (for text).  |left_decorations| is
@@ -139,6 +158,7 @@ void CalculatePositionsHelper(
 // the index of the first right-hand decoration.
 size_t CalculatePositionsInFrame(
     NSRect frame,
+    CGFloat text_width,
     const std::vector<LocationBarDecoration*>& left_decorations,
     const std::vector<LocationBarDecoration*>& right_decorations,
     CGFloat edge_width,
@@ -149,19 +169,35 @@ size_t CalculatePositionsInFrame(
   decoration_frames->clear();
 
   // Layout |left_decorations| against the LHS.
-  CalculatePositionsHelper(frame, left_decorations, NSMinXEdge,
-                           kLeftDecorationXOffset, kLeftDecorationXOffset,
-                           decorations, decoration_frames, &frame);
-  DCHECK_EQ(decorations->size(), decoration_frames->size());
-
-  // Capture the number of visible left-hand decorations.
+  {
+    std::vector<LocationBarDecoration*> result_decorations;
+    std::vector<NSRect> result_frames;
+    NSRect result_frame = NSZeroRect;
+    CalculatePositionsHelper(frame, text_width, left_decorations, NSMinXEdge,
+                             kLeftDecorationXOffset, kLeftDecorationXOffset,
+                             &result_decorations, &result_frames,
+                             &result_frame);
+    HideUnneededSeparators(&result_decorations);

[Scott Hess - ex-Googler, 2013/01/28 21:37:27]

The helper already knows about applying auto-collapse, I don't think it would be
materially worse to have it also know how to check for separators against the
endpoints, at least versus having this code duplication here.  It would need two
tests, one inside the loop and one outside, but HideUnneededSeparators() also
needs two tests.  I don't have the heart to dig into the other-platform code to
see if as-is matches that better.

[sail, 2013/01/29 22:31:08]

The views code also does two passes.
I tried moving the check inside the loop in CalculatePositionsHelper() but the
logic to update the frames became too complicated. 

I find doing two passes a lot easier to understand.

[Scott Hess - ex-Googler, 2013/01/29 23:28:36]

OK.  My main concern is whether there will be subtle issues due to the passes
not returning the same values.

+    CalculatePositionsHelper(frame, text_width, result_decorations, NSMinXEdge,
+                             kLeftDecorationXOffset, kLeftDecorationXOffset,
+                             decorations, decoration_frames, &frame);
+  }
   const size_t left_count = decorations->size();
 
   // Layout |right_decorations| against the RHS.
-  CalculatePositionsHelper(frame, right_decorations, NSMaxXEdge,
-                           RightDecorationXOffset(), edge_width, decorations,
-                           decoration_frames, &frame);
-  DCHECK_EQ(decorations->size(), decoration_frames->size());
+  {
+    std::vector<LocationBarDecoration*> result_decorations;
+    std::vector<NSRect> result_frames;
+    NSRect result_frame = NSZeroRect;
+    CalculatePositionsHelper(frame, text_width, right_decorations, NSMaxXEdge,
+                             RightDecorationXOffset(), edge_width,
+                             &result_decorations, &result_frames,
+                             &result_frame);
+    HideUnneededSeparators(&result_decorations);
+    CalculatePositionsHelper(frame, text_width, result_decorations, NSMaxXEdge,
+                             RightDecorationXOffset(), edge_width,
+                             decorations, decoration_frames, &frame);
+  }
 
   // Reverse the right-hand decorations so that overall everything is
   // sorted left to right.
@@ -219,9 +255,10 @@ size_t CalculatePositionsInFrame(
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(frame, leftDecorations_, rightDecorations_,
-                            [self edgeWidth], &decorations, &decorationFrames,
-                            &textFrame);
+  NSAttributedString* text = [self attributedStringValue];

[Scott Hess - ex-Googler, 2013/01/28 21:37:27]

Suggest either a helper for "[[self attributedStringValue] size].width", or
re-casting CalculatePositionsInFrame() as a method.  I think I'd prefer the
former, a big reason why it's a function in the anon namespace is because I want
to prevent people from reaching out to random area of the profile to influence
layout dynamically.

[sail, 2013/01/29 22:31:08]

Done.
Added a helper method -[AutocompleteTextFieldCell textWidth]

+  CalculatePositionsInFrame(frame, [text size].width, leftDecorations_,
+                            rightDecorations_, [self edgeWidth], &decorations,
+                            &decorationFrames, &textFrame);
 
   return NSWidth(textFrame);
 }
@@ -236,9 +273,10 @@ size_t CalculatePositionsInFrame(
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                            [self edgeWidth], &decorations, &decorationFrames,
-                            &textFrame);
+  NSAttributedString* text = [self attributedStringValue];
+  CalculatePositionsInFrame(cellFrame, [text size].width, leftDecorations_,
+                            rightDecorations_, [self edgeWidth], &decorations,
+                            &decorationFrames, &textFrame);
 
   // Find our decoration and return the corresponding frame.
   std::vector<LocationBarDecoration*>::const_iterator iter =
@@ -248,22 +286,21 @@ size_t CalculatePositionsInFrame(
     return decorationFrames[index];
   }
 
-  // Decorations which are not visible should have been filtered out
-  // at the top, but return |NSZeroRect| rather than a 0-width rect
-  // for consistency.
-  NOTREACHED();
+  // The decoration is marked as visible but was either collapsed or hidden.
   return NSZeroRect;
 }
 
 // Overriden to account for the decorations.
 - (NSRect)textFrameForFrame:(NSRect)cellFrame {
+  NSAttributedString* text = [self attributedStringValue];
+
   // Get the frame adjusted for decorations.
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame = [super textFrameForFrame:cellFrame];
-  CalculatePositionsInFrame(textFrame, leftDecorations_, rightDecorations_,
-                            [self edgeWidth], &decorations, &decorationFrames,
-                            &textFrame);
+  CalculatePositionsInFrame(textFrame, [text size].width, leftDecorations_,
+                            rightDecorations_, [self edgeWidth], &decorations,
+                            &decorationFrames, &textFrame);
 
   // NOTE: This function must closely match the logic in
   // |-drawInteriorWithFrame:inView:|.
@@ -277,13 +314,13 @@ size_t CalculatePositionsInFrame(
 }
 
 - (NSRect)textCursorFrameForFrame:(NSRect)cellFrame {
+  NSAttributedString* text = [self attributedStringValue];
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  size_t left_count =
-      CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                                [self edgeWidth], &decorations,
-                                &decorationFrames, &textFrame);
+  size_t left_count = CalculatePositionsInFrame(
+      cellFrame, [text size].width, leftDecorations_, rightDecorations_,
+      [self edgeWidth], &decorations, &decorationFrames, &textFrame);
 
   // Determine the left-most extent for the i-beam cursor.
   CGFloat minX = NSMinX(textFrame);
@@ -322,9 +359,10 @@ size_t CalculatePositionsInFrame(
   std::vector<NSRect> decorationFrames;
   NSRect workingFrame;
 
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                            [self edgeWidth], &decorations, &decorationFrames,
-                            &workingFrame);
+  NSAttributedString* text = [self attributedStringValue];
+  CalculatePositionsInFrame(cellFrame, [text size].width, leftDecorations_,
+                            rightDecorations_, [self edgeWidth], &decorations,
+                            &decorationFrames, &workingFrame);
 
   // Draw the decorations.
   for (size_t i = 0; i < decorations.size(); ++i) {
@@ -350,11 +388,13 @@ size_t CalculatePositionsInFrame(
   const BOOL flipped = [controlView isFlipped];
   const NSPoint location =
       [controlView convertPoint:[theEvent locationInWindow] fromView:nil];
+  NSAttributedString* text = [self attributedStringValue];
 
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
+  CalculatePositionsInFrame(cellFrame, [text size].width, leftDecorations_,
+                            rightDecorations_,
                             [self edgeWidth], &decorations, &decorationFrames,
                             &textFrame);
 
@@ -507,10 +547,11 @@ size_t CalculatePositionsInFrame(
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
+  NSAttributedString* text = [self attributedStringValue];
   NSRect cellRect = [self clickableFrameForFrame:[view bounds]];
-  CalculatePositionsInFrame(cellRect, leftDecorations_, rightDecorations_,
-                            [self edgeWidth], &decorations, &decorationFrames,
-                            &textFrame);
+  CalculatePositionsInFrame(cellRect, [text size].width, leftDecorations_,
+                            rightDecorations_, [self edgeWidth], &decorations,
+                            &decorationFrames, &textFrame);
 
   // Remove previously-registered tracking areas, since we'll update them below.
   for (CrTrackingArea* area in [view trackingAreas]) {
@@ -725,9 +766,10 @@ static NSString* UnusedLegalNameForNewDropFile(NSURL* saveLocation,
   std::vector<LocationBarDecoration*> decorations;
   std::vector<NSRect> decorationFrames;
   NSRect textFrame;
-  CalculatePositionsInFrame(cellFrame, leftDecorations_, rightDecorations_,
-                            [self edgeWidth], &decorations, &decorationFrames,
-                            &textFrame);
+  NSAttributedString* text = [self attributedStringValue];
+  CalculatePositionsInFrame(cellFrame, [text size].width, leftDecorations_,
+                            rightDecorations_, [self edgeWidth], &decorations,
+                            &decorationFrames, &textFrame);
 
   for (size_t i = 0; i < decorations.size(); ++i) {
     NSString* tooltip = decorations[i]->GetToolTip();