Implement permission bubble view for Cocoa.

chrome/browser/ui/cocoa/permission_bubble_controller.mm

Index: chrome/browser/ui/cocoa/permission_bubble_controller.mm
diff --git a/chrome/browser/ui/cocoa/permission_bubble_controller.mm b/chrome/browser/ui/cocoa/permission_bubble_controller.mm
new file mode 100644
index 0000000000000000000000000000000000000000..9be24570bcbf08885a0d52545dc1003a6a1e6a61
--- /dev/null
+++ b/chrome/browser/ui/cocoa/permission_bubble_controller.mm
@@ -0,0 +1,295 @@
+// Copyright (c) 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/permission_bubble_controller.h"
+
+#include <algorithm>
+
+#include "base/mac/mac_util.h"
+#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"

[groby-ooo-7-16, 2014/01/31 22:31:51]

I don't think you're using these

[leng, 2014/02/03 22:38:04]

Only the sys_strings one.
Done.

+#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/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/permission_bubble_cocoa.h"
+#include "chrome/browser/ui/website_settings/permission_bubble_delegate.h"
+#include "chrome/browser/ui/website_settings/permission_bubble_view.h"
+#include "content/public/browser/user_metrics.h"
+#include "grit/generated_resources.h"
+#include "skia/ext/skia_utils_mac.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+
+using base::UserMetricsAction;
+
+namespace {
+const CGFloat kHorizontalPadding = 20.0f;
+const CGFloat kVerticalPadding = 20.0f;
+const CGFloat kCheckboxYAdjustment = 2;
+
+const CGFloat kButtonWidth = 80.0f;
+const CGFloat kButtonHeight = 30.0f;
+
+const base::char16 kBulletPoint = 0x2022;
+const int kButtonBackgroundColor = 239;

[groby-ooo-7-16, 2014/01/31 22:31:51]

Is that shared across platforms? Would it be worth to have a "theme" file?

[leng, 2014/02/03 22:38:04]

I don't know.  I took the color from the mocks, so I suspect so.  I'm not
familiar with "theme" files - what could I look at for an example?

[groby-ooo-7-16, 2014/02/04 02:39:29]

"theme" file is fancy speak for a shared header with those constants :)

[leng, 2014/02/04 21:48:22]

I see.  Thanks.  :)
This is no longer relevant, though, as I switched to using
ConstrainedWindowButton, as you suggested.

+
+}  // namespace
+
+@interface PermissionBubbleController (PrivateAPI)

[groby-ooo-7-16, 2014/01/31 22:31:51]

just

@interface PermissionBubbleController ()

[leng, 2014/02/03 22:38:04]

Done.

+
+// Called when the 'ok' button is pressed.
+- (void)ok;

[groby-ooo-7-16, 2014/01/31 22:31:51]

NSButton actions always take a sender parameter, i.e
- (void)ok:(id)sender;

[leng, 2014/02/03 22:38:04]

Done.

+
+// Called when the 'allow' button is pressed.
+- (void)allow;

[groby-ooo-7-16, 2014/01/31 22:31:51]

nit: Button callbacks except ok/cancel are usually prefixed with "on", i.e.
"onAllow:", "onBlock:", etc.

[leng, 2014/02/03 22:38:04]

Done.

