Action box menu

chrome/browser/ui/views/browser_action_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/browser_action_view.h"
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/commands/command_service_factory.h"
 #include "chrome/browser/extensions/extension_context_menu_model.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/views/browser_actions_container.h"
+#include "chrome/browser/ui/views/browser_action_view.h"
 #include "chrome/browser/ui/views/toolbar_view.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_manifest_constants.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"
 #include "ui/gfx/canvas.h"
@@ skipping 11 matching lines @@
   alpha.setConfig(SkBitmap::kARGB_8888_Config, image.width(), image.height());
   alpha.allocPixels();
   alpha.eraseColor(SkColorSetARGB(64, 0, 0, 0));
 
   return SkBitmapOperations::CreateMaskedBitmap(image, alpha);
 }
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
+// BrowserActionView
+
+BrowserActionView::BrowserActionView(const Extension* extension,
+                                     Browser* browser,
+                                     BrowserActionView::Delegate* delegate)
+    : browser_(browser),
+      delegate_(delegate),
+      button_(NULL) {
+  button_ = new BrowserActionButton(extension, browser_, delegate_);

[Peter Kasting, 2012/07/26 20:37:17]

Can move this into ViewHierarchyChanged() and remove the manual memory
management if we move the accesses to button() from the caller to this class;
see comments in ActionBoxMenu::PopulateMenu().

[yefimt, 2012/07/31 00:10:11]

Done.

+  button_->set_owned_by_client();
+  button_->set_drag_controller(delegate_);
+}
+
+BrowserActionView::~BrowserActionView() {
+  RemoveChildView(button_);
+  button_->Destroy();
+}
+
+gfx::Canvas* BrowserActionView::GetIconWithBadge() {
+  int tab_id = delegate_->GetCurrentTabId();
+
+  SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id);
+  if (icon.isNull())
+    icon = button_->default_icon();
+
+  gfx::Canvas* canvas =
+      new gfx::Canvas(gfx::ImageSkiaRep(icon, ui::SCALE_FACTOR_100P), false);
+
+  if (tab_id >= 0) {
+    gfx::Rect bounds(icon.width(), icon.height() + ToolbarView::kVertSpacing);
+    button_->extension()->browser_action()->PaintBadge(canvas, bounds, tab_id);
+  }
+
+  return canvas;
+}
+
+void BrowserActionView::Layout() {
+  // We can't rely on button_->GetPreferredSize() here because that's not set
+  // correctly until the first call to
+  // BrowserActionsContainer::RefreshBrowserActionViews(), whereas this can be
+  // called before that when the initial bounds are set (and then not after,
+  // since the bounds don't change).  So instead of setting the height from the
+  // button's preferred size, we use IconHeight(), since that's how big the
+  // button should be regardless of what it's displaying.
+  gfx::Size size = delegate_->GetViewContentOffset();
+  button_->SetBounds(size.width(), size.height(), width(),

[Peter Kasting, 2012/07/26 20:37:17]

Calling width() here isn't right, Layout() is responsible for determining the
width.  Plus the total width would include the view content offset and clearly
the button itself shouldn't.

In fact the whole layout of these objects at all is confusing.  Is
BrowserActionView supposed to contain a button plus the padding on the left and
top?  If so why is the padding included, why don't we just have the button
directly, and then the owner of these views can lay them out with padding?  And
in that case, can't this class die entirely and we just have BrowserActionButton
that extends the base button classes to do whatever additional special things it
needs?

[yefimt, 2012/07/31 00:10:11]

I think you need to ask someone who wrote this code. You may get a meaningful
reply in this case.

[Peter Kasting, 2012/07/31 02:30:42]

You at least need to use width() - size.width() for the button width.  I think
if you do that this function is probably OK.

As for the larger question, if you're not in a position to answer, can you file
a bug against the appropriate person?  It seems a little unusual for you to be
doing implementation work for the action box menu but not know what's up with
the design of the action box menu, but as long as some relevant party gets the
question I'm satisfied.

[yefimt, 2012/07/31 17:19:35]

It has nothing to do with architecture. It was an existing class
BrowserActionView used in toolbar. I'm using this class in Action Box menu. I
dont need to know BrowserActionView implementation details for that. .h file is
enough in most cases. Yes I had to do some minor changes in this class, in this
specific case line in question (line 90) had a hardcoded offset, so I added a
way to get an offset from a parent class.
As for width() I agree it should be width() - size.width(), fixed.

+                     BrowserActionsContainer::IconHeight());
+}
+
+void BrowserActionView::ViewHierarchyChanged(bool is_add,
+                                             View* parent,
+                                             View* child) {
+  if (is_add && (child == this)) {
+    AddChildView(button_);
+    button_->UpdateState();
+  }
+}
+
+void BrowserActionView::GetAccessibleState(ui::AccessibleViewState* state) {
+  state->name = l10n_util::GetStringUTF16(
+      IDS_ACCNAME_EXTENSIONS_BROWSER_ACTION);
+  state->role = ui::AccessibilityTypes::ROLE_GROUPING;
+}
+
+gfx::Size BrowserActionView::GetPreferredSize() {
+  return gfx::Size(BrowserActionsContainer::IconWidth(false),
+                   BrowserActionsContainer::IconHeight());
+}
+
+void BrowserActionView::PaintChildren(gfx::Canvas* canvas) {
+  View::PaintChildren(canvas);
+  ExtensionAction* action = button()->browser_action();
+  int tab_id = delegate_->GetCurrentTabId();
+  if (tab_id >= 0)
+    action->PaintBadge(canvas, gfx::Rect(width(), height()), tab_id);
+}
+
+////////////////////////////////////////////////////////////////////////////////
 // BrowserActionButton
 
 BrowserActionButton::BrowserActionButton(const Extension* extension,
-                                         BrowserActionsContainer* panel)
+                                         Browser* browser,
+                                         BrowserActionView::Delegate* delegate)
     : ALLOW_THIS_IN_INITIALIZER_LIST(
           MenuButton(this, string16(), NULL, false)),
