Changes cocoa implementation of permission bubble to better match mocks.

chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm

Index: chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
diff --git a/chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm b/chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
index 0b6f6f03f8b6c240922da6e39e02ff2e961e8f46..13b88a8db343881c23af2ef06174d6afb04c3f1d 100644
--- a/chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
+++ b/chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/mac/bind_objc_block.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_util.h"
 #include "base/strings/sys_string_conversions.h"
@@ -20,12 +21,15 @@
 #import "chrome/browser/ui/cocoa/info_bubble_view.h"
 #import "chrome/browser/ui/cocoa/info_bubble_window.h"
 #include "chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa.h"
+#include "chrome/browser/ui/cocoa/website_settings/permission_selector_button.h"
 #include "chrome/browser/ui/cocoa/website_settings/split_block_button.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_view.h"
+#include "chrome/browser/ui/website_settings/permission_menu_model.h"
 #include "content/public/browser/user_metrics.h"
 #include "grit/generated_resources.h"
 #include "skia/ext/skia_utils_mac.h"
+#import "ui/base/cocoa/menu_controller.h"
 #include "ui/base/cocoa/window_size_constants.h"
 #import "ui/base/cocoa/menu_controller.h"
 #include "ui/base/l10n/l10n_util_mac.h"
@@ -39,9 +43,9 @@ const CGFloat kHorizontalPadding = 20.0f;
 const CGFloat kVerticalPadding = 20.0f;
 const CGFloat kButtonPadding = 10.0f;
 const CGFloat kTitlePaddingX = 50.0f;
-const CGFloat kCheckboxYAdjustment = 2.0f;
+const CGFloat kTitleFontSize = 15.0f;
+const CGFloat kPermissionFontSize = 12.0f;
 
-const CGFloat kFontSize = 15.0f;
 class MenuDelegate : public ui::SimpleMenuModel::Delegate {
  public:
   explicit MenuDelegate(PermissionBubbleController* bubble)
@@ -50,10 +54,7 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
     return false;
   }
   virtual bool IsCommandIdEnabled(int command_id) const OVERRIDE {
-    // TODO(leng):  Change this to true once setting the bubble to be
-    // customizable works properly.  Ideally, the bubble will alter its
-    // contents, rather than reshowing completely.
-    return false;
+    return true;
   }
   virtual bool GetAcceleratorForCommandId(
       int command_id,
@@ -70,6 +71,61 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
 
 }  // namespace
 
