Action box menu

chrome/browser/ui/views/browser_action_view.cc

Index: chrome/browser/ui/views/browser_action_view.cc
diff --git a/chrome/browser/ui/views/browser_action_view.cc b/chrome/browser/ui/views/browser_action_view.cc
index cc48c2fb60c426dc17b720ae59682d999a5b5867..4a2f8cdc7c8d684321b6d35db81a1c1b1d65cb24 100644
--- a/chrome/browser/ui/views/browser_action_view.cc
+++ b/chrome/browser/ui/views/browser_action_view.cc
@@ -10,6 +10,7 @@
 #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"
@@ -41,16 +42,95 @@ SkBitmap MakeTransparent(const SkBitmap& image) {
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
+// BrowserActionView
+
+BrowserActionView::BrowserActionView(const Extension* extension,
+                                     Browser* browser,
+                                     BrowserActionView::Delegate* delegate)
+    : browser_(browser),
+      delegate_(delegate),
+      button_(NULL),
+      extension_(extension) {
+}
+
+BrowserActionView::~BrowserActionView() {
+}
+
+gfx::Canvas* BrowserActionView::GetIconWithBadge() {
+  int tab_id = delegate_->GetCurrentTabId();
+
+  SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id);

[Peter Kasting, 2012/08/03 23:17:42]

Nit: We don't protect this call with a >= 0 check, but we do protect PaintBadge
(both places it's called).  We should be consistent.  My reading of the
definitions is that protection is unnecessary.

[yefimt, 2012/08/07 00:47:06]

This is another case when asking people who did most of changes on this file
would be better :)
I see that chrome::GetActiveTabContents() could return NULL and in this case
tab_id would be -1.
The reason they dont check for GEtIcon is because it will return default icon if
it doesnt exist.

[Peter Kasting, 2012/08/07 01:06:49]

Right -- I'm saying that PaintBadge() should also do the right thing if the
tab_id is negative, since it will call GetBadgeText() first and that will return
the empty string.  So we can just remove the conditionals that guard the two
PaintBadge() calls and call them all the time.

+  if (icon.isNull())

[Peter Kasting, 2012/08/03 23:17:42]

Nit: Can this happen?  It doesn't seem like it from reading the callee.

[yefimt, 2012/08/07 00:47:06]

Code in the trunk was changed by now, and these two lines are gone.

+    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::Point offset = delegate_->GetViewContentOffset();
+  button_->SetBounds(offset.x(), offset.y(), width() - offset.x(),
+                     BrowserActionsContainer::IconHeight());
+}
+
+void BrowserActionView::ViewHierarchyChanged(bool is_add,
+                                             View* parent,
+                                             View* child) {
+  if (is_add && (child == this)) {
+    button_ = new BrowserActionButton(extension_, browser_, delegate_);
+    button_->set_drag_controller(delegate_);
+
+    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),
+      delegate_(delegate),
       context_menu_(NULL) {
   set_border(NULL);
   set_alignment(TextButton::ALIGN_CENTER);
@@ -59,14 +139,14 @@ BrowserActionButton::BrowserActionButton(const Extension* extension,
   // 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() {
@@ -117,32 +197,12 @@ bool BrowserActionButton::CanHandleAccelerators() const {
 
 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();
 }
 
 void BrowserActionButton::OnImageLoaded(const gfx::Image& image,
@@ -157,10 +217,12 @@ void BrowserActionButton::OnImageLoaded(const gfx::Image& image,
 }
 
 void BrowserActionButton::UpdateState() {
-  int tab_id = panel_->GetCurrentTabId();
+  int tab_id = delegate_->GetCurrentTabId();
   if (tab_id < 0)
     return;
 
+  SetShowMultipleIconStates(delegate_->NeedToShowMultipleIconStates());
+
   if (!IsEnabled(tab_id)) {
     SetState(views::CustomButton::BS_DISABLED);
   } else {
@@ -206,23 +268,20 @@ void BrowserActionButton::UpdateState() {
     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 = GetName();
+  SetTooltipText(delegate_->NeedToShowTooltip() ? name : string16());
   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);
 }
 
@@ -234,7 +293,7 @@ void BrowserActionButton::Observe(int type,
       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: {
@@ -261,7 +320,7 @@ 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.
@@ -308,9 +367,14 @@ bool BrowserActionButton::OnKeyReleased(const views::KeyEvent& 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;
 }
 
@@ -333,8 +397,7 @@ 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(),
@@ -343,18 +406,17 @@ void BrowserActionButton::MaybeRegisterExtensionCommand() {
           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(
@@ -362,72 +424,43 @@ void BrowserActionButton::MaybeUnregisterExtensionCommand(bool only_if_active) {
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action_command,
           NULL)) {
-    panel_->GetFocusManager()->UnregisterAccelerator(*keybinding_.get(), this);
+    GetFocusManager()->UnregisterAccelerator(*keybinding_.get(), this);
   }
 }
 
+string16 BrowserActionButton::GetName() {
+  int tab_id = delegate_->GetCurrentTabId();

[Peter Kasting, 2012/08/03 23:17:42]

Nit: This seems like another case where just calling GetTitle() unilaterally
ought to be fine, even if the tab ID is negative.  (Are we even going to reach
here with a negative tab ID?)

At that point I would just inline the remaining bit of code here into the lone
caller.

[yefimt, 2012/08/07 00:47:06]

Done.

+  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();
-}
-
-BrowserActionView::~BrowserActionView() {
-  RemoveChildView(button_);
-  button_->Destroy();
+  // 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);
 }
 
-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();
+void BrowserActionButton::ShowContextMenuImpl() {
+  if (!extension()->ShowConfigureContextMenus())
+    return;
 
-  // Dim the icon if our button is disabled.
-  if (!button_->IsEnabled(tab_id))
-    icon = MakeTransparent(icon);
+  SetButtonPushed();
 
-  gfx::Canvas* canvas =
-      new gfx::Canvas(gfx::ImageSkiaRep(icon, ui::SCALE_FACTOR_100P), false);
+  // 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());
+  menu_runner_.reset(new views::MenuRunner(menu_model_adapter.CreateMenu()));
 
-  if (tab_id >= 0) {
-    gfx::Rect bounds(icon.width(), icon.height() + ToolbarView::kVertSpacing);
-    button_->extension()->browser_action()->PaintBadge(canvas, bounds, tab_id);
+  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) {
+    menu_runner_.reset();

[Peter Kasting, 2012/08/03 23:17:42]

Don't do this, it is explicitly wrong because |this| is already deleted at this
point.

[yefimt, 2012/08/07 00:47:06]

Done.

+    return;
   }
 
-  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);
+  menu_runner_.reset();
+  SetButtonNotPushed();
+  context_menu_ = NULL;
 }