ToolbarButton: Reset menu runner before deleting its menu's delegate

chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc

+// Copyright 2016 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/toolbar/toolbar_button.h"
+
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile.h"
+#include "ui/base/models/simple_menu_model.h"
+#include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/test/widget_test.h"
+
+namespace test {
+
+// Friend of ToolbarButton to access private members.
+class ToolbarButtonTestApi {
+ public:
+  explicit ToolbarButtonTestApi(ToolbarButton* button) : button_(button) {}
+
+  views::MenuRunner* menu_runner() { return button_->menu_runner_.get(); }
+  bool menu_showing() const { return button_->menu_showing_; }
+
+ private:
+  ToolbarButton* button_;
+
+  DISALLOW_COPY_AND_ASSIGN(ToolbarButtonTestApi);
+};
+
+}  // namespace test
+
+class ToolbarButtonTest : public views::test::WidgetTest {
+ public:
+  ToolbarButtonTest() {}
+
+  // testing::Test:
+  void SetUp() override {
+    WidgetTest::SetUp();
+
+    profile_.reset(new TestingProfile);
+
+    // Usually a BackForwardMenuModel is used, but that needs a Browser*. Make
+    // something simple with at least one item so a menu gets shown. Note that
+    // ToolbarButton takes ownership of the |model|.
+    ui::SimpleMenuModel* model = new ui::SimpleMenuModel(nullptr);
+    model->AddItem(0, base::string16());
+    button_ = new ToolbarButton(profile_.get(), nullptr, model);
+
+    // Need a Widget to show a menu in the appropriate window context.
+    widget_ = CreateNativeDesktopWidget();
+    widget_->GetContentsView()->AddChildView(button_);
+  }
+
+  void TearDown() override {
+    widget_->CloseNow();
+    profile_.reset();
+    WidgetTest::TearDown();
+
+    // This usually happens in ChromeUnitTestSuiteInitializer::OnTestEnd(), but
+    // it has tasks that would want to post to the MessageLoop owned by
+    // ViewsTestBase, which will be destroyed by then.
+    TestingBrowserProcess::DeleteInstance();

[tapted, 2016/04/07 11:32:58]

This was a frustrating trap to discover :/. I think I can save others from the
same trap, and make this test file a bit simpler. See
http://crrev.com/1865223002 -- if that CL makes sense I'll update the CL here to
be a bit simpler.

+  }
+
+ protected:
+  std::unique_ptr<TestingProfile> profile_;
+  ToolbarButton* button_ = nullptr;
+  views::Widget* widget_ = nullptr;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ToolbarButtonTest);
+};
+
+// Test showing and dismissing a menu to verify menu delegate lifetime.
+TEST_F(ToolbarButtonTest, ShowMenu) {
+  test::ToolbarButtonTestApi test_api(button_);
+
+  EXPECT_FALSE(test_api.menu_showing());
+  EXPECT_FALSE(test_api.menu_runner());
+  EXPECT_EQ(views::Button::STATE_NORMAL, button_->state());
+
+  // Show the menu. Note that it is asynchronous.
+  button_->ShowContextMenuForView(nullptr, gfx::Point(), ui::MENU_SOURCE_MOUSE);

[sky, 2016/04/07 19:27:01]

AFAICT this will attempt setcapture and all that fun stuff. I'm a bit worried
that may cause some level of flake. Can you use MenuRunnerHandler here?

[tapted, 2016/04/08 05:32:22]

Yep - I agree a flake is quite likely :/. MenuRunnerHandler sadly wasn't very
useful - it can't handle asynchronous menus. I pulled that thread anyway and
ended up deleting MenuRunnerHandler completely and replacing it with a mock
internal::MenuRunnerImplInterface. See what you think.

It feels a little bit contrived - i.e. there's not as much coverage this way.
And I'm not sure there's a nicer way to replace the test-only virtual I added to
ToolbarButton to get the test MenuRunner hooked in (MenuRunnerHandler would have
needed this too).

One option could be to "globally" bump MenuRunner into a test-only mode that
automatically wraps the MenuRunnerImpls it creates behind a TestMenuRunnerImpl.
I would probably need to add to MenuRunnerImplInterface a bit, e.g., to expose
the MenuDelegate to the TestMenuRunnerImpl. Also the approach probably involves
some scary globals and function pointers.

... another option could be just to move this test file into
interactive_ui_tests

WDYT?

+
+  EXPECT_TRUE(test_api.menu_showing());
+  EXPECT_TRUE(test_api.menu_runner());
+  EXPECT_TRUE(test_api.menu_runner()->IsRunning());
+
+  // Button should appear pressed when the menu is showing.
+  EXPECT_EQ(views::Button::STATE_PRESSED, button_->state());
+
+  test_api.menu_runner()->Cancel();
+
+  // Ensure the ToolbarButton's |menu_runner_| member is reset to null.
+  EXPECT_FALSE(test_api.menu_showing());
+  EXPECT_FALSE(test_api.menu_runner());
+  EXPECT_EQ(views::Button::STATE_NORMAL, button_->state());
+}
+
+// Test deleting a ToolbarButton while its menu is showing.
+TEST_F(ToolbarButtonTest, DeleteWithMenu) {
+  button_->ShowContextMenuForView(nullptr, gfx::Point(), ui::MENU_SOURCE_MOUSE);
+  EXPECT_TRUE(test::ToolbarButtonTestApi(button_).menu_runner());
+  delete button_;
+}