Changes cocoa implementation of permission bubble to better match mocks.

chrome/browser/ui/cocoa/website_settings/permission_bubble_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 "chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h"
 
 #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"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #import "chrome/browser/ui/chrome_style.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_button.h"
 #import "chrome/browser/ui/cocoa/hover_close_button.h"
 #import "chrome/browser/ui/cocoa/hyperlink_text_view.h"
 #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"
 #include "ui/base/models/simple_menu_model.h"
 
 using base::UserMetricsAction;
 
 namespace {
 
 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)
       : bubble_controller_(bubble) {}
   virtual bool IsCommandIdChecked(int command_id) const OVERRIDE {
     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,
       ui::Accelerator* accelerator) OVERRIDE {
     return false;
   }
   virtual void ExecuteCommand(int command_id, int event_flags) OVERRIDE {
     [bubble_controller_ onMenuItemClicked:command_id];
   }
  private:
   PermissionBubbleController* bubble_controller_;  // Weak, owns us.
   DISALLOW_COPY_AND_ASSIGN(MenuDelegate);
 };
 
 }  // 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|.
 - (NSView*)labelForRequest:(PermissionBubbleRequest*)request;
 
 // Returns an autoreleased NSView displaying the title for the bubble
 // 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
                     action:(SEL)action;
 
 // Returns an autoreleased NSView displaying a block button.
 - (NSView*)blockButton;
 
 // Returns an autoreleased NSView with a block button and a drop-down menu
 // with one item, which will change the UI to allow customizing the permissions.
 - (NSView*)blockButtonWithCustomizeMenu;
 
 // Returns an autoreleased NSView displaying the close 'x' button.
 - (NSView*)closeButton;
 
 // Sets the width of both |viewA| and |viewB| to be the larger of the
 // 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;
 
 // Called when the 'allow' button is pressed.
 - (void)onAllow:(id)sender;
 
 // Called when the 'block' button is pressed.
 - (void)onBlock:(id)sender;
 
 // Called when the 'close' button is pressed.
 - (void)onClose:(id)sender;
 
 // 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
 
 - (id)initWithParentWindow:(NSWindow*)parentWindow
                     bridge:(PermissionBubbleCocoa*)bridge {
   DCHECK(parentWindow);
   DCHECK(bridge);
   base::scoped_nsobject<InfoBubbleWindow> window([[InfoBubbleWindow alloc]
       initWithContentRect:ui::kWindowSizeDeterminedLater
@@ skipping 36 matching lines @@
     allowOrOkButton.reset([[self buttonWithTitle:okTitle
                                           action:@selector(ok:)] retain]);
   } else {
     NSString* allowTitle = l10n_util::GetNSString(IDS_PERMISSION_ALLOW);
     allowOrOkButton.reset([[self buttonWithTitle:allowTitle
                                           action:@selector(onAllow:)] retain]);
   }
   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]);
   }
 
   // The maximum width of the above permissions will dictate the width of the
   // bubble.  It is calculated here so that it can be used for the positioning
   // of the buttons.
   NSRect bubbleFrame = NSZeroRect;
   for (NSView* view in [contentView subviews]) {
     bubbleFrame = NSUnionRect(
         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]);
   [contentView addSubview:titleView];
   [titleView setFrameOrigin:NSMakePoint(kHorizontalPadding,
                                         kVerticalPadding + yOffset)];
   yOffset += NSHeight([titleView frame]) + kVerticalPadding;
 
   // The title must fit within the bubble.
   bubbleFrame.size.width = std::max(NSWidth(bubbleFrame),
@@ skipping 63 matching lines @@
   [permissionIcon setFrameSize:[[permissionIcon image] size]];
   [permissionView addSubview:permissionIcon];
 
   base::scoped_nsobject<NSTextField> permissionLabel(
       [[NSTextField alloc] initWithFrame:NSZeroRect]);
   base::string16 label = request->GetMessageTextFragment();
   [permissionLabel setDrawsBackground:NO];
   [permissionLabel setBezeled:NO];
   [permissionLabel setEditable:NO];
   [permissionLabel setSelectable:NO];
+  [permissionLabel setFont:[NSFont systemFontOfSize:kPermissionFontSize]];
   [permissionLabel setStringValue:base::SysUTF16ToNSString(label)];
   [permissionLabel sizeToFit];
   [permissionLabel setFrameOrigin:
       NSMakePoint(NSWidth([permissionIcon frame]), 0)];
   [permissionView addSubview:permissionLabel];
 
   // Match the horizontal centers of the two subviews.  Note that the label's
   // center is rounded down, and the icon's center, up.  It looks better that
   // way - with the text's center slightly lower than the icon's center - if the
   // height delta is not evenly split.
@@ skipping 16 matching lines @@
 - (NSView*)titleWithHostname:(const std::string&)host {
   base::scoped_nsobject<NSTextField> titleView(
       [[NSTextField alloc] initWithFrame:NSZeroRect]);
   [titleView setDrawsBackground:NO];
   [titleView setBezeled:NO];
   [titleView setEditable:NO];
   [titleView setSelectable:NO];
   [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,
                                      NSHeight(titleFrame))];
   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
                     action:(SEL)action {
   base::scoped_nsobject<NSButton> button(
       [[ConstrainedWindowButton alloc] initWithFrame:NSZeroRect]);
   [button setButtonType:NSMomentaryPushInButton];
   [button setTitle:title];
   [button setTarget:self];
   [button setAction:action];
@@ skipping 30 matching lines @@
 
 - (CGFloat)matchWidthsOf:(NSView*)viewA andOf:(NSView*)viewB {
   NSRect frameA = [viewA frame];
   NSRect frameB = [viewB frame];
   CGFloat width = std::max(NSWidth(frameA), NSWidth(frameB));
   [viewA setFrameSize:NSMakeSize(width, NSHeight(frameA))];
   [viewB setFrameSize:NSMakeSize(width, NSHeight(frameB))];
   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();
 }
 
 - (void)onAllow:(id)sender {
   DCHECK(delegate_);
   delegate_->Accept();
 }
 
 - (void)onBlock:(id)sender {
   DCHECK(delegate_);
   delegate_->Deny();
 }
 
 - (void)onClose:(id)sender {
   DCHECK(delegate_);
   delegate_->Closing();
 }
 
 - (void)onCustomize:(id)sender {
   DCHECK(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];
 }
 
 @end  // implementation PermissionBubbleController