+
+// Called when the 'block' button is pressed.
+- (void)block;
+
+// Called when the 'customize' button is pressed.
+- (void)customize;
+
+// Called when a checkbox changes from checked to unchecked, or vice versa.
+- (void)checkboxChanged;
+
+@end
+
+@implementation PermissionBubbleController
+
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+                    bridge:(PermissionBubbleCocoa*)bridge {
+  DCHECK(parentWindow);
+  DCHECK(bridge);
+  base::scoped_nsobject<InfoBubbleWindow> window(
+      [[InfoBubbleWindow alloc] initWithContentRect:NSMakeRect(0, 0, 240, 150)
+                                          styleMask:NSBorderlessWindowMask
+                                            backing:NSBackingStoreBuffered
+                                              defer:NO]);
+  [window setAllowedAnimations:info_bubble::kAnimateNone];
+  if ((self = [super initWithWindow:window
+                       parentWindow:parentWindow
+                         anchoredAt:NSZeroPoint])) {
+    [self setShouldCloseOnResignKey:NO];
+    [[self bubble] setArrowLocation:info_bubble::kTopLeft];
+    bridge_ = bridge;
+  }
+  return self;
+}
+
+- (void)windowWillClose:(NSNotification*)notification {
+  bridge_->OnBubbleClosing();
+  [super windowWillClose:notification];
+}
+
+- (void)showAtAnchor:(NSPoint)anchorPoint
+        withDelegate:(PermissionBubbleView::Delegate*)delegate
+         forRequests:(const std::vector<PermissionBubbleDelegate*>&)requests
+         acceptStates:(const std::vector<bool>&)acceptStates
+    customizationMode:(BOOL)customizationMode {
+  DCHECK(!customizationMode || (requests.size() == acceptStates.size()));
+  delegate_ = delegate;
+
+  base::scoped_nsobject<NSMutableArray> subViews([[NSMutableArray alloc] init]);

[groby-ooo-7-16, 2014/01/31 22:31:51]

Curious: Why a separate NSMutableArray, instead of adding the views directly?

[leng, 2014/02/03 22:38:04]

I was copying the behavior from another class.  Since I can't remember which
class that was, I've changed it.
Because of this change, I added a DCHECK to ensure that this function is never
called when there are already subviews.

+  BOOL singlePermission = requests.size() == 1;
+  NSRect windowFrame = [[self window] frame];
+  CGFloat yOffset = 2 * kVerticalPadding + kButtonHeight;
+  CGFloat maxContentsWidth = 0.0f;
+
+  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]);
+      checkboxes_.push_back(permissionView);
+    } else {
+      permissionView.reset([[self labelForRequest:(*it)
+                                  isSingleRequest:singlePermission] retain]);
+    }
+    NSPoint origin = [permissionView frame].origin;
+    origin.x += kHorizontalPadding;
+    origin.y += yOffset;
+    [permissionView setFrameOrigin:origin];
+    [subViews addObject:permissionView];
+
+    yOffset += [permissionView frame].size.height;

[groby-ooo-7-16, 2014/01/31 22:31:51]

NSHeight([permissionView frame])

[leng, 2014/02/03 22:38:04]

Done.

+    maxContentsWidth = fmax(maxContentsWidth,

[groby-ooo-7-16, 2014/01/31 22:31:51]

Please use std::max

Alternatively, you can simply create all the views, and then loop over them:
NSRect contentFrame = NSZeroRect
for (NSView* view in subviews)
  contentFrame = NSUnionRect(contentFrame, [view frame]);

Simplifies a lot of bookkeeping

[leng, 2014/02/03 22:38:04]

I like it - thanks!
Done.

+                            [permissionView frame].size.width);
+  }
+
+  if (!singlePermission && !customizationMode) {
+    base::scoped_nsobject<NSView> customizeButton(
+        [[self customizationButton] retain]);
+    // The Y center should match the Y centers of the buttons.
+    CGFloat customizeButtonYOffset = kVerticalPadding +
+        (kButtonHeight - [customizeButton frame].size.height) * 0.5f;

[groby-ooo-7-16, 2014/01/31 22:31:51]

You might want to ceil/floor this so the button is not on a half-pixel

[leng, 2014/02/03 22:38:04]

Done.

+    [customizeButton setFrameOrigin:NSMakePoint(kHorizontalPadding,
+                                                customizeButtonYOffset)];
+    [subViews addObject:customizeButton];
+  }
+
+  CGFloat windowWidth = fmax(windowFrame.size.width,

[groby-ooo-7-16, 2014/01/31 22:31:51]

NSWidth(windowFrame) - we like to pretend we don't know the structure of an
NSRect :)

You might want to do a search/replace through the file :)

[leng, 2014/02/03 22:38:04]

I didn't know that the structure of NSRect was so hush-hush.  :)
Done.

+                             maxContentsWidth + 2 * kHorizontalPadding);
+  base::scoped_nsobject<NSView> allowOrOkButton;
+  if (customizationMode) {
+    allowOrOkButton.reset([[self buttonWithTitle:@"OK"
+                                          action:@selector(ok)] retain]);
+  } else {
+    allowOrOkButton.reset([[self buttonWithTitle:@"Allow"
+                                          action:@selector(allow)] retain]);
+  }
+  CGFloat xOrigin = windowWidth - kButtonWidth - kHorizontalPadding;
+  [allowOrOkButton setFrameOrigin:NSMakePoint(xOrigin, kVerticalPadding)];
+  [subViews addObject:allowOrOkButton];
+
+  if (!customizationMode) {
+    base::scoped_nsobject<NSView> blockButton(
+        [[self buttonWithTitle:@"Block" action:@selector(block)] retain]);
+    xOrigin = NSMinX([allowOrOkButton frame]) - [blockButton frame].size.width -
+        kHorizontalPadding * 0.5f;

[groby-ooo-7-16, 2014/01/31 22:31:51]

I assume kHorizontalPadding/2 is the gap you want between buttons? Mind calling
it out as kButtonGap?

[leng, 2014/02/03 22:38:04]

Done.

+    [blockButton setFrameOrigin:NSMakePoint(xOrigin, kVerticalPadding)];
+    [subViews addObject:blockButton];
+  }
+
+  if (!singlePermission) {
+    base::scoped_nsobject<NSView> titleView(
+       [[self titleForMultipleRequests] retain]);
+    [subViews addObject:titleView];
+    [titleView setFrameOrigin:NSMakePoint(kHorizontalPadding,
+                                          kVerticalPadding + yOffset)];
+    yOffset += [titleView frame].size.height + kVerticalPadding;
+  }
+
+  [[[self window] contentView] setSubviews:subViews];
+  windowFrame.size.width = windowWidth;
+  windowFrame.size.height = yOffset + kVerticalPadding;

