Fix closing of touch selection menu

Current implementation of touch selection menu for views has a bug which
can cause a crash if a it is opened after being closed once. This patch
fixes the bug.

BUG=none

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

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

3 files changed, 132 insertions, 24 deletions

diff --git a/ui/views/touchui/touch_selection_menu_runner_views.cc b/ui/views/touchui/touch_selection_menu_runner_views.cc
index 29fd56e..99d9856 100644
--- a/ui/views/touchui/touch_selection_menu_runner_views.cc
+++ b/ui/views/touchui/touch_selection_menu_runner_views.cc
@@ -51,6 +51,8 @@ class TouchSelectionMenuRunnerViews::Menu : public BubbleDelegateView,
   void Close();
 
  private:
+  friend class TouchSelectionMenuRunnerViews::TestApi;
+
   ~Menu() override;
 
   // Queries the |client_| for what commands to show in the menu and sizes the
@@ -60,6 +62,9 @@ class TouchSelectionMenuRunnerViews::Menu : public BubbleDelegateView,
   // Helper method to create a single button.
   Button* CreateButton(const base::string16& title, int tag);
 
+  // Helper to disconnect this menu object from its owning menu runner.
+  void DisconnectOwner();
+
   // BubbleDelegateView:
   void OnPaint(gfx::Canvas* canvas) override;
   void WindowClosing() override;
@@ -163,10 +168,16 @@ Button* TouchSelectionMenuRunnerViews::Menu::CreateButton(
 }
 
 void TouchSelectionMenuRunnerViews::Menu::Close() {
+  DisconnectOwner();
   // Closing the widget will self-destroy this object.
   Widget* widget = GetWidget();
   if (widget && !widget->IsClosed())
     widget->Close();
+}
+
+void TouchSelectionMenuRunnerViews::Menu::DisconnectOwner() {
+  DCHECK(owner_);
+  owner_->menu_ = nullptr;
   owner_ = nullptr;
 }
 
