Add mac UI for password generation

chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm

Index: chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
diff --git a/chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm b/chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
new file mode 100644
index 0000000000000000000000000000000000000000..b881de59bc667eefc4496addd82d6138c96b1509
--- /dev/null
+++ b/chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
@@ -0,0 +1,295 @@
+// 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.
+
+#include <cmath>
+
+#import "chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.h"
+
+#include "base/sys_string_conversions.h"
+#include "chrome/common/autofill_messages.h"
+#include "chrome/browser/autofill/password_generator.h"
+#include "chrome/browser/password_manager/password_manager.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
+#import "chrome/browser/ui/cocoa/info_bubble_view.h"
+#import "chrome/browser/ui/cocoa/info_bubble_window.h"
+#import "chrome/browser/ui/cocoa/styled_text_field_cell.h"
+#import "chrome/browser/ui/cocoa/tracking_area.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/common/password_form.h"
+#include "grit/generated_resources.h"
+#include "grit/theme_resources.h"
+#include "ui/base/resource/resource_bundle.h"
+
+namespace {

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

Line of whitespace before each comment and between kIconOffset and the namespace
close.  Though if you're only using these once, consider just putting them in a
nearby scope.

[Garrett Casto, 2012/11/19 22:17:42]

Done.

[Garrett Casto, 2012/11/19 22:17:42]

Done.

+// We make the icon smaller by this decrease some of the whitespace.
+const int kIconWidthInset = 3;
+// This helps center the image to deal with the fact that there is a border
+// on the right side of the textfield.
+const int kIconOffset = 2;
+}  // namespace
+
+// Customized StyledTextFieldCell to display one button decoration that changes
+// on hover.
+@interface PasswordGenerationTextFieldCell : StyledTextFieldCell {
+ @private
+  PasswordGenerationBubbleController* controller_;
+  BOOL hovering_;
+  scoped_nsobject<NSImage> normalImage_;
+  scoped_nsobject<NSImage> hoverImage_;
+}
+
+- (void)setUpWithController:(PasswordGenerationBubbleController*)controller
+                normalImage:(NSImage*)normalImage
+                 hoverImage:(NSImage*)hoverImage;
+- (void)mouseEntered:(NSEvent*)theEvent
+              inView:(PasswordGenerationTextField*)controlView;
+- (void)mouseExited:(NSEvent*)theEvent
+             inView:(PasswordGenerationTextField*)controlView;
+- (BOOL)mouseDown:(NSEvent*)theEvent
+           inView:(PasswordGenerationTextField*)controlView;
+- (void)setUpTrackingAreaInRect:(NSRect)frame
+                         ofView:(PasswordGenerationTextField*)controlView;
+@end
+
+@implementation PasswordGenerationTextField
+
++ (Class)cellClass {
+  return [PasswordGenerationTextFieldCell class];
+}
+
+- (id)initWithFrame:(NSRect)frame
+     withController:(PasswordGenerationBubbleController*)controller
+        normalImage:(NSImage*)normalImage
+         hoverImage:(NSImage*)hoverImage {
+  self = [super initWithFrame:frame];
+  if (self) {
+    DCHECK([[self cell] isKindOfClass:[PasswordGenerationTextFieldCell class]]);
+    [[self cell] setUpWithController:controller
+                         normalImage:normalImage
+                          hoverImage:hoverImage];
+    [[self cell] setLineBreakMode:NSLineBreakByTruncatingTail];
+    [[self cell] setTruncatesLastVisibleLine:YES];

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

These two look like attributes of the cell, not dynamic optional things, so they
probably belong in an override of the cell's -init*, whatever that is.

[Garrett Casto, 2012/11/19 22:17:42]

AutocompleteTextField does this in awakeFromNib, but that doesn't work here
since there is no Nib. NSCell has 4 designated initializers and overriding all
of them seems unfortunate. I could probably just get away with overriding
initTextCell. Thoughts?

[Scott Hess - ex-Googler, 2012/11/19 23:20:35]

Generally the rule is that less-specific init methods forward to more-specific
init methods.

That said ... I'm feeling mixed on this.  You already have a -setUp* method,
push them there.

[Garrett Casto, 2012/11/27 07:59:49]

Done.

+    [[self cell] setUpTrackingAreaInRect:[self bounds] ofView:self];

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

In Objective-C, all of the [self cell] are dynamic lookups, the compiler cannot
cache them.  So in a case like this, probably worth a local variable to store it
once.  You could use the objc-cast to get it to your custom subclass, or another
trick is to implement - (PGTFC*)cell to do the conversion.  As long as the
subclass method is a more specific implementation of what the superclass method
returns, it's alright.

AutocompleteTextField does this for -cell, but it uses an older casting style,
there's something newer in base/mac/util or somewhere like that.

[Garrett Casto, 2012/11/19 22:17:42]

I didn't notice anything obvious in base/mac/... I switched it over to using a
loca variable.

[Scott Hess - ex-Googler, 2012/11/19 23:20:35]

Basically like:

- (PGTFC*)cell {
  return base::mac::ObjCCastStrict<PGTFC*>([super cell]);
}

then your calls to [self cell] will be of the right type.  This is a problem all
below, because you're calling methods that you're defining.

[Garrett Casto, 2012/11/27 07:59:49]

Done, though as someone new to objective C, can you explain what exactly this
does? Since

PasswordGenerationTextFieldCell* cell = [self cell];

compiles already clearly there isn't strict type checking going on. Does this
make the code faster, allow for better error handling, etc?

[sail, 2012/11/28 06:47:16]

The reason there isn't strict type checking here is because -[NSControl cell] is
declared like this:
- (id)cell;
id tell you that it's a Objective-C object but nothing else. All checks are done
at runtime. It's done this way because there are lots of different cell sub
classes.

In Chrome we like stricter type checking where possible.

+  }
+  return self;
+}
+
+- (void)mouseEntered:(NSEvent*)theEvent {
+  [[self cell] mouseEntered:theEvent inView:self];
+}
+
+- (void)mouseExited:(NSEvent*)theEvent {
+  [[self cell] mouseExited:theEvent inView:self];
+}
+
+- (void)mouseDown:(NSEvent*)theEvent {
+  // Let the cell handle the click if it's in the decoration.
+  if (![[self cell] mouseDown:theEvent inView:self]) {
+    [[self currentEditor] mouseDown:theEvent];
+  }
+}
+
+@end
+
+@implementation PasswordGenerationTextFieldCell
+
+- (void)setUpWithController:(PasswordGenerationBubbleController*)controller
+                normalImage:(NSImage*)normalImage
+                 hoverImage:(NSImage*)hoverImage {
+  controller_ = controller;
+  hovering_ = NO;
+  normalImage_.reset([normalImage retain]);
+  hoverImage_.reset([hoverImage retain]);
+}
+
+- (NSRect)getFrameForIcon:(NSRect*)cellFrame {

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

In all but one case, you use either the return value, or the return-by-reference
value.  I think that it would be better to have one helper for the icon frame,
another for the non-icon frame, and use the specific one as appropriate.

[Garrett Casto, 2012/11/19 22:17:42]

Done.

+  NSRect iconFrame;
+  NSDivideRect(*cellFrame, &iconFrame, cellFrame,
+               [normalImage_ size].width - kIconWidthInset, NSMaxXEdge);

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

I'm not entirely following how you're using this.  Rather than manually reducing
the whitespace by insetting, why not just not have an icon without a bunch of
whitespace in the first place?

[Garrett Casto, 2012/11/19 22:17:42]

I'm using the same icon on both Windows and Mac (actually the same icon used for
the reload button on the location bar) and the spacing is fine on Windows. I
could crop the icon differently for Mac, but then we would have multiple icons
that are basically exactly the same, which seems silly.

