Add mac UI for password generation

Add mac UI for password generation

This is the Mac implementation of the password generation
(http://dev.chromium.org/developers/design-documents/password-generation).

Screenshot - http://oi49.tinypic.com/kcchz6.jpg
Windows UI (for comparison) - http://oi45.tinypic.com/51eauw.jpg

BUG=120776
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172231

[sail, 2012-11-16 23:42:52]

(Drive by, this needs a unit test for the new class and a browser test for the
browser window code).

[sail, 2012-11-16 23:44:20]

On 2012/11/16 23:42:52, sail wrote:
> (Drive by, this needs a unit test for the new class and a browser test for the
> browser window code).

Also, UI changes should include screenshots.

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

Time to go home.  here's a partial review.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.h
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.h:43:
PasswordGenerationTextField* textField_;   // weak
I think these are all indented one char too far.  I think @private indented one
char, the variables two chars, like C++ private: and vars.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.h:54: -
(void) regeneratePassword;
No space right of ) for these three.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:25:
namespace {
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:73:
[[self cell] setTruncatesLastVisibleLine:YES];
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:74:
[[self cell] setUpTrackingAreaInRect:[self bounds] ofView:self];
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:107: -
(NSRect)getFrameForIcon:(NSRect*)cellFrame {
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:110:
[normalImage_ size].width - kIconWidthInset, NSMaxXEdge);
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?

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:139:
inView:controlView]) {
This shouldn't need to wrap.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:159:
NSHeight(frame));
This seems sketchy.  Is there something drawing incorrectly?

Also, frame.size.width += 2.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:182:
NSRect centeredFrame = NSInsetRect(iconFrame, x_inset, y_inset);
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:196:
-(void) setUpTrackingAreaInRect:(NSRect)frame
space before (void), not after.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:230:
NSRect contentRect = NSMakeRect(0, 0, 260, 71);
That seems pretty unlikely to me.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:246:
[self performLayout];
Should be in the if() clause.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:260:
[textField_ setFont:[NSFont boldSystemFontOfSize:11]];
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:279: }
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:650: const gfx::Rect& rect)
const {
This seems to only need the passed parameters - suggest moving it to the
anonymous namespace at the top fo the file, rather than exposing another helper
in the interface.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:686: point.x -= rect.width()/2;
This is expressing a bit more knowledge of the internals of GetPointForBubble()
than I'm comfortable with.  I would prefer adjusting the input rect before
passing it rather than adjusting the output value based on an input value. 
Perhaps inset the rect by width()/2 before calling GetPointForBubble().  Or
maybe just pass in the x and y points relative to rect.

I mean like this for avatar:
  NSPoint point = GetPointForBubble(web_contents, rect.right, rect.bottom);
and like this for here:
  NSPoint point = GetPointForBubble(web_contents, rect.x() + rect.width() / 2,
rect.bottom);

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

Screenshot of UI - http://www.corp.google.com/~gcasto/mac_ui.png
For comparison, this is the Windows UI -
http://www.corp.google.com/~gcasto/windows_ui.png

You can ignore the differences in key icons and bubble location, the Windows
picture is a little old.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.h
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.h:43:
PasswordGenerationTextField* textField_;   // weak
On 2012/11/17 01:22:41, shess wrote:
> I think these are all indented one char too far.  I think @private indented
one
> char, the variables two chars, like C++ private: and vars.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.h:54: -
(void) regeneratePassword;
On 2012/11/17 01:22:41, shess wrote:
> No space right of ) for these three.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:25:
namespace {
On 2012/11/17 01:22:41, shess wrote:
> 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.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:25:
namespace {
On 2012/11/17 01:22:41, shess wrote:
> 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.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:73:
[[self cell] setTruncatesLastVisibleLine:YES];
On 2012/11/17 01:22:41, shess wrote:
> 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.

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?

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:74:
[[self cell] setUpTrackingAreaInRect:[self bounds] ofView:self];
On 2012/11/17 01:22:41, shess wrote:
> 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.

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

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:107: -
(NSRect)getFrameForIcon:(NSRect*)cellFrame {
On 2012/11/17 01:22:41, shess wrote:
> 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.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:110:
[normalImage_ size].width - kIconWidthInset, NSMaxXEdge);
On 2012/11/17 01:22:41, shess wrote:
> 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?

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).

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:139:
inView:controlView]) {
On 2012/11/17 01:22:41, shess wrote:
> This shouldn't need to wrap.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:159:
NSHeight(frame));
On 2012/11/17 01:22:41, shess wrote:
> This seems sketchy.  Is there something drawing incorrectly?
> 

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.

