Changes cocoa implementation of permission bubble to better match mocks.

Specifically, when the bubble is in 'customize' mode, the UI to change
the setting of a permission (allow/deny) will be a drop-down menu rather
than a checkbox.

BUG=None

Review URL: https://codereview.chromium.org/242443005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266689 0039d316-1c4b-4281-b951-d872f2087c98

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,10 @@ 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 kPermissionButtonTitleRightPadding = 4.0f;
 
-const CGFloat kFontSize = 15.0f;
 class MenuDelegate : public ui::SimpleMenuModel::Delegate {
  public:
   explicit MenuDelegate(PermissionBubbleController* bubble)
@@ -50,10 +55,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 +72,65 @@ 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
+  scoped_ptr<PermissionMenuModel> menuModel_;
+  base::scoped_nsobject<MenuController> menuController_;
+}
+
+- (id)initForURL:(const GURL&)url
+         allowed:(BOOL)allow
+           index:(int)index
+        delegate:(PermissionBubbleView::Delegate*)delegate;
+@end
+
+@implementation AllowBlockMenuButton
+
+- (id)initForURL:(const GURL&)url
+         allowed:(BOOL)allow
+           index:(int)index
+        delegate:(PermissionBubbleView::Delegate*)delegate {
+  if (self = [super initWithFrame:NSZeroRect pullsDown:NO]) {
+    ContentSetting setting =
+        allow ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
+    [self setFont:[NSFont systemFontOfSize:kPermissionFontSize]];
+    [self setBordered:NO];
+
+    __block PermissionBubbleView::Delegate* blockDelegate = delegate;
+    PermissionMenuModel::ChangeCallback changeCallback =
+        base::BindBlock(^(const WebsiteSettingsUI::PermissionInfo& permission) {
+            blockDelegate->ToggleAccept(
+                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 selectItemAtIndex:menuModel_->GetIndexOfCommandId(setting)];
+    [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 because it
+    // will fit to the largest element in the menu, not just the selected item.
+    // TODO(leng):  This was copied from PermissionSelectorButton.  Move to a
+    // shared location, so that the code is not duplicated.
+    NSDictionary* textAttributes =
+        [[self attributedTitle] attributesAtIndex:0 effectiveRange:NULL];
+    NSSize titleSize = [[self title] sizeWithAttributes:textAttributes];
+    NSRect frame = [self frame];
+    NSRect titleRect = [[self cell] titleRectForBounds:frame];
+    CGFloat width = titleSize.width + NSWidth(frame) - NSWidth(titleRect);
+    [self setFrameSize:NSMakeSize(width + kPermissionButtonTitleRightPadding,
+                                  NSHeight(frame))];
+  }
+  return self;
+}
+
+@end
+
 @interface PermissionBubbleController ()
 
 // Returns an autoreleased NSView displaying the icon and label for |request|.
@@ -79,10 +140,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
+                  atIndex:(int)index
+                    allow:(BOOL)allow;
 
 // Returns an autoreleased NSView of a button with |title| and |action|.
 - (NSView*)buttonWithTitle:(NSString*)title
@@ -98,10 +160,6 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
 // 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;
-
 // Called when the 'ok' button is pressed.
 - (void)ok:(id)sender;
 
@@ -117,8 +175,13 @@ 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;
+// 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;
 
 @end
 
@@ -176,25 +239,32 @@ 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.
+      [PermissionBubbleController alignCenterOf:menu
+                           verticallyToCenterOf:permissionView];
+      [permissionMenus addObject:menu];
+      [contentView addSubview:menu];
+    }
     yOffset += NSHeight([permissionView frame]);
   }
 
@@ -207,6 +277,18 @@ 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;
+    CGFloat maxMenuWidth = 0;
+    for (NSView* view in permissionMenus.get()) {
+      [view setFrameOrigin:NSMakePoint(xOffset, NSMinY([view frame]))];
+      maxMenuWidth = std::max(maxMenuWidth, NSWidth([view frame]));
+    }
+    // And add the menu width to the bubble's width.
+    bubbleFrame.size.width += maxMenuWidth;
+  }
+
   base::scoped_nsobject<NSView> titleView(
       [[self titleWithHostname:requests[0]->GetRequestingHostname().host()]
           retain]);
@@ -242,7 +324,8 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
       blockButton.reset([[self blockButton] retain]);
     else
       blockButton.reset([[self blockButtonWithCustomizeMenu] retain]);