The other possibility would be to change the spacing on Mac so that it's closer
to Windows, and then I probably wouldn't have to worry about this. The issue
there that I want the textfield and the button to line up but the style of
button I want to use doesn't get any larger than 25px tall (at least it doesn't
seem to).   

[Scott Hess - ex-Googler, 2012/11/19 23:20:35]

Still not sure I understand where you're going, here.  My read on this is that
here, you peel off a slice for the image, leaving the rest for the text.  Later,
you inset the rect by a negative amount to undo this, which implies that your
text area potentially overlaps your icon area.

Maybe it's because you're sharing this calculation in places that mean different
things.  For instance, if you "simply" drew the icon flush right vertically
centered, and used a separate sub-rect calculation (like this one) as the active
area for mouse-tracking and hit-checking, that would make more sense.

[Garrett Casto, 2012/11/27 07:59:49]

I changed this to just crop the image, which hopefully makes more sense.

[Scott Hess - ex-Googler, 2012/11/28 20:01:05]

Sounds like it should.

+  return iconFrame;
+}
+
+- (BOOL)eventIsInDecoration:(NSEvent*)theEvent
+                     inView:(PasswordGenerationTextField*)controlView {
+  NSPoint mouseLocation = [controlView convertPoint:[theEvent locationInWindow]
+                                           fromView:nil];
+  NSRect cellFrame = [controlView bounds];
+  return NSMouseInRect(mouseLocation,
+                       [self getFrameForIcon:&cellFrame],
+                       [controlView isFlipped]);
+}
+
+- (void)mouseEntered:(NSEvent*)theEvent
+              inView:(PasswordGenerationTextField*)controlView {
+  hovering_ = YES;
+  [controlView setNeedsDisplay:YES];
+}
+
+- (void)mouseExited:(NSEvent*)theEvent
+             inView:(PasswordGenerationTextField*)controlView {
+  hovering_ = NO;
+  [controlView setNeedsDisplay:YES];
+}
+
+- (BOOL)mouseDown:(NSEvent*)theEvent
+           inView:(PasswordGenerationTextField*)controlView {
+  if ([self eventIsInDecoration:theEvent
+                         inView:controlView]) {

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

This shouldn't need to wrap.

[Garrett Casto, 2012/11/19 22:17:42]

Done.

+    [controller_ regeneratePassword];
+    return YES;
+  }
+  return NO;
+}
+
+- (NSImage*)getImage {
+  if (hovering_)
+    return hoverImage_;
+  return normalImage_;
+}
+
+- (NSRect)adjustFrameForFrame:(NSRect)frame {
+  // By default, there appears to be a 2 pixel gap between what is considered
+  // part of the textFrame and what is considered part of the icon. I'm guessing
+  // it has to do with inclusivity of the end points.
+  return NSMakeRect(NSMinX(frame),
+                    NSMinY(frame),
+                    NSWidth(frame) + 2,
+                    NSHeight(frame));

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

This seems sketchy.  Is there something drawing incorrectly?

Also, frame.size.width += 2.

[Garrett Casto, 2012/11/19 22:17:42]

Looking at this some more, it looks like this might be an offset issue. A click
slightly to the left of the textfield is triggering a call to -mouseDown as
well. You can actually see this on the location bar as well (which also uses
StyledTextFieldCell). If you click one pixel to the left of the bar, it will
still highlight text. It actually looks like the location bar suffers from the
dead zone between the text field and the decoration as wel. So maybe this just
isn't that big of a deal? It does seem strange to me though.

The two functions are used for -resetCursorRect and -drawingRectForBounds. The
documentation for these functions is not very detailed. If you have any idea how
this offset is being introduced, I'd love to hear it.
Done.

[Scott Hess - ex-Googler, 2012/11/28 20:01:05]

I'm not sure I see the issue you mention clicking just-left or just-right of the
bar.  I only get focus if I click on the border or within.  Keep in mind that
the black tip of the arrow is the operational point, not the white border.

Are you familiar with the control's editor?  This is a text view which is placed
as a subview while the control has focus.  Clicks within the subview are
captured by the editor, clicks outside go to the control, which can cause odd
things.  It's important to make sure that clicks when the field doesn't have
focus do a sensible thing compared to what happens when it has focus.  There's
also a two-pixel border on the left and right margins of the scrollable area
within the editor, which can cause problems if you're trying to position things
too closely to neighbors.  There's probably a bug somewhere about that, because
it's a minor issue in the location bar, but it's hard to fix without adding a
fair bit of non-obvious code.

+}
+
+- (NSRect)textFrameForFrame:(NSRect)cellFrame {
+  // Baseclass insets the rect by baselineAdjust.
+  NSRect textFrame = [super textFrameForFrame:cellFrame];
+  [self getFrameForIcon:&textFrame];
+  return [self adjustFrameForFrame:textFrame];
+}
+
+- (NSRect)textCursorFrameForFrame:(NSRect)cellFrame {
+  [self getFrameForIcon:&cellFrame];
+  return [self adjustFrameForFrame:cellFrame];
+}
+
+- (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
+  NSImage* image = [self getImage];
+  NSRect iconFrame = [self getFrameForIcon:&cellFrame];
+  // Center the image in the available space
+  const CGFloat x_inset =
+      std::floor((NSWidth(iconFrame) - [image size].width) / 2.0);
+  const CGFloat y_inset =
+      std::floor((NSHeight(iconFrame) - [image size].height) / 2.0);
+  NSRect centeredFrame = NSInsetRect(iconFrame, x_inset, y_inset);

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

This all might be cleaner using NSMidX(), NSMidY(), and -drawAtPoint:.  Like you
could do x as std::floor(NSMidX(iconFrame) - [image size].width/2) -
kIconOffset, etc.  I do kind of wish we had a utility function to wrap all of
this size-delta-divide-2 junk.

[Garrett Casto, 2012/11/19 22:17:42]

Personally I find the current implementation slightly easier to read than what I
think your suggesting for NSMidX and NSMidY. Not sure what you mean by the
reference to -drawAtPoint.

[Scott Hess - ex-Googler, 2012/11/19 23:20:35]

NSImage can draw at a point, instead of in a rect.  Then you don't have to worry
about accidentally scaling the image.

+
+  // Shift the image over to help even out the white space on both sides (since
+  // there is a border taking up some space on the right).
+  [image drawInRect:NSOffsetRect(centeredFrame, -kIconOffset, 0)
+           fromRect:NSZeroRect  // Entire image
+          operation:NSCompositeSourceOver
+           fraction:1.0
+     respectFlipped:YES
+              hints:nil];
+
+  [super drawInteriorWithFrame:cellFrame inView:controlView];
+}
+
+-(void) setUpTrackingAreaInRect:(NSRect)frame

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

