zoom bubble: Close if anchor is clicked while bubble is showing.

R=msw@chromium.org
BUG=366092

Review URL: https://codereview.chromium.org/420533002

Cr-Commit-Position: refs/heads/master@{#301043}

7 files changed, 49 insertions, 80 deletions

diff --git a/chrome/browser/ui/views/location_bar/bubble_icon_view.cc b/chrome/browser/ui/views/location_bar/bubble_icon_view.cc
index 68949b4b..d74f329 100644
--- a/chrome/browser/ui/views/location_bar/bubble_icon_view.cc
+++ b/chrome/browser/ui/views/location_bar/bubble_icon_view.cc
@@ -49,17 +49,14 @@ void BubbleIconView::OnMouseReleased(const ui::MouseEvent& event) {
     return;
   }
 
-  if (event.IsOnlyLeftMouseButton() && HitTestPoint(event.location())) {
-    OnExecuting(EXECUTE_SOURCE_MOUSE);
-    command_updater_->ExecuteCommand(command_id_);
-  }
+  if (event.IsOnlyLeftMouseButton() && HitTestPoint(event.location()))
+    ExecuteCommand(EXECUTE_SOURCE_MOUSE);
 }
 
 bool BubbleIconView::OnKeyPressed(const ui::KeyEvent& event) {
   if (event.key_code() == ui::VKEY_SPACE ||
       event.key_code() == ui::VKEY_RETURN) {
-    OnExecuting(EXECUTE_SOURCE_KEYBOARD);
-    command_updater_->ExecuteCommand(command_id_);
+    ExecuteCommand(EXECUTE_SOURCE_KEYBOARD);
     return true;
   }
   return false;
@@ -67,8 +64,13 @@ bool BubbleIconView::OnKeyPressed(const ui::KeyEvent& event) {
 
 void BubbleIconView::OnGestureEvent(ui::GestureEvent* event) {
   if (event->type() == ui::ET_GESTURE_TAP) {
-    OnExecuting(EXECUTE_SOURCE_GESTURE);
-    command_updater_->ExecuteCommand(command_id_);
+    ExecuteCommand(EXECUTE_SOURCE_GESTURE);
     event->SetHandled();
   }
 }
+
+void BubbleIconView::ExecuteCommand(ExecuteSource source) {
+  OnExecuting(source);
+  if (command_updater_)
+    command_updater_->ExecuteCommand(command_id_);
+}
diff --git a/chrome/browser/ui/views/location_bar/bubble_icon_view.h b/chrome/browser/ui/views/location_bar/bubble_icon_view.h
index 98d4a92..76428243 100644
--- a/chrome/browser/ui/views/location_bar/bubble_icon_view.h
+++ b/chrome/browser/ui/views/location_bar/bubble_icon_view.h
@@ -19,7 +19,7 @@ class BubbleIconView : public views::ImageView {
   };
 
   explicit BubbleIconView(CommandUpdater* command_updater, int command_id);
-  virtual ~BubbleIconView();
+  ~BubbleIconView() override;
 
   // Returns true if a related bubble is showing.
   virtual bool IsBubbleShowing() const = 0;
@@ -28,17 +28,20 @@ class BubbleIconView : public views::ImageView {
   virtual void OnExecuting(ExecuteSource execute_source) = 0;
 
   // views::ImageView:
-  virtual void GetAccessibleState(ui::AXViewState* state) override;
-  virtual bool GetTooltipText(const gfx::Point& p, base::string16* tooltip)
-      const override;
-  virtual bool OnMousePressed(const ui::MouseEvent& event) override;
-  virtual void OnMouseReleased(const ui::MouseEvent& event) override;
-  virtual bool OnKeyPressed(const ui::KeyEvent& event) override;
+  void GetAccessibleState(ui::AXViewState* state) override;
+  bool GetTooltipText(const gfx::Point& p, base::string16* tooltip) const
+      override;
+  bool OnMousePressed(const ui::MouseEvent& event) override;
+  void OnMouseReleased(const ui::MouseEvent& event) override;
+  bool OnKeyPressed(const ui::KeyEvent& event) override;
 
   // ui::EventHandler:
-  virtual void OnGestureEvent(ui::GestureEvent* event) override;
+  void OnGestureEvent(ui::GestureEvent* event) override;
 
  private:
+  // Calls OnExecuting and runs |command_id_| with a valid |command_updater_|.
+  void ExecuteCommand(ExecuteSource source);
+
   // The CommandUpdater for the Browser object that owns the location bar.
   CommandUpdater* command_updater_;
 
diff --git a/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc b/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
index 93cd822..081b8b2 100644
--- a/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
+++ b/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
@@ -112,8 +112,9 @@ void ZoomBubbleView::CloseBubble() {
 
 // static
 bool ZoomBubbleView::IsShowing() {
-  // The bubble may be in the process of closing.
-  return zoom_bubble_ != NULL && zoom_bubble_->GetWidget()->IsVisible();
+  // The bubble is considered showing while closing.
+  return zoom_bubble_ != NULL && (zoom_bubble_->GetWidget()->IsVisible() ||
+                                  zoom_bubble_->GetWidget()->IsClosed());
 }
 
 // static
diff --git a/chrome/browser/ui/views/location_bar/zoom_bubble_view.h b/chrome/browser/ui/views/location_bar/zoom_bubble_view.h
index 3440de5..fd4d932 100644
--- a/chrome/browser/ui/views/location_bar/zoom_bubble_view.h
+++ b/chrome/browser/ui/views/location_bar/zoom_bubble_view.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_ZOOM_BUBBLE_VIEW_H_
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/ui/views/frame/immersive_mode_controller.h"
 #include "content/public/browser/notification_observer.h"
@@ -50,6 +51,8 @@ class ZoomBubbleView : public views::BubbleDelegateView,
   static const ZoomBubbleView* GetZoomBubbleForTest();
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(ZoomBubbleBrowserTest, ImmersiveFullscreen);
+
   // Stores information about the extension that initiated the zoom change, if
   // any.
   struct ZoomBubbleExtensionInfo {
diff --git a/chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc b/chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
index c8135e1..c526924 100644
--- a/chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
+++ b/chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
@@ -96,7 +96,7 @@ IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, ImmersiveFullscreen) {
       immersive_controller->GetRevealedLock(
           ImmersiveModeController::ANIMATE_REVEAL_NO));
   ASSERT_TRUE(immersive_controller->IsRevealed());
-  EXPECT_FALSE(ZoomBubbleView::IsShowing());
+  EXPECT_TRUE(ZoomBubbleView::zoom_bubble_->GetWidget()->IsClosed());
 
   // The zoom bubble should be anchored when it is shown in immersive fullscreen
   // and the top-of-window views are revealed.
diff --git a/chrome/browser/ui/views/location_bar/zoom_view.cc b/chrome/browser/ui/views/location_bar/zoom_view.cc
index a71860e..c2dbf04 100644
--- a/chrome/browser/ui/views/location_bar/zoom_view.cc
+++ b/chrome/browser/ui/views/location_bar/zoom_view.cc
@@ -16,8 +16,8 @@
 #include "ui/gfx/size.h"
 
 ZoomView::ZoomView(LocationBarView::Delegate* location_bar_delegate)
-    : location_bar_delegate_(location_bar_delegate) {
-  SetAccessibilityFocusable(true);
+    : BubbleIconView(nullptr, 0),
+      location_bar_delegate_(location_bar_delegate) {
   Update(NULL);
 }
 
@@ -39,44 +39,15 @@ void ZoomView::Update(ZoomController* zoom_controller) {
   SetVisible(true);
 }
 
-void ZoomView::GetAccessibleState(ui::AXViewState* state) {
-  state->name = l10n_util::GetStringUTF16(IDS_ACCNAME_ZOOM);
-  state->role = ui::AX_ROLE_BUTTON;
+bool ZoomView::IsBubbleShowing() const {
+  return ZoomBubbleView::IsShowing();
 }
 
-bool ZoomView::GetTooltipText(const gfx::Point& p,
-                              base::string16* tooltip) const {
-  // Don't show tooltip if the zoom bubble is displayed.
-  return !ZoomBubbleView::IsShowing() && ImageView::GetTooltipText(p, tooltip);
-}
-
-bool ZoomView::OnMousePressed(const ui::MouseEvent& event) {
-  // Do nothing until mouse is released.
-  return true;
-}
-
-void ZoomView::OnMouseReleased(const ui::MouseEvent& event) {
-  if (event.IsOnlyLeftMouseButton() && HitTestPoint(event.location()))
-    ActivateBubble();
-}
-
-bool ZoomView::OnKeyPressed(const ui::KeyEvent& event) {
-  if (event.key_code() != ui::VKEY_SPACE &&
-      event.key_code() != ui::VKEY_RETURN) {
-    return false;
-  }
-
-  ActivateBubble();
-  return true;
-}
-
-void ZoomView::OnGestureEvent(ui::GestureEvent* event) {
-  if (event->type() == ui::ET_GESTURE_TAP) {
-    ActivateBubble();
-    event->SetHandled();
-  }
+void ZoomView::OnExecuting(BubbleIconView::ExecuteSource source) {
+  ZoomBubbleView::ShowBubble(location_bar_delegate_->GetWebContents(), false);
 }
 
-void ZoomView::ActivateBubble() {
-  ZoomBubbleView::ShowBubble(location_bar_delegate_->GetWebContents(), false);
+void ZoomView::GetAccessibleState(ui::AXViewState* state) {
+  BubbleIconView::GetAccessibleState(state);
+  state->name = l10n_util::GetStringUTF16(IDS_ACCNAME_ZOOM);
 }
diff --git a/chrome/browser/ui/views/location_bar/zoom_view.h b/chrome/browser/ui/views/location_bar/zoom_view.h
index bd73236..7242ac1 100644
--- a/chrome/browser/ui/views/location_bar/zoom_view.h
+++ b/chrome/browser/ui/views/location_bar/zoom_view.h
@@ -5,44 +5,33 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_ZOOM_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_ZOOM_VIEW_H_
 
-#include "base/basictypes.h"
-#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "chrome/browser/ui/views/location_bar/bubble_icon_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
-#include "ui/views/controls/image_view.h"
 
 class ZoomController;
 
 // View for the zoom icon in the Omnibox.
-class ZoomView : public views::ImageView {
+class ZoomView : public BubbleIconView {
  public:
   // Clicking on the ZoomView shows a ZoomBubbleView, which requires the current
   // WebContents. Because the current WebContents changes as the user switches
-  // tabs, it cannot be provided in the constructor. Instead, a
-  // LocationBarView::Delegate is passed here so that it can be queried for the
-  // current WebContents as needed.
+  // tabs, a LocationBarView::Delegate is supplied to queried for the current
+  // WebContents when needed.
   explicit ZoomView(LocationBarView::Delegate* location_bar_delegate);
-  virtual ~ZoomView();
+  ~ZoomView() override;
 
   // Updates the image and its tooltip appropriately, hiding or showing the icon
   // as needed.
   void Update(ZoomController* zoom_controller);
 
- private:
-  // views::ImageView:
-  virtual void GetAccessibleState(ui::AXViewState* state) override;
-  virtual bool GetTooltipText(const gfx::Point& p,
-                              base::string16* tooltip) const override;
-  virtual bool OnMousePressed(const ui::MouseEvent& event) override;
-  virtual void OnMouseReleased(const ui::MouseEvent& event) override;
-  virtual bool OnKeyPressed(const ui::KeyEvent& event) override;
-
-  // ui::EventHandler:
-  virtual void OnGestureEvent(ui::GestureEvent* event) override;
-
-  // Helper method to show and focus the zoom bubble associated with this
-  // widget.
-  void ActivateBubble();
+ protected:
+  // BubbleIconView:
+  bool IsBubbleShowing() const override;
+  void OnExecuting(BubbleIconView::ExecuteSource source) override;
+  void GetAccessibleState(ui::AXViewState* state) override;
 
+ private:
   // The delegate used to get the currently visible WebContents.
   LocationBarView::Delegate* location_bar_delegate_;