New custom styling for action box menu on Os X.

chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_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/location_bar/action_box_menu_bubble_controller.h"
+
+#include "base/mac/bundle_locations.h"
+#include "base/mac/mac_util.h"
+#include "base/sys_string_conversions.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
+#import "chrome/browser/ui/cocoa/browser_window_utils.h"
+#import "chrome/browser/ui/cocoa/event_utils.h"
+#import "chrome/browser/ui/cocoa/info_bubble_view.h"
+#import "chrome/browser/ui/cocoa/info_bubble_window.h"
+#include "grit/generated_resources.h"
+#include "grit/theme_resources.h"
+#import "third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
+#include "ui/base/models/simple_menu_model.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image.h"
+
+@interface ActionBoxMenuBubbleController (Private)
+- (ui::MenuModel*)model;
+- (void)keyDown:(NSEvent*)theEvent;
+- (void)moveDown:(id)sender;
+- (void)moveUp:(id)sender;
+- (void)highlightNextItemByDelta:(NSInteger)delta;
+- (void)highlightItem:(ActionBoxMenuItemController*)newItem;
+@end
+
+namespace {
+
+// Some reasonable values for the menu geometry.
+const CGFloat kBubbleMinWidth = 175;
+const CGFloat kBubbleMaxWidth = 800;
+
+// Distance between the top/bottom of the bubble and the first/last menu item.
+const CGFloat kVerticalPadding = 7.0;
+
+// Minimum distance between the right of a menu item and the right border.
+const CGFloat kRightMargin = 20.0;
+
+// Alpha of the black rectangle overlayed on the selected item..

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

Double period.  While you're there, make the comment reflect intent (at least
mention hover somewhere), rather than implementation.

[beaudoin, 2012/10/15 16:42:57]

Done.