space before (void), not after.

[Garrett Casto, 2012/11/19 22:17:42]

Done.

+                         ofView:(PasswordGenerationTextField*)view {
+  NSRect iconFrame = [self getFrameForIcon:&frame];
+  scoped_nsobject<CrTrackingArea> area(
+      [[CrTrackingArea alloc] initWithRect:iconFrame
+                                   options:NSTrackingMouseEnteredAndExited |
+                                           NSTrackingActiveAlways
+                                     owner:view
+                                  userInfo:nil]);
+  [view addTrackingArea:area];
+}
+
+- (CGFloat)baselineAdjust {
+  return 1.0;
+}
+
+- (CGFloat)cornerRadius {
+  return 4.0;
+}
+
+- (BOOL)shouldDrawBezel {
+  return YES;
+}
+
+@end
+
+@implementation PasswordGenerationBubbleController
+
+- (id)initWithBrowser:(Browser*)parentBrowser
+           anchoredAt:(NSPoint)point
+       renderViewHost:(content::RenderViewHost*)renderViewHost
+      passwordManager:(PasswordManager*)passwordManager
+       usingGenerator:(autofill::PasswordGenerator*)passwordGenerator
+              forForm:(const content::PasswordForm&)form {
+  NSRect contentRect = NSMakeRect(0, 0, 260, 71);

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

That seems pretty unlikely to me.

[Garrett Casto, 2012/11/19 22:17:42]

?  An issue with hard coding number?

[Scott Hess - ex-Googler, 2012/11/19 23:20:35]

Yeah.  I have no idea what it means.  I can see why you might need to have
something other than NSZeroRecto, but 260x71?  Those are pretty precise numbers
to be given with no justification for their values.

+  scoped_nsobject<InfoBubbleWindow> window(
+      [[InfoBubbleWindow alloc] initWithContentRect:contentRect
+                                          styleMask:NSBorderlessWindowMask
+                                            backing:NSBackingStoreBuffered
+                                              defer:NO]);
+  if (self = [super initWithWindow:window
+                      parentWindow:parentBrowser->window()->GetNativeWindow()
+                        anchoredAt:point]) {
+    passwordGenerator_ = passwordGenerator;
+    renderViewHost_ = renderViewHost;
+    passwordManager_ = passwordManager;
+    form_ = form;
+    [[self bubble] setArrowLocation:info_bubble::kTopLeft];
+  }
+
+  [self performLayout];

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

Should be in the if() clause.

[Garrett Casto, 2012/11/19 22:17:42]

Done.

+  return self;
+}
+
+- (void)performLayout {
+  NSView* contentView = [[self window] contentView];
+  ResourceBundle& rb = ResourceBundle::GetSharedInstance();
+
+  textField_ =
+      [[PasswordGenerationTextField alloc]
+         initWithFrame:NSMakeRect(9, 9, 172, 23)
+        withController:self
+           normalImage:rb.GetNativeImageNamed(IDR_RELOAD_DIMMED).ToNSImage()
+            hoverImage:rb.GetNativeImageNamed(IDR_RELOAD).ToNSImage()];
+  [textField_ setFont:[NSFont boldSystemFontOfSize:11]];

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

This seems like it should be using some sort of gfx::Font thing for a base.  I
don't recall that stuff offhand, maybe it's always wrong.

[Garrett Casto, 2012/11/19 22:17:42]

Done.

+  [textField_
+    setStringValue:base::SysUTF8ToNSString(passwordGenerator_->Generate())];
+  [contentView addSubview:textField_];

[Scott Hess - ex-Googler, 2012/11/19 23:20:35]

Is this field still editable?  If so, do you want [return] in this field to send
the action, too?  Often/usually you'd have the button be the target and the
action be -performClick:, so that hitting [return] causes the button to show a
click.

[Garrett Casto, 2012/11/27 07:59:49]

I think that I like this better than hooking up the button with setKeyEquivalent
as that makes the button pulse the entire time. However I'm not sure how to
catch the return key press here. Do I have to overwrite -keyDown, or is there
some other way to hook this?

[Scott Hess - ex-Googler, 2012/11/28 20:01:05]

nack, that's not what I meant.  I meant:
 [textField_ setTarget:button];
 [textField_ setAction:@selector(performClick:)];
of course, you'd have to put this down further.

+
+  NSButton* button = [[NSButton alloc]
+                       initWithFrame:NSMakeRect(181, 6, 76, 25)];
+  [button setBezelStyle:NSRoundedBezelStyle];
+  [button setTitle:@"Try it"];
+  [button setTarget:self];
+  [button setAction:@selector(fillPassword:)];
+  [contentView addSubview:button];
+
+  NSTextField* title = [[NSTextField alloc]
+                         initWithFrame:NSMakeRect(8, 42, 138, 17)];
+  [title setEditable:NO];
+  [title setBordered:NO];
+  [title setStringValue:@"Password Suggestion"];
+  [contentView addSubview:title];
+}

