Menu for the OSX app launcher.

Menu for the OSX app launcher, HoverImageMenuButton in src/ui/base/cocoa/controls.

Adds a drop-down menu to the right of the search entry area on the OSX
app launcher. The menu is shown when clicked, and the button responds to
hover effects.

The menu button uses a new class, HoverImageMenuButton, which is derived
from an NSPopUpButton with minor extensions. Notably, it does not have a
dependency on browser themes, as does MenuButton from
chrome/browser/ui/cocoa.  It tracks the mouse hover state and updates
the cell, which extends NSPopUpButtonCell and shows only the image in
the control frame -- no border, bezel, label, or dropdown arrow.

HoverImageMenuButtonCell supports a hover image, which behaves much like
an additional 'alternateImage' from NSButtonCell but for the hover
state, rather than the 'pressed' (or 'lit') state.

The menu shows the currently signed-in user, in a custom view as the
first item. It also (currently) shows menu options for Settings, Help, and
Feedback.

BUG=138633
TEST=Added app_list_unittests AppsSearchBoxMenuTest and
AppsSearchBoxMenuTest and tested manually to ensure the items are
launched correctly. Added ui_unittests HoverImageMenuButtonTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206237

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206930

[sail, 2013-05-31 01:20:16]

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_menu_cocoa.h (right):

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_menu_cocoa.h:18: @interface AppListMenuCocoa :
NSPopUpButton {
How about AppListMenuButton instead?

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_menu_cocoa.mm (right):

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_menu_cocoa.mm:23: @interface AppListMenuCocoaCell :
NSPopUpButtonCell {
How about moving this to ui/base/cocoa and making it a HoverImageButton subclass
instead. Similar to MenuButton from Chrome.

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_menu_cocoa.mm:36: @interface CurrentUserMenuItemView
: NSView;
Can we just use a normal menu item and set it's icon?

[tapted, 2013-05-31 06:05:13]

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_menu_cocoa.h (right):

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_menu_cocoa.h:18: @interface AppListMenuCocoa :
NSPopUpButton {
On 2013/05/31 01:20:16, sail wrote:
> How about AppListMenuButton instead?

Yep - I think that makes more sense. I chose this name initially to mirror
`app_list_menu_views.cc`, but the concepts don't really align in any case.

Done.

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_menu_cocoa.mm (right):

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_menu_cocoa.mm:23: @interface AppListMenuCocoaCell :
NSPopUpButtonCell {
On 2013/05/31 01:20:16, sail wrote:
> How about moving this to ui/base/cocoa and making it a HoverImageButton
subclass
> instead. Similar to MenuButton from Chrome.

I tried using HoverImageButton, and incorporating relevant bits from MenuButton
(code here:
https://codereview.chromium.org/15814009/diff2/1:4002/ui/app_list/cocoa/app_l...
). MenuButton in c/b/ui essentially "hacks" [sic] in parts of NSPopUpButton to
achieve its results, so it becomes a bit messy.

Ultimately, I couldn't figure out how to get HoverImageButton from ui/base to do
what's needed anyway. The problem is that the button comes "up" as soon as the
mouse button is released or leaves the button area, but we want it to stay
"down" until the menu is dismissed. Using NSPopUpButton directly rather than
hacking it in does this job nicely, while also dealing with the other menu-isms
like dragging to select an item.

Adapting NSPopUpButtonCell to handle hover currently feels like the lesser evil,
but maybe I've missed something..

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_menu_cocoa.mm:36: @interface CurrentUserMenuItemView
: NSView;
On 2013/05/31 01:20:16, sail wrote:
> Can we just use a normal menu item and set it's icon?

I gave this a go too, using an attributed string (code here:
https://codereview.chromium.org/16156004/diff2/1:2001/ui/app_list/cocoa/app_l...
). This is the closest I could get it:

http://i.imgur.com/WLOHBGp.png

I couldn't figure out a way to get the icon in the margin, or to line things up
in some other way. I tried setIndentationLevel: but it needs levels of ~1.5 or
"-1" to line up properly, which are not valid.

This custom view gets pretty close (ref: http://i.imgur.com/7nHVuOp.png ). The
views implementation is using a custom view as well, so this is at least
consistent.

However, the exercise did clue me in to a subtle font-rendering bug I had :)
(fixed in patchset 2; before the class rename/file move).

[tapted, 2013-05-31 09:37:39]

As discussed. Patch Set 4 is a refactor of the extended NSPopUpButton to a
reusable control, that I've called HoverImageMenuButton and put into
src/ui/base/cocoa/controls

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button.h (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.h:10: #import
"ui/base/cocoa/controls/hover_image_menu_button_cell.h"
Not sure if this is conventional.. it felt weird to include both button and cell
headers in order to see [[button cell] setHoverImage:]. Maybe it should just be
collapsed into one file?

[Robert Sesek, 2013-05-31 18:25:35]

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_menu_cocoa.mm (right):

https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_menu_cocoa.mm:23: @interface AppListMenuCocoaCell :
NSPopUpButtonCell {
On 2013/05/31 06:05:13, tapted wrote:
> On 2013/05/31 01:20:16, sail wrote:
> > How about moving this to ui/base/cocoa and making it a HoverImageButton
> subclass
> > instead. Similar to MenuButton from Chrome.
> 
> I tried using HoverImageButton, and incorporating relevant bits from
MenuButton
> (code here:
>
https://codereview.chromium.org/15814009/diff2/1:4002/ui/app_list/cocoa/app_l...
> ). MenuButton in c/b/ui essentially "hacks" [sic] in parts of NSPopUpButton to
> achieve its results, so it becomes a bit messy.
> 
> Ultimately, I couldn't figure out how to get HoverImageButton from ui/base to
do
> what's needed anyway. The problem is that the button comes "up" as soon as the
> mouse button is released or leaves the button area, but we want it to stay
> "down" until the menu is dismissed. Using NSPopUpButton directly rather than
> hacking it in does this job nicely, while also dealing with the other
menu-isms
> like dragging to select an item.
> 
> Adapting NSPopUpButtonCell to handle hover currently feels like the lesser
evil,
> but maybe I've missed something..

I think this is because HoverButton isn't examining the -[NSButton state]. If it
did that to check if it's on the OnState rather than the mouseDown/Up flags it
currently sets, this may work.

[sail, 2013-05-31 19:07:03]

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:183: [[menuButton_ cell]
setImage:resourceBundle.GetNativeImageNamed(
use hoverImageMenuButtonCell accessor instead?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:196: if (!appListMenu_)
is this possible?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:206: int itemCount =
appListMenu_->menu_model()->GetItemCount();
do this in addSubviews instead?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:251: rect.size =
NSMakeSize(anchor.x - NSMinX(rect), anchor.y - NSMinY(rect));
how about just converting [menuButton_ bounds] to screen coordinates?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
File ui/app_list/cocoa/current_user_menu_item_view.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:22: // The first item in the
menu is the current user indicator. It shows a static
Don't need this comment in this file since this menu item shouldn't care about
the overall menu.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:35: -
(NSTextField*)addLabelWithFrame:(NSRect)frame
move to bottom of file?

this doesn't actually use the frame provided, it just needs the origin, pass
that instead?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:53:
scoped_nsobject<NSImageView> userIndicator(
how about naming this userImageView?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:55: NSImage* indicatorIcon =
ui::ResourceBundle::GetSharedInstance().
how about naming this userImage?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:68: [emailField
setTextColor:[NSColor grayColor]];
Use constants shared with the views code?

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:85: imageRect.origin.x =
kIndicatorIconPadding;
This is a bit confusing, the image view is enlarged by kIndicatorIconPadding and
then you offset it by an additional kIndicatorIconPadding.

I think several things would make this code easier to follow:
  - don't enlarge the image view
  - make this view flipped so that the origin is top left
  - layout controls in the following order: image view, top label, bottom label
  - size the parent view based on the bottom label frame

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button.h (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.h:10: #import
"ui/base/cocoa/controls/hover_image_menu_button_cell.h"
On 2013/05/31 09:37:39, tapted wrote:
> Not sure if this is conventional.. it felt weird to include both button and
cell
> headers in order to see [[button cell] setHoverImage:]. Maybe it should just
be
> collapsed into one file?

I think it's ok. But you don't actually use it in this file so can you move this
into the .mm file?

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.h:17: // Initialize with
initWithFrame:pullsDown:.
Don't need this comment. This is the designated initializer for NSPopupButton.
Unless you specify a new designated initializer callers should already know to
use that one.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.h:22: 
Maybe add a
- (HoverImageMenuButtonCell*)hoverImageMenuButtonCell;
method here?

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button_cell.h (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.h:24: -
(void)setHoverImage:(NSImage*)newImage;
use @property to declare this?

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button_cell.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:29: return [self image];
How does this work? Shouldn't this be [buttonMenuItem image]?

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:33: [self
setUsesItemFromMenu:NO];
Since the button class explicitly states that the first menu item is not used I
think it makes sense to always enforce that. How about moving this to init
instead?

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:34:
scoped_nsobject<NSMenuItem> buttonItem([[NSMenuItem alloc] init]);
how about buttonMenuItem?

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button_unittest.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:25: Init();
don't need this (base class does it already)

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:35: [[menu_button_
cell] setImage:ResetImage(kIdrNormal, &normal_)];
How about using system provided images instead of depending on ResourceBundle?
Something like NSImageNameUserAccounts.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:37: [[menu_button_
cell] setHoverImage:ResetImage(kIdrHovered, &hovered_)];
use hoverImageMenuButtonCell instead of cell ?

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:50:
scoped_nsobject<NSImage> normal_;
how about setting these in the constructor, then you don't need ResetImage()

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:64:
EXPECT_NSNE(normal_, pressed_);
I don't this is necessary.

[Robert Sesek, 2013-05-31 19:16:09]

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:213: -
(void)addItemToMenu:(NSMenu*)menu
Have you considered using MenuController?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

It'd need to be moved to ui/base though

[tapted, 2013-06-03 12:49:18]

I remembered to rev up by x64 build this time, so there are also some added
FOO_EXPORT attributes.

Note that patchset 6 is just for the rebase, but there are some non-trivial
changes to setDelegate there.

PTAL.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:183: [[menuButton_ cell]
setImage:resourceBundle.GetNativeImageNamed(
On 2013/05/31 19:07:03, sail wrote:
> use hoverImageMenuButtonCell accessor instead?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:196: if (!appListMenu_)
On 2013/05/31 19:07:03, sail wrote:
> is this possible?

Initially I wasn't sure, but I realised the menu is not cleared in setDelegate
-- it should be! So, I do that now, and I've changed this to a DCHECK (done.)

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:206: int itemCount =
appListMenu_->menu_model()->GetItemCount();
On 2013/05/31 19:07:03, sail wrote:
> do this in addSubviews instead?

I initially did it here because it means the creation of the menu and custom
view can be delayed until the menu is actually requested. But, I guess that's a
small component, and there's a bit more complexity since I really need to update
the menu in setDelegate, not addSubviews.

Done (moved to setDelegate)

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:251: rect.size =
NSMakeSize(anchor.x - NSMinX(rect), anchor.y - NSMinY(rect));
On 2013/05/31 19:07:03, sail wrote:
> how about just converting [menuButton_ bounds] to screen coordinates?

Sadly, NSWindow convertRectToScreen: is OSX 10.7+ (unless there's an alternative
I've missed). I've updated to use NSView's convertRect: toView: to remove
kMenuButtonDimension from the equation though.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
File ui/app_list/cocoa/current_user_menu_item_view.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:22: // The first item in the
menu is the current user indicator. It shows a static
On 2013/05/31 19:07:03, sail wrote:
> Don't need this comment in this file since this menu item shouldn't care about
> the overall menu.

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:35: -
(NSTextField*)addLabelWithFrame:(NSRect)frame
On 2013/05/31 19:07:03, sail wrote:
> move to bottom of file?
> 
> this doesn't actually use the frame provided, it just needs the origin, pass
> that instead?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:53:
scoped_nsobject<NSImageView> userIndicator(
On 2013/05/31 19:07:03, sail wrote:
> how about naming this userImageView?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:55: NSImage* indicatorIcon =
ui::ResourceBundle::GetSharedInstance().
On 2013/05/31 19:07:03, sail wrote:
> how about naming this userImage?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:68: [emailField
setTextColor:[NSColor grayColor]];
On 2013/05/31 19:07:03, sail wrote:
> Use constants shared with the views code?

The views code doesn't have a constant... it just sets the text field to be
'disabled'. But, I just found [NSColor disabledControlTextColor], so I've
switched to that. (Done.)

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:85: imageRect.origin.x =
kIndicatorIconPadding;
On 2013/05/31 19:07:03, sail wrote:
> This is a bit confusing, the image view is enlarged by kIndicatorIconPadding
and
> then you offset it by an additional kIndicatorIconPadding.
> 
> I think several things would make this code easier to follow:
>   - don't enlarge the image view
>   - make this view flipped so that the origin is top left
>   - layout controls in the following order: image view, top label, bottom
label
>   - size the parent view based on the bottom label frame

Good advice! Done. That additional offset actually masked some padding I needed.

Comparison shot: http://i.imgur.com/NlzyJQj.png

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button.h (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.h:10: #import
"ui/base/cocoa/controls/hover_image_menu_button_cell.h"
On 2013/05/31 19:07:03, sail wrote:
> On 2013/05/31 09:37:39, tapted wrote:
> > Not sure if this is conventional.. it felt weird to include both button and
> cell
> > headers in order to see [[button cell] setHoverImage:]. Maybe it should just
> be
> > collapsed into one file?
> 
> I think it's ok. But you don't actually use it in this file so can you move
this
> into the .mm file?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.h:17: // Initialize with
initWithFrame:pullsDown:.
On 2013/05/31 19:07:03, sail wrote:
> Don't need this comment. This is the designated initializer for NSPopupButton.
> Unless you specify a new designated initializer callers should already know to
> use that one.

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.h:22: 
On 2013/05/31 19:07:03, sail wrote:
> Maybe add a
> - (HoverImageMenuButtonCell*)hoverImageMenuButtonCell;
> method here?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button.mm:27: [[self menu]
addItemWithTitle:[NSString string]
note: I've removed this from HoverImageMenuButton. It doesn't work with the
fixed setDelegate, and it will play nicer with c/b/ui/cocoa/menu_controller.mm
this way too.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button_cell.h (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.h:24: -
(void)setHoverImage:(NSImage*)newImage;
On 2013/05/31 19:07:03, sail wrote:
> use @property to declare this?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button_cell.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:29: return [self image];
On 2013/05/31 19:07:03, sail wrote:
> How does this work? Shouldn't this be [buttonMenuItem image]?

It does work - there's even tests ;). This felt more in line with how a cell
should retrieve its default image. Maybe it will cover the (hypothetical) case
for setUsesItemFromMenu:YES as well, but I haven't tested that.

I've added a comment, but let me know if I should change it.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:33: [self
setUsesItemFromMenu:NO];
On 2013/05/31 19:07:03, sail wrote:
> Since the button class explicitly states that the first menu item is not used
I
> think it makes sense to always enforce that. How about moving this to init
> instead?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:34:
scoped_nsobject<NSMenuItem> buttonItem([[NSMenuItem alloc] init]);
On 2013/05/31 19:07:03, sail wrote:
> how about buttonMenuItem?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
File ui/base/cocoa/controls/hover_image_menu_button_unittest.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:25: Init();
On 2013/05/31 19:07:03, sail wrote:
> don't need this (base class does it already)

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:35: [[menu_button_
cell] setImage:ResetImage(kIdrNormal, &normal_)];
On 2013/05/31 19:07:03, sail wrote:
> How about using system provided images instead of depending on ResourceBundle?
> Something like NSImageNameUserAccounts.

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:37: [[menu_button_
cell] setHoverImage:ResetImage(kIdrHovered, &hovered_)];
On 2013/05/31 19:07:03, sail wrote:
> use hoverImageMenuButtonCell instead of cell ?

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:50:
scoped_nsobject<NSImage> normal_;
On 2013/05/31 19:07:03, sail wrote:
> how about setting these in the constructor, then you don't need ResetImage()

Done.

https://codereview.chromium.org/15955003/diff/22011/ui/base/cocoa/controls/ho...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:64:
EXPECT_NSNE(normal_, pressed_);
On 2013/05/31 19:07:03, sail wrote:
> I don't this is necessary.

Done.

[tapted, 2013-06-03 12:54:24]

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:213: -
(void)addItemToMenu:(NSMenu*)menu
On 2013/05/31 19:16:09, rsesek wrote:
> Have you considered using MenuController?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> It'd need to be moved to ui/base though

I have! But, it would need further refactoring to support the menu item with
custom view. I think I'll still be moving it to base soon, to do context menus
on grid items. Then and I'll follow that up to see whether there is merit making
menu_controller more generic to support the use case here.

[Robert Sesek, 2013-06-03 15:48:03]

On 2013/06/03 12:54:24, tapted wrote:
>
https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
> File ui/app_list/cocoa/apps_search_box_controller.mm (right):
> 
>
https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
> ui/app_list/cocoa/apps_search_box_controller.mm:213: -
> (void)addItemToMenu:(NSMenu*)menu
> On 2013/05/31 19:16:09, rsesek wrote:
> > Have you considered using MenuController?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > 
> > It'd need to be moved to ui/base though
> 
> I have! But, it would need further refactoring to support the menu item with
> custom view. I think I'll still be moving it to base soon, to do context menus
> on grid items. Then and I'll follow that up to see whether there is merit
making
> menu_controller more generic to support the use case here.

No, you can just subclass it and override -addItemToMenu:atIndex:fromModel:,
like done in for the custom views in the wrench menu.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[tapted, 2013-06-04 00:27:45]

On 2013/06/03 15:48:03, rsesek wrote:
> On 2013/06/03 12:54:24, tapted wrote:
> >
>
https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
> > File ui/app_list/cocoa/apps_search_box_controller.mm (right):
> > 
> >
>
https://codereview.chromium.org/15955003/diff/22011/ui/app_list/cocoa/apps_se...
> > ui/app_list/cocoa/apps_search_box_controller.mm:213: -
> > (void)addItemToMenu:(NSMenu*)menu
> > On 2013/05/31 19:16:09, rsesek wrote:
> > > Have you considered using MenuController?
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > > 
> > > It'd need to be moved to ui/base though
> > 
> > I have! But, it would need further refactoring to support the menu item with
> > custom view. I think I'll still be moving it to base soon, to do context
menus
> > on grid items. Then and I'll follow that up to see whether there is merit
> making
> > menu_controller more generic to support the use case here.
> 
> No, you can just subclass it and override -addItemToMenu:atIndex:fromModel:,
> like done in for the custom views in the wrench menu.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

OK - let me try that and get up something based off moving menu_controller to
ui/base

[tapted, 2013-06-05 09:00:39]

Patch Set 10 (v 9) is the result of switching over to a menu_controller.h in
ui/base, based off http://crrev.com/15870006

In Patch Set 11 I discovered [NSFont menuFontOfSize:0] which makes things
simpler elsewhere.

Review comments always welcome, but I will need to get http://crrev.com/15870006
and http://crrev.com/15653008 landed before this.

[Robert Sesek, 2013-06-05 21:06:33]

On 2013/05/31 18:25:35, rsesek wrote:
>
https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
> File ui/app_list/cocoa/app_list_menu_cocoa.mm (right):
> 
>
https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
> ui/app_list/cocoa/app_list_menu_cocoa.mm:23: @interface AppListMenuCocoaCell :
> NSPopUpButtonCell {
> On 2013/05/31 06:05:13, tapted wrote:
> > On 2013/05/31 01:20:16, sail wrote:
> > > How about moving this to ui/base/cocoa and making it a HoverImageButton
> > subclass
> > > instead. Similar to MenuButton from Chrome.
> > 
> > I tried using HoverImageButton, and incorporating relevant bits from
> MenuButton
> > (code here:
> >
>
https://codereview.chromium.org/15814009/diff2/1:4002/ui/app_list/cocoa/app_l...
> > ). MenuButton in c/b/ui essentially "hacks" [sic] in parts of NSPopUpButton
to
> > achieve its results, so it becomes a bit messy.
> > 
> > Ultimately, I couldn't figure out how to get HoverImageButton from ui/base
to
> do
> > what's needed anyway. The problem is that the button comes "up" as soon as
the
> > mouse button is released or leaves the button area, but we want it to stay
> > "down" until the menu is dismissed. Using NSPopUpButton directly rather than
> > hacking it in does this job nicely, while also dealing with the other
> menu-isms
> > like dragging to select an item.
> > 
> > Adapting NSPopUpButtonCell to handle hover currently feels like the lesser
> evil,
> > but maybe I've missed something..
> 
> I think this is because HoverButton isn't examining the -[NSButton state]. If
it
> did that to check if it's on the OnState rather than the mouseDown/Up flags it
> currently sets, this may work.

FYI I just wrote https://codereview.chromium.org/16098012/ that may fix this.
I'd be curious to see if this resolves your issue and allows you to not need the
two custom subclasses. I tried patching in this CL to test myself, but it didn't
apply.

[tapted, 2013-06-06 08:17:13]

On 2013/06/05 21:06:33, rsesek wrote:
> On 2013/05/31 18:25:35, rsesek wrote:
> >
>
https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
> > File ui/app_list/cocoa/app_list_menu_cocoa.mm (right):
> > 
> >
>
https://codereview.chromium.org/15955003/diff/4001/ui/app_list/cocoa/app_list...
> > ui/app_list/cocoa/app_list_menu_cocoa.mm:23: @interface AppListMenuCocoaCell
:
> > NSPopUpButtonCell {
> > On 2013/05/31 06:05:13, tapted wrote:
> > > On 2013/05/31 01:20:16, sail wrote:
> > > > How about moving this to ui/base/cocoa and making it a HoverImageButton
> > > subclass
> > > > instead. Similar to MenuButton from Chrome.
> > > 
> > > I tried using HoverImageButton, and incorporating relevant bits from
> > MenuButton
> > > (code here:
> > >
> >
>
https://codereview.chromium.org/15814009/diff2/1:4002/ui/app_list/cocoa/app_l...
> > > ). MenuButton in c/b/ui essentially "hacks" [sic] in parts of
NSPopUpButton
> to
> > > achieve its results, so it becomes a bit messy.
> > > 
> > > Ultimately, I couldn't figure out how to get HoverImageButton from ui/base
> to
> > do
> > > what's needed anyway. The problem is that the button comes "up" as soon as
> the
> > > mouse button is released or leaves the button area, but we want it to stay
> > > "down" until the menu is dismissed. Using NSPopUpButton directly rather
than
> > > hacking it in does this job nicely, while also dealing with the other
> > menu-isms
> > > like dragging to select an item.
> > > 
> > > Adapting NSPopUpButtonCell to handle hover currently feels like the lesser
> > evil,
> > > but maybe I've missed something..
> > 
> > I think this is because HoverButton isn't examining the -[NSButton state].
If
> it
> > did that to check if it's on the OnState rather than the mouseDown/Up flags
it
> > currently sets, this may work.
> 
> FYI I just wrote https://codereview.chromium.org/16098012/ that may fix this.
> I'd be curious to see if this resolves your issue and allows you to not need
the
> two custom subclasses. I tried patching in this CL to test myself, but it
didn't
> apply.

I patched in your change and did some experiments. diffs are at
http://crrev.com/15725016/ . There is an improvement - the button stays "down"
while the menu is showing. However, it only behaves correctly when using
setAction, which responds to mouseUp. For the menu-like behavior, the menu
should pop-up on mouseDown. But then extending HoverImageButton to show the menu
on mouseDown makes the image behave oddly again. It can also get into weird
states with dangling menus when the view goes away, and doesn't behave like a
proper menu when doing click-hold + drag to select menu items. If you want to do
your own tests, you'll need to patch in CL 15653008 first (and CL 15870006 if it
gets clobbered by the CQ again).

menu_button.mm in c/b/ui/cocoa has a lot of hacks to get around these issues,
since it doesn't inherit from NSPopUpButton either. But then it uses an
NSPopUpButton to show the menu anyway (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
). In http://crrev.com/15725016/ I tried it without an NSPopUpButton, but it
only works for this CL that way because there's a confinement rect here that
makes using popUpMenuPositioningItem simpler.

[tapted, 2013-06-06 08:40:43]

On 2013/06/05 09:00:39, tapted wrote:
> Review comments always welcome, but I will need to get
http://crrev.com/15870006
> and http://crrev.com/15653008 landed before this.

PTAL - I think this is ready for final review. CL-15870006 is in the queue and
CL-15653008 is stable.

I've also put up an annotated grab at http://i.imgur.com/JmUUNwo.png for
alignment nits in the menu. The red line annotations are 4px wide each.

[sail, 2013-06-11 00:55:21]

lgtm!

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.mm:385: NSPoint anchorPoint =
[[menuButton window] convertBaseToScreen:NSMakePoint(
convertBaseToScreen is deprecated

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
File ui/app_list/cocoa/current_user_menu_item_view.h (right):

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
ui/app_list/cocoa/current_user_menu_item_view.h:18: @interface
CurrentUserMenuItemView : NSView;
no ;

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
File ui/app_list/cocoa/current_user_menu_item_view.mm (right):

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
ui/app_list/cocoa/current_user_menu_item_view.mm:54: // Size the container view
to fit, and ensure the longest label appears
Don't need comma

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
ui/app_list/cocoa/current_user_menu_item_view.mm:55: // centered (if it is the
longest item in the menu).
I don't understand how the labels would appear centered since you're just
expanding the container to fit the widest label.

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button.mm (right):

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button.mm:38: [self
setNeedsDisplay:YES];
Don't need since this is done in cell? Also below.

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button_cell.h (right):

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button_cell.h:13: // A custom
NSPopUpButtonCell that permits a hover image, and draws only an
no comma before and?

[tapted, 2013-06-11 02:32:44]

+thakis for OWNERS in ui (gyp changes)
rsesek/thakis: Do the other changes in ui/base look good? Note that
hover_image_menu_button{,_cell}.{h,mm} is in a ui/base/controls/ subfolder in
patch set 14.

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.mm:385: NSPoint anchorPoint =
[[menuButton window] convertBaseToScreen:NSMakePoint(
On 2013/06/11 00:55:22, sail wrote:
> convertBaseToScreen is deprecated

I know... but convertRectToScreen is not available in 10.6, and I couldn't find
preferred techniques when poking around in c/b/ui/cocoa. Is there another
alternative/workaround?

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
File ui/app_list/cocoa/current_user_menu_item_view.h (right):

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
ui/app_list/cocoa/current_user_menu_item_view.h:18: @interface
CurrentUserMenuItemView : NSView;
On 2013/06/11 00:55:22, sail wrote:
> no ;

Done.

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
File ui/app_list/cocoa/current_user_menu_item_view.mm (right):

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
ui/app_list/cocoa/current_user_menu_item_view.mm:54: // Size the container view
to fit, and ensure the longest label appears
On 2013/06/11 00:55:22, sail wrote:
> Don't need comma

Done.

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/curren...
ui/app_list/cocoa/current_user_menu_item_view.mm:55: // centered (if it is the
longest item in the menu).
On 2013/06/11 00:55:22, sail wrote:
> I don't understand how the labels would appear centered since you're just
> expanding the container to fit the widest label.

I guess a better way to say it would be to ensure the longest menu item has
equal padding on both sides, but that's implicit from "fit the longest label".
So, comment is now just "Size the container view to fit the longest label." :)

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button.mm (right):

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button.mm:38: [self
setNeedsDisplay:YES];
On 2013/06/11 00:55:22, sail wrote:
> Don't need since this is done in cell? Also below.

Done. x2

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button_cell.h (right):

https://codereview.chromium.org/15955003/diff/108001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button_cell.h:13: // A custom
NSPopUpButtonCell that permits a hover image, and draws only an
On 2013/06/11 00:55:22, sail wrote:
> no comma before and?

Done.

[sail, 2013-06-11 18:01:24]

second LGTM

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/108001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.mm:385: NSPoint anchorPoint =
[[menuButton window] convertBaseToScreen:NSMakePoint(
On 2013/06/11 02:32:44, tapted wrote:
> On 2013/06/11 00:55:22, sail wrote:
> > convertBaseToScreen is deprecated
> 
> I know... but convertRectToScreen is not available in 10.6, and I couldn't
find
> preferred techniques when poking around in c/b/ui/cocoa. Is there another
> alternative/workaround?

Actually this looks ok. We do this everywhere else too.

[Robert Sesek, 2013-06-11 18:07:46]

LGTM. Sorry this fell off my radar. Please ping if I don't respond within 24
hours!

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.h (right):

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.h:21: @class HoverImageMenuButton;
nit: alphabetize

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.mm:195: ui::ResourceBundle&
resourceBundle = ui::ResourceBundle::GetSharedInstance();
optional nit: we normally just call this rb. It then makes a lot of things
shorter.

[Nico, 2013-06-11 18:09:33]

https://codereview.chromium.org/15955003/diff/121001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button.h (right):

https://codereview.chromium.org/15955003/diff/121001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button.h:18: // index 0 is ignored and
client code should populate it with a dummy item.
Does it make sense to combine this with chrome/browser/ui/cocoa/menu_button.mm
somehow?

[tapted, 2013-06-11 23:43:34]

https://codereview.chromium.org/15955003/diff/121001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button.h (right):

https://codereview.chromium.org/15955003/diff/121001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button.h:18: // index 0 is ignored and
client code should populate it with a dummy item.
On 2013/06/11 18:09:33, Nico wrote:
> Does it make sense to combine this with chrome/browser/ui/cocoa/menu_button.mm
> somehow?

I gave this a go in some earlier attempts. menu_button.mm has a sticky web of
dependencies: clickhold_button_cell -> image_button_cell -> {themed_window,
c/b/themes/theme_service.h} -> {extensions, profiles, everything}. I can have a
crack at taking it out and pairing it up with some kind of ThemeDelegate, but it
would be a pretty disruptive change..

Also, there's not much beyond NSPopUpButton that's actually needed (e.g.
patchset 1). chrome/brower's MenuButton doesn't use NSPopUpButton, but NSButton
(via ToolbarButton) instead, so I think it's also a bit more complex than it
needs to be. Perhaps a follow-up could be to look at where menu_button.h is
being used in c/b now, and see if there are cases better suited to the trim
version here?

[Nico, 2013-06-11 23:45:10]

On Tue, Jun 11, 2013 at 4:43 PM, <tapted@chromium.org> wrote:

>
> https://codereview.chromium.**org/15955003/diff/121001/ui/**
>
base/cocoa/controls/hover_**image_menu_button.h<https://codereview.chromium.org/15955003/diff/121001/ui/base/cocoa/controls/hover_image_menu_button.h>
> File ui/base/cocoa/controls/hover_**image_menu_button.h (right):
>
> https://codereview.chromium.**org/15955003/diff/121001/ui/**
>
base/cocoa/controls/hover_**image_menu_button.h#newcode18<https://codereview.chromium.org/15955003/diff/121001/ui/base/cocoa/controls/hover_image_menu_button.h#newcode18>
> ui/base/cocoa/controls/hover_**image_menu_button.h:18: // index 0 is
> ignored and client code should populate it with a dummy item.
> On 2013/06/11 18:09:33, Nico wrote:
>
>> Does it make sense to combine this with
>>
> chrome/browser/ui/cocoa/menu_**button.mm <http://menu_button.mm>
>
>> somehow?
>>
>
> I gave this a go in some earlier attempts. menu_button.mm has a sticky
> web of dependencies: clickhold_button_cell -> image_button_cell ->
> {themed_window, c/b/themes/theme_service.h} -> {extensions, profiles,
> everything}. I can have a crack at taking it out and pairing it up with
> some kind of ThemeDelegate, but it would be a pretty disruptive change..
>
> Also, there's not much beyond NSPopUpButton that's actually needed (e.g.
> patchset 1). chrome/brower's MenuButton doesn't use NSPopUpButton, but
> NSButton (via ToolbarButton) instead, so I think it's also a bit more
> complex than it needs to be. Perhaps a follow-up could be to look at
> where menu_button.h is being used in c/b now, and see if there are cases
> better suited to the trim version here?
>

Makes sense; might want to mention some of that in the CL description.
Thanks!


>
>
https://codereview.chromium.**org/15955003/<https://codereview.chromium.org/1...
>

[tapted, 2013-06-12 00:01:10]

On 2013/06/11 23:45:10, Nico wrote:
> On Tue, Jun 11, 2013 at 4:43 PM, <mailto:tapted@chromium.org> wrote:
> Makes sense; might want to mention some of that in the CL description.
> Thanks!

Done!

Now reads,


Menu for the OSX app launcher, HoverImageMenuButton in
src/ui/base/cocoa/controls. 

Adds a drop-down menu to the right of the search entry area on the OSX 
app launcher. The menu is shown when clicked, and the button responds to 
hover effects. 

The menu button uses a new class, HoverImageMenuButton, which is derived 
from an NSPopUpButton with minor extensions. Notably, it does not have a 
dependency on browser themes, as does MenuButton from 
chrome/browser/ui/cocoa. It tracks the mouse hover state and updates 
the cell, which extends NSPopUpButtonCell and shows only the image in 
the control frame -- no border, bezel, label, or dropdown arrow. 

HoverImageMenuButtonCell supports a hover image, which behaves much like 
an additional 'alternateImage' from NSButtonCell but for the hover 
state, rather than the 'pressed' (or 'lit') state. 

The menu shows the currently signed-in user, in a custom view as the 
first item. It also (currently) shows menu options for Settings, Help, and 
Feedback. 

BUG=138633
TEST=Added app_list_unittests AppsSearchBoxMenuTest and 
AppsSearchBoxMenuTest and tested manually to ensure the items are 
launched correctly. Added ui_unittests HoverImageMenuButtonTest.*

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.h (right):

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.h:21: @class HoverImageMenuButton;
On 2013/06/11 18:07:47, rsesek wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/15955003/diff/121001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.mm:195: ui::ResourceBundle&
resourceBundle = ui::ResourceBundle::GetSharedInstance();
On 2013/06/11 18:07:47, rsesek wrote:
> optional nit: we normally just call this rb. It then makes a lot of things
> shorter.

Done.

[tapted, 2013-06-12 23:49:49]

thakis: ping? (lgty?)

[Nico, 2013-06-12 23:52:15]

On 2013/06/12 23:49:49, tapted wrote:
> thakis: ping? (lgty?)

Yes, lgtm. Sorry,  I thought I said that already.

[commit-bot: I haz the power, 2013-06-12 23:58:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/15955003/136001

[commit-bot: I haz the power, 2013-06-13 02:32:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/15955003/136001

[commit-bot: I haz the power, 2013-06-13 06:47:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-13 09:39:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/15955003/136001

[commit-bot: I haz the power, 2013-06-13 09:45:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-13 22:15:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/15955003/136001

[commit-bot: I haz the power, 2013-06-14 00:12:54]

Change committed as 206237

[tapted, 2013-06-14 09:27:29]

Ended up reverting this due some stuff valgrind picked up. My theories below.
I'll see if I can convince my snow leopard macbook to run valgrind and get a
repro (and a fix!) next week. Details: http://crbug.com/249630

https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button_cell.mm (right):

https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:53: -
(void)setImage:(NSImage*)defaultImage {
Annoyingly, AppKit calls this from [super initTextCell:], and it seems to use
setUsesItemFromMenu: as an indication whether or not to release things. Leading
to leaks (see http://crbug.com/249630 ).

Current plan: rename this function, e.g. to setDefaultImage.

https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button_unittest.mm (right):

https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button_unittest.mm:40: [NSImage
imageNamed:NSImageNameStatusAvailable]) retain]);
This seems to cause AppKit to use a custom class with a -[NSCoreUIImageRep draw]
which goes over an uninitialized jump that valgrind picked up. Probably an Apple
bug like what was reported at http://crbug.com/69436 . (more details in
http://crbug.com/249630 ). 

Maybe it's fixed after 10.6... Not sure whether to do something different here
or add a suppression (the old suppression for NSCoreUIImageRep is gone). Will
think about it over the weekend.

[Robert Sesek, 2013-06-14 16:48:09]

https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button_cell.mm (right):

https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button_cell.mm:53: -
(void)setImage:(NSImage*)defaultImage {
On 2013/06/14 09:27:30, tapted wrote:
> Current plan: rename this function, e.g. to setDefaultImage.

SGTM

[tapted, 2013-06-17 07:58:10]

Robert, PTAL at the update in Patchset 17.

I got a local repro in 10.6 and tested the fix with
tools/valgrind/chrome_tests.sh . Changing this function name actually clears up
both valgrind errors on my macbook, so suppressions shouldn't be needed.

(there's a valgrind tryjob on the CL, but I think ui_unittests is only
configured on the main waterfall).

On 2013/06/14 16:48:09, rsesek wrote:
>
https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
> File ui/base/cocoa/controls/hover_image_menu_button_cell.mm (right):
> 
>
https://codereview.chromium.org/15955003/diff/136001/ui/base/cocoa/controls/h...
> ui/base/cocoa/controls/hover_image_menu_button_cell.mm:53: -
> (void)setImage:(NSImage*)defaultImage {
> On 2013/06/14 09:27:30, tapted wrote:
> > Current plan: rename this function, e.g. to setDefaultImage.
> 
> SGTM

Done.

[Robert Sesek, 2013-06-17 15:46:26]

lgtm

https://codereview.chromium.org/15955003/diff/197001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button_cell.h (right):

https://codereview.chromium.org/15955003/diff/197001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button_cell.h:14: // image in its frame;
no border, bezel or drop-down arrow. Use setDefaultImage
nit: setDefaultImage: is the name (colons are part of the name)

[commit-bot: I haz the power, 2013-06-17 23:09:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/15955003/210001

[tapted, 2013-06-17 23:10:33]

https://codereview.chromium.org/15955003/diff/197001/ui/base/cocoa/controls/h...
File ui/base/cocoa/controls/hover_image_menu_button_cell.h (right):

https://codereview.chromium.org/15955003/diff/197001/ui/base/cocoa/controls/h...
ui/base/cocoa/controls/hover_image_menu_button_cell.h:14: // image in its frame;
no border, bezel or drop-down arrow. Use setDefaultImage
On 2013/06/17 15:46:27, rsesek wrote:
> nit: setDefaultImage: is the name (colons are part of the name)

Done.

[commit-bot: I haz the power, 2013-06-18 00:28:50]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-18 00:39:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/15955003/210001

[commit-bot: I haz the power, 2013-06-18 11:09:34]

Change committed as 206930