+// NSPopUpButton with a menu containing two items: allow and block.
+// One AllowBlockMenuButton is used for each requested permission, but only when
+// the permission bubble is in 'customize' mode.
+@interface AllowBlockMenuButton : NSPopUpButton {
+ @private
+  int index_;
+  scoped_ptr<PermissionMenuModel> menuModel_;
+  base::scoped_nsobject<MenuController> menuController_;
+}
+
+- (id)initWithIndex:(int)index
+             forURL:(const GURL&)url
+            allowed:(BOOL)allow
+       withCallback:(void (^)(int, bool))callback;
+@end
+
+@implementation AllowBlockMenuButton
+
+- (id)initWithIndex:(int)index
+             forURL:(const GURL&)url
+            allowed:(BOOL)allow
+       withCallback:(void (^)(int, bool))callback {
+  if (self = [super initWithFrame:NSMakeRect(0, 0, 1, 1) pullsDown:NO]) {
+    index_ = index;
+    ContentSetting setting =
+        allow ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
+    [self setFont:[NSFont systemFontOfSize:kPermissionFontSize]];
+    [self setBordered:NO];
+
+    PermissionMenuModel::ChangeCallback changeCallback =
+        base::BindBlock(^(const WebsiteSettingsUI::PermissionInfo& permission) {
+            callback(index_, permission.setting == CONTENT_SETTING_ALLOW);
+        });
+
+    menuModel_.reset(new PermissionMenuModel(url, setting, changeCallback));
+    menuController_.reset([[MenuController alloc] initWithModel:menuModel_.get()
+                                         useWithPopUpButtonCell:NO]);
+    [self setMenu:[menuController_ menu]];
+    [[self cell] selectItemAtIndex:menuModel_->GetIndexOfCommandId(setting)];

[groby-ooo-7-16, 2014/04/18 18:18:18]

I don't think you need the cell for selectItemAtIndex:

[leng, 2014/04/18 22:48:46]

The first time I tried, it didn't work.  Must've had something else wrong.
Done.

+    [self sizeToFit];
+    // Adjust the size to fit the current title.  Using only -sizeToFit leaves
+    // an ugly amount of whitespace between the title and the arrows.
+    [self setFrameSize:[PermissionSelectorButton sizeForTitle:[self title]

[groby-ooo-7-16, 2014/04/18 18:18:18]

I'm not happy with the tight coupling between the  two classes. Any way the to
have the controller do the sizing? (Also - does this still need sizeToFit
above?)

[leng, 2014/04/18 22:48:46]

As per our conversation offline, I'll make a follow-up CL that moves
website_settings_bubble_controller into website_settings/ so that both
controllers can share a helper function.   Code is copied, with a TODO, for now.

+                                                    forButton:self]];
+  }
+  return self;
+}
+
+// Getter for testing
+- (int)index {
+  return index_;
+}
+
+@end
+
 @interface PermissionBubbleController ()
 
 // Returns an autoreleased NSView displaying the icon and label for |request|.
@@ -79,10 +135,11 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
 // requesting settings for |host|.
 - (NSView*)titleWithHostname:(const std::string&)host;
 
-// Returns an autoreleased NSView displaying a checkbox for |request|.  The
-// checkbox will be initialized as checked if |checked| is YES.
-- (NSView*)checkboxForRequest:(PermissionBubbleRequest*)request
-                      checked:(BOOL)checked;
+// Returns an autoreleased NSView displaying a menu for |request|.  The
+// menu will be initialized as 'allow' if |allow| is YES.
+- (NSView*)menuForRequest:(PermissionBubbleRequest*)request

[groby-ooo-7-16, 2014/04/18 18:18:18]

menuViewForRequest? 

[leng, 2014/04/18 22:48:46]

Would you like me to change the other names, as well?  If not, I'd rather keep
this - it matches 'labelForRequest' and 'buttonWithTitle', which both also
return NSViews.

+                  atIndex:(int)index
+                    allow:(BOOL)allow;
 
 // Returns an autoreleased NSView of a button with |title| and |action|.
 - (NSView*)buttonWithTitle:(NSString*)title
@@ -102,6 +159,10 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
 // two views' widths.  Does not change either view's origin or height.
 - (CGFloat)matchWidthsOf:(NSView*)viewA andOf:(NSView*)viewB;
 
+// Sets the offset of |viewA| so that its vertical center is aligned with the
+// vertical center of |viewB|.
+- (void)alignCenterOf:(NSView*)viewA verticallyToCenterOf:(NSView*)viewB;

[groby-ooo-7-16, 2014/04/18 18:18:18]

