Update Mac password generation autofill popup to match latest mocks.

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

Index: chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm
diff --git a/chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm b/chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm
index a8a85fdc74e95a18c14aa2f3dad1cb9f3c4f0ab9..535318997340245af065ed4bcc3688c5c55dd97f 100644
--- a/chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm
+++ b/chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <cmath>

[groby-ooo-7-16, 2014/08/21 04:40:06]

After password_generation_popup_view_cocoa.h, please. (The file's header should
_always_ be first)

[dconnelly, 2014/08/21 10:44:25]

Done.

+
 #import "chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h"
 
 #include "base/logging.h"
@@ -14,6 +16,7 @@
 #import "chrome/browser/ui/cocoa/hyperlink_text_view.h"
 #import "chrome/browser/ui/cocoa/l10n_util.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
+#include "grit/theme_resources.h"
 #include "skia/ext/skia_utils_mac.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/font_list.h"
@@ -24,18 +27,26 @@
 #include "ui/gfx/text_constants.h"
 
 using autofill::AutofillPopupView;
+using autofill::PasswordGenerationPopupController;
 using autofill::PasswordGenerationPopupView;
 using base::scoped_nsobject;
 
 namespace {
 
+// The width of the divider between the password and help sections, in pixels.
+const CGFloat kDividerHeight = 1;
+
+// The amount of whitespace, in pixels, between lines of text in the password
+// section.
+const CGFloat kPasswordSectionVerticalSeparation = 5;
+
 NSColor* DividerColor() {
   return gfx::SkColorToCalibratedNSColor(
       PasswordGenerationPopupView::kDividerColor);
 }
 
-NSColor* HelpTextBackgroundColor() {
-  return gfx::SkColorToCalibratedNSColor(
+CGColorRef HelpTextBackgroundColor() {
+  return gfx::CGColorCreateFromSkColor(
       PasswordGenerationPopupView::kExplanatoryTextBackgroundColor);
 }
 
@@ -65,30 +76,56 @@ NSColor* HelpLinkColor() {
   if (self = [super initWithDelegate:controller frame:frame]) {
     controller_ = controller;
 
-    passwordField_ = [self textFieldWithText:controller_->password()
-                                       color:[self nameColor]
-                                   alignment:NSLeftTextAlignment];
-    [self addSubview:passwordField_];
-
-    passwordSubtextField_ = [self textFieldWithText:controller_->SuggestedText()
-                                              color:[self subtextColor]
-                                          alignment:NSRightTextAlignment];
-    [self addSubview:passwordSubtextField_];
-
-    scoped_nsobject<HyperlinkTextView> helpTextView(
-        [[HyperlinkTextView alloc] initWithFrame:NSZeroRect]);
-    [helpTextView setMessage:base::SysUTF16ToNSString(controller_->HelpText())
-                    withFont:[self textFont]
-                messageColor:HelpTextColor()];
-    [helpTextView addLinkRange:controller_->HelpTextLinkRange().ToNSRange()
-                      withName:@""
-                     linkColor:HelpLinkColor()];
-    [helpTextView setDelegate:self];
-    [[helpTextView textContainer] setLineFragmentPadding:0.0f];
-    [helpTextView setVerticallyResizable:YES];
-    [self addSubview:helpTextView];
-    helpTextView_ = helpTextView.get();
-  }
+    passwordSection_.reset([[NSView alloc] initWithFrame:NSZeroRect]);
+    [self addSubview:passwordSection_];
+
+    passwordField_.reset(
+        [[self textFieldWithText:controller_->password()
+                      attributes:[self passwordAttributes]] retain]);
+    [passwordSection_ addSubview:passwordField_];
+
+    passwordTitleField_.reset(
+        [[self textFieldWithText:controller_->SuggestedText()
+                      attributes:[self passwordTitleAttributes]] retain]);
+    [passwordSection_ addSubview:passwordTitleField_];
+
+    keyIcon_.reset([[NSImageView alloc] initWithFrame:NSZeroRect]);
+    NSImage* keyImage = ResourceBundle::GetSharedInstance()
+        .GetImageNamed(IDR_GENERATE_PASSWORD_KEY)
+        .ToNSImage();
+    [keyIcon_ setImage:keyImage];
+    [passwordSection_ addSubview:keyIcon_];
+
+    divider_.reset([[NSBox alloc] initWithFrame:NSZeroRect]);
+    [divider_ setBoxType:NSBoxCustom];
+    [divider_ setBorderType:NSLineBorder];
+    [divider_ setBorderColor:DividerColor()];
+    [self addSubview:divider_];
+
+    helpSection_.reset([[NSView alloc] init]);

[groby-ooo-7-16, 2014/08/21 04:40:06]

You can actually skip |helpSection_|, since |helpTextView_| is its only subview,
and the section only provides padding and background AFAICT

You can do those directly on |helpTextView_|:
* Padding : -setTextContainerInset:
* Background: -setDrawsBackground and backgroundColor

[dconnelly, 2014/08/21 10:44:25]

Done.

+    [self addSubview:helpSection_];
+    // Add a background. NSViews don't have root layers by default.
+    base::scoped_nsobject<CALayer> rootLayer([[CALayer alloc] init]);
+    [helpSection_ setLayer:rootLayer];
+    [helpSection_ setWantsLayer:YES];
+    [rootLayer setBackgroundColor:HelpTextBackgroundColor()];
+
+    helpTextView_.reset([[HyperlinkTextView alloc] initWithFrame:NSZeroRect]);
+    [helpTextView_ setMessage:base::SysUTF16ToNSString(controller_->HelpText())
+                     withFont:[self textFont]
+                 messageColor:HelpTextColor()];
+    [helpTextView_ addLinkRange:controller_->HelpTextLinkRange().ToNSRange()
+                       withName:@""
+                      linkColor:HelpLinkColor()];
+    [helpTextView_ setDelegate:self];
+    [helpTextView_ setVerticallyResizable:YES];

[groby-ooo-7-16, 2014/08/21 04:40:06]

Are you sure you need this? This is only required if the text view is supposed
to resize itself according to text changes?

[dconnelly, 2014/08/21 10:44:25]

Done.

+    // Remove the underlining.
+    NSTextStorage* text = [helpTextView_ textStorage];
+    [text addAttribute:NSUnderlineStyleAttributeName
+                 value:[NSNumber numberWithInt:NSUnderlineStyleNone]

[groby-ooo-7-16, 2014/08/21 04:40:06]

Instead of numberWithInt, just use value:@(NSUnderlineStyleNone)

[dconnelly, 2014/08/21 10:44:25]

Done.

+                 range:controller_->HelpTextLinkRange().ToNSRange()];
+    [helpSection_ addSubview:helpTextView_];
+}
 
   return self;
 }
@@ -96,6 +133,8 @@ NSColor* HelpLinkColor() {
 #pragma mark NSView implementation:
 
 - (void)drawRect:(NSRect)dirtyRect {
+  [super drawRect:dirtyRect];
+
   // If the view is in the process of being destroyed, don't bother drawing.
   if (!controller_)
     return;
@@ -104,30 +143,137 @@ NSColor* HelpLinkColor() {
 
   if (controller_->password_selected()) {
     // Draw a highlight under the suggested password.
-    NSRect highlightBounds = [self passwordBounds];
+    NSRect highlightBounds = [passwordSection_ frame];
     [[self highlightColor] set];
     [NSBezierPath fillRect:highlightBounds];

[groby-ooo-7-16, 2014/08/21 04:40:06]

Curious: Why NSBezierPath instead of NSRectFill?. (Not that it matters, I'm just
wondering if I miss something:)

[dconnelly, 2014/08/21 10:44:25]

No idea. This was inherited.

   }
+}
 
-  // Render the background of the help text.
-  [HelpTextBackgroundColor() set];
-  [NSBezierPath fillRect:[self helpBounds]];
+#pragma mark Public API:
 
-  // Render the divider.
-  [DividerColor() set];
-  [NSBezierPath fillRect:[self dividerBounds]];
+- (NSSize)preferredSize {
+  const NSSize passwordTitleSize =
+      [base::SysUTF16ToNSString(controller_->SuggestedText())
+          sizeWithAttributes:@{ NSFontAttributeName : [self boldFont] }];
+  const NSSize passwordSize = [base::SysUTF16ToNSString(controller_->password())
+      sizeWithAttributes:@{ NSFontAttributeName : [self textFont] }];
+
+  CGFloat width =
+      autofill::kPopupBorderThickness +
+      controller_->kHorizontalPadding +
+      [[keyIcon_ image] size].width +
+      controller_->kHorizontalPadding +
+      std::max(passwordSize.width, passwordTitleSize.width) +
+      controller_->kHorizontalPadding +
+      autofill::kPopupBorderThickness;
+
+  width = std::max(width, (CGFloat)controller_->GetMinimumWidth());
+
+  CGFloat height =
+      autofill::kPopupBorderThickness +
+      controller_->kHelpVerticalPadding +
+      [self helpSizeForPopupWidth:width].height +
+      controller_->kHelpVerticalPadding +
+      autofill::kPopupBorderThickness;
+
+  if (controller_->display_password())
+    height += controller_->kPopupPasswordSectionHeight;
+
+  return NSMakeSize(width, height);
 }
 
-#pragma mark Public API:
+- (void)updatePassword {

[groby-ooo-7-16, 2014/08/21 04:40:06]

Since it's public API, should it be in the header? Or is this private and it
should be in a different section?

[dconnelly, 2014/08/21 10:44:25]

It's private. Thanks, moved.

+  NSAttributedString* currentPassword = [passwordField_ attributedStringValue];

[groby-ooo-7-16, 2014/08/21 04:40:06]

Why not just create a new NSAttributedString and set that, instead of copying
the old one, replacing the contents, and then setting that copy?

[dconnelly, 2014/08/21 10:44:26]

Done.

+  base::scoped_nsobject<NSMutableAttributedString> updatedPassword(
+      [[NSMutableAttributedString alloc]
+          initWithAttributedString:currentPassword]);
+  [updatedPassword
+      replaceCharactersInRange:NSMakeRange(0, [currentPassword length])
+                    withString:base::SysUTF16ToNSString(
+                                   controller_->password())];
+  [passwordField_ setAttributedStringValue:updatedPassword];
+}
 
 - (void)updateBoundsAndRedrawPopup {

[groby-ooo-7-16, 2014/08/21 04:40:06]

I'm wondering - it seems the width of all UI elements is determined by
|popupWidth| Could you simplify the layout further by using autoresizingMask on
the main view, and have it resize its children? (I'm not sure if that works - it
might well be the layout is too complex for this - so don't feel compelled
unless it immediately makes sense)

[dconnelly, 2014/08/21 10:44:26]

After thinking through this, it seems like that would involve one of the
following two refactorings:

1. Move all of this logic into resizeSubviewsWithOldSize:
2. Create NSView subclasses for each of the subviews that will update their
sizes properly, and only keep the origin calculations in
resizeSubviewsWithOldSize:

I'm not convinced that's any clearer than this is. Have I missed something? I
suppose I could also use autolayout, but this is clearer to me and works
already.

[groby-ooo-7-16, 2014/08/21 20:05:36]

Both approaches sound like overkill, and we can't have constraint layout (10.7+
only), so ignore me :)

-  [self positionView:passwordField_ inRect:[self passwordBounds]];
-  [self positionView:passwordSubtextField_ inRect:[self passwordBounds]];
-  [self positionView:helpTextView_ inRect:[self helpBounds]];
+  const CGFloat popupWidth = controller_->popup_bounds().width();
+  const CGFloat contentWidth =
+      popupWidth - (2 * autofill::kPopupBorderThickness);
+  const CGFloat contentHeight = controller_->popup_bounds().height() -
+                                (2 * autofill::kPopupBorderThickness);
+
+  if (controller_->display_password()) {
+    // The password can change while the bubble is shown: If the user has

[groby-ooo-7-16, 2014/08/21 04:40:06]

-updateBoundsAndRedrawPopup is only called for the initial -show, though - is
that comment relevant?

[dconnelly, 2014/08/21 10:44:26]

It's actually called as the mouse moves over the bubble, via the C++ bridge
class. That appears to be part of the superclass implementation.

[groby-ooo-7-16, 2014/08/21 20:05:36]

Ah. Foiled by cs.chromium.org having an incomplete corpus. 

+    // accepted the password and then selects the form again and starts deleting
+    // the password, the field will be initially invisible and then become
+    // visible.
+    [self updatePassword];
+
+    // Layout the password section, which includes the key icon, the title, and
+    // the suggested password.
+    [passwordSection_
+        setFrame:NSMakeRect(autofill::kPopupBorderThickness,
+                            autofill::kPopupBorderThickness,
+                            contentWidth,
+                            controller_->kPopupPasswordSectionHeight)];
+
+    // The key icon falls to the left of the title and password.
+    const NSSize imageSize = [[keyIcon_ image] size];
+    const CGFloat keyX = controller_->kHorizontalPadding;
+    const CGFloat keyY =
+        std::ceil(controller_->kPopupPasswordSectionHeight / 2.0) -

[groby-ooo-7-16, 2014/08/21 04:40:06]

One std::ceil for the total term is probably enough :)

[dconnelly, 2014/08/21 10:44:26]

It looked nicer this way :( Done.

+        std::ceil(imageSize.height / 2.0);
+    [keyIcon_ setFrame:{ NSMakePoint(keyX, keyY), [[keyIcon_ image] size] }];

[groby-ooo-7-16, 2014/08/21 04:40:06]

-setFrameOrigin

[dconnelly, 2014/08/21 10:44:26]

It doesn't work, actually -- this size stays 0,0. And the other examples I've
found in chrome/browser/ui/cocoa seem to manually set the view size to the image
size as well.

[groby-ooo-7-16, 2014/08/21 20:05:36]

a [keyIcon_ sizeToFit] at creation time would address that, IIRC.

+
+    // The title and password fall to the right of the key icon and are centered
+    // vertically as a group with some padding in between.
+    [passwordTitleField_ sizeToFit];
+    [passwordField_ sizeToFit];
+    const CGFloat groupHeight = NSHeight([passwordField_ frame]) +
+                                kPasswordSectionVerticalSeparation +
+                                NSHeight([passwordTitleField_ frame]);
+    const CGFloat groupX =
+        NSMaxX([keyIcon_ frame]) + controller_->kHorizontalPadding;
+    const CGFloat groupY =
+        std::ceil(controller_->kPopupPasswordSectionHeight / 2.0) -

[groby-ooo-7-16, 2014/08/21 04:40:06]

One ceil, as above

[dconnelly, 2014/08/21 10:44:26]

Done.

+        std::ceil(groupHeight / 2.0);
+    [passwordField_ setFrameOrigin:NSMakePoint(groupX, groupY)];
+    const CGFloat titleY = groupY +
+                           NSHeight([passwordField_ frame]) +
+                           kPasswordSectionVerticalSeparation;
+    [passwordTitleField_ setFrameOrigin:NSMakePoint(groupX, titleY)];
+
+    // Layout the divider, which falls immediately below the password section.
+    const CGFloat dividerX = autofill::kPopupBorderThickness;
+    const CGFloat dividerY = NSMaxY([passwordSection_ frame]);
+    NSRect dividerFrame =
+        NSMakeRect(dividerX, dividerY, contentWidth, kDividerHeight);
+    [divider_ setFrame:dividerFrame];
+  }
+
+  // Layout the help section beneath the divider (if applicable, otherwise
+  // beneach the border).
+  const CGFloat helpX = autofill::kPopupBorderThickness;
+  const CGFloat helpY = controller_->display_password()
+      ? NSMaxY([divider_ frame])
+      : autofill::kPopupBorderThickness;
+  const CGFloat helpHeight = contentHeight - helpY;
+  [helpSection_ setFrame:NSMakeRect(helpX, helpY, contentWidth, helpHeight)];
+
+  // Layout the help text in the help section with appropriate padding.
+  NSRect helpTextFrame = {
+      NSMakePoint(controller_->kHorizontalPadding,
+                  controller_->kHelpVerticalPadding),
+      [self helpSizeForPopupWidth:popupWidth]
+  };
+  [helpTextView_ setFrame:helpTextFrame];
 
   [super updateBoundsAndRedrawPopup];
 }
 
+- (BOOL)isPointInPasswordBounds:(NSPoint)point {
+  return NSPointInRect(point, [passwordSection_ frame]);
+}
+
 - (void)controllerDestroyed {
   controller_ = NULL;
   [super delegateDestroyed];
@@ -144,27 +290,37 @@ NSColor* HelpLinkColor() {
 
 #pragma mark Private helpers:
 
-- (NSTextField*)textFieldWithText:(const base::string16&)text
-                            color:(NSColor*)color
-                        alignment:(NSTextAlignment)alignment {
+- (NSDictionary*)passwordTitleAttributes {
   scoped_nsobject<NSMutableParagraphStyle> paragraphStyle(
       [[NSMutableParagraphStyle alloc] init]);
-  [paragraphStyle setAlignment:alignment];
+  [paragraphStyle setAlignment:NSLeftTextAlignment];
+  return @{
+    NSFontAttributeName : [self boldFont],
+    NSForegroundColorAttributeName : [self nameColor],
+    NSParagraphStyleAttributeName : paragraphStyle.autorelease()
+  };
+}
 
-  NSDictionary* textAttributes = @{
+- (NSDictionary*)passwordAttributes {
+  scoped_nsobject<NSMutableParagraphStyle> paragraphStyle(
+      [[NSMutableParagraphStyle alloc] init]);
+  [paragraphStyle setAlignment:NSLeftTextAlignment];
+  return @{
     NSFontAttributeName : [self textFont],
-    NSForegroundColorAttributeName : color,
-    NSParagraphStyleAttributeName : paragraphStyle
+    NSForegroundColorAttributeName : [self nameColor],
+    NSParagraphStyleAttributeName : paragraphStyle.autorelease()
   };
+}
 
+- (NSTextField*)textFieldWithText:(const base::string16&)text
+                       attributes:(NSDictionary*)attributes {
+  NSTextField* textField =
+      [[[NSTextField alloc] initWithFrame:NSZeroRect] autorelease];
   scoped_nsobject<NSAttributedString> attributedString(
       [[NSAttributedString alloc]
           initWithString:base::SysUTF16ToNSString(text)
-              attributes:textAttributes]);
-
-  NSTextField* textField =
-      [[[NSTextField alloc] initWithFrame:NSZeroRect] autorelease];
-  [textField setAttributedStringValue:attributedString];
+              attributes:attributes]);
+  [textField setAttributedStringValue:attributedString.autorelease()];
   [textField setEditable:NO];
   [textField setSelectable:NO];
   [textField setDrawsBackground:NO];
@@ -172,31 +328,25 @@ NSColor* HelpLinkColor() {
   return textField;
 }
 
-- (void)positionView:(NSView*)view inRect:(NSRect)bounds {
-  NSRect frame = NSInsetRect(bounds, controller_->kHorizontalPadding, 0);
-  [view setFrame:frame];
-
-  // Center the text vertically within the bounds.
-  NSSize delta = cocoa_l10n_util::WrapOrSizeToFit(view);
-  [view setFrameOrigin:
-      NSInsetRect(frame, 0, floor(-delta.height/2)).origin];
-}
-
-- (NSRect)passwordBounds {
-  return NSZeroRect;
-}
-
-- (NSRect)helpBounds {
-  return NSZeroRect;
+- (NSSize)helpSizeForPopupWidth:(CGFloat)width {
+  const CGFloat helpWidth = width -
+                            2 * controller_->kHorizontalPadding -
+                            2 * autofill::kPopupBorderThickness;
+  const NSSize size = NSMakeSize(helpWidth, MAXFLOAT);
+  NSRect textFrame = [base::SysUTF16ToNSString(controller_->HelpText())
+      boundingRectWithSize:size
+                   options:NSLineBreakByWordWrapping |
+                           NSStringDrawingUsesLineFragmentOrigin
+                attributes:@{ NSFontAttributeName : [self textFont] }];
+  return textFrame.size;
 }
 
-- (NSRect)dividerBounds {
-  return NSZeroRect;
+- (NSFont*)boldFont {
+  return [NSFont boldSystemFontOfSize:[NSFont smallSystemFontSize]];
 }
 
 - (NSFont*)textFont {
-  return ResourceBundle::GetSharedInstance().GetFontList(
-      ResourceBundle::SmallFont).GetPrimaryFont().GetNativeFont();
+  return [NSFont systemFontOfSize:[NSFont smallSystemFontSize]];
 }
 
 @end