Update Mac password generation autofill popup to match latest mocks.

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

 // Copyright 2014 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/password_generation_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/autofill_popup_view.h"
 #include "chrome/browser/ui/autofill/popup_constants.h"
 #include "chrome/browser/ui/chrome_style.h"
 #include "chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h"
 #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"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/point.h"
 #include "ui/gfx/range/range.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/text_constants.h"
 
 using autofill::AutofillPopupView;
+using autofill::PasswordGenerationPopupController;
 using autofill::PasswordGenerationPopupView;
 using base::scoped_nsobject;
 
 namespace {
 
+const CGFloat kDividerWidth = 1;
+const CGFloat kPasswordSectionVerticalSeparation = 5;

[Ilya Sherman, 2014/08/19 22:32:17]

nit: Please document these, including units.

[dconnelly, 2014/08/20 17:07:06]

Done.

+
+CGFloat MidY(NSRect frame) {
+  return (NSMaxY(frame) + NSMinY(frame)) / 2.0;

[Ilya Sherman, 2014/08/19 22:32:17]

Any particular reason not to just use NSMidY?

[dconnelly, 2014/08/20 17:07:06]

Amusingly, I googled for it and only found crap, because apparently the
Foundation docs for it weren't in the top half of the results, and so I assumed
that no such function existed. Done.

+}
+
 NSColor* DividerColor() {
   return gfx::SkColorToCalibratedNSColor(
       PasswordGenerationPopupView::kDividerColor);
 }
 
 NSColor* HelpTextBackgroundColor() {
   return gfx::SkColorToCalibratedNSColor(
       PasswordGenerationPopupView::kExplanatoryTextBackgroundColor);
 }
 
@@ skipping 18 matching lines @@
 }
 
 - (id)initWithController:
     (autofill::PasswordGenerationPopupController*)controller
                    frame:(NSRect)frame {
   if (self = [super initWithDelegate:controller frame:frame]) {
     controller_ = controller;
 
     passwordField_ = [self textFieldWithText:controller_->password()
                                        color:[self nameColor]
+                                        font:[self textFont]
                                    alignment:NSLeftTextAlignment];
     [self addSubview:passwordField_];
 
-    passwordSubtextField_ = [self textFieldWithText:controller_->SuggestedText()
-                                              color:[self subtextColor]
-                                          alignment:NSRightTextAlignment];
-    [self addSubview:passwordSubtextField_];
+    passwordTitleField_ = [self textFieldWithText:controller_->SuggestedText()
+                                            color:[self nameColor]
+                                             font:[self boldFont]
+                                        alignment:NSLeftTextAlignment];
+    [self addSubview:passwordTitleField_];
+
+    keyIcon_ = ResourceBundle::GetSharedInstance()

[groby-ooo-7-16, 2014/08/19 23:32:18]

There's nothing retaining keyIcon_ - it can (and will) be autoreleased. I
suggest scoped_nsobject.

[dconnelly, 2014/08/20 17:07:05]

Done.

+        .GetImageNamed(IDR_GENERATE_PASSWORD_KEY)
+        .ToNSImage();
 
     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];
+    // Remove the underlining.
     [[helpTextView textContainer] setLineFragmentPadding:0.0f];

[Ilya Sherman, 2014/08/19 22:32:17]

Is this still needed, in addition to the code you added below?

[dconnelly, 2014/08/20 17:07:06]

Done.

     [helpTextView setVerticallyResizable:YES];
+    [helpTextView setLinkTextAttributes:nil];

[groby-ooo-7-16, 2014/08/19 23:32:18]

setLinkTextAttributes:nil does nothing for HyperlinkTextView

[dconnelly, 2014/08/20 17:07:06]

Done.

+    NSTextStorage* text = [helpTextView textStorage];
+    [text addAttribute:NSUnderlineStyleAttributeName
+                 value:[NSNumber numberWithInt:NSUnderlineStyleNone]
+                 range:controller_->HelpTextLinkRange().ToNSRange()];

[groby-ooo-7-16, 2014/08/19 23:32:18]

Is this cleared with UI review? HyperlinkTextView is meant to encapsulate a
Chrome-wide style for links used in UI text.

[dconnelly, 2014/08/20 17:07:06]

This came from UI mocks:
https://code.google.com/p/chromium/issues/detail?id=318977#c35

There are many examples of non-underlined links in UI on both Views and Cocoa
platforms. For example, the "sign in" link in the Bookmarks bubble. Not sure why
that's not the default.

     [self addSubview:helpTextView];
     helpTextView_ = helpTextView.get();

[groby-ooo-7-16, 2014/08/19 23:32:18]

Why? Why not make helpTextView_ a scoped_nsobject?

[dconnelly, 2014/08/20 17:07:06]

Agreed. I've now converted all the __weak pointer properties in the class to
scoped_nsobjects. Additionally: I don't think adding __weak does anything in our
codebase.

   }
 
   return self;
 }
 
 #pragma mark NSView implementation:
 
 - (void)drawRect:(NSRect)dirtyRect {

[groby-ooo-7-16, 2014/08/19 23:32:18]

This is missing a call to [super drawRect:] - intentional?

[dconnelly, 2014/08/20 17:07:06]

I don't know; this is inherited code. I can't tell that adding it breaks
anything, so I've added it.

   // If the view is in the process of being destroyed, don't bother drawing.
   if (!controller_)
     return;
 
   [self drawBackgroundAndBorder];
 
+  const CGFloat width =
+      NSWidth(NSRectFromCGRect(controller_->popup_bounds().ToCGRect()));
+
   if (controller_->password_selected()) {
     // Draw a highlight under the suggested password.
-    NSRect highlightBounds = [self passwordBounds];
+    NSRect highlightBounds = [self passwordSectionBoundsForWidth:width];
     [[self highlightColor] set];
     [NSBezierPath fillRect:highlightBounds];
   }
 
   // Render the background of the help text.
   [HelpTextBackgroundColor() set];
-  [NSBezierPath fillRect:[self helpBounds]];
+  [NSBezierPath fillRect:[self helpBoundsForWidth:width]];

[groby-ooo-7-16, 2014/08/19 23:32:18]

Why not just set a background color on the help text?

[dconnelly, 2014/08/20 17:07:06]

Done.

 
   // Render the divider.
   [DividerColor() set];

[groby-ooo-7-16, 2014/08/19 23:32:17]

Usually, you draw dividers by just having an NSBox of type NSBoxSeparator.

[dconnelly, 2014/08/20 17:07:06]

You can't change its color that way. I've replaced this with an NSBox modeled
after the BookmarkSyncPromoController's separator.

-  [NSBezierPath fillRect:[self dividerBounds]];
+  [NSBezierPath fillRect:[self dividerBoundsForWidth:width]];
+
+  // Render the key icon.
+  [keyIcon_ drawInRect:[self iconBoundsForWidth:width]

[groby-ooo-7-16, 2014/08/19 23:32:18]

Can you use an NSImageView instead?

[dconnelly, 2014/08/20 17:07:05]

Done.

+              fromRect:{ NSZeroPoint, [keyIcon_ size] }

[groby-ooo-7-16, 2014/08/19 23:32:18]

Passing in NSZeroRect automatically specifies the whole image - no need to build
your own rect :)

[dconnelly, 2014/08/20 17:07:05]

Done.

+             operation:NSCompositeSourceOver
+              fraction:1.0
+        respectFlipped:YES
+                 hints:nil];
 }
 
 #pragma mark Public API:
 