I'd probably do that as a standalone function, because it doesn't require self.
(It's also a very generic function which maybe one day can be lifted out...)

[leng, 2014/04/18 22:48:46]

Done.

+
 // Called when the 'ok' button is pressed.
 - (void)ok:(id)sender;
 
@@ -117,9 +178,6 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
 // Called when the 'customize' button is pressed.
 - (void)onCustomize:(id)sender;
 
-// Called when a checkbox changes from checked to unchecked, or vice versa.
-- (void)onCheckboxChanged:(id)sender;
-
 @end
 
 @implementation PermissionBubbleController
@@ -176,25 +234,31 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   CGFloat yOffset = 2 * kVerticalPadding + NSMaxY([allowOrOkButton frame]);
   BOOL singlePermission = requests.size() == 1;
 
-  checkboxes_.reset(customizationMode ? [[NSMutableArray alloc] init] : nil);
+  base::scoped_nsobject<NSMutableArray> permissionMenus;
+  if (customizationMode)
+    permissionMenus.reset([[NSMutableArray alloc] init]);
+
   for (auto it = requests.begin(); it != requests.end(); it++) {
-    base::scoped_nsobject<NSView> permissionView;
-    if (customizationMode) {
-      int index = it - requests.begin();
-      permissionView.reset(
-          [[self checkboxForRequest:(*it)
-                            checked:acceptStates[index] ? YES : NO] retain]);
-      [base::mac::ObjCCastStrict<NSButton>(permissionView) setTag:index];
-      [checkboxes_ addObject:permissionView];
-    } else {
-      permissionView.reset([[self labelForRequest:(*it)] retain]);
-    }
+    base::scoped_nsobject<NSView> permissionView(
+        [[self labelForRequest:(*it)] retain]);
     NSPoint origin = [permissionView frame].origin;
     origin.x += kHorizontalPadding;
     origin.y += yOffset;
     [permissionView setFrameOrigin:origin];
     [contentView addSubview:permissionView];
 
+    if (customizationMode) {
+      int index = it - requests.begin();
+      base::scoped_nsobject<NSView> menu(
+          [[self menuForRequest:(*it)
+                        atIndex:index
+                          allow:acceptStates[index] ? YES : NO] retain]);
+      // Align vertically.  Horizontal alignment will be adjusted once the
+      // widest permission is know.
+      [self alignCenterOf:menu verticallyToCenterOf:permissionView];
+      [permissionMenus addObject:menu];
+      [contentView addSubview:menu];
+    }
     yOffset += NSHeight([permissionView frame]);
   }
 
@@ -207,6 +271,17 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
         bubbleFrame, NSInsetRect([view frame], -kHorizontalPadding, 0));
   }
 
+  if (customizationMode) {
+    // Adjust the horizontal origin for each menu.
+    CGFloat xOffset = NSWidth(bubbleFrame) - kHorizontalPadding;
+    for (NSView* view in permissionMenus.get()) {
+      [view setFrameOrigin:NSMakePoint(xOffset, NSMinY([view frame]))];
+    }
+    // And add the menu width to the bubble's width.
+    NSView* menu = base::mac::ObjCCast<NSView>(permissionMenus[0]);

[groby-ooo-7-16, 2014/04/18 18:18:18]

Menu 0 is OK? We don't have varying sizes?

Also: Didn't the NSUnionRect above take care of incorporating the menu size?

[leng, 2014/04/18 22:48:46]

Changed to use a max width.
The NSUnionRect, above, does not handle this.  Until we adjust the menu
horizontally, the union won't help the overall width - the frame's origin is at
(0, y).

+    bubbleFrame.size.width += NSWidth([menu frame]);
+  }
+
   base::scoped_nsobject<NSView> titleView(
       [[self titleWithHostname:requests[0]->GetRequestingHostname().host()]
           retain]);
@@ -290,6 +365,7 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   [permissionLabel setBezeled:NO];
   [permissionLabel setEditable:NO];
   [permissionLabel setSelectable:NO];
