Action box menu

chrome/browser/ui/views/location_bar/action_box_button_view.cc

Index: chrome/browser/ui/views/location_bar/action_box_button_view.cc
diff --git a/chrome/browser/ui/views/location_bar/action_box_button_view.cc b/chrome/browser/ui/views/location_bar/action_box_button_view.cc
index b53bd1597e75e9e1fbb04ca9cbfb7a887468e89c..1778b32ac1484b397bfd6c2b98f4d6a57116a626 100644
--- a/chrome/browser/ui/views/location_bar/action_box_button_view.cc
+++ b/chrome/browser/ui/views/location_bar/action_box_button_view.cc
@@ -8,7 +8,12 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/command_updater.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/tab_contents/tab_contents.h"
+#include "chrome/browser/ui/toolbar/action_box_menu_model.h"
 #include "chrome/browser/ui/view_ids.h"
+#include "chrome/browser/ui/views/action_box_menu.h"
 #include "chrome/browser/ui/views/browser_dialogs.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
@@ -30,9 +35,12 @@ const SkColor kPushedBorderColor = SkColorSetRGB(191, 191, 191);
 }  // namespace
 
 
-ActionBoxButtonView::ActionBoxButtonView(ExtensionService* extension_service)
+ActionBoxButtonView::ActionBoxButtonView(Profile* profile,
+                                         Delegate* delegate)
     : views::MenuButton(NULL, string16(), this, false),
-      extension_service_(extension_service) {
+      profile_(profile),
+      delegate_(delegate),
+      bookmark_state_(false) {
   set_id(VIEW_ID_ACTION_BOX_BUTTON);
   SetTooltipText(l10n_util::GetStringUTF16(IDS_TOOLTIP_ACTION_BOX_BUTTON));
   SetIcon(*ui::ResourceBundle::GetSharedInstance().GetBitmapNamed(
@@ -73,5 +81,18 @@ void ActionBoxButtonView::GetAccessibleState(ui::AccessibleViewState* state) {
 
 void ActionBoxButtonView::OnMenuButtonClicked(View* source,
                                               const gfx::Point& point) {
-  // TODO(yefim): Implement menu here.
+  ExtensionService* extension_service =
+      extensions::ExtensionSystem::Get(profile_)->extension_service();
+  if (!extension_service)

[Peter Kasting, 2012/07/18 01:37:25]

Can either of these NULL-checks ever actually fail?

[yefimt, 2012/07/18 23:18:13]

I dont know enough yet to say for sure, but there are 7 other  similar places in
code that do the same check (extension_service).
I do see browser NULL at time of this class creation. I dont know if user can
click this button fast enough, probably not, but I think better not to gamble.

[Peter Kasting, 2012/07/19 04:27:31]

The extension service might be able to be NULL when unit-testing.  Is this
particular code unittested?  I'd be very surprised if it is.

If you're still unsure, check with someone who wrote/owns/works on
ExtensionService.  We should NULL-check if and only if NULL is actually
possible.
I am surprised that GetBrowser() returns NULL at that point because we ought to
have a Browser before this class is created -- that's what ultimately owns the
location bar.  This suggests -- as I noted before -- that the right thing to do
is to pass the Browser to the constructor explicitly instead of passing in some
sort of delegate object that muddies the lifetime picture.

[yefimt, 2012/07/19 20:00:15]

It was my original implementation to pass browser to constructor. But it is NULL
at the time of button creation. Maybe it is the way location bar getting browser
(it gets WebContents and then FindBrowserWithWebContents()). At the time button
created WebContents is NULL.

[Peter Kasting, 2012/07/19 20:45:11]

The Browser isn't NULL at this point, the LocationBarView is simply trying to
get it in a way that isn't very good.

You can see if you look in ToolbarView that where |location_bar_| is created we
have a non-NULL |browser_|.  What you should do is pass that directly to the
LocationBarView and have it cache it.  (This will also let you stop passing in
the various arguments calculated on the caller side that use |browser_| in one
way or another; the LocationBarView can do those calculations in the constructor
as needed.)  Then the LocationBarView can get rid of GetBrowserFromDelegate()
and you can eliminate your new delegate entirely.

+    return;
+
+  Browser* browser = delegate_->GetBrowser();
+  if (!browser)
+    return;
+
+  ActionBoxMenuModel model(extension_service);
+  action_box_menu_.reset(new ActionBoxMenu(browser, &model, bookmark_state_));
+  action_box_menu_->Init();
+  action_box_menu_->RunMenu(this);
+  action_box_menu_.reset();
 }