-    CGFloat width = [self matchWidthsOf:blockButton andOf:allowOrOkButton];
+    CGFloat width = [PermissionBubbleController matchWidthsOf:blockButton
+                                                        andOf:allowOrOkButton];
     // Ensure the allow/ok button is still in the correct position.
     xOrigin = NSWidth(bubbleFrame) - width - kHorizontalPadding;
     [allowOrOkButton setFrameOrigin:NSMakePoint(xOrigin, kVerticalPadding)];
@@ -290,6 +373,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 +410,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 +418,17 @@ 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_);
+  base::scoped_nsobject<AllowBlockMenuButton> button(
+      [[AllowBlockMenuButton alloc] initForURL:request->GetRequestingHostname()
+                                       allowed:allow
+                                         index:index
+                                      delegate:delegate_]);
+  return button.autorelease();
 }
 
 - (NSView*)buttonWithTitle:(NSString*)title
@@ -389,15 +470,6 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   return button.autorelease();
 }
 
-- (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)ok:(id)sender {
   DCHECK(delegate_);
   delegate_->Closing();
@@ -423,15 +495,26 @@ 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];
 }
 
++ (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 {
+  NSRect frameA = [viewA frame];
+  NSRect frameB = [viewB frame];
+  frameA.origin.y =
+      NSMinY(frameB) + std::floor((NSHeight(frameB) - NSHeight(frameA)) / 2);
+  [viewA setFrameOrigin:frameA.origin];
+}
+
 @end  // implementation PermissionBubbleController

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

@@ -19,6 +19,8 @@
 #import "ui/events/test/cocoa_test_event_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
+@class ConstrainedWindowButton;
+
 @interface PermissionBubbleController (ExposedForTesting)
 - (void)ok:(id)sender;
 - (void)onAllow:(id)sender;
@@ -73,19 +75,28 @@ class PermissionBubbleControllerTest : public CocoaTest,
   }
 
   NSButton* FindButtonWithTitle(const std::string& title) {
-    return FindButtonWithTitle(base::SysUTF8ToNSString(title));
+    return FindButtonWithTitle(base::SysUTF8ToNSString(title),
+                               [ConstrainedWindowButton class]);
   }
 
   NSButton* FindButtonWithTitle(int title_id) {
-    return FindButtonWithTitle(l10n_util::GetNSString(title_id));
+    return FindButtonWithTitle(l10n_util::GetNSString(title_id),
+                               [ConstrainedWindowButton class]);
   }
 
