MacViews: Fix flaky MenuRunnerCocoaTests.

ui/views/controls/menu/menu_runner_cocoa_unittest.mm

 // Copyright 2014 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.
 
 #import "ui/views/controls/menu/menu_runner_impl_cocoa.h"
 
 #import <Cocoa/Cocoa.h>
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #import "testing/gtest_mac.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/events/event_utils.h"
 #include "ui/views/test/views_test_base.h"
 
 namespace views {
 namespace test {
 namespace {
 
+void MenuCancelCallback(internal::MenuRunnerImplCocoa* runner) {

[tapted, 2016/04/06 06:38:37]

I think these all would be better as member functions on MenuRunnerCocoaTest.
you just need to base base::Unretained(this) into base::Bind

[karandeepb, 2016/04/06 10:25:16]

Done.

+  runner->Cancel();
+  EXPECT_FALSE(runner->IsRunning());
+}
+
+void MenuDeleteCallback(internal::MenuRunnerImplCocoa** runner) {
+  (*runner)->Release();
+  *runner = nullptr;
+}
+
+void RunMenuWrapperCallback(const base::Closure& callback,
+                            internal::MenuRunnerImplCocoa* runner) {
+  EXPECT_TRUE(runner->IsRunning());
+  callback.Run();
+}
+
+void RunMenuAtCallback(views::Widget* parent,
+                       internal::MenuRunnerImplCocoa* runner,
+                       NSRect* last_anchor_frame) {
+  NSArray* subviews = [parent->GetNativeView() subviews];
+  EXPECT_EQ(2u, [subviews count]);
+  *last_anchor_frame = [[subviews objectAtIndex:1] frame];
+  runner->Cancel();
+}
+
 class TestModel : public ui::SimpleMenuModel {
  public:
   TestModel() : ui::SimpleMenuModel(&delegate_), delegate_(this) {}
 
   void set_checked_command(int command) { checked_command_ = command; }
 
+  void set_menu_open_callback(const base::Closure& callback) {
+    menu_open_callback_ = callback;
+  }
+
  private:
   class Delegate : public ui::SimpleMenuModel::Delegate {
    public:
     explicit Delegate(TestModel* model) : model_(model) {}
     bool IsCommandIdChecked(int command_id) const override {
       return command_id == model_->checked_command_;
     }
     bool IsCommandIdEnabled(int command_id) const override { return true; }
     bool GetAcceleratorForCommandId(int command_id,
                                     ui::Accelerator* accelerator) override {
       return false;
     }
     void ExecuteCommand(int command_id, int event_flags) override {}
 
+    void MenuWillShow(SimpleMenuModel* source) override {
+      if (model_->menu_open_callback_.is_null())

[tapted, 2016/04/06 06:38:37]

nit:
    if (!model_->menu_open_callback_.is_null())
      model_->menu_open_callback_.Run();

but also can we skip `if`? (is there a code path where we expect it to be null?)

[karandeepb, 2016/04/06 10:25:15]

Done.

+        return;
+      model_->menu_open_callback_.Run();
+    }
+
    private:
     TestModel* model_;
 
     DISALLOW_COPY_AND_ASSIGN(Delegate);
   };
 
  private:
   int checked_command_ = -1;
   Delegate delegate_;
+  base::Closure menu_open_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(TestModel);
 };
 
 }  // namespace
 
 class MenuRunnerCocoaTest : public ViewsTestBase {
  public:
   enum {
     kWindowHeight = 200,
@@ skipping 23 matching lines @@
   void TearDown() override {
     if (runner_) {
       runner_->Release();
       runner_ = NULL;
     }
 
     parent_->CloseNow();
     ViewsTestBase::TearDown();
   }
 
-  // Runs the menu after scheduling |block| on the run loop.
-  MenuRunner::RunResult RunMenu(dispatch_block_t block) {
-    CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopCommonModes, ^{
-        EXPECT_TRUE(runner_->IsRunning());
-        block();
-    });
-    return runner_->RunMenuAt(parent_, NULL, gfx::Rect(), MENU_ANCHOR_TOPLEFT,
-                              MenuRunner::CONTEXT_MENU);
+  // Runs the menu after registering |callback| as the menu open callback.
+  MenuRunner::RunResult RunMenu(const base::Closure& callback) {
+    menu_->set_menu_open_callback(
+        base::Bind(&RunMenuWrapperCallback, callback, runner_));
+    return runner_->RunMenuAt(parent_, nullptr, gfx::Rect(),
+                              MENU_ANCHOR_TOPLEFT, MenuRunner::CONTEXT_MENU);
   }
 
   // Runs then cancels a combobox menu and captures the frame of the anchoring
   // view.
   MenuRunner::RunResult RunMenuAt(const gfx::Rect& anchor) {
     last_anchor_frame_ = NSZeroRect;
 
     // Should be one child (the compositor layer) before showing, and it should
     // go up by one (the anchor view) while the menu is shown.
     EXPECT_EQ(1u, [[parent_->GetNativeView() subviews] count]);
-    CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopCommonModes, ^{
-      NSArray* subviews = [parent_->GetNativeView() subviews];
-      EXPECT_EQ(2u, [subviews count]);
-      last_anchor_frame_ = [[subviews objectAtIndex:1] frame];
-      runner_->Cancel();
-    });
+
+    menu_->set_menu_open_callback(
+        base::Bind(&RunMenuAtCallback, parent_, runner_, &last_anchor_frame_));
+
     MenuRunner::RunResult result = runner_->RunMenuAt(
         parent_, nullptr, anchor, MENU_ANCHOR_TOPLEFT, MenuRunner::COMBOBOX);
 
     // Ensure the anchor view is removed.
     EXPECT_EQ(1u, [[parent_->GetNativeView() subviews] count]);
     return result;
   }
 
  protected:
   scoped_ptr<TestModel> menu_;
   internal::MenuRunnerImplCocoa* runner_ = nullptr;
   views::Widget* parent_ = nullptr;
   NSRect last_anchor_frame_ = NSZeroRect;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MenuRunnerCocoaTest);
 };
 
 TEST_F(MenuRunnerCocoaTest, RunMenuAndCancel) {
   base::TimeDelta min_time = ui::EventTimeForNow();
 
-  MenuRunner::RunResult result = RunMenu(^{
-      runner_->Cancel();
-      EXPECT_FALSE(runner_->IsRunning());
-  });
+  MenuRunner::RunResult result =
+      RunMenu(base::Bind(&MenuCancelCallback, runner_));
 
   EXPECT_EQ(MenuRunner::NORMAL_EXIT, result);
   EXPECT_FALSE(runner_->IsRunning());
 
   EXPECT_GE(runner_->GetClosingEventTime(), min_time);
   EXPECT_LE(runner_->GetClosingEventTime(), ui::EventTimeForNow());
 
   // Cancel again.
   runner_->Cancel();
   EXPECT_FALSE(runner_->IsRunning());
 }
 
 TEST_F(MenuRunnerCocoaTest, RunMenuAndDelete) {
-  MenuRunner::RunResult result = RunMenu(^{
-      runner_->Release();
-      runner_ = NULL;
-  });
-
+  MenuRunner::RunResult result =
+      RunMenu(base::Bind(&MenuDeleteCallback, &runner_));
   EXPECT_EQ(MenuRunner::MENU_DELETED, result);
 }
 
 TEST_F(MenuRunnerCocoaTest, RunMenuTwice) {
   for (int i = 0; i < 2; ++i) {
-    MenuRunner::RunResult result = RunMenu(^{
-        runner_->Cancel();
-    });
+    MenuRunner::RunResult result =
+        RunMenu(base::Bind(&MenuCancelCallback, runner_));
     EXPECT_EQ(MenuRunner::NORMAL_EXIT, result);
     EXPECT_FALSE(runner_->IsRunning());
   }
 }
 
 TEST_F(MenuRunnerCocoaTest, CancelWithoutRunning) {
   runner_->Cancel();
   EXPECT_FALSE(runner_->IsRunning());
   EXPECT_EQ(base::TimeDelta(), runner_->GetClosingEventTime());
 }
@@ skipping 40 matching lines @@
   // In RTL, Cocoa messes up the positioning unless the anchor rectangle is
   // offset to the right of the view. The offset for the checkmark is also
   // skipped, to give a better match to native behavior.
   base::i18n::SetICUDefaultLocale("he");
   RunMenuAt(anchor_rect);
   EXPECT_EQ(combobox_rect.right(), last_anchor_frame_.origin.x);
 }
 
 }  // namespace test
 }  // namespace views