fixed the distorted behavior on the mediastreaminfobar on Mac

chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm

 // 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.
 
 #import "chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.h"
 
 #import <Cocoa/Cocoa.h>
 #include <string>
 
 #import "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/media/media_stream_devices_menu_model.h"
 #include "chrome/browser/ui/cocoa/hover_close_button.h"
 #include "chrome/browser/ui/cocoa/infobars/infobar.h"
+#import "chrome/browser/ui/cocoa/infobars/infobar_utilities.h"
 #include "chrome/browser/ui/media_stream_infobar_delegate.h"
 #include "grit/generated_resources.h"
 #import "third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
 #include "ui/base/l10n/l10n_util.h"
 
+using InfoBarUtilities::CreateLabel;
+using InfoBarUtilities::MoveControl;
+using InfoBarUtilities::VerifyControlOrderAndSpacing;
 using l10n_util::GetNSStringWithFixup;
 
-static const CGFloat kSpaceBetweenControls = 8;
-
 InfoBar* MediaStreamInfoBarDelegate::CreateInfoBar(InfoBarTabHelper* owner) {
   MediaStreamInfoBarController* infobar_controller =
       [[MediaStreamInfoBarController alloc] initWithDelegate:this
                                                        owner:owner];
   return new InfoBar(infobar_controller, this);
 }
 
-namespace {
-
-// Puts |toMove| to the right or left of |anchor|.
-void SizeAndPlaceControl(NSView* toModify, NSView* anchor, bool after) {
-  [GTMUILocalizerAndLayoutTweaker sizeToFitView:toModify];
-  NSRect toModifyFrame = [toModify frame];
-  NSRect anchorFrame = [anchor frame];
-  if (after) {
-    toModifyFrame.origin.x = NSMaxX(anchorFrame) + kSpaceBetweenControls;
-  } else {
-    toModifyFrame.origin.x = NSMinX(anchorFrame) - kSpaceBetweenControls -
-        NSWidth(toModifyFrame);
-  }
-  [toModify setFrame:toModifyFrame];
-}
-
-}  // namespace
-
 
 @interface MediaStreamInfoBarController(Private)
 
 // Adds text for all buttons and text fields.
 - (void)setLabelTexts;
 
 // Populates the device menu with device id:s.
 - (void)rebuildDeviceMenu;
 
 // Arranges all buttons and pup-up menu relative each other.
 - (void)arrangeInfobarLayout;
 
+// Create all the components for the infobar.
+- (void)constructView;
+
 @end
 
 
 @implementation MediaStreamInfoBarController
 
 - (id)initWithDelegate:(MediaStreamInfoBarDelegate*)delegate
                  owner:(InfoBarTabHelper*)owner {
   if (self = [super initWithDelegate:delegate owner:owner]) {
     deviceMenuModel_.reset(new MediaStreamDevicesMenuModel(delegate));
-
-    label1_.reset([[NSTextField alloc] init]);
-    [label1_ setEditable:NO];
-    [label1_ setDrawsBackground:NO];
-    [label1_ setBordered:NO];
-
-    [okButton_ setBezelStyle:NSTexturedRoundedBezelStyle];
-    [cancelButton_ setBezelStyle:NSTexturedRoundedBezelStyle];
-
-    deviceMenu_.reset([[NSPopUpButton alloc] init]);
-    [deviceMenu_ setBezelStyle:NSTexturedRoundedBezelStyle];
-    [deviceMenu_ setAutoresizingMask:NSViewMaxXMargin];
-    [[deviceMenu_ cell] setArrowPosition:NSPopUpArrowAtBottom];
-    NSMenu* menu = [deviceMenu_ menu];
-    [menu setDelegate:nil];
-    [menu setAutoenablesItems:NO];
   }
   return self;
 }
 
 - (void)dealloc {
   [[NSNotificationCenter defaultCenter] removeObserver:self];
   [deviceMenu_ removeAllItems];
   [super dealloc];
 }
 
