Action box menu

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

 // 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.
 
 #include "chrome/browser/ui/views/location_bar/action_box_button_view.h"
 
 #include "base/utf_string_conversions.h"
 #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"
 #include "ui/base/accessibility/accessible_view_state.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
 namespace {
 
 // Colors used for button backgrounds.
 const SkColor kNormalBackgroundColor = SkColorSetRGB(255, 255, 255);
 const SkColor kHotBackgroundColor = SkColorSetRGB(239, 239, 239);
 const SkColor kPushedBackgroundColor = SkColorSetRGB(207, 207, 207);
 
 const SkColor kNormalBorderColor = SkColorSetRGB(255, 255, 255);
 const SkColor kHotBorderColor = SkColorSetRGB(223, 223, 223);
 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(
       IDR_ACTION_BOX_BUTTON));
   set_accessibility_focusable(true);
   set_border(NULL);
 }
 
 ActionBoxButtonView::~ActionBoxButtonView() {
 }
@@ skipping 20 matching lines @@
   }
 }
 
 void ActionBoxButtonView::GetAccessibleState(ui::AccessibleViewState* state) {
   MenuButton::GetAccessibleState(state);
   state->name = l10n_util::GetStringUTF16(IDS_ACCNAME_ACTION_BOX_BUTTON);
 }
 
 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();
 }