[Scott Hess - ex-Googler, 2012/11/17 01:22:41]

Sorry, but all of this hard-coded positioning is just not kosher.  I'm not
averse to alternatives to using nib files for layout, but I don't think just
ad-hoc arbitrary numbers is going to fly.

[Garrett Casto, 2012/11/19 22:17:42]

So what's the right way of doing this then? I want to express something like, 
the textfield and the button should line up vertically. In IB you just drag into
the appropriate space, but if you do the layout manually there are borders that
make this hard (as you can see the button and textfield have different sizes and
y offsets, even though they line up when drawn). If I could ask an NSButton for
what it's visible size I should be able to make this more general, but I'm not
sure how to do that.

I can switch to using nibs if I have to, though I'd sort of prefer not to at
this point. For reference I started doing this programmatically after asking
dhollowa about which was preferred and it sounded like people generally leaned
towards programatic at this point for simple UI's (which I think this qualifies
for).

[Scott Hess - ex-Googler, 2012/11/19 23:20:35]

At minimum, you could be laying things out relative to each other.  So the
textField_ is somewhere in the lower left, the button is N pixels right of that
field, perhaps with an offset (or inset around the horizontal center line,
perhaps?).  The label could be N pixels above the field, again perhaps with an
offset.  Then if someone needs to shift things around, they aren't having to
shift in multiple places.  Also, it makes it clearer which field the button is
right of and which field the label is above.

