mac: Update layout of the autofill element to get contact permissions.

chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm

 // Copyright (c) 2012 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/autofill/autofill_popup_view_cocoa.h"
 
 #include "base/logging.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/ui/autofill/autofill_popup_controller.h"
 #include "chrome/browser/ui/autofill/popup_constants.h"
@@ skipping 10 matching lines @@
 using autofill::AutofillPopupView;
 
 @interface AutofillPopupViewCocoa ()
 
 #pragma mark -
 #pragma mark Private methods
 
 // Draws an Autofill suggestion in the given |bounds|, labeled with the given
 // |name| and |subtext| hint.  If the suggestion |isSelected|, then it is drawn
 // with a highlight.  |index| determines the font to use, as well as the icon,
-// if the row requires it -- such as for credit cards.
+// if the row requires it -- such as for credit cards. |imageFirst| indicates
+// whether the image should be drawn before the name, and with the same
+// alignment, or whether it should be drawn afterwards, with the opposite
+// alignment.
 - (void)drawSuggestionWithName:(NSString*)name
                        subtext:(NSString*)subtext
                          index:(size_t)index
                         bounds:(NSRect)bounds
-                      selected:(BOOL)isSelected;
+                      selected:(BOOL)isSelected
+                    imageFirst:(BOOL)imageFirst;
+

[groby-ooo-7-16, 2014/05/22 21:22:17]

Since this covers three functions, might want to make that clear in comment :)

[erikchen, 2014/05/22 22:24:35]

Done.

+// If rightAlign == YES

[groby-ooo-7-16, 2014/05/22 21:22:17]

nit:|rightAlign|, period

[erikchen, 2014/05/22 22:24:35]

Done.

+//   Draws the widget with right border aligned to |x|.
+//   Returns the x value of left border of the widget.
+// If rightAlign == NO.
+//   Draws the widget with left border aligned to |x|.
+//   Returns the x value of right border of the widget.
+- (CGFloat)drawName:(NSString*)name
+                atX:(CGFloat)x
+              index:(size_t)index
+         rightAlign:(BOOL)rightAlign
+             bounds:(NSRect)bounds;
+- (CGFloat)drawIconAtIndex:(size_t)index
+                       atX:(CGFloat)x
+                rightAlign:(BOOL)rightAlign
+                    bounds:(NSRect)bounds;
+- (CGFloat)drawSubtext:(NSString*)subtext
+                   atX:(CGFloat)x
+            rightAlign:(BOOL)rightAlign
+                bounds:(NSRect)bounds;
 
 // Returns the icon for the row with the given |index|, or |nil| if there is
 // none.
 - (NSImage*)iconAtIndex:(size_t)index;
 
 @end
 
 @implementation AutofillPopupViewCocoa
 
 #pragma mark -
@@ skipping 23 matching lines @@
 
   [self drawBackgroundAndBorder];
 
   for (size_t i = 0; i < controller_->names().size(); ++i) {
     // Skip rows outside of the dirty rect.
     NSRect rowBounds =
         NSRectFromCGRect(controller_->GetRowBounds(i).ToCGRect());
     if (!NSIntersectsRect(rowBounds, dirtyRect))
       continue;
 
+    BOOL needsDraw = NO;
+    BOOL imageFirst = NO;
     if (controller_->identifiers()[i] == autofill::POPUP_ITEM_ID_SEPARATOR) {
       [self drawSeparatorWithBounds:rowBounds];

[groby-ooo-7-16, 2014/05/22 21:22:17]

Add a return here, then you can skip needsDraw altogether. I.e

if(SEPARATOR) {
  drawSeparator;
  return;
}

imageFirst = (identifier == ACCESS_CONTACTS);
...

[erikchen, 2014/05/22 22:24:35]

Done. I also simplified the following else if/else logic.

+    } else if (controller_->identifiers()[i] ==
+               autofill::POPUP_ITEM_ID_MAC_ACCESS_CONTACTS) {
+      needsDraw = YES;
+      imageFirst = YES;
     } else {
+      needsDraw = YES;
+      imageFirst = NO;
+    }
+
+    if (needsDraw) {
       NSString* name = SysUTF16ToNSString(controller_->names()[i]);
       NSString* subtext = SysUTF16ToNSString(controller_->subtexts()[i]);
       BOOL isSelected = static_cast<int>(i) == controller_->selected_line();
       [self drawSuggestionWithName:name
                            subtext:subtext
                              index:i
                             bounds:rowBounds
-                          selected:isSelected];
+                          selected:isSelected
+                        imageFirst:imageFirst];
     }
   }
 }
 
 #pragma mark -
 #pragma mark Public API:
 
 - (void)controllerDestroyed {
   // Since the |controller_| either already has been destroyed or is about to
   // be, about the only thing we can safely do with it is to null it out.
   controller_ = NULL;
   [super delegateDestroyed];
 }
 
 - (void)invalidateRow:(size_t)row {
   NSRect dirty_rect =
       NSRectFromCGRect(controller_->GetRowBounds(row).ToCGRect());
   [self setNeedsDisplayInRect:dirty_rect];
 }
 
 #pragma mark -
 #pragma mark Private API:
 
 - (void)drawSuggestionWithName:(NSString*)name
                        subtext:(NSString*)subtext
                          index:(size_t)index
                         bounds:(NSRect)bounds