[groby-ooo-7-16, 2014/01/31 22:31:51]

Please use -frameRectForContentRect to convert the rect into a frame that allows
for potential window decorations.

[leng, 2014/02/03 22:38:04]

Good catch.  I've changed the name of the variable to make the distinction
clearer (for myself, mostly).

+  [[self window] setFrame:windowFrame display:NO];
+
+  [self setAnchorPoint:anchorPoint];
+  [self showWindow:nil];
+}
+
+- (NSView*)labelForRequest:(PermissionBubbleDelegate*)request
+           isSingleRequest:(BOOL)singleRequest {
+  DCHECK(request);
+  base::scoped_nsobject<NSTextField> permissionLabel(
+      [[NSTextField alloc] initWithFrame:NSZeroRect]);
+  base::string16 label;
+  if (!singleRequest) {
+    label.push_back(kBulletPoint);
+    label.push_back(' ');
+  }
+  // TODO(leng):  Make the appropriate call when it's working.

[groby-ooo-7-16, 2014/01/31 22:31:51]

When what is working?

[leng, 2014/02/03 22:38:04]

When the correct text-query function is working.  I added a comment - any
clearer?

[groby-ooo-7-16, 2014/02/04 02:39:29]

Much - thank you!

[groby-ooo-7-16, 2014/02/04 02:39:29]

Much - thank you!

+  if (singleRequest) {
+    label += request->GetMessageTextFragment();
+  } else {
+    label += request->GetMessageTextFragment();

[groby-ooo-7-16, 2014/01/31 22:31:51]

Can that if/else collapse, or is that related to the TODO?

[leng, 2014/02/03 22:38:04]

Related to the TODO.

+  }
+  [permissionLabel setDrawsBackground:NO];
+  [permissionLabel setBezeled:NO];
+  [permissionLabel setEditable:NO];
+  [permissionLabel setSelectable:NO];
+  [permissionLabel setStringValue:base::SysUTF16ToNSString(label)];
+  [permissionLabel sizeToFit];
+  return permissionLabel.autorelease();
+}
+
+- (NSView*)titleForMultipleRequests {
+  base::scoped_nsobject<NSTextField> titleView(
+      [[NSTextField alloc] initWithFrame:NSZeroRect]);
+  [titleView setDrawsBackground:NO];
+  [titleView setBezeled:NO];
+  [titleView setEditable:NO];
+  [titleView setSelectable:NO];
+  [titleView setStringValue:@"This site would like to:"];
+  [titleView setFont:[NSFont systemFontOfSize:15.0]];

[groby-ooo-7-16, 2014/01/31 22:31:51]

Might want to call this out as constant

[leng, 2014/02/03 22:38:04]

Done.

+  [titleView sizeToFit];
+  return titleView.autorelease();
+}
+
+- (NSView*)checkboxForRequest:(PermissionBubbleDelegate*)request
+                      checked:(BOOL)checked {
+  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];

[groby-ooo-7-16, 2014/01/31 22:31:51]

nit: Parens around ternary

[leng, 2014/02/03 22:38:04]

Done.