Slightly different might be to make it relative to the containing content view. 
Say, the button all the way right, the main field taking up everything else in
that horizontal section, and the label at the top of the window and the same
width as the content view.

Beyond that - where's your localization support?

[Garrett Casto, 2012/11/27 07:59:49]

Done. I guess the issue I was having is that there seems to be a lot of strange
offsets that are hard to account for (i.e. the textfield has a 3 pixel white
border on the top edge but none of the others, buttons have a 6ish pixel border
on the sides, and a 3 pixel border on it's top and bottom). I was hoping that
there was a programatic way of determining these, but I can just hard code them
for now.
Previously I was punting on localization until we were closer to launch (in case
the UI got changed) but at this point it seems reasonably likely that it's going
to remain the same.

[Scott Hess - ex-Googler, 2012/11/28 20:01:05]

You can punt on the actual localization, but unless you want to rewrite
everything when you get closer to launch, you probably shouldn't punt on
localization support.  Especially if your approach is to do things manually
rather than using the well-trodden frameworks.

[Scott Hess - ex-Googler, 2012/11/28 20:01:05]

Oh, it's worse than that - when you hard-code the values, you'll find that
anytime to make a change to the items, such as using a different font, or
bold/not-bold, or localization, everything moves around.

+
+- (IBAction)fillPassword:(id)sender {
+  renderViewHost_->Send(
+      new AutofillMsg_GeneratedPasswordAccepted(
+          renderViewHost_->GetRoutingID(),
+          base::SysNSStringToUTF16([textField_ stringValue])));
+  passwordManager_->SetFormHasGeneratedPassword(form_);
+  [self close];
+}
+
+- (void)regeneratePassword {
+  [textField_
+    setStringValue:base::SysUTF8ToNSString(passwordGenerator_->Generate())];
+}
+
+@end