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;

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

Should use of this also be replaced by spaceBetweenControls_?

[no longer working on chromium, 2012/07/26 10:17:47]

removed

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

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

AFAICT, this is the only user of kSpaceBetweenControls, and this is no longer
called by anyone.

[no longer working on chromium, 2012/07/26 10:17:47]

Good catch. They are removed now.

   [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;
+- (void)rebuildDeviceMenu:(BOOL)hideTitle;
 
 // Arranges all buttons and pup-up menu relative each other.
 - (void)arrangeInfobarLayout;
 
+// Create all the components for the infobar.
+- (void)constructView;
+
+// Used for setting the correct styles on each label and button.
+- (NSArray*)allControls;
+
 @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 17 matching lines @@
 - (void)cancel:(id)sender {
   static_cast<MediaStreamInfoBarDelegate*>([self delegate])->Deny();
 
   // Remove the infobar, we're done.
   [super removeSelf];
 }
 
 - (IBAction)deviceMenuChanged:(id)item {
   // Notify the menu model about the change and rebuild the menu.
   deviceMenuModel_->ExecuteCommand([item tag]);
-  [self rebuildDeviceMenu];
+  [self rebuildDeviceMenu:NO];
 }
 
 - (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);

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

There are cases in our UI where the effective boundaries of buttons extends
somewhat beyond the visual boundaries, resulting in a slight overlap.  This
would be more robust if the condition is NSMinX() < NSMinX().

[no longer working on chromium, 2012/07/26 10:17:47]

The condition is to guarantee that the spaceBetweenControls_ is a positive
value, in case switching to NSMinX() < NSMinX(), is there any chance of getting
a negative value when the overlap happens?

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

Hmm.  I read the condition as making sure that it calculated the right
difference regardless of the ordering of the buttons.

As I mentioned, there have been cases where overlap between buttons was
indicated.  If the button overlap is intentional, then you should be using a
negative number.  If the button overlap is unintentional, then taking the
absolute value will not make it correct.  If you want to enforce sanity, just
take the difference and use a DCHECK_GT(space, 0), don't arbitrarily invert the
nib file's spacing.

 
   // Arrange the initial layout.
   [self arrangeInfobarLayout];
 
-  // Build the device popup menu.
-  [self rebuildDeviceMenu];
+  // Add and configure controls that are visible in all modes.
+  [deviceMenu_ setAutoresizingMask:NSViewMinXMargin];

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

Can this line move to the initialization section in -constructView?  It looks
like it was originally in the -init*, after -setBezelStyle.

[no longer working on chromium, 2012/07/26 10:17:47]

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

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

Logically, this feels like it belongs somewhere else, like maybe -constructView
(since it's always in the hierarchy), or -arrangeInfobarLayout (if it were to
optionally be in the hierarchy).

[no longer working on chromium, 2012/07/26 10:17:47]

Done.

+  [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_];

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

This seems like it might be contrary to your -sizeToFit calls when arranging the
layout.  Maybe it wouldn't work right in some locales?  In the previous code
SizeAndPlaceControl() was calling this when placing things.

[no longer working on chromium, 2012/07/26 10:17:47]

removed this -sizeToFit function here.

 
   // 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 24 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_, okButton_, cancelButton_ and deviceMenu_.

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

The comment implies that it's label, cancel, ok.  From the code below, it looks
like deviceMenu_ actually is maybe left of closeButton_, with it slightly
different than right of okButton_, so maybe don't include it in this list at
all.

[no longer working on chromium, 2012/07/26 10:17:47]

thanks. In linux and windows, the cancel button is right of the ok button, I
messed them up in the comments.

+  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:NO];
+  [[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 deviceMenu_ in case there is overlap.

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

I think the comment could be clearer, it's only hidden if the smaller version
cannot fit.

[no longer working on chromium, 2012/07/26 10:17:47]

Done.

+  if (!VerifyControlOrderAndSpacing(okButton_, deviceMenu_))  {
+    [self rebuildDeviceMenu:YES];

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

I think that this would make more sense if you had -rebuildDeviceMenu as it used
to be, and then call [[[deviceMenu_ menu] itemAtIndex:0] setTitle:@""] here.  Or
wrap it in a -hideDeviceMenuTitle method if you like.

[no longer working on chromium, 2012/07/26 10:17:47]

Good idea, then we don't need to rebuild the menu for each resize.

+    NSRect oldFrame = [deviceMenu_ frame];
+    oldFrame.size.width = NSHeight(oldFrame);
+    [deviceMenu_ setFrame:oldFrame];

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

Does -sizeToFit work here?  Or does that allow things to get _too_ small?

If things get too small in that case, then I suggest capturing the deviceMenu_
frame _before_ title-hiding, to make sure you don't get a re-calculated size.

[no longer working on chromium, 2012/07/26 10:17:47]

Thanks for pointing it out. -sizeToFit works the same here.

+    [[deviceMenu_ cell] setArrowPosition:NSPopUpArrowAtCenter];
+    MoveControl(closeButton_, deviceMenu_, spaceBetweenControls_, false);
+    if (!VerifyControlOrderAndSpacing(okButton_, deviceMenu_)) {
+      [deviceMenu_ setHidden:YES];
+    }
+  }
 }
 
-- (void)rebuildDeviceMenu {
+- (void)constructView {
+  // Use the ok button frame as inital frame for all components.
+  NSRect initFrame = [okButton_ frame];
+
+  // Setup up 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()];
+  label_.reset();
+
+  deviceMenu_.reset([[NSPopUpButton alloc] initWithFrame:initFrame
+                                               pullsDown:YES]);
+  [deviceMenu_ setBezelStyle:NSTexturedRoundedBezelStyle];
+  NSMenu* menu = [deviceMenu_ menu];
+  [menu setDelegate:nil];
+  [menu setAutoenablesItems:NO];
+
+  // Add text to all buttons and the text field.
+  [self setLabelTexts];
+
+  // Set each of the label and buttons to the NSTexturedRoundedBezelStyle
+  // (metal-looking) style.
+  NSArray* allControls = [self allControls];

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

Is -allControls used anywhere else.  As things currently stand, I think this
loop is worse than unrolling things for the two affected buttons.

[Unless you have a lot of controls, like 8, I would greatly prefer just
replicating the code.  It's a little like table-driven tests, it's easier to see
what the actual items are being set to when they're just listed out.]

[no longer working on chromium, 2012/07/26 10:17:47]

Done.

+  for (NSControl* control in allControls) {
+    if (![control isKindOfClass:[NSButton class]])
+      continue;
+    NSButton* button = (NSButton*)control;
+    [button setBezelStyle:NSTexturedRoundedBezelStyle];
+    if ([button isKindOfClass:[NSPopUpButton class]]) {
+      [[button cell] setArrowPosition:NSPopUpArrowAtBottom];

[Scott Hess - ex-Googler, 2012/07/25 18:28:15]

My guess is that at one point you had deviceMenu_ in -allControls, and then had
to back it out?

[no longer working on chromium, 2012/07/26 10:17:47]

sorry for the uncleaned debug code, I had moved the call out to
-arrangeInfobarLayout 

+    }
+  }
+}
+
+- (NSArray*)allControls {
+  return [NSArray arrayWithObjects:label1_.get(), okButton_,
+          cancelButton_, nil];
+}
+
+- (void)rebuildDeviceMenu:(BOOL)hideTitle {
   // Remove all old menu items and rebuild from scratch.
   [deviceMenu_ removeAllItems];
   NSMenu* menu = [deviceMenu_ menu];
 
   // Add title item.
-  NSString* menuTitle = GetNSStringWithFixup(
+  NSString* menuTitle = hideTitle ? @"" : GetNSStringWithFixup(
       IDS_MEDIA_CAPTURE_DEVICES_MENU_TITLE);
   NSMenuItem* titleItem =
       [menu addItemWithTitle:menuTitle
                       action:NULL
                keyEquivalent:@""];
   [titleItem setState:NSOffState];
 
   [menu addItem:[NSMenuItem separatorItem]];
 
   // Add all capture devices.
@@ skipping 15 matching lines @@
     }
   }
   [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_];
 }
 
 - (void)didChangeFrame:(NSNotification*)notification {
   [self arrangeInfobarLayout];
 }
 
 @end  // implementation MediaStreamInfoBarController