> Also, frame.size.width += 2.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:182:
NSRect centeredFrame = NSInsetRect(iconFrame, x_inset, y_inset);
On 2012/11/17 01:22:41, shess wrote:
> 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.

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:196:
-(void) setUpTrackingAreaInRect:(NSRect)frame
On 2012/11/17 01:22:41, shess wrote:
> space before (void), not after.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:230:
NSRect contentRect = NSMakeRect(0, 0, 260, 71);
On 2012/11/17 01:22:41, shess wrote:
> That seems pretty unlikely to me.

?  An issue with hard coding number?

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:246:
[self performLayout];
On 2012/11/17 01:22:41, shess wrote:
> Should be in the if() clause.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:260:
[textField_ setFont:[NSFont boldSystemFontOfSize:11]];
On 2012/11/17 01:22:41, shess wrote:
> 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.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:279: }
On 2012/11/17 01:22:41, shess wrote:
> 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.

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).

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:650: const gfx::Rect& rect)
const {
On 2012/11/17 01:22:41, shess wrote:
> This seems to only need the passed parameters - suggest moving it to the
> anonymous namespace at the top fo the file, rather than exposing another
helper
> in the interface.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:686: point.x -= rect.width()/2;
On 2012/11/17 01:22:41, shess wrote:
> This is expressing a bit more knowledge of the internals of
GetPointForBubble()
> than I'm comfortable with.  I would prefer adjusting the input rect before
> passing it rather than adjusting the output value based on an input value. 
> Perhaps inset the rect by width()/2 before calling GetPointForBubble().  Or
> maybe just pass in the x and y points relative to rect.
> 
> I mean like this for avatar:
>   NSPoint point = GetPointForBubble(web_contents, rect.right, rect.bottom);
> and like this for here:
>   NSPoint point = GetPointForBubble(web_contents, rect.x() + rect.width() / 2,
> rect.bottom);

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:686: point.x -= rect.width()/2;
On 2012/11/17 01:22:41, shess wrote:
> This is expressing a bit more knowledge of the internals of
GetPointForBubble()
> than I'm comfortable with.  I would prefer adjusting the input rect before
> passing it rather than adjusting the output value based on an input value. 
> Perhaps inset the rect by width()/2 before calling GetPointForBubble().  Or
> maybe just pass in the x and y points relative to rect.
> 
> I mean like this for avatar:
>   NSPoint point = GetPointForBubble(web_contents, rect.right, rect.bottom);
> and like this for here:
>   NSPoint point = GetPointForBubble(web_contents, rect.x() + rect.width() / 2,
> rect.bottom);

Done.

[sail, 2012-11-19 22:27:57]

On 2012/11/19 22:17:42, Garrett Casto wrote:
> Screenshot of UI - http://www.corp.google.com/%7Egcasto/mac_ui.png
> For comparison, this is the Windows UI -
> http://www.corp.google.com/%7Egcasto/windows_ui.png

The spacing in the Mac version seems odd. Particularly the space between the
"Password Suggestion" label and the text field.

Also, the screenshot links should go in the CL description. I'd also recommend
using a public link.

Finally, I don't see any tests.

[sail, 2012-11-19 22:28:45]

Also, another quick question. What happens when you press enter or escape?

[Garrett Casto, 2012-11-19 23:07:12]

On 2012/11/19 22:27:57, sail wrote:
> On 2012/11/19 22:17:42, Garrett Casto wrote:
> > Screenshot of UI - http://www.corp.google.com/%257Egcasto/mac_ui.png
> > For comparison, this is the Windows UI -
> > http://www.corp.google.com/%257Egcasto/windows_ui.png
> 
> The spacing in the Mac version seems odd. Particularly the space between the
> "Password Suggestion" label and the text field.
> 

You're right. Late in the process I ended up changing the textfield height and
didn't adjust the overall size accordingly. I've updated the link. I still don't
like the spacing as much as in Windows, but I'm a little constrained by button
size.

> Also, the screenshot links should go in the CL description. I'd also recommend
> using a public link.
> 

Done. Also, image hosting sites are super sketchy.

> Finally, I don't see any tests.

Working on it. Windows and Linux don't seem to test, so I need to figure out
what I should be testing here. By the way, when you said browser tests, what do
you mean? brower_window_cocoa_unittest.mm is basically empty and I wouldn't know
what to test there anyway. Or did you mean something else?

[Garrett Casto, 2012-11-19 23:07:58]

On 2012/11/19 22:28:45, sail wrote:
> Also, another quick question. What happens when you press enter or escape?

Escape closes the bubble. Enter, while focused on the textfield, does nothing.
We should probably change that, though it is consistent across platforms at the
moment.

[sail, 2012-11-19 23:13:03]

On 2012/11/19 23:07:58, Garrett Casto wrote:
> On 2012/11/19 22:28:45, sail wrote:
> > Also, another quick question. What happens when you press enter or escape?
> 
> Escape closes the bubble. Enter, while focused on the textfield, does nothing.
> We should probably change that, though it is consistent across platforms at
the
> moment.

You can use -[NSButotn setKeyEquivalent:kKeyEquivalentReturn] to make the enter
key work.

[sail, 2012-11-19 23:16:03]

> Working on it. Windows and Linux don't seem to test, so I need to figure out
> what I should be testing here. By the way, when you said browser tests, what
do
> you mean? brower_window_cocoa_unittest.mm is basically empty and I wouldn't
know
> what to test there anyway. Or did you mean something else?

browser_window_controller_browsertest.mm might be a good place to start.

It's not actually important whether you use a browser test or a unit test as
long as their's code coverage. It might be easier to use a browser test in this
case for the browser_window_cocoa.mm changes.

[Scott Hess - ex-Googler, 2012-11-19 23:18:19]

On 2012/11/19 23:07:12, Garrett Casto wrote:
> On 2012/11/19 22:27:57, sail wrote:
> > Also, the screenshot links should go in the CL description. I'd also
recommend
> > using a public link.
> 
> Done. Also, image hosting sites are super sketchy.

Could you put the images in the bug and reference those?

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

Hmm.  I'm going OOT for a week tomorrow, so I don't think I'm going to be able
to complete this review.  At best, I'll be back in a week and then completely
underwater for the rest of that week.  So maybe treat my points as "Things that
are going to annoy me a lot if I run across them."  But I think you'll have to
rely on Sailesh or someone else if you want to get this checked in relatively
soon.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:73:
[[self cell] setTruncatesLastVisibleLine:YES];
On 2012/11/19 22:17:42, Garrett Casto wrote:
> On 2012/11/17 01:22:41, shess wrote:
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:74:
[[self cell] setUpTrackingAreaInRect:[self bounds] ofView:self];
On 2012/11/19 22:17:42, Garrett Casto wrote:
> On 2012/11/17 01:22:41, shess wrote:
> > 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.
> 
> I didn't notice anything obvious in base/mac/... I switched it over to using a
> loca variable.

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:110:
[normalImage_ size].width - kIconWidthInset, NSMaxXEdge);
On 2012/11/19 22:17:42, Garrett Casto wrote:
> On 2012/11/17 01:22:41, shess wrote:
> > 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?
> 
> 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).   

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:182:
NSRect centeredFrame = NSInsetRect(iconFrame, x_inset, y_inset);
On 2012/11/19 22:17:42, Garrett Casto wrote:
> On 2012/11/17 01:22:41, shess wrote:
> > 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.
> 
> 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.

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

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:230:
NSRect contentRect = NSMakeRect(0, 0, 260, 71);
On 2012/11/19 22:17:42, Garrett Casto wrote:
> On 2012/11/17 01:22:41, shess wrote:
> > That seems pretty unlikely to me.
> 
> ?  An issue with hard coding number?

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:263:
[contentView addSubview:textField_];
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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:279: }
On 2012/11/19 22:17:42, Garrett Casto wrote:
> On 2012/11/17 01:22:41, shess wrote:
> > 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.
> 
> 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.

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]

