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"
+#import "ui/base/cocoa/menu_controller.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/events/event_utils.h"
 #include "ui/views/test/views_test_base.h"
 
+// A helper class to handle menu open notifications.
+@interface MenuOpenWatcher : NSObject {
+  dispatch_block_t openCallback_;

[tapted, 2016/03/24 02:10:36]

nit: @private

+}
+
+// Method to handle menu open notification.
+- (void)menuWillOpen:(NSNotification*)notification;
+
+// Block to be invoked on menu open notification. Weak. Clients must ensure that
+// it remains in a valid state.
+@property(nonatomic, assign) dispatch_block_t openCallback;
+
+@end
+
+@implementation MenuOpenWatcher
+
+@synthesize openCallback = openCallback_;
+
+- (id)init {
+  if (self = [super init]) {

[tapted, 2016/03/24 02:10:36]

nit: extra parens around assignment used as condition

+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(menuWillOpen:)
+               name:kMenuControllerMenuWillOpenNotification
+             object:nil];
+  }
+  return self;
+}
+
+- (void)dealloc {
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [super dealloc];
+}
+
+- (void)menuWillOpen:(NSNotification*)notification {
+  if (openCallback_)
+    openCallback_();
+}
+
+@end
+
 namespace views {
 namespace test {
 namespace {
 
 class TestModel : public ui::SimpleMenuModel {

[tapted, 2016/03/24 02:10:36]

Can we override MenuModel::MenuWillShow? Here, or on the delegate (looks like
either will work). I think that will make the code more readable for future
spelunkers who might find ObjC block notation scary and intimidating. Also,
Notifications are generally frowned upon as a logic control mechanism, since it
makes the codepaths hard to follow. For example, it would be a good win to code
health to remove kMenuControllerMenuWillOpenNotification. Looks like it's
currently only used in browser_action_button_interactive_uitest.mm (so,
additionally, it's also good to avoid code in production that would only be used
for tests)

And instead of dispatch_block_t, it would be nicer to use base::Callback (or
base::Closure) with base::Bind, since more Chrome engineers will be familiar
with that.

[karandeepb, 2016/04/06 05:52:48]

Done. Though the code looks slightly uglier now since I can't use lambda
functions.

  public:
   TestModel() : ui::SimpleMenuModel(&delegate_), delegate_(this) {}
 
   void set_checked_command(int command) { checked_command_ = command; }
 
  private:
   class Delegate : public ui::SimpleMenuModel::Delegate {
    public:
     explicit Delegate(TestModel* model) : model_(model) {}
     bool IsCommandIdChecked(int command_id) const override {
@@ skipping 39 matching lines @@
     menu_->AddCheckItem(0, base::ASCIIToUTF16("Menu Item"));
 
     parent_ = new views::Widget();
     parent_->Init(CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS));
     parent_->SetBounds(
         gfx::Rect(kWindowOffset, kWindowOffset, kWindowWidth, kWindowHeight));
     parent_->Show();
 
     runner_ = new internal::MenuRunnerImplCocoa(menu_.get());
     EXPECT_FALSE(runner_->IsRunning());
+    menu_watcher_.reset([[MenuOpenWatcher alloc] init]);
   }
 
   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);
+    [menu_watcher_ setOpenCallback:^{
+      EXPECT_TRUE(runner_->IsRunning());
+      block();
+    }];
+    MenuRunner::RunResult result =
+        runner_->RunMenuAt(parent_, NULL, gfx::Rect(), MENU_ANCHOR_TOPLEFT,

[tapted, 2016/03/24 02:10:36]

nit (while you're here) NULL -> nullptr

[karandeepb, 2016/04/06 05:52:48]

Done.

+                           MenuRunner::CONTEXT_MENU);
+    [menu_watcher_ setOpenCallback:nil];
+    return result;
   }
 
   // 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, ^{
+
+    [menu_watcher_ setOpenCallback:^{
       NSArray* subviews = [parent_->GetNativeView() subviews];
       EXPECT_EQ(2u, [subviews count]);
       last_anchor_frame_ = [[subviews objectAtIndex:1] frame];
       runner_->Cancel();
-    });
+    }];
+
     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]);
+    [menu_watcher_ setOpenCallback:nil];
     return result;
   }
 
  protected:
   scoped_ptr<TestModel> menu_;
+  base::scoped_nsobject<MenuOpenWatcher> menu_watcher_;
   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();
@@ skipping 81 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