+  [permissionLabel setFont:[NSFont systemFontOfSize:kPermissionFontSize]];
   [permissionLabel setStringValue:base::SysUTF16ToNSString(label)];
   [permissionLabel sizeToFit];
   [permissionLabel setFrameOrigin:
@@ -326,7 +402,7 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   [titleView setStringValue:
       l10n_util::GetNSStringF(IDS_PERMISSIONS_BUBBLE_PROMPT,
                               base::UTF8ToUTF16(host))];
-  [titleView setFont:[NSFont systemFontOfSize:kFontSize]];
+  [titleView setFont:[NSFont systemFontOfSize:kTitleFontSize]];
   [titleView sizeToFit];
   NSRect titleFrame = [titleView frame];
   [titleView setFrameSize:NSMakeSize(NSWidth(titleFrame) + kTitlePaddingX,
@@ -334,20 +410,21 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   return titleView.autorelease();
 }
 
-- (NSView*)checkboxForRequest:(PermissionBubbleRequest*)request
-                      checked:(BOOL)checked {
+- (NSView*)menuForRequest:(PermissionBubbleRequest*)request
+                  atIndex:(int)index
+                    allow:(BOOL)allow {
   DCHECK(request);
-  base::scoped_nsobject<NSButton> checkbox(
-      [[NSButton alloc] initWithFrame:NSZeroRect]);
-  [checkbox setButtonType:NSSwitchButton];
-  base::string16 permission = request->GetMessageTextFragment();
-  [checkbox setTitle:base::SysUTF16ToNSString(permission)];
-  [checkbox setState:(checked ? NSOnState : NSOffState)];
-  [checkbox setTarget:self];
-  [checkbox setAction:@selector(onCheckboxChanged:)];
-  [checkbox sizeToFit];
-  [checkbox setFrameOrigin:NSMakePoint(0, kCheckboxYAdjustment)];
-  return checkbox.autorelease();
+  DCHECK(delegate_);
+  PermissionBubbleView::Delegate* blockDelegate = delegate_;
+  base::scoped_nsobject<AllowBlockMenuButton> button(
+      [[AllowBlockMenuButton alloc]
+          initWithIndex:index
+                 forURL:request->GetRequestingHostname()
+                allowed:allow
+           withCallback:^(int index, bool allow) {

[groby-ooo-7-16, 2014/04/18 18:18:18]

Why not just pass down the blockDelegate and save ourselves an NSBlock in the
process? (Prebind it with index, that way the button doesn't need to keep track
of that)

If you want to make bigger changes, there's this solution:

Have a standalone function:
    void AllowCallback(BlockDelegate* delegate, int index, const
WebsiteSettingsUI::PermissionInfo& permission)  
    {
      delegate_->ToggleAccept(index, permission.setting ==
CONTENT_SETTING_ALLOW);
    }
And turn that into PermissionMenuModel::ChangeCallback via
    callback = base::Bind(AllowCallback, base::Unretained(delegate_), index)

(We could save ourselves even more messiness if ToggleAccept took a
PermissionInfo.)

[leng, 2014/04/18 22:48:46]

Good idea.  Done.  I did the simplest version, which I find most
straightforward.

+               blockDelegate->ToggleAccept(index, allow);
+           }]);
+  return button.autorelease();
 }
 
 - (NSView*)buttonWithTitle:(NSString*)title
@@ -398,6 +475,14 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   return width;
 }
 
+- (void)alignCenterOf:(NSView*)viewA verticallyToCenterOf:(NSView*)viewB {

[groby-ooo-7-16, 2014/04/18 18:18:18]

Grumble. I wish we had helper functions for this.

[leng, 2014/04/18 22:48:46]

Me, too.

+  NSRect frameA = [viewA frame];
+  NSRect frameB = [viewB frame];
+  frameA.origin.y =
+      NSMinY(frameB) + std::floor((NSHeight(frameB) - NSHeight(frameA)) / 2);
+  [viewA setFrameOrigin:frameA.origin];
+}
+
 - (void)ok:(id)sender {
   DCHECK(delegate_);
   delegate_->Closing();
@@ -423,12 +508,6 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   delegate_->SetCustomizationMode();
 }
 
-- (void)onCheckboxChanged:(id)sender {
-  DCHECK(delegate_);
-  NSButton* checkbox = base::mac::ObjCCastStrict<NSButton>(sender);
-  delegate_->ToggleAccept([checkbox tag], [checkbox state] == NSOnState);
-}
-
 - (void)onMenuItemClicked:(int)commandId {
   DCHECK(commandId == 0);
   [self onCustomize:nil];