[Mac] Misc fixes for the new avatar bubble

chrome/browser/ui/cocoa/profiles/profile_chooser_controller.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 <Cocoa/Cocoa.h>
 
 #import "chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h"
 
 #include "base/mac/bundle_locations.h"
 #include "base/prefs/pref_service.h"
@@ skipping 314 matching lines @@
   // The observer can be removed both when closing the browser, and by just
   // closing the avatar bubble. However, in the case of closing the browser,
   // the avatar bubble will also be closed afterwards, resulting in a second
   // attempt to remove the observer. This ensures the observer is only
   // removed once.
   bool token_observer_registered_;
 
   DISALLOW_COPY_AND_ASSIGN(ActiveProfileObserverBridge);
 };
 
+// Custom button cell that adds a left padding to a button.

[groby-ooo-7-16, 2014/06/10 19:53:23]

If you want to add padding, -drawInteriorWithFrame: is a better choice. As it
stands, drawImage for this button would ignore the padding for example.

[noms (inactive), 2014/06/11 15:09:00]

Hmm, I might have done it wrong, but -drawInteriorWithFrame adds a margin (so
indents the whole control), whereas drawTitle just indents the title, but keeps
the same control frame.

This is relevant because CustomPaddingImageButtonCell (which inherits from this)
is used in a button that, on hover, sets the background color of the button, so
if the entire button is indented, and not just the text, it looks weird. I'm not
sure I've explained this clearly... :(

On 2014/06/10 19:53:23, groby wrote:

[groby-ooo-7-16, 2014/06/11 18:11:21]

I'm not sure I follow - how is the button indented? From what? (Sorry. I'm slow.
It's easiest if people draw me pictures :)

[noms (inactive), 2014/06/11 19:29:57]

Yeah, my bad. This entire CL is "really annoying and minor UI fixes that are a
pain to explain and fix".
Let's give this another try. Here are some drawings:
https://docs.google.com/drawings/d/1gvNMEs0AJpI0ZTa2x6wv24qGh0uJPdkjN3kJe2aVu...

The problem we are trying to fix here is that "monica" isn't centered (see the
"status quo" images) -- it's off to the left by the width of the pencil. If you
look at the "pressed" state, the grey background covers essentially the entire
bubble, minus some edge padding.

If I add the left padding in drawTitle, then the button stays the same width (so
the grey on pressed state looks the same), but the button title gets shifted to
the right and gets centered correctly.

If i add the left padding in drawInteriorWithFrame (with essentially the same
code as in drawTitle), the frame.origin.x += padding call will add the padding
to the _entire_ button -- so it will end up looking like the "after
drawInteriorWithFrame" image. The text is centered correctly, but that's because
the whole button moved, which is bad news bears.

Am I doing this wrong? I bet I'm doing this wrong.

On 2014/06/11 18:11:21, groby wrote:

[groby-ooo-7-16, 2014/06/12 01:48:12]

No, you're not. I misunderstood what you want padding to do. The picture helped
:)

But you can still do it via drawInteriorWithFrame - you just need to take care
of the background yourself.

- (void)drawInteriorWithFrame:(NSRect)cellFrame inView:(NSView *)controlView {
  NSRect marginRect;
  NSRect interiorRect;

  NSDivideRect(cellFrame, &marginRect, &interiorRect, leftMarginSpacing_,
NSMinXEdge);

   // You might possibly need to fill the entire cellFrame instead if the border
shows aliasing issues.
   if(![self isBordered]){
     [[self backgroundColor] setFill];
     NSRectFill(marginRect);
  }

  [super drawInteriorWithFrame:interiorRect];
}
  

[noms (inactive), 2014/06/16 19:17:10]

Ok. The -drawInterior was almost perfect, but because you can edit the profile
name while the bubble is open, it needs to redraw while you're looking at it,
and the original background flashes before the NSRectFill() call is made.

However, not all is lost: I managed to merge the two classes together and make
it work for both cases. Better?

