Password bubble: Introduce a command to open the bubble.

chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc

Index: chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
index 116e7c0f59deab45baa7c434347c4033263a252f..47d1360e16d7fead8cda98565f15d7465e5abcb7 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/views/passwords/manage_passwords_icon_view.h"
 
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/command_updater.h"
 #include "chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h"
@@ -13,12 +15,17 @@
 #include "ui/base/resource/resource_bundle.h"
 
 ManagePasswordsIconView::ManagePasswordsIconView(
-    LocationBarView::Delegate* location_bar_delegate)
-    : location_bar_delegate_(location_bar_delegate) {
+    LocationBarView::Delegate* location_bar_delegate,
+    CommandUpdater* command_updater)
+    : BubbleIconView(command_updater, IDC_MANAGE_PASSWORDS_FOR_PAGE),
+      location_bar_delegate_(location_bar_delegate),
+      command_updater_(command_updater) {
   set_id(VIEW_ID_MANAGE_PASSWORDS_ICON_BUTTON);
   SetAccessibilityFocusable(true);
   LocationBarView::InitTouchableLocationBarChildView(this);
-  SetState(ManagePasswordsIcon::INACTIVE_STATE);
+  // We call the internal method here as the default state is "inactive"; this
+  // call would otherwise be deduped away.

[Peter Kasting, 2014/04/25 22:12:02]

This is confusing.  Your change must be built atop some other change that isn't
checked in, because I assume this refers to something in the ManagePasswordsIcon
class, which I can't find in ViewVC.

The issue is presumably that ManagePasswordsIcon() calls SetState() or
something, but because the subclass is not yet constructed, it doesn't wind up
calling SetStateInternal() here.  If so, consider moving the SetState() call to
an Init() method, or expecting subclasses to call it, or something.

[Mike West, 2014/04/28 10:52:56]

This CL is based on https://codereview.chromium.org/250353003/, which I did a
terrible job of making clear when asking for reviews. Sorry about that.
The issue is that the default value of the internal 'state_' variable is
INACTIVE_STATE, and the side-effects of setting the state are wrapped up in the
SetStateInternal call. I'll rename this internal method to something more
appropriate, perhaps |UpdateVisibleUI()|.

+  SetStateInternal(ManagePasswordsIcon::INACTIVE_STATE);
 }
 
 ManagePasswordsIconView::~ManagePasswordsIconView() {}
@@ -27,6 +34,8 @@ void ManagePasswordsIconView::SetStateInternal(
     ManagePasswordsIcon::State state) {
   if (state == ManagePasswordsIcon::INACTIVE_STATE) {
     SetVisible(false);
+    command_updater_->UpdateCommandEnabled(
+        IDC_MANAGE_PASSWORDS_FOR_PAGE, false);

[Peter Kasting, 2014/04/25 22:12:02]

Is it actually necessary to disable the command ever?  Unless the user can
trigger the command with a keyboard shortcut that needs to check the activation
state, or we switch UI based on the activation state, this command should just
be set to initially be enabled (we have a block that does this for many similar
browser commands) and never touched after this.

[Mike West, 2014/04/28 10:52:56]

Makes sense. There's no real need to disable the command in the current model;
the icon is the only way of triggering the UI, and the command is only called
via the icon. I'll drop this; we can always add it back in the future.

     if (ManagePasswordsBubbleView::IsShowing())
       ManagePasswordsBubbleView::CloseBubble();
     return;
@@ -46,6 +55,7 @@ void ManagePasswordsIconView::SetStateInternal(
   SetImage(
       ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(which_icon));
   SetTooltipText(l10n_util::GetStringUTF16(which_text));
+  command_updater_->UpdateCommandEnabled(IDC_MANAGE_PASSWORDS_FOR_PAGE, true);
 }
 
 void ManagePasswordsIconView::ShowBubbleWithoutUserInteraction() {
@@ -53,36 +63,13 @@ void ManagePasswordsIconView::ShowBubbleWithoutUserInteraction() {
   if (location_bar_delegate_->GetToolbarModel()->input_in_progress())
     return;
 
-  ManagePasswordsBubbleView::ShowBubble(
-      location_bar_delegate_->GetWebContents(),
-      ManagePasswordsBubbleView::AUTOMATIC);
+  command_updater_->ExecuteCommand(IDC_MANAGE_PASSWORDS_FOR_PAGE);
 }
 
-bool ManagePasswordsIconView::GetTooltipText(const gfx::Point& p,
-                                             base::string16* tooltip) const {
-  // Don't show tooltip if the password bubble is displayed.
-  return !ManagePasswordsBubbleView::IsShowing() &&
-      ImageView::GetTooltipText(p, tooltip);
+bool ManagePasswordsIconView::IsBubbleShowing() const {
+  return ManagePasswordsBubbleView::IsShowing();
 }
 
-void ManagePasswordsIconView::OnGestureEvent(ui::GestureEvent* event) {
-  if (event->type() == ui::ET_GESTURE_TAP) {
-    ManagePasswordsBubbleView::ShowBubble(
-        location_bar_delegate_->GetWebContents(),
-        ManagePasswordsBubbleView::USER_ACTION);
-    event->SetHandled();
-  }
-}
-
-bool ManagePasswordsIconView::OnMousePressed(const ui::MouseEvent& event) {
-  // Do nothing until the mouse button is released.
-  return true;
-}
-
-void ManagePasswordsIconView::OnMouseReleased(const ui::MouseEvent& event) {
-  if (event.IsOnlyLeftMouseButton() && HitTestPoint(event.location())) {
-    ManagePasswordsBubbleView::ShowBubble(
-        location_bar_delegate_->GetWebContents(),
-        ManagePasswordsBubbleView::USER_ACTION);
-  }
+void ManagePasswordsIconView::OnExecuting(
+    BubbleIconView::ExecuteSource source) {
 }