Add a menu to the Action Box Button.

chrome/browser/ui/cocoa/location_bar/plus_decoration.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/plus_decoration.h"
 
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/command_updater.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
 #import "chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h"
+#import "chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h"
+#import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.h"
+#import "chrome/browser/ui/cocoa/menu_controller.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util_mac.h"
+#include "ui/base/models/simple_menu_model.h"
+#include "ui/gfx/image/image_skia_util_mac.h"
 
-PlusDecoration::PlusDecoration(CommandUpdater* command_updater)
-    : command_updater_(command_updater) {
+namespace {
+// The offset to reach the end of the omnibox from the plus decoration icon.
+// TODO(beaudoin): Most likely wont work in High DPI.
+const CGFloat kOmniboxXOffset = 10.0;
+}  // namespace
+
+PlusDecoration::PlusDecoration(LocationBarViewMac* owner,
+    CommandUpdater* command_updater, Browser* browser)
+    :owner_(owner),

[Scott Hess - ex-Googler, 2012/07/27 20:31:06]

Either this line is indented wrong, or the others are.  I think you also need a
space after :.

[beaudoin, 2012/08/01 02:23:26]

Done.

+    command_updater_(command_updater),
+    browser_(browser) {

[Scott Hess - ex-Googler, 2012/07/27 20:31:06]

You aren't actually using browser_ at this time, AFAICT.

[beaudoin, 2012/08/01 02:23:26]

If you don't mind I'll keep browser_ here as I have a CL already ready that
needs it.

   SetVisible(true);
 
   const int image_id = IDR_ACTION_BOX_BUTTON;
   SetImage(OmniboxViewMac::ImageForResource(image_id));
   const int tip_id = IDS_TOOLTIP_ACTION_BOX_BUTTON;
   tooltip_.reset([l10n_util::GetNSStringWithFixup(tip_id) retain]);
 }
 
 PlusDecoration::~PlusDecoration() {
+  popUpCell_.reset(nil);

[Scott Hess - ex-Googler, 2012/07/27 20:31:06]

Sorry, I may have made an assumption, here.  Usually, scoped_nsobject<> can just
be destructed implicitly.  Sometimes an explicit reset is necessary because the
object has weak references to things which might be destructed before the
scoped_nsobject<> is destructed, so you get use-after-free.  It's possible that
this is just here because you saw it somewhere else, in which case you could
just delete this line.

[beaudoin, 2012/08/01 02:23:26]

Done.

 }
 
 bool PlusDecoration::AcceptsMousePress() {
   return true;
 }
 
 bool PlusDecoration::OnMousePressed(NSRect frame) {
-  // TODO(macourteau): trigger the menu when caitkp@ and beaudoin@'s CL is
-  // ready.
+  menu_model_.reset(new ui::SimpleMenuModel(NULL));
+
+  menu_model_->InsertItemWithStringIdAt(0, IDC_CHROME_TO_MOBILE_PAGE,
+                                        IDS_CHROME_TO_MOBILE);
+  menu_model_->InsertItemWithStringIdAt(1, IDC_BOOKMARK_PAGE,
+                                        IDS_BOOKMARK_STAR);
+  menu_model_->SetIcon(0, gfx::ImageSkiaFromNSImage(
+      OmniboxViewMac::ImageForResource(IDR_MOBILE)));
+  menu_model_->SetIcon(1, gfx::ImageSkiaFromNSImage(
+      OmniboxViewMac::ImageForResource(IDR_STAR)));
+
+  menuController_.reset(
+      [[MenuController alloc] initWithModel:menu_model_.get()
+                     useWithPopUpButtonCell:YES]);

[Scott Hess - ex-Googler, 2012/07/27 20:31:06]

I think you should pull the menu_model_, menuController_, and popUpCell_
building out to the constructor, rather than doing it ad-hoc.  This function
should be targetted at popping up the menu.

I think the code itself looks fine, I just would rather it wasn't a mixture of
setting up the context and setting up the activity resulting from the mouse
press.

[beaudoin, 2012/08/01 02:23:26]

At some point the menu model will be dynamic. Since we've decided to go with a
local MenuModel rather than a custom controller, the menu could be out-of-sync
if we create it in the constructor. Creating it at mouse pressed time ensures it
is always synced when displayed.

I've extracted the instantiation to a private method, let me know if it works
for you.

[Scott Hess - ex-Googler, 2012/08/08 15:00:14]

To be clear - there's two parts, here, there's setting up the objects used to
construct the menu, and there's getting the menu.  I expect the menu model to
end up being something which you get from some other code, either at setup or by
asking the browser or something, so it could exist before this method.

The menu controller I'm less certain about.  I _think_ that you could probably
have it scoped to this function, as the -performClickWithFrame:inView: will run
a nested message loop.  If that is not the case, then you need to reset the
controller every time and re-create it, because it caches the menu built.

[beaudoin, 2012/08/08 21:38:08]

On the windows side of things the menu model is created on-the-fly, via new
ActionBoxMenuModel(), whenever a menu is needed. I too, would have expected it
to be a longer lived object, but it's not. I guess it's simpler this way as it
does not have to observe the content that can change dynamically.

Not sure I understand your comment for the controller.

If you want me to change anything in here please let me know.

[Scott Hess - ex-Googler, 2012/08/08 22:04:00]

OK, follow Windows, then.
Basically, I think that -performClickWithFrame: will not return until the menu
is dismissed (it runs a nested message loop).  So you can scope the menu
controller and model to this function, rather than to the object.  You should
check that I'm speaking sense, of course!

[beaudoin, 2012/08/09 16:05:49]

Made model and controller method-scoped. It works and I confirmed that
performClickWithFrame is blocking (via logging).

Done.

+
+  NSMenu* menu = [menuController_ menu];
+
+  frame.origin.x -= (menu.size.width - frame.size.width) - kOmniboxXOffset;
+  frame.size.height += 10.0;

[Scott Hess - ex-Googler, 2012/07/27 20:31:06]

This magic constant could use some explaining.  Actually, the other one, too
:-).  It looks like approximately half the decoration frame height, so it might
be appropriate to use NSMidY() versus NSMinY() or something like that.

Actually - OSX coordinate space can take a little getting used to, maybe it
would be easier to ask where you want it relative to the frame of the
decoration?  I _think_ you're trying to pull it up relative to the overall
omnibox, rather than the decoration.  In that case, you should just use
owner_->GetAutocompleteTextField() to get the omnibox, and call -bounds and
adjust the width and x-pos of that appropriately.

Probably something like:

  NSRect popupFrame = [field bounds];
  popupFrame.origin.x = NSMaxX(popupFrame) - menu.size.width;
  popupFrame.size.width = menu.size.width;

[-frame returns the rectangle a view is in within its parent view, whereas
-bounds returns the rectangle the view thinks of itself in.  To this function
|frame| is within the containing view, which would be in the same coordinate
space as what -bounds returns from that view.]

[beaudoin, 2012/08/01 02:23:26]

Done.

+
+  if (!popUpCell_.get()) {
+    popUpCell_ .reset([[NSPopUpButtonCell alloc] initTextCell:@""
+                       pullsDown:YES]);

[Scott Hess - ex-Googler, 2012/07/27 20:31:06]

Auto-magic alignment is sometimes sketchy with Objective-C, but I think you need
to align with : if possible.

[beaudoin, 2012/08/01 02:23:26]

Copied the height adjustment from somewhere where it was not documented. Dropped
it and it still seems to work. :)

Done.

+  }
+  DCHECK(popUpCell_.get());
+  AutocompleteTextField* field = owner_->GetAutocompleteTextField();
+  [popUpCell_ setMenu:menu];
+  [popUpCell_ selectItem:nil];
+  [popUpCell_ attachPopUpWithFrame:frame inView:field];
+  [popUpCell_ performClickWithFrame:frame inView:field];
   return true;
 }
 
 NSString* PlusDecoration::GetToolTip() {
   return tooltip_.get();
 }