+const CGFloat kSelectionAlpha = 0.06;
+
+}  // namespace
+
+@implementation ActionBoxMenuBubbleController
+
+- (id)initWithBrowser:(Browser*)parentBrowser
+           usingModel:(scoped_ptr<ui::MenuModel>)model
+           anchoredAt:(NSPoint)point {
+  if ((self = [self initWithModel:model.Pass()
+                     parentWindow:parentBrowser->window()->GetNativeWindow()
+                       anchoredAt:point])) {
+  }
+  return self;
+}
+
+- (ui::MenuModel*)model {
+  return model_.get();
+}
+
+- (IBAction)itemSelected:(id)sender {
+  // Close the current window and activate the parent browser window, otherwise
+  // the bookmark popup refuses to show.
+  [self close];
+  [BrowserWindowUtils
+      activateWindowForController:[[self parentWindow] windowController]];
+  size_t modelIndex = [sender modelIndex];
+  DCHECK(model_.get());
+  int event_flags = event_utils::EventFlagsFromNSEvent([NSApp currentEvent]);

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

You're in Objective-C code, here, so you need eventUtils.  I'm just going to
mention this here, rather than enumerate all the cases below.

[beaudoin, 2012/10/15 16:42:57]

event_utils is a common namespace, I assume you mean event_flags? Corrected here
and did a quick pass everywhere else.

Done.

+  model_->ActivatedAt(modelIndex, event_flags);
+}
+
+// Private /////////////////////////////////////////////////////////////////////
+
+- (id)initWithModel:(scoped_ptr<ui::MenuModel>)model
+       parentWindow:(NSWindow*)parent
+         anchoredAt:(NSPoint)point {
+  // Use an arbitrary height because it will reflect the size of the content.
+  NSRect contentRect = NSMakeRect(0, 0, kBubbleMinWidth, 150);

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

Is there any reason not to just use NSZeroRect?

[beaudoin, 2012/10/15 16:42:57]

An NSZeroRect gives me this:

[84157:-1400145368:1015/110439:FATAL:underlay_opengl_hosting_window.mm(102)]
Check failed: !NSIsEmptyRect(contentRect). 

within the call to -initWithContentRect below.

[Scott Hess - ex-Googler, 2012/10/15 21:15:18]

OK.

+  // Create an empty window into which content is placed.
+  scoped_nsobject<InfoBubbleWindow> window(
+      [[InfoBubbleWindow alloc] initWithContentRect:contentRect
+                                          styleMask:NSBorderlessWindowMask
+                                            backing:NSBackingStoreBuffered
+                                              defer:NO]);
+  if (self = [super initWithWindow:window
+                      parentWindow:parent
+                        anchoredAt:[parent convertBaseToScreen:point]]) {

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

This method has a pretty similar signature to the method you're implementing.  I
think that anchoredAt: should be phrased using the same coordinate base in both
cases.

[beaudoin, 2012/10/15 16:42:57]

Moved the coordinate conversion upstream to plus_decoration.mm. Since it forced
me to access the parent window there I've refactored the code somewhat and
dropped one of the initializer. Looks cleaner to me.

Done.

+    model_.reset(model.release());
+
+    [[self bubble] setAlignment:info_bubble::kAlignRightEdgeToAnchorEdge];
+    [[self bubble] setArrowLocation:info_bubble::kNoArrow];
+    [[self bubble] setBackgroundColor:
+        [NSColor colorWithDeviceRed:(251.0f/255.0f)
+                              green:(251.0f/255.0f)
+                               blue:(251.0f/255.0f)

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

colorWithDeviceWhite:alpha: seems like it would match the intent better, unless
you're going to tweak the balance later.

[beaudoin, 2012/10/15 16:42:57]

Done.

+                              alpha:1.0]];
+    [self performLayout];
+  }
+  return self;
+}
+
+- (void)performLayout {
+  NSView* contentView = [[self window] contentView];
+
+  // Reset the array of controllers and remove all the views.
+  items_.reset([[NSMutableArray alloc] init]);
+  [contentView setSubviews:[NSArray array]];
+
+  // Leave some space at the bottom of the menu.
+  CGFloat yOffset = kVerticalPadding;
+
+  // Loop over the items in reverse, constructing the menu items.
+  CGFloat width = kBubbleMinWidth;
+  for (int i = model_->GetItemCount() - 1; i >= 0; --i) {

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

Is that really returning an int?  I'd have expected size_t.  Why are you
constructing things in reverse?

[beaudoin, 2012/10/15 16:42:57]

Totally agree, but it's an int!
Ref:
http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/base/models/menu_model....

+    // Create the item controller. Autorelease it because it will be owned
+    // by the |items_| array.
+    ActionBoxMenuItemController* itemController =
+        [[[ActionBoxMenuItemController alloc] initWithModelIndex:i
+                                              menuController:self] autorelease];

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

Generally use scoped_nsobject<> rather than -autorelease for code where you're
not returning the object to some caller.

Also, alignment of the ':'.

[beaudoin, 2012/10/15 16:42:57]

Done.

+
+    // Adjust the name field to fit the string.
+    [GTMUILocalizerAndLayoutTweaker sizeToFitView:itemController.nameField];
+
+    // Expand the size of the window if required to fit the menu item.
+    width = std::max(width,
+        NSMaxX([itemController.nameField frame]) + kRightMargin);

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

Is this accumulating the max width, or the max x-coord?  I'm guessing it's
because the field is offset, but you should still make sure you're taking a
delta from some reference, like NSMinX([contentView bounds]).

[beaudoin, 2012/10/15 16:42:57]

Brings up a question: when does [someView bounds] have an NSMinX/NSMinY
different from 0? I'm really new to cocoa but I've not yet seen this situation
arise.

Done.

[Scott Hess - ex-Googler, 2012/10/15 21:15:18]

It never happens except when it happens.  Usually it's when someone is being
tricky with clipping.  If you tried to have a CL which did play with a
bounds.x/y != 0.0, you'd get complaints.  I don't care enough about it to try to
change the system, because about the time you get complacent about this it bites
you again...

+
+    // Add the item to the content view.
+    [[itemController view] setFrameOrigin:NSMakePoint(0, yOffset)];
+    [contentView addSubview:[itemController view]];
+    yOffset += NSHeight([[itemController view] frame]);
+
+    // Keep track of the view controller.
+    [items_ addObject:itemController];
+  }
+
+  // Leave some space at the top of the menu.
+  yOffset += kVerticalPadding;
+
+  // Set the window frame, clamping the width at a sensible max.
+  NSRect frame = [[self window] frame];
+  frame.size.height = yOffset;
+  frame.size.width = std::min(width, kBubbleMaxWidth);
+  [[self window] setFrame:frame display:YES];
+}
+
+- (NSMutableArray*)items {

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

Do you expect the caller to modify it?

[beaudoin, 2012/10/15 16:42:57]

Done.

+  return items_.get();
+}
+
+- (void)keyDown:(NSEvent*)theEvent {
+  [self interpretKeyEvents:[NSArray arrayWithObject:theEvent]];

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

This is unusual enough that it deserves a comment as to what things you expect
it to handle.

[beaudoin, 2012/10/15 16:42:57]

Taken from AvatarMenuBubbleController, documented it somewhat, let me know if it
makes sense.

+}
+
+- (void)moveDown:(id)sender {
+  [self highlightNextItemByDelta:-1];
+}
+
+- (void)moveUp:(id)sender {
+  [self highlightNextItemByDelta:1];
+}
+
+- (void)highlightNextItemByDelta:(NSInteger)delta {
+  NSUInteger count = [items_ count];
+  if (count == 0)
+    return;
+
+  NSInteger old_index = -1;
+  for (NSUInteger i = 0; i < count; ++i) {
+    if ([[items_ objectAtIndex:i] isHighlighted]) {
+      old_index = i;

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

Casting missing, here, but I don't think you need casting at all in this code.

Something like:
  NSUInteger new_index = delta < 0 ? (count - 1) : 0;
  for (NSUInteger i = 0; i < count; i++) {
    if ([[items_ objectAtIndex:i] isHighlighted]) {
      if (delta < 0) {
        new_index = i > 0 ? i - 1 : i;
      } else {
        new_index = i < count - 1 ? i + 1 : i;
      }
      break;
    }
  }

[beaudoin, 2012/10/15 16:42:57]

Simplified that entire method quite a bit.

Done.

+      break;
+    }
+  }
+
+  NSInteger new_index;
+  // If nothing is selected then start at the top if we're going down and start
+  // at the bottom if we're going up.
+  if (old_index == -1)
+    new_index = delta < 0 ? (count - 1) : 0;
+  else
+    new_index = old_index + delta;
+
+  // Cap the index. We don't wrap around to match the behavior of Mac menus.
+  new_index =
+      std::min(std::max(static_cast<NSInteger>(0), new_index),
+               static_cast<NSInteger>(count - 1));
+
+  [self highlightItem:[items_ objectAtIndex:new_index]];
+}
+
+- (void)highlightItem:(ActionBoxMenuItemController*)newItem {
+  ActionBoxMenuItemController* oldItem = nil;
+  for (ActionBoxMenuItemController* item in items_.get()) {
+    if ([item isHighlighted]) {
+      oldItem = item;
+      break;
+    }
+  }
+
+  if (oldItem == newItem)
+    return;
+
+  [oldItem setIsHighlighted:NO];
+  [newItem setIsHighlighted:YES];
+}
+
+@end
+
+// Menu Item Controller ////////////////////////////////////////////////////////
+
+@implementation ActionBoxMenuItemController
+
+@synthesize modelIndex = modelIndex_;
+@synthesize isHighlighted = isHighlighted_;
+@synthesize nameField = nameField_;
+
+- (id)initWithModelIndex:(size_t)modelIndex
+          menuController:(ActionBoxMenuBubbleController*)controller {
+  if ((self = [super initWithNibName:@"ActionBoxMenuItem"
+                              bundle:base::mac::FrameworkBundle()])) {
+    modelIndex_ = modelIndex;
+    controller_ = controller;
+
+    [self loadView];
+
+    gfx::Image icon = gfx::Image();
+
+    if (controller.model->GetIconAt(modelIndex_, &icon))
+      iconView_.image = icon.ToNSImage();
+    else
+      iconView_.image = nil;
+    nameField_.stringValue = base::SysUTF16ToNSString(
+        controller.model->GetLabelAt(modelIndex_));
+  }
+  return self;
+}
+
+- (void)dealloc {
+  static_cast<ActionBoxMenuItemView*>(self.view).viewController = nil;

[Scott Hess - ex-Googler, 2012/10/15 06:06:35]

There's an objective-c cast somewhere in base/mac.  This feels odd, though.

[beaudoin, 2012/10/15 16:42:57]

Switched to ObjCCastStrict.

It was taken from AvatarMenuBubbleController. I agree it's strange, but I
imagine the goal is to ensure our view does not keep a reference to us when
we're deallocated. Is this required?

Done.

[Scott Hess - ex-Googler, 2012/10/15 21:15:18]

I'm kind of imagining that I shouldn't go poking around
AvatarMenuBubbleController :-).