I've added tests, though the unittest currently fails because all of my attempts
to simulate a button press have failed :(. Any pointers on how to correctly do
this would be appreciated. I'm assuming it's because the coordinate system is
wrong somehow.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:73:
[[self cell] setTruncatesLastVisibleLine:YES];
On 2012/11/19 23:20:35, shess wrote:
> On 2012/11/19 22:17:42, Garrett Casto wrote:
> > On 2012/11/17 01:22:41, shess wrote:
> > > 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.
> > 
> > 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?
> 
> 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.

Done.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:74:
[[self cell] setUpTrackingAreaInRect:[self bounds] ofView:self];
On 2012/11/19 23:20:35, shess wrote:
> On 2012/11/19 22:17:42, Garrett Casto wrote:
> > On 2012/11/17 01:22:41, shess wrote:
> > > 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.
> > 
> > I didn't notice anything obvious in base/mac/... I switched it over to using
a
> > loca variable.
> 
> 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.

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?

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:110:
[normalImage_ size].width - kIconWidthInset, NSMaxXEdge);
On 2012/11/19 23:20:35, shess wrote:
> On 2012/11/19 22:17:42, Garrett Casto wrote:
> > On 2012/11/17 01:22:41, shess wrote:
> > > 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?
> > 
> > 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).   
> 
> 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.

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

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:263:
[contentView addSubview:textField_];
On 2012/11/19 23:20:35, shess wrote:
> 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.

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?

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:279: }
On 2012/11/19 23:20:35, shess wrote:
> On 2012/11/19 22:17:42, Garrett Casto wrote:
> > On 2012/11/17 01:22:41, shess wrote:
> > > 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.
> > 
> > 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.
> 
> 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.
> 

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.