+  [checkbox sizeToFit];
+  [checkbox setFrameOrigin:NSMakePoint(0, kCheckboxYAdjustment)];
+  return checkbox.autorelease();
+}
+
+- (NSView*)customizationButton {
+  NSColor* linkColor =
+      gfx::SkColorToCalibratedNSColor(chrome_style::GetLinkColor());
+  base::scoped_nsobject<NSButton> customizeButton(
+      [[NSButton alloc] initWithFrame:NSZeroRect]);
+  [customizeButton setButtonType:NSMomentaryChangeButton];
+  [customizeButton setAttributedTitle:[[NSAttributedString alloc]
+      initWithString:@"Customize"
+         attributes:@{ NSForegroundColorAttributeName : linkColor }]];

[groby-ooo-7-16, 2014/01/31 22:31:51]

Is the customizeButton supposed to be an actual link? There is HyperlinkTextView
for that.

[leng, 2014/02/03 22:38:04]

I tried that, but not only is it significantly more code, I don't know how to
remove the underline from a HyperlinkTextView.  The mocks don't have the text
underlined.

[groby-ooo-7-16, 2014/02/04 02:39:29]

Odd - it should be very simple:

linkView([[HyperLinkTextView alloc] initWithFrameNSZeroRect];)
[linkView setMessage:@"Customize" font:[NSFont labelFontOfSize:0]
color:linkColor]

Also, we should probably talk to the UX guys about links without underline.
It's... weird.

Feel free to file a bug against UX and we can revisit this later. (If we want
all Chrome UI to not show underlines, we should fix HyperLinkText View)

[leng, 2014/02/04 21:48:22]

So I think I ignored, or glossed over, the question you asked initially - it's
not actually a link.  It's supposed to look like one, but it changes the UI
within the bubble - it does not pull up a website.  I think that's the
difference you're curious about.  I've left it as a NSButton, because it doesn't
actually show a website, but I'm happy to change it if you like.

+  [customizeButton setTarget:self];
+  [customizeButton setAction:@selector(customize:)];
+  [customizeButton sizeToFit];
+  return customizeButton.autorelease();
+}
+
+- (NSView*)buttonWithTitle:(NSString*)title
+                    action:(SEL)action {
+  NSRect frame = { {0, 0}, {kButtonWidth, kButtonHeight} };

[groby-ooo-7-16, 2014/01/31 22:31:51]

NSRect frame = NSMakeRect(0, 0, kButtonWidth, kButtonHeight);

[leng, 2014/02/03 22:38:04]

Done.

+  NSColor* buttonBackgroundColor =
+      gfx::SkColorToCalibratedNSColor(SkColorSetRGB(kButtonBackgroundColor,
+                                                    kButtonBackgroundColor,
+                                                    kButtonBackgroundColor));
+
+  base::scoped_nsobject<NSButton> button(
+      [[NSButton alloc] initWithFrame:frame]);
+  [button setButtonType:NSMomentaryPushInButton];
+  [button setTitle:title];
+  [button setTarget:self];
+  [button setAction:action];
+  [button setBordered:NO];
+  [[button cell] setBackgroundColor:buttonBackgroundColor];
+  return button.autorelease();
+}
+
+- (void)ok {
+  if (delegate_)

[groby-ooo-7-16, 2014/01/31 22:31:51]

I'd prefer a DCHECK delegate_ - I don't think this could ever be NULL?

[leng, 2014/02/03 22:38:04]

It was when I was first testing - I have an outstanding CL to fix that.
Changed to DCHECK.

+    delegate_->Closing();
+}
+
+- (void)allow {
+  if (delegate_)
+    delegate_->Accept();
+}
+
+- (void)block {
+  if (delegate_)
+    delegate_->Deny();
+}
+
+- (void)customize {
+  if (delegate_)
+    delegate_->SetCustomizationMode();
+}
+
+- (void)checkboxChanged:(id)sender {
+  if (!delegate_)
+    return;
+  auto iter = std::find(
+      checkboxes_.begin(), checkboxes_.end(), (NSView*)sender);
+  if (iter != checkboxes_.end()) {

[groby-ooo-7-16, 2014/01/31 22:31:51]

Again, probably DCHECK. Can never happen

[leng, 2014/02/03 22:38:04]

Done.

+    NSButton *checkbox = (NSButton*)(*iter);

[groby-ooo-7-16, 2014/01/31 22:31:51]

base::ObjCCast or base::ObjCCastStrict when casting NS* values.

The latter DCHECKs the cast worked, which is probably preferrable here.

[leng, 2014/02/03 22:38:04]

I had no idea about ObjCCast and ObjCCastStrict.  Nice.
Done.

+    delegate_->ToggleAccept(iter - checkboxes_.begin(),
+                            [checkbox state] == NSOnState);
+  }
+}
+
+@end  // implementation BookmarkBubbleController(ExposedForUnitTesting)

[groby-ooo-7-16, 2014/01/31 22:31:51]

ExposedForUnitTesting?

[leng, 2014/02/03 22:38:04]

BookmarkBubbleController?
Now you know how I started this file...  :)

[groby-ooo-7-16, 2014/02/04 02:39:29]

Heh - I didn't even notice that part. Bad reviewer! No cookie! ;)

[groby-ooo-7-16, 2014/02/04 02:39:29]

Heh - I didn't even notice that part. Bad reviewer! No cookie! ;)

[leng, 2014/02/04 21:48:22]

Am I supposed to pay you with cookies?  Are you allergic to nuts?