I'm not sure if it's required here.  There's a common issue of Chrome-side
object lifetimes not matching Cocoa-side lifetimes, so destructors and -dealloc
calls need to wipe out the opposing side's pointers to prevent use-after-free
cases.  Probably better safe than sorry, I guess.

+  [super dealloc];
+}
+
+- (void)highlightForEventType:(NSEventType)type {
+  switch (type) {
+    case NSMouseEntered:
+      [controller_ highlightItem:self];
+      break;
+
+    case NSMouseExited:
+      [controller_ highlightItem:nil];
+      break;
+
+    default:
+      NOTREACHED();
+  };
+}
+
+- (IBAction)itemSelected:(id)sender {
+  [controller_ itemSelected:self];
+}
+
+- (void)setIsHighlighted:(BOOL)isHighlighted {
+  if (isHighlighted_ == isHighlighted)
+    return;
+
+  isHighlighted_ = isHighlighted;
+  [[self view] setNeedsDisplay:YES];
+}
+
+@end
+
+// Items from the action box menu //////////////////////////////////////////////
+
+@implementation ActionBoxMenuItemView
+
+@synthesize viewController = viewController_;
+
+- (void)updateTrackingAreas {
+  if (trackingArea_.get())
+    [self removeTrackingArea:trackingArea_.get()];
+
+  trackingArea_.reset(
+      [[CrTrackingArea alloc] initWithRect:[self bounds]
+                                   options:NSTrackingMouseEnteredAndExited |
+                                           NSTrackingActiveInKeyWindow
+                                     owner:self
+                                  userInfo:nil]);
+  [self addTrackingArea:trackingArea_.get()];
+
+  [super updateTrackingAreas];
+}
+
+- (BOOL)acceptsFirstResponder {
+  return YES;
+}
+
+- (void)mouseUp:(NSEvent*)theEvent {
+  [viewController_ itemSelected:self];
+}
+
+- (void)mouseEntered:(id)sender {
+  [viewController_ highlightForEventType:[[NSApp currentEvent] type]];
+  [self setNeedsDisplay:YES];
+}
+
+- (void)mouseExited:(id)sender {
+  [viewController_ highlightForEventType:[[NSApp currentEvent] type]];
+  [self setNeedsDisplay:YES];
+}
+
+- (void)drawRect:(NSRect)dirtyRect {
+  NSColor* backgroundColor = nil;
+  if ([viewController_ isHighlighted]) {
+    backgroundColor = [NSColor colorWithDeviceWhite:0.0 alpha:kSelectionAlpha];
+  } else {
+    backgroundColor = [NSColor clearColor];
+  }
+
+  [backgroundColor set];
+  [NSBezierPath fillRect:[self bounds]];
+}
+
+// Make sure the element is focusable for accessibility.
+- (BOOL)canBecomeKeyView {
+  return YES;
+}
+
+- (BOOL)accessibilityIsIgnored {
+  return NO;
+}
+
+- (NSArray*)accessibilityAttributeNames {
+  NSMutableArray* attributes =
+      [[[super accessibilityAttributeNames] mutableCopy] autorelease];
+  [attributes addObject:NSAccessibilityTitleAttribute];
+  [attributes addObject:NSAccessibilityEnabledAttribute];
+
+  return attributes;
+}
+
+- (NSArray*)accessibilityActionNames {
+  NSArray* parentActions = [super accessibilityActionNames];
+  return [parentActions arrayByAddingObject:NSAccessibilityPressAction];
+}
+
+- (id)accessibilityAttributeValue:(NSString*)attribute {
+  if ([attribute isEqual:NSAccessibilityRoleAttribute])
+    return NSAccessibilityButtonRole;
+
+  if ([attribute isEqual:NSAccessibilityRoleDescriptionAttribute])
+    return NSAccessibilityRoleDescription(NSAccessibilityButtonRole, nil);
+
+  if ([attribute isEqual:NSAccessibilityEnabledAttribute])
+    return [NSNumber numberWithBool:YES];
+
+  return [super accessibilityAttributeValue:attribute];
+}
+
+- (void)accessibilityPerformAction:(NSString*)action {
+  if ([action isEqual:NSAccessibilityPressAction]) {
+    [viewController_ itemSelected:self];
+    return;
+  }
+
+  [super accessibilityPerformAction:action];
+}
+
+@end