+- (NSSize)preferredSize {
+  const CGFloat popupWidth = controller_->GetMinimumWidth();
+  NSRect helpBounds = [self helpBoundsForWidth:popupWidth];
+  CGFloat height =
+      (controller_->display_password()
+           ? NSHeight([self passwordSectionBoundsForWidth:popupWidth])
+           : 0) +
+      NSHeight([self dividerBoundsForWidth:popupWidth]) + NSHeight(helpBounds) +
+      2 * autofill::kPopupBorderThickness;
+  return NSMakeSize(popupWidth + 2 * autofill::kPopupBorderThickness, height);
+}
+
+- (void)updatePassword {
+  [self setText:controller_->password()
+          color:[self nameColor]
+           font:[self textFont]
+      alignment:NSLeftTextAlignment
+       forField:passwordField_];

[Ilya Sherman, 2014/08/19 22:32:17]

Is it not possible to update the text without needing to redefine all of these
attributes?

[dconnelly, 2014/08/20 17:07:05]

I thought not, but I found a way. Done.

+}
+
 - (void)updateBoundsAndRedrawPopup {
-  [self positionView:passwordField_ inRect:[self passwordBounds]];
-  [self positionView:passwordSubtextField_ inRect:[self passwordBounds]];
-  [self positionView:helpTextView_ inRect:[self helpBounds]];
+  const CGFloat width =
+      NSWidth(NSRectFromCGRect(controller_->popup_bounds().ToCGRect()));

[groby-ooo-7-16, 2014/08/19 23:32:18]

Just width = controller->popup_bounds().width()

Neither ToCGRect nor NSRectFromCGRect touch width.

[dconnelly, 2014/08/20 17:07:05]

Done.

+  // The password can change while the bubble is shown: if the user has

[Ilya Sherman, 2014/08/19 22:32:17]

nit: "if" -> "If"

[dconnelly, 2014/08/20 17:07:06]

TIL. Done.

+  // 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];

[Ilya Sherman, 2014/08/19 22:32:17]

nit: Can this be moved into the if-stmt below?

[dconnelly, 2014/08/20 17:07:05]

Done.

+  if (controller_->display_password()) {
+    [self positionView:passwordTitleField_
+                inRect:[self passwordSectionTitleBoundsForWidth:width]];
+    [self positionView:passwordField_
+                inRect:[self passwordBoundsForWidth:width]];
+  }
+  [self positionView:helpTextView_ inRect:[self helpBoundsForWidth:width]];
+  [super updateBoundsAndRedrawPopup];
+}
 