> 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?

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.

[sail, 2012-11-28 06:47:15]

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:74:
[[self cell] setUpTrackingAreaInRect:[self bounds] ofView:self];
On 2012/11/27 07:59:49, Garrett Casto wrote:
> On 2012/11/19 23:20:35, shess wrote:
> > On 2012/11/19 22:17:42, Garrett Casto wrote:
> > > On 2012/11/17 01:22:41, shess wrote:
> > > > 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.
> > > 
> > > I didn't notice anything obvious in base/mac/... I switched it over to
using
> a
> > > loca variable.
> > 
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
File
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:25:
controller_ = [[PasswordGenerationBubbleController alloc]
does this delete it self? if so add a comment above?

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:38:
PasswordGenerationBubbleController* controller_;
initialize this to nil in the constructor?

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:47:
for (NSView* subview in [contents subviews]) {
it would be better to just add an accessor to PasswordGenerationBubbleController

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:57:
rect = [[textfield window] convertRectToScreen:rect];
try adding "rect = [textfield convertRect:rect toView:nil]" before this line.

This will convert the rect from the view's coordinate system to the window's
coordinate system.

Note that I'm not sure doing this is worth it. This stuff can get flaky on test
bots. It might be simpler to expose a method to do this more directly.

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

Sorry if this is incoherent.  First day back from vacation, and I'm barely
managing containment.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:110:
[normalImage_ size].width - kIconWidthInset, NSMaxXEdge);
On 2012/11/27 07:59:49, Garrett Casto wrote:
> I changed this to just crop the image, which hopefully makes more sense.

Sounds like it should.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:159:
NSHeight(frame));
On 2012/11/19 22:17:42, Garrett Casto wrote:
> On 2012/11/17 01:22:41, shess wrote:
> > This seems sketchy.  Is there something drawing incorrectly?
> 
> 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.

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:263:
[contentView addSubview:textField_];
On 2012/11/27 07:59:49, Garrett Casto wrote:
> On 2012/11/19 23:20:35, shess wrote:
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:279: }
On 2012/11/27 07:59:49, Garrett Casto wrote:
> On 2012/11/19 23:20:35, shess wrote:
> > Beyond that - where's your localization support?
> 
> 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.

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.

https://codereview.chromium.org/11416047/diff/9/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:279: }
On 2012/11/27 07:59:49, Garrett Casto wrote:
> 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.

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.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:124: -
(void)mouseDown:(NSEvent*)theEvent {
I'm looking at this and wondering what happens if the control loses focus
somehow?  In that case [self currentEditor] will be nil and the -mouseDown: will
be ignored.  In that case it should probably pass to super.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:209:
frame.size.width += 2;
Per comment I made probably against an earlier patch - put a very long string in
there and make sure that when you scroll around, you aren't drawing in places
you don't expect.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:234:
const CGFloat y_offset = ([image size].height - (NSHeight(iconFrame))) / 2.0;
xOffset, yOffset.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:293:
info_bubble::kBubbleArrowHeight);
These should probably both be CGFloat.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:337:
kButtonVerticalPadding);
CGFloat here, too.  kButtonVerticalPadding when calculating button_x doesn't
seem right.

Also, NSMaxX([textField_ frame]) + kHorizontalPadding it probably less brittle
WRT future changes.

Hmm, also, buttonX, or something like that, since this is ObjC.  Sorry.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:354:
kBorderSize + kTextFieldHeight + kVerticalSpacing,
As with the earlier comment, this could be NSMaxY([textField_ frame]) +
kVerticalSpacing.