+@interface LeftMarginButtonCell : NSButtonCell {
+ @protected
+  // Padding added to the left margin of the button.
+  int leftMarginSpacing_;
+}
+
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing;
+@end
+
+@implementation LeftMarginButtonCell
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing {
+  if ((self = [super init])) {
+    leftMarginSpacing_ = leftMarginSpacing;
+  }
+  return self;
+}
+
+- (NSRect)drawTitle:(NSAttributedString*)title
+          withFrame:(NSRect)frame
+             inView:(NSView*)controlView {
+  // The title frame origin isn't aware of the left margin spacing added
+  // in -drawImage, so it must be added when drawing the title as well.
+  frame.origin.x += leftMarginSpacing_;
+  frame.size.width -= leftMarginSpacing_;
+  return [super drawTitle:title withFrame:frame inView:controlView];
+}
+
+- (NSSize)cellSize {
+  NSSize buttonSize = [super cellSize];
+  buttonSize.width += leftMarginSpacing_;
+  return buttonSize;
+}
+
+@end
+
+// Custom button cell that adds a left padding before the button image, and
+// a custom spacing between the button image and title.
+@interface CustomPaddingImageButtonCell : LeftMarginButtonCell {

[groby-ooo-7-16, 2014/06/10 19:53:23]

You might want to _just_ keep CustomPaddingImageButtonCell.

LeftMarginImageButtonCell is just this with an |imageTitleSpacing_| of 0, no?

[noms (inactive), 2014/06/11 15:09:00]

Hmm, it's a little worse than that (I'm noticing this is a poorly named
control). CustomPaddingImageButtonCell is hardcoded to always place the image on
the left. LeftMarginButtonCell just adds a padding to the left of the title, but
you can have the image position be on the right (which is how it's used in the
profile name control). I should rename these, shouldn't I?

On 2014/06/10 19:53:23, groby wrote:

[groby-ooo-7-16, 2014/06/11 18:11:22]

You might want to. Also, with the image forced to the left, RTL is going to
be... fun. If you could make a quick sketch what you want to pad, exactly,
because I think I misunderstand things?

[noms (inactive), 2014/06/11 19:29:57]

Does the sketch above explain the padding? I was talking about the "space I need
to shift the button title to the right so that it's centered". 

On 2014/06/11 18:11:22, groby wrote:

[groby-ooo-7-16, 2014/06/12 01:48:11]

If you use the drawInteriorWithFrame approach, you don't have to make
assumptions about the image alignment any more. Or I'm still missing the point.
It's 50/50 ;)