@@ -186,7 +197,7 @@ void TouchSelectionMenuRunnerViews::Menu::WindowClosing() {
   DCHECK(!owner_ || owner_->menu_ == this);
   BubbleDelegateView::WindowClosing();
   if (owner_)
-    owner_->menu_ = nullptr;
+    DisconnectOwner();
 }
 
 void TouchSelectionMenuRunnerViews::Menu::ButtonPressed(
@@ -199,6 +210,28 @@ void TouchSelectionMenuRunnerViews::Menu::ButtonPressed(
     client_->RunContextMenu();
 }
 
+TouchSelectionMenuRunnerViews::TestApi::TestApi(
+    TouchSelectionMenuRunnerViews* menu_runner)
+    : menu_runner_(menu_runner) {
+  DCHECK(menu_runner_);
+}
+
+TouchSelectionMenuRunnerViews::TestApi::~TestApi() {}
+
+gfx::Rect TouchSelectionMenuRunnerViews::TestApi::GetAnchorRect() const {
+  TouchSelectionMenuRunnerViews::Menu* menu = menu_runner_->menu_;
+  return menu ? menu->GetAnchorRect() : gfx::Rect();
+}
+
+Button* TouchSelectionMenuRunnerViews::TestApi::GetFirstButton() const {
+  TouchSelectionMenuRunnerViews::Menu* menu = menu_runner_->menu_;
+  return menu ? static_cast<Button*>(menu->child_at(0)) : nullptr;
+}
+
+Widget* TouchSelectionMenuRunnerViews::TestApi::GetWidget() const {
+  TouchSelectionMenuRunnerViews::Menu* menu = menu_runner_->menu_;
+  return menu ? menu->GetWidget() : nullptr;
+}
 TouchSelectionMenuRunnerViews::TouchSelectionMenuRunnerViews()
     : menu_(nullptr) {
 }
@@ -207,10 +240,6 @@ TouchSelectionMenuRunnerViews::~TouchSelectionMenuRunnerViews() {
   CloseMenu();
 }
 
-gfx::Rect TouchSelectionMenuRunnerViews::GetAnchorRectForTest() const {
-  return menu_ ? menu_->GetAnchorRect() : gfx::Rect();
-}
-
 bool TouchSelectionMenuRunnerViews::IsMenuAvailable(
     const ui::TouchSelectionMenuClient* client) const {
   return TouchSelectionMenuRunnerViews::Menu::IsMenuAvailable(client);
@@ -231,9 +260,9 @@ void TouchSelectionMenuRunnerViews::CloseMenu() {
   if (!menu_)
     return;
 
-  // Closing the menu will eventually delete the object.
+  // Closing the menu sets |menu_| to nullptr and eventually deletes the object.
   menu_->Close();
-  menu_ = nullptr;
+  DCHECK(!menu_);
 }
 
 bool TouchSelectionMenuRunnerViews::IsRunning() const {
diff --git a/ui/views/touchui/touch_selection_menu_runner_views.h b/ui/views/touchui/touch_selection_menu_runner_views.h
index b3ba517..03a8967 100644
--- a/ui/views/touchui/touch_selection_menu_runner_views.h
+++ b/ui/views/touchui/touch_selection_menu_runner_views.h
@@ -5,26 +5,42 @@
 #ifndef UI_VIEWS_TOUCHUI_TOUCH_SELECTION_MENU_RUNNER_VIEWS_H_
 #define UI_VIEWS_TOUCHUI_TOUCH_SELECTION_MENU_RUNNER_VIEWS_H_
 
+#include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "ui/touch_selection/touch_selection_menu_runner.h"
 #include "ui/views/views_export.h"
 
 namespace views {
+class Button;
+class Widget;
 
 // Views implementation for TouchSelectionMenuRunner.
 class VIEWS_EXPORT TouchSelectionMenuRunnerViews
     : public ui::TouchSelectionMenuRunner {
  public:
+  // Test API to access internal state in tests.
+  class VIEWS_EXPORT TestApi {
+   public:
+    explicit TestApi(TouchSelectionMenuRunnerViews* menu_runner);
+    ~TestApi();
+
+    gfx::Rect GetAnchorRect() const;
+    Button* GetFirstButton() const;
+    Widget* GetWidget() const;
+
+   private:
+    TouchSelectionMenuRunnerViews* menu_runner_;
+
+    DISALLOW_COPY_AND_ASSIGN(TestApi);
+  };
+
   TouchSelectionMenuRunnerViews();
   ~TouchSelectionMenuRunnerViews() override;
 
  private:
-  friend class TouchSelectionMenuRunnerViewsTest;
+  friend class TouchSelectionMenuRunnerViewsTestApi;
   class Menu;
 
-  // Helper for tests.
-  gfx::Rect GetAnchorRectForTest() const;
-
   // ui::TouchSelectionMenuRunner:
   bool IsMenuAvailable(
       const ui::TouchSelectionMenuClient* client) const override;
diff --git a/ui/views/touchui/touch_selection_menu_runner_views_unittest.cc b/ui/views/touchui/touch_selection_menu_runner_views_unittest.cc
index eee4258..424f82c 100644
--- a/ui/views/touchui/touch_selection_menu_runner_views_unittest.cc
+++ b/ui/views/touchui/touch_selection_menu_runner_views_unittest.cc
@@ -2,8 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "ui/events/event_utils.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/touch_selection/touch_selection_menu_runner.h"
+#include "ui/views/controls/button/button.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/touchui/touch_selection_menu_runner_views.h"
 
@@ -22,7 +24,8 @@ const int kMenuCommandCount = 3;
 class TouchSelectionMenuRunnerViewsTest : public ViewsTestBase,
                                           public ui::TouchSelectionMenuClient {
  public:
-  TouchSelectionMenuRunnerViewsTest() : no_command_available_(false) {}
+  TouchSelectionMenuRunnerViewsTest()
+      : no_command_available_(false), last_executed_command_id_(0) {}
   ~TouchSelectionMenuRunnerViewsTest() override {}
 
  protected:
@@ -30,12 +33,7 @@ class TouchSelectionMenuRunnerViewsTest : public ViewsTestBase,
     no_command_available_ = no_command;
   }
 
-  gfx::Rect GetMenuAnchorRect() {
-    TouchSelectionMenuRunnerViews* menu_runner =
-        static_cast<TouchSelectionMenuRunnerViews*>(
-            ui::TouchSelectionMenuRunner::GetInstance());
-    return menu_runner->GetAnchorRectForTest();
-  }
+  int last_executed_command_id() const { return last_executed_command_id_; }
 
  private:
   // ui::TouchSelectionMenuClient:
@@ -43,17 +41,23 @@ class TouchSelectionMenuRunnerViewsTest : public ViewsTestBase,
     return !no_command_available_;
   }
 
-  void ExecuteCommand(int command_id, int event_flags) override {}
+  void ExecuteCommand(int command_id, int event_flags) override {
+    last_executed_command_id_ = command_id;
+  }
 
   void RunContextMenu() override {}
 
-  // When set to true, no command would be availble and menu should not be
+  // When set to true, no command would be available and menu should not be
   // shown.
   bool no_command_available_;
 
+  int last_executed_command_id_;
+
   DISALLOW_COPY_AND_ASSIGN(TouchSelectionMenuRunnerViewsTest);
 };
 
+// Tests that the default touch selection menu runner is installed and opening
+// and closing the menu works properly.
 TEST_F(TouchSelectionMenuRunnerViewsTest, InstalledAndWorksProperly) {
   gfx::Rect menu_anchor(0, 0, 10, 10);
   gfx::Size handle_size(10, 10);
@@ -62,7 +66,7 @@ TEST_F(TouchSelectionMenuRunnerViewsTest, InstalledAndWorksProperly) {
   EXPECT_TRUE(ui::TouchSelectionMenuRunner::GetInstance());
   EXPECT_FALSE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
 
-  // Run menu. Since commands are availble, this should bring up menus.
+  // Run menu. Since commands are available, this should bring up menus.
   ui::TouchSelectionMenuRunner::GetInstance()->OpenMenu(
       this, menu_anchor, handle_size, GetContext());
   EXPECT_TRUE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
@@ -79,10 +83,13 @@ TEST_F(TouchSelectionMenuRunnerViewsTest, InstalledAndWorksProperly) {
   EXPECT_FALSE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
 }
 
-// Tests if anchor rect for the quick menu is adjusted correctly based on the
+// Tests that anchor rect for the quick menu is adjusted correctly based on the
 // distance of handles.
 TEST_F(TouchSelectionMenuRunnerViewsTest, QuickMenuAdjustsAnchorRect) {
   gfx::Size handle_size(10, 10);
+  TouchSelectionMenuRunnerViews::TestApi test_api(
+      static_cast<TouchSelectionMenuRunnerViews*>(
+          ui::TouchSelectionMenuRunner::GetInstance()));
 
   // Calculate the width of quick menu. In addition to |kMenuCommandCount|
   // commands, there is an item for ellipsis.
@@ -95,7 +102,7 @@ TEST_F(TouchSelectionMenuRunnerViewsTest, QuickMenuAdjustsAnchorRect) {
   ui::TouchSelectionMenuRunner::GetInstance()->OpenMenu(
       this, anchor_rect, handle_size, GetContext());
   anchor_rect.Inset(0, 0, 0, -handle_size.height());
-  EXPECT_EQ(anchor_rect, GetMenuAnchorRect());
+  EXPECT_EQ(anchor_rect, test_api.GetAnchorRect());
 
   // Set anchor rect's width a bit greater than the quick menu width plus handle
   // image width and check that anchor rect's height is not adjusted.
@@ -103,10 +110,66 @@ TEST_F(TouchSelectionMenuRunnerViewsTest, QuickMenuAdjustsAnchorRect) {
       gfx::Rect(0, 0, quick_menu_width + handle_size.width() + 10, 20);
   ui::TouchSelectionMenuRunner::GetInstance()->OpenMenu(
       this, anchor_rect, handle_size, GetContext());
-  EXPECT_EQ(anchor_rect, GetMenuAnchorRect());
+  EXPECT_EQ(anchor_rect, test_api.GetAnchorRect());
 
   ui::TouchSelectionMenuRunner::GetInstance()->CloseMenu();
   RunPendingMessages();
 }
 
+// Tests that running one of menu actions closes the menu properly.
+TEST_F(TouchSelectionMenuRunnerViewsTest, RunningActionClosesProperly) {
+  gfx::Rect menu_anchor(0, 0, 10, 10);
+  gfx::Size handle_size(10, 10);
+  TouchSelectionMenuRunnerViews::TestApi test_api(
+      static_cast<TouchSelectionMenuRunnerViews*>(
+          ui::TouchSelectionMenuRunner::GetInstance()));
+
+  // Initially, no menu should be running.
+  EXPECT_FALSE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
+
+  // Run menu. Since commands are available, this should bring up menus.
+  ui::TouchSelectionMenuRunner::GetInstance()->OpenMenu(
+      this, menu_anchor, handle_size, GetContext());
+  EXPECT_TRUE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
+
+  // Tap the first action on the menu and check taht the menu is closed
+  // properly.
+  Button* button = test_api.GetFirstButton();
+  DCHECK(button);
+  gfx::Point button_center = button->bounds().CenterPoint();
+  ui::GestureEventDetails details(ui::ET_GESTURE_TAP);
+  details.set_tap_count(1);
+  ui::GestureEvent tap(button_center.x(), button_center.y(), 0,
+                       ui::EventTimeForNow(), details);
+  button->OnGestureEvent(&tap);
+  RunPendingMessages();
+  EXPECT_NE(0, last_executed_command_id());
+  EXPECT_FALSE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
+}
+
+// Tests that closing the menu widget cleans up the menu runner state properly.
+TEST_F(TouchSelectionMenuRunnerViewsTest, ClosingWidgetClosesProperly) {
+  gfx::Rect menu_anchor(0, 0, 10, 10);
+  gfx::Size handle_size(10, 10);
+  TouchSelectionMenuRunnerViews::TestApi test_api(
+      static_cast<TouchSelectionMenuRunnerViews*>(
+          ui::TouchSelectionMenuRunner::GetInstance()));
+
+  // Initially, no menu should be running.
+  EXPECT_FALSE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
+
+  // Run menu. Since commands are available, this should bring up menus.
+  ui::TouchSelectionMenuRunner::GetInstance()->OpenMenu(
+      this, menu_anchor, handle_size, GetContext());
+  EXPECT_TRUE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
+
+  // Close the menu widget and check that menu runner correctly knows that menu
+  // is not running anymore.
+  Widget* widget = test_api.GetWidget();
+  DCHECK(widget);
+  widget->Close();
+  RunPendingMessages();
+  EXPECT_FALSE(ui::TouchSelectionMenuRunner::GetInstance()->IsRunning());
+}
+
 }  // namespace views