-  NSButton* FindButtonWithTitle(NSString* title) {
-    NSView* parent = base::mac::ObjCCastStrict<NSView>([controller_ bubble]);
-    for (NSView* child in [parent subviews]) {
-      NSButton* button = base::mac::ObjCCast<NSButton>(child);
-      if ([title isEqualToString:[button title]]) {
-        return button;
+  NSButton* FindMenuButtonWithTitle(int title_id) {
+    return FindButtonWithTitle(l10n_util::GetNSString(title_id),
+                               [NSPopUpButton class]);
+  }
+
+  // IDS_PERMISSION_ALLOW and IDS_PERMISSION_DENY are used for two distinct
+  // UI elements, both of which derive from NSButton.  So check the expected
+  // class, not just NSButton, as well as the title.
+  NSButton* FindButtonWithTitle(NSString* title, Class button_class) {
+    for (NSButton* view in [[controller_ bubble] subviews]) {
+      if ([view isKindOfClass:button_class] &&
+          [title isEqualToString:[view title]]) {
+        return view;
       }
     }
     return nil;
@@ -109,6 +120,15 @@ class PermissionBubbleControllerTest : public CocoaTest,
     return textField;
   }
 
+  void ChangePermissionMenuSelection(NSButton* menu_button, int next_title_id) {
+    NSMenu* menu = [base::mac::ObjCCastStrict<NSPopUpButton>(menu_button) menu];
+    NSString* next_title = l10n_util::GetNSString(next_title_id);
+    EXPECT_EQ([[menu itemWithTitle:[menu_button title]] state], NSOnState);
+    NSMenuItem* next_item = [menu itemWithTitle:next_title];
+    EXPECT_EQ([next_item state], NSOffState);
+    [menu performActionForItemAtIndex:[menu indexOfItem:next_item]];
+  }
+
   NSMenuItem* FindCustomizeMenuItem() {
     NSButton* button = FindButtonWithTitle(IDS_PERMISSION_DENY);
     if (!button || ![button isKindOfClass:[SplitBlockButton class]])
@@ -170,25 +190,35 @@ TEST_F(PermissionBubbleControllerTest, ShowMultiplePermissions) {
   EXPECT_FALSE(FindButtonWithTitle(IDS_OK));
 }
 
-TEST_F(PermissionBubbleControllerTest, ShowCustomizationMode) {
-  AddRequest(kPermissionB);
-
+TEST_F(PermissionBubbleControllerTest, ShowCustomizationModeAllow) {
   accept_states_.push_back(true);
-  accept_states_.push_back(false);
+  [controller_ showAtAnchor:NSZeroPoint
+               withDelegate:this
+                forRequests:requests_
+               acceptStates:accept_states_
+          customizationMode:YES];
+
+  // Test that there is one menu, with 'Allow' visible.
+  EXPECT_TRUE(FindMenuButtonWithTitle(IDS_PERMISSION_ALLOW));
+  EXPECT_FALSE(FindMenuButtonWithTitle(IDS_PERMISSION_DENY));
 
+  EXPECT_TRUE(FindButtonWithTitle(IDS_OK));
+  EXPECT_FALSE(FindButtonWithTitle(IDS_PERMISSION_ALLOW));
+  EXPECT_FALSE(FindButtonWithTitle(IDS_PERMISSION_DENY));
+  EXPECT_FALSE(FindCustomizeMenuItem());
+}
+
+TEST_F(PermissionBubbleControllerTest, ShowCustomizationModeBlock) {
+  accept_states_.push_back(false);
   [controller_ showAtAnchor:NSZeroPoint
               withDelegate:this
                forRequests:requests_
               acceptStates:accept_states_
          customizationMode:YES];
 
-  // Test that each checkbox is visible and only the first is checked.
-  NSButton* checkbox_a = FindButtonWithTitle(kPermissionA);
-  NSButton* checkbox_b = FindButtonWithTitle(kPermissionB);
-  EXPECT_TRUE(checkbox_a);
-  EXPECT_TRUE(checkbox_b);
-  EXPECT_EQ(NSOnState, [checkbox_a state]);
-  EXPECT_EQ(NSOffState, [checkbox_b state]);
+  // Test that there is one menu, with 'Block' visible.
+  EXPECT_TRUE(FindMenuButtonWithTitle(IDS_PERMISSION_DENY));
+  EXPECT_FALSE(FindMenuButtonWithTitle(IDS_PERMISSION_ALLOW));
 
   EXPECT_TRUE(FindButtonWithTitle(IDS_OK));
   EXPECT_FALSE(FindButtonWithTitle(IDS_PERMISSION_ALLOW));
@@ -230,7 +260,7 @@ TEST_F(PermissionBubbleControllerTest, Deny) {
   [FindButtonWithTitle(IDS_PERMISSION_DENY) performClick:nil];
 }
 
-TEST_F(PermissionBubbleControllerTest, ToggleCheckbox) {
+TEST_F(PermissionBubbleControllerTest, ChangePermissionSelection) {
   AddRequest(kPermissionB);
 
   accept_states_.push_back(true);
@@ -244,8 +274,10 @@ TEST_F(PermissionBubbleControllerTest, ToggleCheckbox) {
 
   EXPECT_CALL(*this, ToggleAccept(0, false)).Times(1);
   EXPECT_CALL(*this, ToggleAccept(1, true)).Times(1);
-  [FindButtonWithTitle(kPermissionA) performClick:nil];
-  [FindButtonWithTitle(kPermissionB) performClick:nil];
+  NSButton* menu_a = FindMenuButtonWithTitle(IDS_PERMISSION_ALLOW);
+  NSButton* menu_b = FindMenuButtonWithTitle(IDS_PERMISSION_DENY);
+  ChangePermissionMenuSelection(menu_a, IDS_PERMISSION_DENY);
+  ChangePermissionMenuSelection(menu_b, IDS_PERMISSION_ALLOW);
 }
 
 TEST_F(PermissionBubbleControllerTest, ClickCustomize) {

chrome/browser/ui/website_settings/permission_menu_model.cc

@@ -58,9 +58,9 @@ PermissionMenuModel::PermissionMenuModel(const GURL& url,
   permission_.setting = setting;
   permission_.default_setting = CONTENT_SETTING_NUM_SETTINGS;
   AddCheckItem(CONTENT_SETTING_ALLOW,
-               l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_MENU_ITEM_ALLOW));
+               l10n_util::GetStringUTF16(IDS_PERMISSION_ALLOW));
   AddCheckItem(CONTENT_SETTING_BLOCK,
-               l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_MENU_ITEM_BLOCK));
+               l10n_util::GetStringUTF16(IDS_PERMISSION_DENY));
 }
 
 PermissionMenuModel::~PermissionMenuModel() {}