[Garrett Casto, 2012-11-30 20:56:26]

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:124: -
(void)mouseDown:(NSEvent*)theEvent {
On 2012/11/28 20:01:06, shess wrote:
> I'm looking at this and wondering what happens if the control loses focus
> somehow?  In that case [self currentEditor] will be nil and the -mouseDown:
will
> be ignored.  In that case it should probably pass to super.

Changed.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:209:
frame.size.width += 2;
On 2012/11/28 20:01:06, shess wrote:
> Per comment I made probably against an earlier patch - put a very long string
in
> there and make sure that when you scroll around, you aren't drawing in places
> you don't expect.

Looks right to me.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:234:
const CGFloat y_offset = ([image size].height - (NSHeight(iconFrame))) / 2.0;
On 2012/11/28 20:01:06, shess wrote:
> xOffset, yOffset.

Done.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:293:
info_bubble::kBubbleArrowHeight);
On 2012/11/28 20:01:06, shess wrote:
> These should probably both be CGFloat.

Done.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:337:
kButtonVerticalPadding);
On 2012/11/28 20:01:06, shess wrote:
> CGFloat here, too.  kButtonVerticalPadding when calculating button_x doesn't
> seem right.
> 

Yep, good catch. I was trying to figure out why the UI was off by a few pixels.

> Also, NSMaxX([textField_ frame]) + kHorizontalPadding it probably less brittle
> WRT future changes.
> 
> Hmm, also, buttonX, or something like that, since this is ObjC.  Sorry.

Done.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:354:
kBorderSize + kTextFieldHeight + kVerticalSpacing,
On 2012/11/28 20:01:06, shess wrote:
> As with the earlier comment, this could be NSMaxY([textField_ frame]) +
> kVerticalSpacing.

This isn't equivalent. The way I've been trying to set this up is that other
than the offset constants, everything is actual visible pixel size. Since
textfields apparently have a 3 pixel border on the top, the actual vertical
space would be 3 pixels greater than kVerticalSpacing. Would you prefer
NSMaxY([textField_ frame]) - kTextFieldTopOffset + kVerticalSpacing instead?

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
File
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:25:
controller_ = [[PasswordGenerationBubbleController alloc]
On 2012/11/28 06:47:16, sail wrote:
> does this delete it self? if so add a comment above?

Done.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:38:
PasswordGenerationBubbleController* controller_;
On 2012/11/28 06:47:16, sail wrote:
> initialize this to nil in the constructor?

Done.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:47:
for (NSView* subview in [contents subviews]) {
On 2012/11/28 06:47:16, sail wrote:
> it would be better to just add an accessor to
PasswordGenerationBubbleController

Done.

https://codereview.chromium.org/11416047/diff/7002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller_unittest.mm:57:
rect = [[textfield window] convertRectToScreen:rect];
On 2012/11/28 06:47:16, sail wrote:
> try adding "rect = [textfield convertRect:rect toView:nil]" before this line.
> 

I actually tried this before, and it doesn't work. I just exposed that method
directly as you suggested. This doesn't seem worth grappling with.

> This will convert the rect from the view's coordinate system to the window's
> coordinate system.
> 
> Note that I'm not sure doing this is worth it. This stuff can get flaky on
test
> bots. It might be simpler to expose a method to do this more directly.

[Scott Hess - ex-Googler, 2012-12-03 21:50:42]

lgtm

https://codereview.chromium.org/11416047/diff/5005/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/5005/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:7:
#include "base/sys_string_conversions.h"
This should order below base/mac.

https://codereview.chromium.org/11416047/diff/5005/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:9:
#include "chrome/common/autofill_messages.h"
Below chrome/browser section.

[Garrett Casto, 2012-12-10 18:49:18]

+sky for chrome/test/... and chrome/browser/... changes.

https://codereview.chromium.org/11416047/diff/5005/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://codereview.chromium.org/11416047/diff/5005/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:7:
#include "base/sys_string_conversions.h"
On 2012/12/03 21:50:42, shess wrote:
> This should order below base/mac.

Done.

https://codereview.chromium.org/11416047/diff/5005/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:9:
#include "chrome/common/autofill_messages.h"
On 2012/12/03 21:50:42, shess wrote:
> Below chrome/browser section.

Done.

[sky, 2012-12-10 22:12:42]

LGTM

[commit-bot: I haz the power, 2012-12-10 22:48:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/11416047/18004

[commit-bot: I haz the power, 2012-12-11 01:49:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/11416047/18004

[commit-bot: I haz the power, 2012-12-11 02:04:12]

Change committed as 172231

[Nico, 2013-03-04 14:11:10]

https://chromiumcodereview.appspot.com/11416047/diff/18004/chrome/browser/ui/...
File chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm
(right):

https://chromiumcodereview.appspot.com/11416047/diff/18004/chrome/browser/ui/...
chrome/browser/ui/cocoa/browser/password_generation_bubble_controller.mm:366:
kTitleHeight)];
This is leaked. Fix at https://codereview.chromium.org/12390058.