-  [super updateBoundsAndRedrawPopup];
+- (BOOL)isPointInPasswordBounds:(CGPoint)point {
+  const CGFloat width =
+      NSWidth(NSRectFromCGRect(controller_->popup_bounds().ToCGRect()));
+  return CGRectContainsPoint(
+      NSRectToCGRect([self passwordSectionBoundsForWidth:width]), point);

[Ilya Sherman, 2014/08/19 22:32:17]

Can you use NSPointInRect instead?  (In general, NS types and functions are
preferred to CG ones).

[dconnelly, 2014/08/20 17:07:05]

Done.

 }
 
 - (void)controllerDestroyed {
   controller_ = NULL;
   [super delegateDestroyed];
 }
 
 #pragma mark NSTextViewDelegate implementation:
 
 - (BOOL)textView:(NSTextView*)textView
    clickedOnLink:(id)link
          atIndex:(NSUInteger)charIndex {
   controller_->OnSavedPasswordsLinkClicked();
   return YES;
 }
 
 #pragma mark Private helpers:
 
-- (NSTextField*)textFieldWithText:(const base::string16&)text
-                            color:(NSColor*)color
-                        alignment:(NSTextAlignment)alignment {
+- (void)setText:(const base::string16&)text
+          color:(NSColor*)color
+           font:(NSFont*)font
+      alignment:(NSTextAlignment)alignment
+       forField:(NSTextField*)field {

[groby-ooo-7-16, 2014/08/19 23:32:17]

This seem overkill, but I'm not sure how many different text styles you use.
Ideally, just define the styles (as dictionaries) and use as needed.

(If there are many different combinations of color/font/alignment, I suggest
pinging UI peeps)

[dconnelly, 2014/08/20 17:07:06]

Done.

   scoped_nsobject<NSMutableParagraphStyle> paragraphStyle(
       [[NSMutableParagraphStyle alloc] init]);
   [paragraphStyle setAlignment:alignment];
 
   NSDictionary* textAttributes = @{
-    NSFontAttributeName : [self textFont],
+    NSFontAttributeName : font,
     NSForegroundColorAttributeName : color,
     NSParagraphStyleAttributeName : paragraphStyle
   };
 
   scoped_nsobject<NSAttributedString> attributedString(
       [[NSAttributedString alloc]
           initWithString:base::SysUTF16ToNSString(text)
               attributes:textAttributes]);
 
+  [field setAttributedStringValue:attributedString];
+}
+
+- (NSTextField*)textFieldWithText:(const base::string16&)text
+                            color:(NSColor*)color
+                             font:(NSFont*)font
+                        alignment:(NSTextAlignment)alignment {
   NSTextField* textField =
       [[[NSTextField alloc] initWithFrame:NSZeroRect] autorelease];
-  [textField setAttributedStringValue:attributedString];
+  [self setText:text
+          color:color
+           font:font
+      alignment:alignment
+       forField:textField];
   [textField setEditable:NO];
   [textField setSelectable:NO];
   [textField setDrawsBackground:NO];
   [textField setBezeled:NO];
   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)passwordSectionBoundsForWidth:(CGFloat)width {

[groby-ooo-7-16, 2014/08/19 23:32:18]

Using bounds in the name is confusing. Usually, [self bounds] is the local
coordinate system - if the password section were a view, this would be a rect.
I.e. passwordRectForWidth:

[dconnelly, 2014/08/20 17:07:06]

I think all this is confusing, so I've removed these methods and replaced both
preferredSize and updateBoundsAndRedrawPopup with what I think are more
straightforward implementations.

+  if (!controller_->display_password())
+    return NSZeroRect;
+  return NSMakeRect(
+      autofill::kPopupBorderThickness,
+      autofill::kPopupBorderThickness,
+      width - 2 * autofill::kPopupBorderThickness,
+      PasswordGenerationPopupController::kPopupPasswordSectionHeight);
 }
 
-- (NSRect)helpBounds {
-  return NSZeroRect;
+- (NSSize)sizeOfSingleLineString:(const base::string16&)string
+                        withFont:(NSFont*)font {
+  NSRect textFrame = [base::SysUTF16ToNSString(string)
+      boundingRectWithSize:NSMakeSize(MAXFLOAT, MAXFLOAT)
+                   options:0
+                attributes:@{ NSFontAttributeName : font }];
+  return textFrame.size;

[Ilya Sherman, 2014/08/19 22:32:17]

I feel like this code must already exist somewhere.  Does
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica...:
do what you need?

[dconnelly, 2014/08/20 17:07:06]

Done.

 }
 
-- (NSRect)dividerBounds {
-  return NSZeroRect;
+- (NSRect)passwordSectionTitleBoundsForWidth:(CGFloat)width {
+  if (!controller_->display_password())
+    return NSZeroRect;
+  NSRect passwordSectionBounds = [self passwordSectionBoundsForWidth:width];
+  NSSize passwordSize = [self sizeOfSingleLineString:controller_->password()
+                                            withFont:[self boldFont]];
+  const CGFloat y = MidY(passwordSectionBounds) -
+                    (kPasswordSectionVerticalSeparation / 2.0) -

[groby-ooo-7-16, 2014/08/19 23:32:18]

Be careful with rounding. Currently, kPasswordSectionVerticalSeparation is odd,
so chance are y might not be an integer coordinate. If you position controls
using non-integer coordinates, you get a blurred view. I'd suggest
std::ceil/std::floor

[dconnelly, 2014/08/20 17:07:05]

Done.

+                    passwordSize.height;

[groby-ooo-7-16, 2014/08/19 23:32:18]

You seem to just care about the height of the text - [NSFont defaultLineHeight]
might be a better choice.

[dconnelly, 2014/08/20 17:07:06]

It's deprecated. I think this should be fine, as the field fits the text.

+  const CGFloat x = NSMaxX([self iconBoundsForWidth:width]);
+  return NSMakeRect(
+      x, y, NSWidth(passwordSectionBounds) - x, passwordSize.height);
+}
+
+- (NSRect)passwordBoundsForWidth:(CGFloat)width {
+  NSRect passwordSectionBounds = [self passwordSectionBoundsForWidth:width];
+  NSSize titleSize = [self sizeOfSingleLineString:controller_->SuggestedText()
+                                         withFont:[self textFont]];
+  const CGFloat y = MidY(passwordSectionBounds) +
+                    (kPasswordSectionVerticalSeparation / 2.0);
+  const CGFloat x = NSMaxX([self iconBoundsForWidth:width]);
+  return NSMakeRect(x, y, NSWidth(passwordSectionBounds) - x, titleSize.height);
+}
+
+- (NSRect)dividerBoundsForWidth:(CGFloat)width {
+  if (!controller_->display_password())
+    return NSZeroRect;
+  return NSMakeRect(autofill::kPopupBorderThickness,
+                    NSMaxY([self passwordSectionBoundsForWidth:width]),
+                    width - 2 * autofill::kPopupBorderThickness,
+                    kDividerWidth);

[groby-ooo-7-16, 2014/08/19 23:32:18]

Why is the last param kDividerWidth? Shouldn't that be height?

[dconnelly, 2014/08/20 17:07:06]

Poor naming. I was thinking of the line as having length and width, not width
and height. Fixed.

+}
+
+- (NSRect)helpBoundsForWidth:(CGFloat)width {
+  NSSize size =
+      NSMakeSize(width - 2 * autofill::kPopupBorderThickness, MAXFLOAT);
+  NSStringDrawingOptions options =
+      NSLineBreakByWordWrapping | NSStringDrawingUsesLineFragmentOrigin;
+  NSDictionary* attributes = @{ NSFontAttributeName : [self textFont] };
+  NSRect textFrame = [base::SysUTF16ToNSString(controller_->HelpText())
+      boundingRectWithSize:size
+                   options:options
+                attributes:attributes];

[groby-ooo-7-16, 2014/08/19 23:32:18]

Ideally, you don't want to compute field sizes at all, but just use sizeToFit
and let it figure out the perfect size. Would that approach work here?

[dconnelly, 2014/08/20 17:07:05]

Done.

+  const int verticalPadding =
+      autofill::PasswordGenerationPopupController::kHelpVerticalPadding;
+  size.height = NSHeight(textFrame) + 2 * verticalPadding;
+
+  // The help section falls immediately above and to the right of the border.
+  NSPoint origin = NSMakePoint(autofill::kPopupBorderThickness,
+                               controller_->display_password()
+                                   ? NSMaxY([self dividerBoundsForWidth:width])
+                                   : autofill::kPopupBorderThickness);
+
+  return { origin, size };
+}
+
+- (NSRect)iconBoundsForWidth:(CGFloat)width {
+  if (!controller_->display_password())
+    return NSZeroRect;
+  NSRect passwordSectionBounds = [self passwordSectionBoundsForWidth:width];
+  CGFloat passwordMidY = MidY(passwordSectionBounds);
+  NSSize iconSize = [keyIcon_ size];
+  return NSMakeRect(PasswordGenerationPopupController::kHorizontalPadding,
+                    passwordMidY - (iconSize.height / 2.0),
+                    iconSize.width,
+                    iconSize.height);
+}
+
+- (NSFont*)boldFont {
+  return ResourceBundle::GetSharedInstance()

[groby-ooo-7-16, 2014/08/19 23:32:18]

Fonts grabbed with DeriveWithStyle don't always look native. Are none of the
system fonts suitable?

[dconnelly, 2014/08/20 17:07:06]

Done.

+      .GetFontList(ResourceBundle::SmallFont)
+      .DeriveWithStyle(gfx::Font::BOLD)
+      .GetPrimaryFont()
+      .GetNativeFont();
 }
 
 - (NSFont*)textFont {
   return ResourceBundle::GetSharedInstance().GetFontList(
       ResourceBundle::SmallFont).GetPrimaryFont().GetNativeFont();

[Ilya Sherman, 2014/08/19 22:32:17]

nit: Perhaps format this to match the formatting above, so that a visual diff is
easier.

[dconnelly, 2014/08/20 17:07:05]

Done.

 }
 
 @end