@@ skipping 21 matching lines @@
   [super removeSelf];
 }
 
 - (IBAction)deviceMenuChanged:(id)item {
   // Notify the menu model about the change and rebuild the menu.
   deviceMenuModel_->ExecuteCommand([item tag]);
   [self rebuildDeviceMenu];
 }
 
 - (void)addAdditionalControls {
-  // Get a frame to use as inital frame for all buttons.
-  NSRect initFrame = [okButton_ frame];
+  // Create all the components for the infobar.
+  [self constructView];
 
-  // Setup up the text label and add the device menu to the infobar.
-  // TODO(mflodman) Use |label_| instead.
-  [label1_ setFrame:[label_ frame]];
-  [[label_ superview] replaceSubview:label_ with:label1_.get()];
-  label_.reset();
+  // Get layout information from the NIB.
+  NSRect cancelButtonFrame = [cancelButton_ frame];
+  NSRect okButtonFrame = [okButton_ frame];
 
-  [deviceMenu_ setFrame:initFrame];
-  [infoBarView_ addSubview:deviceMenu_];
-
-  // Add text to all buttons and the text field.
-  [self setLabelTexts];
+  spaceBetweenControls_ = NSMaxX(cancelButtonFrame) < NSMinX(okButtonFrame) ?
+      NSMinX(okButtonFrame) - NSMaxX(cancelButtonFrame) :
+      NSMinX(cancelButtonFrame) - NSMaxX(okButtonFrame);
 
   // Arrange the initial layout.
   [self arrangeInfobarLayout];
 
-  // Build the device popup menu.
-  [self rebuildDeviceMenu];
-
   // Make sure we get notified of infobar view size changes.
   // TODO(mflodman) Find if there is a way to use 'setAutorezingMask' instead.
   [infoBarView_ setPostsFrameChangedNotifications:YES];
   [[NSNotificationCenter defaultCenter]
       addObserver:self
          selector:@selector(didChangeFrame:)
              name:NSViewFrameDidChangeNotification
            object:infoBarView_];
 }
 
@@ skipping 23 matching lines @@
   [okButton_ setTitle:GetNSStringWithFixup(IDS_MEDIA_CAPTURE_ALLOW)];
   [cancelButton_ setTitle:GetNSStringWithFixup(IDS_MEDIA_CAPTURE_DENY)];
 
   // Populating |deviceMenu_| is handled separately.
 }
 
 - (void)arrangeInfobarLayout {
   // Set correct size for text frame.
   [GTMUILocalizerAndLayoutTweaker sizeToFitView:label1_];
 
-  // Place |okButton_| to the right of the text field.
-  SizeAndPlaceControl(okButton_, label1_, true);
+  // From left to right: label_, cancelButton_ and okButton_.
+  MoveControl(label1_, cancelButton_, spaceBetweenControls_, true);
+  MoveControl(cancelButton_, okButton_, spaceBetweenControls_, true);
 
-  // Place |cancelButton| to the right of [okButton_|.
-  SizeAndPlaceControl(cancelButton_, okButton_, true);
+  // Build the device option popup menu.
+  [deviceMenu_ setHidden:NO];
+  [self rebuildDeviceMenu];
+  [[deviceMenu_ cell] setArrowPosition:NSPopUpArrowAtBottom];
+  [deviceMenu_ sizeToFit];
+  MoveControl(closeButton_, deviceMenu_, spaceBetweenControls_, false);
 
-  // |deviceMenu_| is floating to the right, besides |closeButton_|.
-  SizeAndPlaceControl(deviceMenu_, closeButton_, false);
+  // Hide the title of deviceMenu_ or even the whole menu when it overlaps
+  // with the okButton_.
+  if (!VerifyControlOrderAndSpacing(okButton_, deviceMenu_))  {
+    [[[deviceMenu_ menu] itemAtIndex:0] setTitle:@""];
+    [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_];
+    [[deviceMenu_ cell] setArrowPosition:NSPopUpArrowAtCenter];
+    MoveControl(closeButton_, deviceMenu_, spaceBetweenControls_, false);
+    if (!VerifyControlOrderAndSpacing(okButton_, deviceMenu_)) {
+      [deviceMenu_ setHidden:YES];
+    }
+  }
+}
+
+- (void)constructView {
+  // Use the ok button frame as inital frame for all components.
+  NSRect initFrame = [okButton_ frame];
+
+  // Setup the text label and add the device menu to the infobar.
+  // TODO(mflodman) Use |label_| instead.
+  label1_.reset([[NSTextField alloc] init]);
+  [label1_ setEditable:NO];
+  [label1_ setDrawsBackground:NO];
+  [label1_ setBordered:NO];
+  [label1_ setFrame:[label_ frame]];
+  [[label_ superview] replaceSubview:label_ with:label1_.get()];

[Scott Hess - ex-Googler, 2012/07/26 19:39:41]

Does this really need the get() on label1_?  It shouldn't need it if label_
doesn't need it.

[no longer working on chromium, 2012/07/27 10:09:55]

Old code, but strangely all the replaceSubview API is called with .get() in
chrome. 
But I checked the scoped_nsobject source code, it should be all right to skip
the .get()

[Scott Hess - ex-Googler, 2012/07/27 16:35:20]

I think the rule of thumb is to use the non-get() version unless it doesn't
compile.  Objective-C is dynamic, so having the right typing info only changes
compile-time warnings.  But if the method's receiver is ambiguously typed, that
can make the method typing ambiguous because it doesn't know which is the real
receiver, and then it doesn't know how to derive the type correctly.

+  label_.reset();
+
+  [okButton_ setBezelStyle:NSTexturedRoundedBezelStyle];
+  [cancelButton_ setBezelStyle:NSTexturedRoundedBezelStyle];
+
+  // Add text to all buttons and the text field.
+  [self setLabelTexts];
+
+  // Setup the device options menu.
+  deviceMenu_.reset([[NSPopUpButton alloc] initWithFrame:initFrame
+                                               pullsDown:YES]);
+  [deviceMenu_ setBezelStyle:NSTexturedRoundedBezelStyle];
+  NSMenu* menu = [deviceMenu_ menu];
+  [menu setDelegate:nil];
+  [menu setAutoenablesItems:NO];
+
+  [deviceMenu_ setAutoresizingMask:NSViewMinXMargin];

[Scott Hess - ex-Googler, 2012/07/26 19:39:41]

Move this above the NSMenu line so the deviceMenu_ setup is together.

[no longer working on chromium, 2012/07/27 10:09:55]

Done.

+  // Add "options" popup z-ordered below all other controls so when we
+  // resize the toolbar it doesn't hide them.
+  [infoBarView_ addSubview:deviceMenu_
+                positioned:NSWindowBelow
+                relativeTo:nil];
 }
 
 - (void)rebuildDeviceMenu {
   // Remove all old menu items and rebuild from scratch.
   [deviceMenu_ removeAllItems];
   NSMenu* menu = [deviceMenu_ menu];
 
   // Add title item.
   NSString* menuTitle = GetNSStringWithFixup(
       IDS_MEDIA_CAPTURE_DEVICES_MENU_TITLE);
@@ skipping 16 matching lines @@
                                          action:@selector(deviceMenuChanged:)
                                   keyEquivalent:@""];
       [item setTarget:self];
       [item setTag:deviceMenuModel_->GetCommandIdAt(i)];
       if (deviceMenuModel_->IsItemCheckedAt(i)) {
         [item setState:NSOnState];
       }
       [item setEnabled:deviceMenuModel_->IsEnabledAt(i)];
     }
   }
