New custom styling for action box menu on Os X.

New custom styling for action box menu on Os X.

BUG=138118
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162058

[beaudoin, 2012-10-12 01:45:10]

@ben : For c/*
@shess : For c/app/nibs/*  and  c/b/ui/cocoa/*

This replaces the native menu for action box by a custom-built menu in a bubble.

[Ben Goodger (Google), 2012-10-12 21:44:43]

Note that you can always TBR gyp changes like this to me. LGTM.

[Scott Hess - ex-Googler, 2012-10-12 23:02:47]

checkpoint, I need to finish other stuff today.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/inf...
File chrome/browser/ui/cocoa/info_bubble_view.mm (right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/inf...
chrome/browser/ui/cocoa/info_bubble_view.mm:22: backgroundColor_.reset([NSColor
whiteColor]);
I think [NSColor whiteColor] logically returns an autoreleased item, not an
allocated item, so this should include a retain.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h
(right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h:26:
scoped_ptr<ui::MenuModel> model_;
Doesn't look weak to me.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h:65:
@property(assign, nonatomic) IBOutlet NSTextField* nameField;
IBOutlet is an empty macro which says "Hey, Interface Builder, this is an
outlet."  It needs to be on the instance variable, and is not necessary here.

Actually, I don't even think you need these @property lines unless you intend to
access the member variable from outside the class.  You might have to change
cases like self.iconView to iconView in your implementation file.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h:96:
IBOutlet ActionBoxMenuItemController* viewController;
This seems like it could be one line?

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:288:
[self updateTrackingAreas];
You sure you need this?  At this point, I wouldn't expect the view to even be in
a window or anything.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:333:
alpha:0.06];
Could probably get by with +colorWithDeviceWhite:0.0 alpha:0.06.  Should
document your magic alpha.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:353:
[[super accessibilityAttributeNames] mutableCopy];
I would pull the -autorelease right into here so there is no failure window.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:37: NSPoint anchor =
NSMakePoint(NSMaxX(bounds) - 1, NSMaxY(bounds) - 2);
Provide some explanation of the magic constants.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:39: return [[field
window] convertBaseToScreen:anchor];
-[ChromeToMobileBubbleController showWindow:] also calls -convertBaseToScreen:
on this.  I imagine it has alignment concerns, also.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:66: void
PlusDecoration::ShowMenu(scoped_ptr<ActionBoxMenuModel> menu_model) {
This will close the existing menu and open a new one if you click on it while a
menu is up.  Not sure if that's an issue for now, but seems odd.

[beaudoin, 2012-10-13 00:20:55]

Answered first round of comments.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/inf...
File chrome/browser/ui/cocoa/info_bubble_view.mm (right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/inf...
chrome/browser/ui/cocoa/info_bubble_view.mm:22: backgroundColor_.reset([NSColor
whiteColor]);
On 2012/10/12 23:02:47, shess wrote:
> I think [NSColor whiteColor] logically returns an autoreleased item, not an
> allocated item, so this should include a retain.

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h
(right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h:26:
scoped_ptr<ui::MenuModel> model_;
On 2012/10/12 23:02:47, shess wrote:
> Doesn't look weak to me.

Leftover from old implementation. Good catch!

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h:65:
@property(assign, nonatomic) IBOutlet NSTextField* nameField;
On 2012/10/12 23:02:47, shess wrote:
> IBOutlet is an empty macro which says "Hey, Interface Builder, this is an
> outlet."  It needs to be on the instance variable, and is not necessary here.
> 
> Actually, I don't even think you need these @property lines unless you intend
to
> access the member variable from outside the class.  You might have to change
> cases like self.iconView to iconView in your implementation file.

Still use nameField outside, so I left it. Took out the non-needed stuff.

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h:96:
IBOutlet ActionBoxMenuItemController* viewController;
On 2012/10/12 23:02:47, shess wrote:
> This seems like it could be one line?

By moving the IBOutlet it now fits!

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:288:
[self updateTrackingAreas];
On 2012/10/12 23:02:47, shess wrote:
> You sure you need this?  At this point, I wouldn't expect the view to even be
in
> a window or anything.

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:333:
alpha:0.06];
On 2012/10/12 23:02:47, shess wrote:
> Could probably get by with +colorWithDeviceWhite:0.0 alpha:0.06.  Should
> document your magic alpha.

As in: The UI designer said 6%? :)

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:353:
[[super accessibilityAttributeNames] mutableCopy];
On 2012/10/12 23:02:47, shess wrote:
> I would pull the -autorelease right into here so there is no failure window.

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right):

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:37: NSPoint anchor =
NSMakePoint(NSMaxX(bounds) - 1, NSMaxY(bounds) - 2);
On 2012/10/12 23:02:47, shess wrote:
> Provide some explanation of the magic constants.

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:39: return [[field
window] convertBaseToScreen:anchor];
On 2012/10/12 23:02:47, shess wrote:
> -[ChromeToMobileBubbleController showWindow:] also calls -convertBaseToScreen:
> on this.  I imagine it has alignment concerns, also.

The alignment concerns are slighty different here since the menu has to appear
attached to the omnibox in just the right way, which includes a slight vertical
overlap.

I have moved the call to -convertBaseToScreen to ActionBoxMenuBubbleController
to make it more consistent with ChromeToMobile.

Done.

http://codereview.chromium.org/11103042/diff/7001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:66: void
PlusDecoration::ShowMenu(scoped_ptr<ActionBoxMenuModel> menu_model) {
On 2012/10/12 23:02:47, shess wrote:
> This will close the existing menu and open a new one if you click on it while
a
> menu is up.  Not sure if that's an issue for now, but seems odd.

This is exactly what happened. I have created crbug.com/155667 to track this.

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

Some more feedback.

This really feels like you're re-implementing an NSMatrix of custom
NSButtonCell.  I don't see any commentary or anything indicating why this is
necessary rather than using regular OSX idioms.  Is there something missing that
would explain that?

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/bas...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:264: // Account for the 1
pixel borders.
Is this a TODO, or is it descriptive of what the following line does?

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:44: //
Alpha of the black rectangle overlayed on the selected item..
Double period.  While you're there, make the comment reflect intent (at least
mention hover somewhere), rather than implementation.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:73:
int event_flags = event_utils::EventFlagsFromNSEvent([NSApp currentEvent]);
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.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:83:
NSRect contentRect = NSMakeRect(0, 0, kBubbleMinWidth, 150);
Is there any reason not to just use NSZeroRect?

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:92:
anchoredAt:[parent convertBaseToScreen:point]]) {
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.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:100:
blue:(251.0f/255.0f)
colorWithDeviceWhite:alpha: seems like it would match the intent better, unless
you're going to tweak the balance later.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:119:
for (int i = model_->GetItemCount() - 1; i >= 0; --i) {
Is that really returning an int?  I'd have expected size_t.  Why are you
constructing things in reverse?

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:124:
menuController:self] autorelease];
Generally use scoped_nsobject<> rather than -autorelease for code where you're
not returning the object to some caller.

Also, alignment of the ':'.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:131:
NSMaxX([itemController.nameField frame]) + kRightMargin);
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]).

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:152: -
(NSMutableArray*)items {
Do you expect the caller to modify it?

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:157:
[self interpretKeyEvents:[NSArray arrayWithObject:theEvent]];
This is unusual enough that it deserves a comment as to what things you expect
it to handle.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:176:
old_index = i;
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;
    }
  }

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:245:
static_cast<ActionBoxMenuItemView*>(self.view).viewController = nil;
There's an objective-c cast somewhere in base/mac.  This feels odd, though.

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

We considered using NSMatrix/NSButtonCell but in the end decided to imitate
AvatarMenuBubbleController that has roughly the same specs as the ActionBoxMenu.
The goal is to extract some common code for these two menus at some point. I've
created an issue to track this:
  crbug.com/155868

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/bas...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:264: // Account for the 1
pixel borders.
On 2012/10/15 06:06:35, shess wrote:
> Is this a TODO, or is it descriptive of what the following line does?

Leftover. (Now taken care of upstream, similarly to kAlignEdgeToAnchorEdge.)

Done.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:44: //
Alpha of the black rectangle overlayed on the selected item..
On 2012/10/15 06:06:35, shess wrote:
> Double period.  While you're there, make the comment reflect intent (at least
> mention hover somewhere), rather than implementation.

Done.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:73:
int event_flags = event_utils::EventFlagsFromNSEvent([NSApp currentEvent]);
On 2012/10/15 06:06:35, shess wrote:
> 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.

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

Done.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:83:
NSRect contentRect = NSMakeRect(0, 0, kBubbleMinWidth, 150);
On 2012/10/15 06:06:35, shess wrote:
> Is there any reason not to just use NSZeroRect?

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.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:92:
anchoredAt:[parent convertBaseToScreen:point]]) {
On 2012/10/15 06:06:35, shess wrote:
> 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.

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.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:100:
blue:(251.0f/255.0f)
On 2012/10/15 06:06:35, shess wrote:
> colorWithDeviceWhite:alpha: seems like it would match the intent better,
unless
> you're going to tweak the balance later.

Done.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:119:
for (int i = model_->GetItemCount() - 1; i >= 0; --i) {
On 2012/10/15 06:06:35, shess wrote:
> Is that really returning an int?  I'd have expected size_t.  Why are you
> constructing things in reverse?

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

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:124:
menuController:self] autorelease];
On 2012/10/15 06:06:35, shess wrote:
> Generally use scoped_nsobject<> rather than -autorelease for code where you're
> not returning the object to some caller.
> 
> Also, alignment of the ':'.

Done.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:131:
NSMaxX([itemController.nameField frame]) + kRightMargin);
On 2012/10/15 06:06:35, shess wrote:
> 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]).

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.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:152: -
(NSMutableArray*)items {
On 2012/10/15 06:06:35, shess wrote:
> Do you expect the caller to modify it?

Done.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:157:
[self interpretKeyEvents:[NSArray arrayWithObject:theEvent]];
On 2012/10/15 06:06:35, shess wrote:
> This is unusual enough that it deserves a comment as to what things you expect
> it to handle.

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

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:176:
old_index = i;
On 2012/10/15 06:06:35, shess wrote:
> 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;
>     }
>   }

Simplified that entire method quite a bit.

Done.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:245:
static_cast<ActionBoxMenuItemView*>(self.view).viewController = nil;
On 2012/10/15 06:06:35, shess wrote:
> There's an objective-c cast somewhere in base/mac.  This feels odd, though.

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]

On 2012/10/15 16:42:57, beaudoin wrote:
> We considered using NSMatrix/NSButtonCell but in the end decided to imitate
> AvatarMenuBubbleController that has roughly the same specs as the
ActionBoxMenu.
> The goal is to extract some common code for these two menus at some point.
I've
> created an issue to track this:
>   crbug.com/155868

I'd bet that NSMatrix/NSButtonCell would remove at least half of the code,
because it already handles things like accessibility and tracking the mouse for
highlighting.

I think it would be useful to have at least a minimal unit test to make sure
that all of this code runs and connects correctly, switches which node is
highlighted, that sort of thing.  I won't treat it as a blocking issue, but it
feels like you're creating a bunch of cocoa-side wiring which someone needs to
make sure stays right.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:83:
NSRect contentRect = NSMakeRect(0, 0, kBubbleMinWidth, 150);
On 2012/10/15 16:42:57, beaudoin wrote:
> On 2012/10/15 06:06:35, shess wrote:
> > Is there any reason not to just use NSZeroRect?
> 
> 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.

OK.

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:131:
NSMaxX([itemController.nameField frame]) + kRightMargin);
On 2012/10/15 16:42:57, beaudoin wrote:
> On 2012/10/15 06:06:35, shess wrote:
> > 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]).
> 
> 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.

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...

http://codereview.chromium.org/11103042/diff/7003/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:245:
static_cast<ActionBoxMenuItemView*>(self.view).viewController = nil;
On 2012/10/15 16:42:57, beaudoin wrote:
> On 2012/10/15 06:06:35, shess wrote:
> > There's an objective-c cast somewhere in base/mac.  This feels odd, though.
> 
> 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.

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.

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:165: }
I seem to see this basic loop three or four times.  Suggest a helper like
-highlightedItem (NSMenu uses that), then if you need to convert to an index,
use -[NSArray indexOfObject:].

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:183:
for (NSUInteger i = startIndex; i != endIndex; i -= delta) {
This is 37% too tricky.  You don't really care which order you iterate in, you
only care whether you've found the single highlighted item or not.  I think if
you just find the highlighted item and then make your decisions based on that,
the code won't be any longer, and it will be more straight-forward.

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:306:
[viewController_ highlightForEventType:[[NSApp currentEvent] type]];
I think |sender| is the event you want, here.  Maybe even just pass the event
and let the receiver decode the type.

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:307:
[self setNeedsDisplay:YES];
The -setNeedsDisplay: probably belong up in -highlightItem:.  Also for
-mouseExited:.

[beaudoin, 2012-10-15 22:38:38]

I've added the need for unit test to a separate crbug.com/155971

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:165: }
On 2012/10/15 21:15:18, shess wrote:
> I seem to see this basic loop three or four times.  Suggest a helper like
> -highlightedItem (NSMenu uses that), then if you need to convert to an index,
> use -[NSArray indexOfObject:].

Done.

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:183:
for (NSUInteger i = startIndex; i != endIndex; i -= delta) {
On 2012/10/15 21:15:18, shess wrote:
> This is 37% too tricky.  You don't really care which order you iterate in, you
> only care whether you've found the single highlighted item or not.  I think if
> you just find the highlighted item and then make your decisions based on that,
> the code won't be any longer, and it will be more straight-forward.

I think my code was much more elegant. But it's true that new helper method
makes the "vanilla" algorithm somewhat acceptable. ;)

Done.

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:306:
[viewController_ highlightForEventType:[[NSApp currentEvent] type]];
On 2012/10/15 21:15:18, shess wrote:
> I think |sender| is the event you want, here.  Maybe even just pass the event
> and let the receiver decode the type.

Done.

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:307:
[self setNeedsDisplay:YES];
On 2012/10/15 21:15:18, shess wrote:
> The -setNeedsDisplay: probably belong up in -highlightItem:.  Also for
> -mouseExited:.

In fact it's already called by -setIsHighlighted.

Done.

[Scott Hess - ex-Googler, 2012-10-15 23:03:22]

I'm going to say LGTM.  I think -items can go.  Suggest either mentioning
nil/NSNotFound case in a comment, or testing for nil around that bit, as-is it
looks a little suspicious even though it's not.

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:140: -
(const NSArray*)items {
Man, wish I'd seen that before :-).  const is tricky with ObjC, generally stick
with NSSomething/NSMutableSomething.

Is this method still necessary?

http://codereview.chromium.org/11103042/diff/19001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:183:
for (NSUInteger i = startIndex; i != endIndex; i -= delta) {
On 2012/10/15 22:38:39, beaudoin wrote:
> On 2012/10/15 21:15:18, shess wrote:
> > This is 37% too tricky.  You don't really care which order you iterate in,
you
> > only care whether you've found the single highlighted item or not.  I think
if
> > you just find the highlighted item and then make your decisions based on
that,
> > the code won't be any longer, and it will be more straight-forward.
> 
> I think my code was much more elegant. But it's true that new helper method
> makes the "vanilla" algorithm somewhat acceptable. ;)
> 
> Done.

:-).  Mostly I was thinking "If I'm in here debugging something, the first thing
I'm going to look at it this bi-directional loop".

http://codereview.chromium.org/11103042/diff/12012/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/12012/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:84: 
When does this come up?

Oh, nevermind, I see -insertNewline:.  I'm not sure how you're getting the event
in the nil case, though.

http://codereview.chromium.org/11103042/diff/12012/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:188:
if (oldIndex != NSNotFound) {
Is NSNotFound the nil case?

[beaudoin, 2012-10-15 23:31:18]

Thanks for the review! Wrapped up the final fixes, submitting now.

http://codereview.chromium.org/11103042/diff/12012/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

http://codereview.chromium.org/11103042/diff/12012/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:84: 
On 2012/10/15 23:03:23, shess wrote:
> When does this come up?
> 
> Oh, nevermind, I see -insertNewline:.  I'm not sure how you're getting the
event
> in the nil case, though.

If you presse Enter before pressing any arrow, -insertNewline is called but
nothing is highlighted. I did not very this, but just assumed it could happen.

Added a clarifying comment.

Done.

http://codereview.chromium.org/11103042/diff/12012/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:188:
if (oldIndex != NSNotFound) {
On 2012/10/15 23:03:23, shess wrote:
> Is NSNotFound the nil case?

It is. Added a clarifying comment.

Done.

[commit-bot: I haz the power, 2012-10-15 23:31:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11103042/18014

[commit-bot: I haz the power, 2012-10-16 03:04:32]

Change committed as 162058

[Nico, 2012-10-17 17:26:34]

Why isn't this just a popup menu? This looks like a badly reimplemented regular
menu.

[beaudoin, 2012-10-17 17:34:50]

On 2012/10/17 17:26:34, Nico wrote:
> Why isn't this just a popup menu? This looks like a badly reimplemented
regular
> menu.

This was the issue of intense debates. IIUC, basically, we're moving away from
native menus on all platforms (for all menus?) and it appeared that the custom
styling we needed couldn't be implemented in regular menus. Also, the Avatar
menu went that route before.

[Ben Goodger (Google), 2012-10-17 17:35:40]

After seeing what this looks like I agree. I talked to glen yesterday and
he said it's supposed to be a bubble, not a menu. So that's what should be
done instead.


On Wed, Oct 17, 2012 at 10:26 AM, <thakis@chromium.org> wrote:

> Why isn't this just a popup menu? This looks like a badly reimplemented
> regular
> menu.
>
>
https://chromiumcodereview.**appspot.com/11103042/<https://chromiumcodereview...
>