+ @private
+  // Spacing between the cell image and title.
+  int imageTitleSpacing_;
+}
+
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
+              imageTitleSpacing:(int)imageTitleSpacing;
+@end
+
+@implementation CustomPaddingImageButtonCell
+- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
+              imageTitleSpacing:(int)imageTitleSpacing {
+  if ((self = [super initWithLeftMarginSpacing:leftMarginSpacing])) {
+    imageTitleSpacing_ = imageTitleSpacing;
+  }
+  return self;
+}
+
+- (NSRect)drawTitle:(NSAttributedString*)title
+          withFrame:(NSRect)frame
+             inView:(NSView*)controlView {
+  // The title frame origin isn't aware of the left margin spacing added
+  // in -drawImage, so it must be added when drawing the title as well.
+  frame.origin.x += imageTitleSpacing_;
+  frame.size.width -= imageTitleSpacing_;
+  return [super drawTitle:title withFrame:frame inView:controlView];
+}
+
+- (void)drawImage:(NSImage*)image
+       withFrame:(NSRect)frame
+          inView:(NSView*)controlView {
+  frame.origin.x = leftMarginSpacing_;
+  [super drawImage:image withFrame:frame inView:controlView];
+}
+
+- (NSSize)cellSize {
+  NSSize buttonSize = [super cellSize];
+  buttonSize.width += imageTitleSpacing_;
+  return buttonSize;
+}
+
+@end
+
 // A custom button that has a transparent backround.
 @interface TransparentBackgroundButton : NSButton
 @end
 
 @implementation TransparentBackgroundButton
 - (id)initWithFrame:(NSRect)frameRect {
   if ((self = [super initWithFrame:frameRect])) {
     [self setBordered:NO];
     [self setFont:[NSFont labelFontOfSize:kTextFontSize]];
     [self setButtonType:NSMomentaryChangeButton];
   }
   return self;
 }
 
 - (void)drawRect:(NSRect)dirtyRect {
-  NSColor* backgroundColor = [NSColor colorWithCalibratedWhite:1 alpha:0.4f];
+  NSColor* backgroundColor = [NSColor colorWithCalibratedWhite:1 alpha:0.6f];
   [backgroundColor setFill];
   NSRectFillUsingOperation(dirtyRect, NSCompositeSourceAtop);
   [super drawRect:dirtyRect];
 }
 @end
 
 // A custom image control that shows a "Change" button when moused over.
 @interface EditableProfilePhoto : NSImageView {
  @private
   AvatarMenu* avatarMenu_;  // Weak; Owned by ProfileChooserController.
@@ skipping 119 matching lines @@
 @implementation EditableProfileNameButton
 - (id)initWithFrame:(NSRect)frameRect
             profile:(Profile*)profile
         profileName:(NSString*)profileName
      editingAllowed:(BOOL)editingAllowed
      withController:(ProfileChooserController*)controller {
   if ((self = [super initWithFrame:frameRect])) {
     profile_ = profile;
     controller_ = controller;
 
-    [self setBordered:NO];
-    [self setFont:[NSFont labelFontOfSize:kTitleFontSize]];
-    [self setAlignment:NSCenterTextAlignment];
-    [[self cell] setLineBreakMode:NSLineBreakByTruncatingTail];
-    [self setTitle:profileName];
-
     if (editingAllowed) {
       // Show an "edit" pencil icon when hovering over. In the default state,
       // we need to create an empty placeholder of the correct size, so that
       // the text doesn't jump around when the hovered icon appears.
       ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
       NSImage* hoverImage = rb->GetNativeImageNamed(
           IDR_ICON_PROFILES_EDIT_HOVER).AsNSImage();
+
+      // In order to center the button title, we need to add a left padding of
+      // the same width as the pencil icon.
+      base::scoped_nsobject<LeftMarginButtonCell> cell(
+          [[LeftMarginButtonCell alloc]
+              initWithLeftMarginSpacing:[hoverImage size].width]);
+      [self setCell:cell.get()];
+
       NSImage* placeholder = [[NSImage alloc] initWithSize:[hoverImage size]];
       [self setDefaultImage:placeholder];
       [self setHoverImage:hoverImage];
       [self setAlternateImage:
           rb->GetNativeImageNamed(IDR_ICON_PROFILES_EDIT_PRESSED).AsNSImage()];
       [self setImagePosition:NSImageRight];
       [self setTarget:self];
       [self setAction:@selector(showEditableView:)];
 
       // We need to subtract the width of the bezel from the frame rect, so that
@@ skipping 10 matching lines @@
       [profileNameTextField_ setAlignment:NSCenterTextAlignment];
       [[profileNameTextField_ cell] setWraps:NO];
       [[profileNameTextField_ cell] setLineBreakMode:
           NSLineBreakByTruncatingTail];
       [profileNameTextField_ setDelegate:self];
       [self addSubview:profileNameTextField_];
 
       // Hide the textfield until the user clicks on the button.
       [profileNameTextField_ setHidden:YES];
     }
+
+    [self setBordered:NO];
+    [self setFont:[NSFont labelFontOfSize:kTitleFontSize]];
+    [self setAlignment:NSCenterTextAlignment];
+    [[self cell] setLineBreakMode:NSLineBreakByTruncatingTail];
+    [self setTitle:profileName];
   }
   return self;
 }
 
 // NSTextField objects send an NSNotification to a delegate if
 // it implements this method:
 - (void)controlTextDidEndEditing:(NSNotification *)obj {
   NSString* text = [profileNameTextField_ stringValue];
   // Empty profile names are not allowed, and are treated as a cancel.
   if ([text length] > 0) {
     profiles::UpdateProfileName(profile_, base::SysNSStringToUTF16(text));
     [controller_
         postActionPerformed:ProfileMetrics::PROFILE_DESKTOP_MENU_EDIT_NAME];
     [self setTitle:text];
   }
+
+  [[self window] makeFirstResponder:nil];

[groby-ooo-7-16, 2014/06/10 19:53:23]

Are you sure you want the window to be first responder, not the next UI element?

(If the latter, [[self window] selectNextKeyView:self] makes more sense.

[noms (inactive), 2014/06/11 15:09:00]

Giant incoming comment:

If I do what you suggested, what ends up happening (crbug.com/343688) is that
you can end up typing into the textfield even though it's hidden. To repro it,
all you have to do is try to edit the profile name (press the
EditableProfileButton), but without changing the contents, press enter and
commit the changes. Next time you try to press the button a) the text field
won't become visible but b) you can type things, press enter, and they will be
committed.

Disabling the text field fixes it, hence my original change. What also fixes it
is this line, but this adds a blue focus ring around the pencil image of the
NsButton.
[[self window] makeFirstResponder:[profileNameTextField_ nextResponder]];

I can remove the focus ring with [self setFocusRingType:NSFocusRingTypeNone],
but I don't know if that's poor form.

Do you have a preference between the two evils?

On 2014/06/10 19:53:23, groby wrote:

[noms (inactive), 2014/06/11 15:09:00]

Setting the next UI element to be the first responder adds a blue focus border
around the pencil icon, which is awful looking. :(

On 2014/06/10 19:53:23, groby wrote:

[groby-ooo-7-16, 2014/06/11 18:11:21]

You know that you can actually specify the key view loop order, right? ;)

[groby-ooo-7-16, 2014/06/11 18:11:21]

That's odd. Pressing the button should make the text field visible again, if I
read showEditableView correctly.

Also, hidden views really don't get key events. Quoth the doc: " hidden view
disappears from its window and does not receive input events.". So it looks like
there's a draw issue, not an input event issue. 

What happens if you take out _all_ firstResponder/keyview handling, and the
setEnabled code? (That _should_ do the entire trick)

Removing the firstResponder/keyView code has the added benefit that you don't
interfere with focus behavior if the user tabs through instead of pressing
enter.
That is _almost_ the equivalent of [[self window] selectNextKeyView:self].
(Except selectNextKeyView skips hidden fields)

You don't need either, though. setHidden automatically selects the next valid
key view.
That's poor form :)
I'd prefer we figure out why setHidden alone is not enough. (I may well be wrong
in assuming it's enough, but if so, I'd like to know why.)

[noms (inactive), 2014/06/11 19:29:57]

I completely agree it should (which is why the code is like that), but it leads
to the following crazy bug:
1. Start Canary with --new-profile-management
2. Press on the avatar button, which should show you the avatar bubble.
3. Press on the profile name in the bubble. It will go into edit mode and
display the text field. 
Change the name from whatever it is to (say) "boom". Press enter. Name has been
saved, everyone is 
happy, text field is hidden.
4. Repeat step 3, and change the name to "bork". Everyone is still happy.
5. Repeat step 3, only instead of changing the name, press enter as soon as the
text field is displayed.
6. Try to press on the button again. It won't go into editable mode until you
reopen the bubble, 
and you are now a sad panda.
7. It's even worse! Press on the button (even though the text field isn't
visible, i promise you it's enabled), type something and press enter. Your
hidden, typed name should be saved.
8. Halp.
Taking out all the firstResponder stuff makes everything work normally (bug
isn't reproducible), but
then the text doesn't get auto-selected when you go in edit mode (that's why the
firstResponder stuff 
is there in the first place).
If i do [[self window] selectNextKeyView:self], then the bug is still
reproducible :(

[groby-ooo-7-16, 2014/06/12 01:48:11]

So there's something different if you don't change it and only hit enter.
Hmmmm.....
This sounds as if the text field is still active and never relinquished control.
So you'll never get the button delegate to fire.

And finally, my brain switches on - you _so_ don't want to do this in
controlTextDidEndEditing. It's doing things to firstResponder internally, and
calling makeFirstResponder here confuses it. So does calling setHidden. It
probably also interacts in unholy ways with the embedded field editor.
(NSTextView is a pain...)

All you want is change the text if the user is done editing, no? Just set an
action on the profileNameTextField_.

 In that action, extract the text and then only do a setHidden on the text
field.
In showEditableView, only do setHidden:NO, followed by makeFirstResponder. That
should do the trick.
If it's invisible but taking input, it got activated via makeFirstResponder, not
selectNextValidKeyView. (firstResponder can be invisible. KeyViews can't)
The above should halp. I hope. I hate NSTextView.
OK, you _do_ need the makeFirstResponder in showEditableView. But you shouldn't
need to do it in controlTextDidEndEditing. setHidden should jump you to the next
view as if by magic.
You want setNextValidKeyView, sorry. I'm really curious what it thinks the
nextKeyView is, though. Maybe you can log that in the buggy version? (Ignore
this. See above. You don't need to do anything with key views)

[noms (inactive), 2014/06/16 19:17:10]

DONE! \o/

   [profileNameTextField_ setHidden:YES];
-  [profileNameTextField_ resignFirstResponder];
+  [profileNameTextField_ setEnabled:NO];

[groby-ooo-7-16, 2014/06/10 19:53:23]

You shouldn't need to disable if it's hidden.

 }
 
 - (void)showEditableView:(id)sender {
   [profileNameTextField_ setHidden:NO];
-  [profileNameTextField_ becomeFirstResponder];
+  [profileNameTextField_ setEnabled:YES];
+  [[self window] makeFirstResponder:profileNameTextField_];
 }
 
 @end
-
-// Custom button cell that adds a left padding before the button image, and
-// a custom spacing between the button image and title.
-@interface CustomPaddingImageButtonCell : NSButtonCell {
- @private
-  // Padding between the left margin of the button and the cell image.
-  int leftMarginSpacing_;
-  // Spacing between the cell image and title.
-  int imageTitleSpacing_;
-}
-
-- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
-              imageTitleSpacing:(int)imageTitleSpacing;
-@end
-
-@implementation CustomPaddingImageButtonCell
-- (id)initWithLeftMarginSpacing:(int)leftMarginSpacing
-              imageTitleSpacing:(int)imageTitleSpacing {
-  if ((self = [super init])) {
-    leftMarginSpacing_ = leftMarginSpacing;
-    imageTitleSpacing_ = imageTitleSpacing;
-  }
-  return self;
-}
-
-- (NSRect)drawTitle:(NSAttributedString*)title
-          withFrame:(NSRect)frame
-             inView:(NSView*)controlView {
-  // The title frame origin isn't aware of the left margin spacing added
-  // in -drawImage, so it must be added when drawing the title as well.
-  frame.origin.x += leftMarginSpacing_ + imageTitleSpacing_;
-  frame.size.width -= (imageTitleSpacing_ + leftMarginSpacing_);
-  return [super drawTitle:title withFrame:frame inView:controlView];
-}
-
-- (void)drawImage:(NSImage*)image
-       withFrame:(NSRect)frame
-          inView:(NSView*)controlView {
-  frame.origin.x = leftMarginSpacing_;
-  [super drawImage:image withFrame:frame inView:controlView];
-}
-
-- (NSSize)cellSize {
-  NSSize buttonSize = [super cellSize];
-  buttonSize.width += leftMarginSpacing_ + imageTitleSpacing_;
-  return buttonSize;
-}
-
-@end
 
 // A custom button that allows for setting a background color when hovered over.
 @interface BackgroundColorHoverButton : HoverImageButton {
  @private
   base::scoped_nsobject<NSColor> backgroundColor_;
   base::scoped_nsobject<NSColor> hoverColor_;
 }
 @end
 
 @implementation BackgroundColorHoverButton
@@ skipping 361 matching lines @@
       currentProfileView = [self createCurrentProfileView:item];
       enableLock = item.signed_in;
     } else {
       [otherProfiles addObject:[self createOtherProfileView:i]];
     }
   }
   if (!currentProfileView)  // Guest windows don't have an active profile.
     currentProfileView = [self createGuestProfileView];
 
   // |yOffset| is the next position at which to draw in |container|
-  // coordinates.
-  CGFloat yOffset = 0;
+  // coordinates. Add a pixel offset so that the bottom option buttons don't
+  // overlap the bubble's rounded corners.
+  CGFloat yOffset = 1;
 
   // Option buttons. Only available with the new profile management flag.
   if (switches::IsNewProfileManagement()) {
     NSRect rect = NSMakeRect(0, yOffset, kFixedMenuWidth, 0);
     NSView* optionsView = [self createOptionsViewWithRect:rect
                                                enableLock:enableLock];
     [container addSubview:optionsView];
     rect.origin.y = NSMaxY([optionsView frame]);
 
-    NSBox* separator = [self separatorWithFrame:rect];
+    NSBox* separator = [self horizontalSeparatorWithFrame:rect];
     [container addSubview:separator];
     yOffset = NSMaxY([separator frame]);
   }
 
   if (viewMode_ == profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER &&
       switches::IsFastUserSwitching()) {
     // Other profiles switcher. The profiles have already been sorted
     // by their y-coordinate, so they can be added in the existing order.
     for (NSView *otherProfileView in otherProfiles.get()) {
       [otherProfileView setFrameOrigin:NSMakePoint(0, yOffset)];
       [container addSubview:otherProfileView];
       yOffset = NSMaxY([otherProfileView frame]);
 
-      NSBox* separator =
-          [self separatorWithFrame:NSMakeRect(0, yOffset, kFixedMenuWidth, 0)];
+      NSBox* separator = [self horizontalSeparatorWithFrame:NSMakeRect(
+          0, yOffset, kFixedMenuWidth, 0)];
       [container addSubview:separator];
       yOffset = NSMaxY([separator frame]);
     }
   } else if (viewMode_ == profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT) {
     NSView* currentProfileAccountsView = [self createCurrentProfileAccountsView:
         NSMakeRect(0, yOffset, kFixedMenuWidth, 0)];
     [container addSubview:currentProfileAccountsView];
     yOffset = NSMaxY([currentProfileAccountsView frame]);
 
-    NSBox* accountsSeparator = [self separatorWithFrame:
+    NSBox* accountsSeparator = [self horizontalSeparatorWithFrame:
         NSMakeRect(0, yOffset, kFixedMenuWidth, 0)];
     [container addSubview:accountsSeparator];
     yOffset = NSMaxY([accountsSeparator frame]);
   }
 
   // For supervised users, add the disclaimer text.
   if (browser_->profile()->IsManaged()) {
     yOffset += kSmallVerticalSpacing;
     NSView* disclaimerContainer = [self createSupervisedUserDisclaimerView];
     [disclaimerContainer setFrameOrigin:NSMakePoint(0, yOffset)];
     [container addSubview:disclaimerContainer];
     yOffset = NSMaxY([disclaimerContainer frame]);
     yOffset += kSmallVerticalSpacing;
 
-    NSBox* separator =
-        [self separatorWithFrame:NSMakeRect(0, yOffset, kFixedMenuWidth, 0)];
+    NSBox* separator = [self horizontalSeparatorWithFrame:NSMakeRect(
+        0, yOffset, kFixedMenuWidth, 0)];
     [container addSubview:separator];
     yOffset = NSMaxY([separator frame]);
   }
 
   // Active profile card.
   if (currentProfileView) {
     yOffset += kVerticalSpacing;
     [currentProfileView setFrameOrigin:NSMakePoint(0, yOffset)];
     [container addSubview:currentProfileView];
     yOffset = NSMaxY([currentProfileView frame]) + kVerticalSpacing;
@@ skipping 364 matching lines @@
   [profileButton setBordered:NO];
   [profileButton setTag:itemIndex];
   [profileButton setTarget:self];
   [profileButton setAction:@selector(switchToProfile:)];
 
   return profileButton.autorelease();
 }
 
 - (NSView*)createOptionsViewWithRect:(NSRect)rect
                           enableLock:(BOOL)enableLock {
-  int widthOfLockButton = enableLock? 2 * kHorizontalSpacing + 12 : 0;
+  int widthOfLockButton = enableLock ? 2 * kHorizontalSpacing + 14 : 0;
   NSRect viewRect = NSMakeRect(0, 0,
                                rect.size.width - widthOfLockButton,
                                kBlueButtonHeight + kVerticalSpacing);
   NSString* text = isGuestSession_ ?
       l10n_util::GetNSString(IDS_PROFILES_EXIT_GUEST) :
       l10n_util::GetNSStringF(IDS_PROFILES_NOT_YOU_BUTTON,
           profiles::GetAvatarNameForProfile(browser_->profile()));
   NSButton* notYouButton =
       [self hoverButtonWithRect:viewRect
                            text:text
                 imageResourceId:IDR_ICON_PROFILES_MENU_AVATAR
        alternateImageResourceId:IDR_ICON_PROFILES_MENU_AVATAR
                          action:isGuestSession_? @selector(exitGuest:) :
                                                  @selector(showUserManager:)];
 
   rect.size.height = NSMaxY([notYouButton frame]);
   base::scoped_nsobject<NSView> container([[NSView alloc] initWithFrame:rect]);
   [container addSubview:notYouButton];
 
   if (enableLock) {
     viewRect.origin.x = NSMaxX([notYouButton frame]);
+    NSBox* separator = [self verticalSeparatorWithFrame:viewRect];
+    [container addSubview:separator];
+
+    viewRect.origin.x = NSMaxX([separator frame]);
     viewRect.size.width = widthOfLockButton;
     NSButton* lockButton =
         [self hoverButtonWithRect:viewRect
                              text:@""
                   imageResourceId:IDR_ICON_PROFILES_MENU_LOCK
          alternateImageResourceId:IDR_ICON_PROFILES_MENU_LOCK
                            action:@selector(lockProfile:)];
     [container addSubview:lockButton];
   }
 
@@ skipping 87 matching lines @@
           source, false /* auto_close */, true /* is_constrained */),
       content::Referrer(),
       content::PAGE_TRANSITION_AUTO_TOPLEVEL,
       std::string());
   NSView* webview = webContents_->GetNativeView();
   [webview setFrameSize:NSMakeSize(kFixedGaiaViewWidth, kFixedGaiaViewHeight)];
   [container addSubview:webview];
   yOffset = NSMaxY([webview frame]);
 
   // Adds the title card.
-  NSBox* separator = [self separatorWithFrame:
+  NSBox* separator = [self horizontalSeparatorWithFrame:
       NSMakeRect(0, yOffset, kFixedGaiaViewWidth, 0)];
   [container addSubview:separator];
   yOffset = NSMaxY([separator frame]) + kSmallVerticalSpacing;
 
   NSView* titleView = BuildTitleCard(
       NSMakeRect(0, yOffset, kFixedGaiaViewWidth,0),
       addSecondaryAccount ? IDS_PROFILES_GAIA_ADD_ACCOUNT_TITLE :
                             IDS_PROFILES_GAIA_SIGNIN_TITLE,
       self /* backButtonTarget*/,
       @selector(navigateBackFromSigninPage:) /* backButtonAction */);
@@ skipping 51 matching lines @@
     NSTextField* contentLabel = BuildLabel(contentStr, contentFrameOrigin,
         GetDialogBackgroundColor(), nil /* text_color */);
     [contentLabel setFrameSize:NSMakeSize(availableWidth, 0)];
     [GTMUILocalizerAndLayoutTweaker sizeToFitFixedWidthTextField:contentLabel];
     contentView = contentLabel;
   }
   [container addSubview:contentView];
   yOffset = NSMaxY([contentView frame]) + kVerticalSpacing;
 
   // Adds the title card.
-  NSBox* separator = [self separatorWithFrame:
+  NSBox* separator = [self horizontalSeparatorWithFrame:
       NSMakeRect(0, yOffset, kFixedAccountRemovalViewWidth, 0)];
   [container addSubview:separator];
   yOffset = NSMaxY([separator frame]) + kSmallVerticalSpacing;
 
   NSView* titleView = BuildTitleCard(
       NSMakeRect(0, yOffset, kFixedAccountRemovalViewWidth,0),
       IDS_PROFILES_ACCOUNT_REMOVAL_TITLE,
       self /* backButtonTarget*/,
       @selector(showAccountManagement:) /* backButtonAction */);
   [container addSubview:titleView];
@@ skipping 30 matching lines @@
   NSString* contentStr =
       l10n_util::GetNSString(IDS_PROFILES_END_PREVIEW_TEXT);
   NSTextField* contentLabel = BuildLabel(contentStr, contentFrameOrigin,
       GetDialogBackgroundColor(), nil /* text_color */);
   [contentLabel setFrameSize:NSMakeSize(availableWidth, 0)];
   [GTMUILocalizerAndLayoutTweaker sizeToFitFixedWidthTextField:contentLabel];
   [container addSubview:contentLabel];
   yOffset = NSMaxY([contentLabel frame]) + kVerticalSpacing;
 
   // Adds the title card.
-  NSBox* separator = [self separatorWithFrame:
+  NSBox* separator = [self horizontalSeparatorWithFrame:
       NSMakeRect(0, yOffset, kFixedEndPreviewViewWidth, 0)];
   [container addSubview:separator];
   yOffset = NSMaxY([separator frame]) + kSmallVerticalSpacing;
 
   NSView* titleView = BuildTitleCard(
       NSMakeRect(0, yOffset, kFixedEndPreviewViewWidth, 0),
       IDS_PROFILES_END_PREVIEW,
       self /* backButtonTarget*/,
       @selector(showSendFeedbackTutorial:) /* backButtonAction */);
   [container addSubview:titleView];
@@ skipping 94 matching lines @@
   [button addSubview:deleteButton];
   return button.autorelease();
 }
 
 - (void)postActionPerformed:(ProfileMetrics::ProfileDesktopMenu)action {
   ProfileMetrics::LogProfileDesktopMenu(action, serviceType_);
   serviceType_ = signin::GAIA_SERVICE_TYPE_NONE;
 }
 
 @end