-  [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_];

[Scott Hess - ex-Googler, 2012/07/26 19:39:41]

It looks to me like the GTM version of sizeToFit has substantial changes WRT
buttons.  I suspect that you probably want to use that version of sizeToFit
instead of the button's default -sizeToFit.  Maybe test both ways and see what
the visual impact is, like does the button shift position, or does the text w/in
the button shift position, relative to the baseline of the other overview text.

[no longer working on chromium, 2012/07/27 10:09:55]

I guess you what meant is deviceMenu_ sizeToFit]; in line 163.
It works the same as using  GTM, so I changed it to GTM.

[Scott Hess - ex-Googler, 2012/07/27 16:35:20]

Basically, I think you probably need to use the GTM version for all -[NSButton
sizeToFit] calls, and never mix them for the same control unless you're really
careful.  I don't fully understand what GTM is doing, here, my best guess is
that it's trying to maintain some Interface Builder sizing quirks so that manual
changes don't cause weird sizing changes.

The only case I'm really concerned about is with the @"" title, where it might
fit to a smaller image-only outline.  If that happens, you might want to grab
the height of okButton_ and make it square.  But if it works as-is, leave it be
(it might also fit to the font for height and the text+image for width).

[no longer working on chromium, 2012/07/30 11:36:31]

Thanks for pointing out. I just found out that it was not a square button in @""
title case. So we we need to make it square explicitly. Meanwhile,  we need to
skid calling GTM sizeToFit function in this case, otherwise the square size
won't apply. Hope it is fine.

 }
 
 - (void)didChangeFrame:(NSNotification*)notification {
   [self arrangeInfobarLayout];
 }
 
 @end  // implementation MediaStreamInfoBarController