+      browser_(browser),
       browser_action_(extension->browser_action()),
       extension_(extension),
       ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(this)),
-      panel_(panel),
-      context_menu_(NULL) {
+      delegate_(delegate),
+      context_menu_(NULL),
+      disable_tooltip_(false) {
   set_border(NULL);
   set_alignment(TextButton::ALIGN_CENTER);
   set_context_menu_controller(this);
 
   // No UpdateState() here because View hierarchy not setup yet. Our parent
   // should call UpdateState() after creation.
 
+  content::NotificationSource notification_source =
+      content::Source<Profile>(browser_->profile()->GetOriginalProfile());
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_UPDATED,
                  content::Source<ExtensionAction>(browser_action_));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_COMMAND_ADDED,
-                 content::Source<Profile>(
-                     panel_->profile()->GetOriginalProfile()));
+                 notification_source);
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_COMMAND_REMOVED,
-                 content::Source<Profile>(
-                     panel_->profile()->GetOriginalProfile()));
+                 notification_source);
 }
 
 void BrowserActionButton::Destroy() {
   MaybeUnregisterExtensionCommand(false);
 
   if (context_menu_) {
     context_menu_->Cancel();
     MessageLoop::current()->DeleteSoon(FROM_HERE, this);
   } else {
     delete this;
@@ skipping 30 matching lines @@
 
 bool BrowserActionButton::CanHandleAccelerators() const {
   // View::CanHandleAccelerators() checks to see if the view is visible before
   // allowing it to process accelerators. This is not appropriate for browser
   // actions buttons, which can be hidden inside the overflow area.
   return true;
 }
 
 void BrowserActionButton::ButtonPressed(views::Button* sender,
                                         const views::Event& event) {
-  panel_->OnBrowserActionExecuted(this);
+  delegate_->OnBrowserActionExecuted(this);
 }
 
 void BrowserActionButton::ShowContextMenuForView(View* source,
                                                  const gfx::Point& point) {
-  if (!extension()->ShowConfigureContextMenus())
-    return;
-
-  SetButtonPushed();
-
-  // Reconstructs the menu every time because the menu's contents are dynamic.
-  scoped_refptr<ExtensionContextMenuModel> context_menu_contents_(
-      new ExtensionContextMenuModel(extension(), panel_->browser()));
-  views::MenuModelAdapter menu_model_adapter(context_menu_contents_.get());
-  views::MenuRunner menu_runner(menu_model_adapter.CreateMenu());
-
-  context_menu_ = menu_runner.GetMenu();
-  gfx::Point screen_loc;
-  views::View::ConvertPointToScreen(this, &screen_loc);
-  if (menu_runner.RunMenuAt(GetWidget(), NULL, gfx::Rect(screen_loc, size()),
-          views::MenuItemView::TOPLEFT, views::MenuRunner::HAS_MNEMONICS) ==
-      views::MenuRunner::MENU_DELETED)
-    return;
-
-  SetButtonNotPushed();
-  context_menu_ = NULL;
+  ShowContextMenuImpl();

[Peter Kasting, 2012/07/26 20:37:17]

Nit: Should you have a TODO about handling |source| and |point|, or not?  What
do these args mean?  (2 places)

[yefimt, 2012/07/31 00:10:11]

I dont know, it is an existing implementation and I'm trying to focus on a
functionality I add, not on fixing code I dont understand well enough.

[Peter Kasting, 2012/07/31 02:30:42]

But you're adding one of the calls to this function.  How can you not understand
at least the code you're adding?

If you don't understand the callsite here and need to file a bug against the
relevant person to investigate (see comment elsewhere), that's arguably OK, but
you need to at least know what the callsite you're adding elsewhere in this file
does.

[yefimt, 2012/07/31 17:19:35]

:)
As you noted earlier it were two functions with identical bodies, both didn't
use function's parameters. So per your request I moved body to a third function.
I doesn't require much understanding consider that both functions not related to
action box directly.  

[Peter Kasting, 2012/07/31 18:29:30]

I'm talking about BrowserActionButton::ShowContextMenu(), which is far as I can
tell is brand new in your change.

[yefimt, 2012/08/03 17:29:21]

It is so strange. Somehow codereview is confused.
If you look at original file (not in codereview), you will see
BrowserActionButton::ShowContextMenu() is in the same place and it was one of
the functions you asked me to fix before.

 }
 
 void BrowserActionButton::OnImageLoaded(const gfx::Image& image,
                                         const std::string& extension_id,
                                         int index) {
   if (!image.IsEmpty())
     default_icon_ = *image.ToSkBitmap();
 
   // Call back to UpdateState() because a more specific icon might have been set
   // while the load was outstanding.
   UpdateState();
 }
 
 void BrowserActionButton::UpdateState() {
-  int tab_id = panel_->GetCurrentTabId();
+  int tab_id = delegate_->GetCurrentTabId();
   if (tab_id < 0)
     return;
 
   if (!IsEnabled(tab_id)) {
     SetState(views::CustomButton::BS_DISABLED);
   } else {
     SetState(menu_visible_ ?
              views::CustomButton::BS_NORMAL :
              views::CustomButton::BS_PUSHED);
   }
@@ skipping 28 matching lines @@
     SkBitmap bg_p;
     rb.GetBitmapNamed(IDR_BROWSER_ACTION_P)->copyTo(&bg_p,
         SkBitmap::kARGB_8888_Config);
     SkCanvas bg_p_canvas(bg_p);
     bg_p_canvas.drawBitmap(icon,
         SkIntToScalar((bg_p.width() - icon.width()) / 2),
         SkIntToScalar((bg_p.height() - icon.height()) / 2), &paint);
     SetPushedIcon(bg_p);
   }
 
-  // If the browser action name is empty, show the extension name instead.
-  string16 name = UTF8ToUTF16(browser_action()->GetTitle(tab_id));
-  if (name.empty())
-    name = UTF8ToUTF16(extension()->name());
-  SetTooltipText(name);
+  string16 name = GetTextForTooltip();
+  SetTooltipText(disable_tooltip_ ? string16() : name);
   SetAccessibleName(name);
 
   parent()->SchedulePaint();
 }
 
 bool BrowserActionButton::IsPopup() {
-  int tab_id = panel_->GetCurrentTabId();
+  int tab_id = delegate_->GetCurrentTabId();
   return (tab_id < 0) ? false : browser_action_->HasPopup(tab_id);
 }
 
 GURL BrowserActionButton::GetPopupUrl() {
-  int tab_id = panel_->GetCurrentTabId();
+  int tab_id = delegate_->GetCurrentTabId();
   return (tab_id < 0) ? GURL() : browser_action_->GetPopupUrl(tab_id);
 }
 
 void BrowserActionButton::Observe(int type,
                                   const content::NotificationSource& source,
                                   const content::NotificationDetails& details) {
   switch (type) {
     case chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_UPDATED:
       UpdateState();
       // The browser action may have become visible/hidden so we need to make
       // sure the state gets updated.
-      panel_->OnBrowserActionVisibilityChanged();
+      delegate_->OnBrowserActionVisibilityChanged();
       break;
     case chrome::NOTIFICATION_EXTENSION_COMMAND_ADDED:
     case chrome::NOTIFICATION_EXTENSION_COMMAND_REMOVED: {
       std::pair<const std::string, const std::string>* payload =
           content::Details<std::pair<const std::string, const std::string> >(
               details).ptr();
       if (extension_->id() == payload->first &&
           payload->second ==
               extension_manifest_values::kBrowserActionKeybindingEvent) {
         if (type == chrome::NOTIFICATION_EXTENSION_COMMAND_ADDED)
           MaybeRegisterExtensionCommand();
         else
           MaybeUnregisterExtensionCommand(true);
       }
       break;
     }
     default:
       NOTREACHED();
       break;
   }
 }
 
 bool BrowserActionButton::Activate() {
   if (!IsPopup())
     return true;
 
-  panel_->OnBrowserActionExecuted(this);
+  delegate_->OnBrowserActionExecuted(this);
 
   // TODO(erikkay): Run a nested modal loop while the mouse is down to
   // enable menu-like drag-select behavior.
 
   // The return value of this method is returned via OnMousePressed.
   // We need to return false here since we're handing off focus to another
   // widget/view, and true will grab it right back and try to send events
   // to us.
   return false;
 }
