Password bubble: Introduce a command to open the bubble.

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

 // Copyright 2013 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/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"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #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() {}
 
 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;
   }
 
   // Start with the correct values for ManagePasswordsIcon::MANAGE_STATE, and
   // adjust things accordingly if we're either in BLACKLISTED_STATE or
   // PENDING_STATE.
   int which_icon = IDR_SAVE_PASSWORD;
   int which_text = IDS_PASSWORD_MANAGER_TOOLTIP_MANAGE;
   if (state == ManagePasswordsIcon::BLACKLISTED_STATE)
     which_icon = IDR_SAVE_PASSWORD_BLACKLISTED;
   else if (state == ManagePasswordsIcon::PENDING_STATE)
     which_text = IDS_PASSWORD_MANAGER_TOOLTIP_SAVE;
 
   SetVisible(true);
   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() {
   // Suppress the bubble if the user is working in the omnibox.
   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();
-  }
+void ManagePasswordsIconView::OnExecuting(
+    BubbleIconView::ExecuteSource source) {
 }
-
-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);
-  }
-}