-                      selected:(BOOL)isSelected {
+                      selected:(BOOL)isSelected
+                    imageFirst:(BOOL)imageFirst {
   // If this row is selected, highlight it.
   if (isSelected) {
     [[self highlightColor] set];
     [NSBezierPath fillRect:bounds];
   }
 
   BOOL isRTL = controller_->IsRTL();
 
+  // The X values of the left and right borders of the autofill widget.
+  CGFloat leftX = bounds.origin.x + AutofillPopupView::kEndPadding;

[groby-ooo-7-16, 2014/05/22 21:22:17]

NSMinX(bounds) + padding

[erikchen, 2014/05/22 22:24:35]

Done.

+  CGFloat rightX =
+      bounds.origin.x + bounds.size.width - AutofillPopupView::kEndPadding;

[groby-ooo-7-16, 2014/05/22 21:22:17]

NSMaxX(bounds) + padding;

[erikchen, 2014/05/22 22:24:35]

Done.

+
+  // All comments within the scope of this conditional assume RTL == NO. This
+  // allows the comments to more succinctly describe the intended behavior. All
+  // logic is simply inverted for RTL == YES.

[groby-ooo-7-16, 2014/05/22 21:22:17]

I _think_ all that can be expressed a bit more concisely, but there's nothing
wrong with the current path.

if (isRTL) 
  std::swap(rightX, leftX);

// Draw left side.
CGFloat x = leftX; 
if (imageFirst)
  x = [self drawIconAtIndex:index atX:x rightAlign:isRTL bounds:bounds];
[self drawName:name atX:x index:index rightAlign:isRTL bounds:bounds];

// Draw right side.
x = rightX;
if (!imageFirst)
  x = [self drawIconAtIndex:index atX:x rightAlign:!isRTL bounds:bounds];
[self drawSubtext:subtext atX:x rightAlign:!isRTL bounds:bounds];

[erikchen, 2014/05/22 22:24:35]

I simplified the logic following your example, but avoided the std::swap between
rightX and leftX, as that muddles the meaning of the variables and seems prone
to future errors.

+  if (imageFirst) {
+    // Draw the icon at the left border.
+    CGFloat x = isRTL ? rightX : leftX;
+    x = [self drawIconAtIndex:index atX:x rightAlign:isRTL bounds:bounds];
+    // Draw the name immediately to the right of the icon.
+    [self drawName:name atX:x index:index rightAlign:isRTL bounds:bounds];
+
+    // Draw the subtext at the right border, right aligned.
+    x = isRTL ? leftX : rightX;
+    [self drawSubtext:subtext atX:x rightAlign:!isRTL bounds:bounds];
+  } else {
+    // Draw the name at the left border.
+    CGFloat x = isRTL ? rightX : leftX;
+    [self drawName:name atX:x index:index rightAlign:isRTL bounds:bounds];
+
+    // Draw the icon at the right border, right aligned.
+    x = isRTL ? leftX : rightX;
+    x = [self drawIconAtIndex:index atX:x rightAlign:!isRTL bounds:bounds];
+    // Draw the subtext immediately to the left of the icon, right aligned.
+    [self drawSubtext:subtext atX:x rightAlign:!isRTL bounds:bounds];
+  }
+}
+
+- (CGFloat)drawName:(NSString*)name
+                atX:(CGFloat)x
+              index:(size_t)index
+         rightAlign:(BOOL)rightAlign
+             bounds:(NSRect)bounds {
   NSColor* nameColor =
       controller_->IsWarning(index) ? [self warningColor] : [self nameColor];
   NSDictionary* nameAttributes =
       [NSDictionary dictionaryWithObjectsAndKeys:
            controller_->GetNameFontListForRow(index).GetPrimaryFont().
                GetNativeFont(),
            NSFontAttributeName, nameColor, NSForegroundColorAttributeName,
            nil];
   NSSize nameSize = [name sizeWithAttributes:nameAttributes];
-  CGFloat x = bounds.origin.x +
-      (isRTL ?
-       bounds.size.width - AutofillPopupView::kEndPadding - nameSize.width :
-       AutofillPopupView::kEndPadding);
+  x -= rightAlign ? nameSize.width : 0;
   CGFloat y = bounds.origin.y + (bounds.size.height - nameSize.height) / 2;
 
   [name drawAtPoint:NSMakePoint(x, y) withAttributes:nameAttributes];
 
-  // The x-coordinate will be updated as each element is drawn.
-  x = bounds.origin.x +
-      (isRTL ?
-       AutofillPopupView::kEndPadding :
-       bounds.size.width - AutofillPopupView::kEndPadding);
+  x += rightAlign ? 0 : nameSize.width;
+  return x;
+}
 
-  // Draw the Autofill icon, if one exists.
+- (CGFloat)drawIconAtIndex:(size_t)index
+                       atX:(CGFloat)x
+                rightAlign:(BOOL)rightAlign
+                    bounds:(NSRect)bounds {
   NSImage* icon = [self iconAtIndex:index];
-  if (icon) {
-    NSSize iconSize = [icon size];
-    x += isRTL ? 0 : -iconSize.width;
-    y = bounds.origin.y + (bounds.size.height - iconSize.height) / 2;
+  if (!icon)
+    return x;
+  NSSize iconSize = [icon size];
+  x -= rightAlign ? iconSize.width : 0;
+  CGFloat y = bounds.origin.y + (bounds.size.height - iconSize.height) / 2;
     [icon drawInRect:NSMakeRect(x, y, iconSize.width, iconSize.height)
             fromRect:NSZeroRect
            operation:NSCompositeSourceOver
             fraction:1.0
       respectFlipped:YES
                hints:nil];
 
-    x += isRTL ?
-        iconSize.width + AutofillPopupView::kIconPadding :
-        -AutofillPopupView::kIconPadding;
-  }
+    x += rightAlign ? -AutofillPopupView::kIconPadding
+                    : iconSize.width + AutofillPopupView::kIconPadding;
+    return x;
+}
 
-  // Draw the subtext.
+- (CGFloat)drawSubtext:(NSString*)subtext
+                   atX:(CGFloat)x
+            rightAlign:(BOOL)rightAlign
+                bounds:(NSRect)bounds {
   NSDictionary* subtextAttributes =
       [NSDictionary dictionaryWithObjectsAndKeys:
            controller_->subtext_font_list().GetPrimaryFont().GetNativeFont(),
            NSFontAttributeName,
            [self subtextColor],
            NSForegroundColorAttributeName,
            nil];
   NSSize subtextSize = [subtext sizeWithAttributes:subtextAttributes];
-  x += isRTL ? 0 : -subtextSize.width;
-  y = bounds.origin.y + (bounds.size.height - subtextSize.height) / 2;
+  x -= rightAlign ? subtextSize.width : 0;
+  CGFloat y = bounds.origin.y + (bounds.size.height - subtextSize.height) / 2;
 
   [subtext drawAtPoint:NSMakePoint(x, y) withAttributes:subtextAttributes];
+  x += rightAlign ? 0 : subtextSize.width;
+  return x;
 }
 
 - (NSImage*)iconAtIndex:(size_t)index {
   if (controller_->icons()[index].empty())
     return nil;
 
   int iconId = controller_->GetIconResourceID(controller_->icons()[index]);
   DCHECK_NE(-1, iconId);
 
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   return rb.GetNativeImageNamed(iconId).ToNSImage();
 }
 
 @end