@@ skipping 26 matching lines @@
     MenuButton::OnMouseExited(event);
   else
     TextButton::OnMouseExited(event);
 }
 
 bool BrowserActionButton::OnKeyReleased(const views::KeyEvent& event) {
   return IsPopup() ? MenuButton::OnKeyReleased(event)
                    : TextButton::OnKeyReleased(event);
 }
 
+void BrowserActionButton::ShowContextMenu(const gfx::Point& p,
+                                          bool is_mouse_gesture) {
+  ShowContextMenuImpl();
+}
+
 bool BrowserActionButton::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
-  panel_->OnBrowserActionExecuted(this);
+  delegate_->OnBrowserActionExecuted(this);
   return true;
 }
 
 void BrowserActionButton::SetButtonPushed() {
   SetState(views::CustomButton::BS_PUSHED);
   menu_visible_ = true;
 }
 
 void BrowserActionButton::SetButtonNotPushed() {
   SetState(views::CustomButton::BS_NORMAL);
   menu_visible_ = false;
 }
 
+void BrowserActionButton::SetTooltipDisabled(bool disable_tooltip) {
+  disable_tooltip_ = disable_tooltip;
+  SetTooltipText(disable_tooltip_ ? string16() : GetTextForTooltip());
+}
+
 bool BrowserActionButton::IsEnabled(int tab_id) const {
   return browser_action_->GetIsVisible(tab_id);
 }
 
 BrowserActionButton::~BrowserActionButton() {
 }
 
 void BrowserActionButton::MaybeRegisterExtensionCommand() {
   extensions::CommandService* command_service =
-      extensions::CommandServiceFactory::GetForProfile(
-          panel_->browser()->profile());
+      extensions::CommandServiceFactory::GetForProfile(browser_->profile());
   extensions::Command browser_action_command;
   if (command_service->GetBrowserActionCommand(
           extension_->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action_command,
           NULL)) {
     keybinding_.reset(new ui::Accelerator(
         browser_action_command.accelerator()));
-    panel_->GetFocusManager()->RegisterAccelerator(
+    GetFocusManager()->RegisterAccelerator(
         *keybinding_.get(), ui::AcceleratorManager::kHighPriority, this);
   }
 }
 
 void BrowserActionButton::MaybeUnregisterExtensionCommand(bool only_if_active) {
-  if (!keybinding_.get() || !panel_->GetFocusManager())
+  if (!keybinding_.get() || !GetFocusManager())
     return;
 
   extensions::CommandService* command_service =
-      extensions::CommandServiceFactory::GetForProfile(
-          panel_->browser()->profile());
+      extensions::CommandServiceFactory::GetForProfile(browser_->profile());
 
   extensions::Command browser_action_command;
   if (!only_if_active || !command_service->GetBrowserActionCommand(
           extension_->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action_command,
           NULL)) {
-    panel_->GetFocusManager()->UnregisterAccelerator(*keybinding_.get(), this);
+    GetFocusManager()->UnregisterAccelerator(*keybinding_.get(), this);
   }
 }
 
+string16 BrowserActionButton::GetTextForTooltip() {
+  int tab_id = delegate_->GetCurrentTabId();
+  if (tab_id < 0)
+    return string16();
 
-////////////////////////////////////////////////////////////////////////////////
-// BrowserActionView
-
-BrowserActionView::BrowserActionView(const Extension* extension,
-                                     BrowserActionsContainer* panel)
-    : panel_(panel) {
-  button_ = new BrowserActionButton(extension, panel);
-  button_->set_drag_controller(panel_);
-  AddChildView(button_);
-  button_->UpdateState();
+  // If the browser action name is empty, show the extension name instead.
+  std::string name = browser_action()->GetTitle(tab_id);
+  return UTF8ToUTF16(name.empty() ? extension()->name() : name);
 }
 
-BrowserActionView::~BrowserActionView() {
-  RemoveChildView(button_);
-  button_->Destroy();
+void BrowserActionButton::ShowContextMenuImpl() {
+  if (!extension()->ShowConfigureContextMenus())
+    return;
+
+  SetButtonPushed();
+
+  // Reconstructs the menu every time because the menu's contents are dynamic.
+  scoped_refptr<ExtensionContextMenuModel> context_menu_contents_(
+      new ExtensionContextMenuModel(extension(), browser_));
+  views::MenuModelAdapter menu_model_adapter(context_menu_contents_.get());
+  views::MenuRunner menu_runner(menu_model_adapter.CreateMenu());

[Peter Kasting, 2012/07/26 20:37:17]

MenuRunner objects should never be created on the stack; they should always be
scoped_ptr<> members of the owning class, so that if the browser is torn down,
the MenuRunner will get deleted correctly.

[yefimt, 2012/07/31 00:10:11]

Done.

+
+  context_menu_ = menu_runner.GetMenu();
+  gfx::Point screen_loc;
+  views::View::ConvertPointToScreen(this, &screen_loc);
+  if (menu_runner.RunMenuAt(GetWidget(), NULL, gfx::Rect(screen_loc, size()),
+          views::MenuItemView::TOPLEFT, views::MenuRunner::HAS_MNEMONICS) ==
+      views::MenuRunner::MENU_DELETED)
+    return;
+
+  SetButtonNotPushed();
+  context_menu_ = NULL;
 }
-
-gfx::Canvas* BrowserActionView::GetIconWithBadge() {
-  int tab_id = panel_->GetCurrentTabId();
-
-  SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id);
-  if (icon.isNull())
-    icon = button_->default_icon();
-
-  // Dim the icon if our button is disabled.
-  if (!button_->IsEnabled(tab_id))
-    icon = MakeTransparent(icon);
-
-  gfx::Canvas* canvas =
-      new gfx::Canvas(gfx::ImageSkiaRep(icon, ui::SCALE_FACTOR_100P), false);
-
-  if (tab_id >= 0) {
-    gfx::Rect bounds(icon.width(), icon.height() + ToolbarView::kVertSpacing);
-    button_->extension()->browser_action()->PaintBadge(canvas, bounds, tab_id);
-  }
-
-  return canvas;
-}
-
-void BrowserActionView::Layout() {
-  // We can't rely on button_->GetPreferredSize() here because that's not set
-  // correctly until the first call to
-  // BrowserActionsContainer::RefreshBrowserActionViews(), whereas this can be
-  // called before that when the initial bounds are set (and then not after,
-  // since the bounds don't change).  So instead of setting the height from the
-  // button's preferred size, we use IconHeight(), since that's how big the
-  // button should be regardless of what it's displaying.
-  button_->SetBounds(0, ToolbarView::kVertSpacing, width(),
-                     BrowserActionsContainer::IconHeight());
-}
-
-void BrowserActionView::GetAccessibleState(ui::AccessibleViewState* state) {
-  state->name = l10n_util::GetStringUTF16(
-      IDS_ACCNAME_EXTENSIONS_BROWSER_ACTION);
-  state->role = ui::AccessibilityTypes::ROLE_GROUPING;
-}
-
-void BrowserActionView::PaintChildren(gfx::Canvas* canvas) {
-  View::PaintChildren(canvas);
-  ExtensionAction* action = button()->browser_action();
-  int tab_id = panel_->GetCurrentTabId();
-  if (tab_id >= 0)
-    action->PaintBadge(canvas, gfx::Rect(width(), height()), tab_id);
-}