Fix BaseBubbleController close issue.

chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm

 // Copyright (c) 2012 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 "chrome/browser/ui/cocoa/base_bubble_controller.h"
 
 #include "base/mac/mac_util.h"
 #import "base/mac/scoped_nsobject.h"
 #import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
 #import "chrome/browser/ui/cocoa/info_bubble_view.h"
 #import "chrome/browser/ui/cocoa/run_loop_testing.h"
 #import "ui/events/test/cocoa_test_event_utils.h"
 
 namespace {
 const CGFloat kBubbleWindowWidth = 100;
 const CGFloat kBubbleWindowHeight = 50;
 const CGFloat kAnchorPointX = 400;
 const CGFloat kAnchorPointY = 300;
 }  // namespace
 
+@interface ContextMenuController : NSObject<NSMenuDelegate> {
+  @private

[Robert Sesek, 2014/06/05 14:22:50]

nit: @private is only indented 1 space

+  NSMenu* menu_;
+  NSWindow* window_;
+  BOOL isMenuOpen_;
+  BOOL didOpen_;
+}
+
+- (id)initWithMenu:(NSMenu*) menu AndWindow:(NSWindow*) window;

[Robert Sesek, 2014/06/05 14:22:50]

nit: No space after ) for arguments. I'm mentioning it here, but please fix
throughout the test.

[Robert Sesek, 2014/06/05 14:22:50]

nit: AndWindow: -> andWindow:

+
+- (BOOL)isMenuOpen;
+- (BOOL)didOpen;
+- (BOOL)isWindowVisible;
+
+// NSMenuDelegate methods
+- (void)menuWillOpen:(NSMenu*) menu;
+- (void)menuDidClose:(NSMenu*) menu;
+
+@end
+
+@implementation ContextMenuController
+
+- (id)initWithMenu:(NSMenu*) menu AndWindow:(NSWindow*)window {
+  if (self = [super init]) {
+    menu_ = menu;
+    window_ = window;
+    isMenuOpen_ = NO;
+    didOpen_ = NO;
+    [menu_ setDelegate: self];

[Robert Sesek, 2014/06/05 14:22:50]

nit: No space after : in method invocations. Please fix throughout your new
test.

+  }
+  return self;
+}
+
+- (BOOL)isMenuOpen {
+  return isMenuOpen_;
+}
+
+- (BOOL)didOpen {
+  return didOpen_;
+}
+
+- (BOOL)isWindowVisible {
+  if (window_) {
+    return [window_ isVisible];
+  }
+  return NO;
+}
+
+- (void)menuWillOpen:(NSMenu *)menu {

[Robert Sesek, 2014/06/05 14:22:50]

nit: No space before * in arguments. Same with line 82.

+  isMenuOpen_ = YES;
+  didOpen_ = NO;
+
+  NSArray* modes = [NSArray arrayWithObjects:NSEventTrackingRunLoopMode,

[Robert Sesek, 2014/06/05 14:22:50]

You can use the ObjC array literal syntax @[ NSEventTrackingRunLoopMode,
NSDefaultRunLoopMode ]

+                                             NSDefaultRunLoopMode,
+                                             nil];
+  [menu_ performSelector:@selector(cancelTracking)
+              withObject:nil
+              afterDelay:0.1
+                 inModes:modes];
+}
+
+- (void)menuDidClose:(NSMenu *)menu {
+  isMenuOpen_ = NO;
+  didOpen_ = YES;
+}
+
+@end
+
 class BaseBubbleControllerTest : public CocoaTest {
  public:
   virtual void SetUp() OVERRIDE {
     bubbleWindow_.reset([[NSWindow alloc]
         initWithContentRect:NSMakeRect(0, 0, kBubbleWindowWidth,
                                        kBubbleWindowHeight)
                   styleMask:NSBorderlessWindowMask
                     backing:NSBackingStoreBuffered
                       defer:YES]);
 
@@ skipping 136 matching lines @@
   else
     [controller_ windowDidResignKey:notif];
 
 
   EXPECT_FALSE([bubble_window isVisible]);
   EXPECT_TRUE([other_window isVisible]);
 }
 
 // Test that clicking outside the window causes the bubble to close if
 // shouldCloseOnResignKey is YES.
-TEST_F(BaseBubbleControllerTest, LionClickOutsideCloses) {
+TEST_F(BaseBubbleControllerTest, LionClickOutsideClosesWithoutContextMenu) {
   // The event tap is only installed on 10.7+.
   if (!base::mac::IsOSLionOrLater())
     return;
 
   // Closing the bubble will autorelease the controller.
   base::scoped_nsobject<BaseBubbleController> keep_alive([controller_ retain]);
   NSWindow* window = [controller_ window];
 
   EXPECT_TRUE([controller_ shouldCloseOnResignKey]);  // Verify default value.
   EXPECT_FALSE([window isVisible]);
 
   [controller_ showWindow:nil];
 
   EXPECT_TRUE([window isVisible]);
 
   [controller_ setShouldCloseOnResignKey:NO];
   NSEvent* event = cocoa_test_event_utils::LeftMouseDownAtPointInWindow(
       NSMakePoint(10, 10), test_window());
   [NSApp sendEvent:event];
-  chrome::testing::NSRunLoopRunAllPending();

[Robert Sesek, 2014/06/05 14:22:50]

What's the rationale for removing these calls?

[Xuefei Ren, 2014/06/06 03:41:44]

Before my patch, the bubble window close depends on RegisterKeyStateEventTap
which monitors NSLeftMouseDownMask. And it will run a block that uses
[self performSelector:@selector(windowDidRegisterKey:).
Now my patch change it in two ways:
1. Add NSRightMouseDownMask.
2. Replace performSelector with executing windowDidRegisterKey directly.
   [self windowDidRegisterKey]
That is because when right-click with a contextual menu displayed, the
windowDidRegisterKey will not run.

Using chrome::testing::NSRunLoopRunAllPending() is to make sure
windowDidResignKey; is executed before verifying EXPECT_TURE([window
isVisible]);
Since we use no PerformSelector, we don't need it anymore.

 
   EXPECT_TRUE([window isVisible]);
 
+  event = cocoa_test_event_utils::RightMouseDownAtPointInWindow(
+      NSMakePoint(10, 10), test_window());
+  [NSApp sendEvent:event];
+
+  EXPECT_TRUE([window isVisible]);
+
   [controller_ setShouldCloseOnResignKey:YES];
   event = cocoa_test_event_utils::LeftMouseDownAtPointInWindow(
       NSMakePoint(10, 10), test_window());
   [NSApp sendEvent:event];
-  chrome::testing::NSRunLoopRunAllPending();
+
+  EXPECT_FALSE([window isVisible]);
+
+  [controller_ showWindow:nil]; // Show it again
+  EXPECT_TRUE([window isVisible]);
+  EXPECT_TRUE([controller_ shouldCloseOnResignKey]);  // Verify.
+
+  event = cocoa_test_event_utils::RightMouseDownAtPointInWindow(
+      NSMakePoint(10, 10), test_window());
+  [NSApp sendEvent:event];
 
   EXPECT_FALSE([window isVisible]);
 }
+
+// Test that right-clicking the window with displaying a context menu causes
+// the bubble  to close.
+TEST_F(BaseBubbleControllerTest, LionRightClickOutsideClosesWithContextMenu) {
+  // The event tap is only installed on 10.7+.
+  if (!base::mac::IsOSLionOrLater())
+    return;
+
+  // Closing the bubble will autorelease the controller.
+  base::scoped_nsobject<BaseBubbleController> keep_alive([controller_ retain]);
+  NSWindow* window = [controller_ window];
+
+  EXPECT_TRUE([controller_ shouldCloseOnResignKey]);  // Verify default value.
+  EXPECT_FALSE([window isVisible]);
+
+  [controller_ showWindow:nil];
+
+  EXPECT_TRUE([window isVisible]);
+
+  base::scoped_nsobject<NSMenu> context_menu(
+                                            [[NSMenu alloc] initWithTitle:@""]);

[Robert Sesek, 2014/06/05 14:22:50]

nit: Only indent 6 spaces total.

+  [context_menu addItemWithTitle: @"ContextMenuTest"
+                         action: nil
+                  keyEquivalent:@""];
+  ContextMenuController* menu_controller = [[ContextMenuController alloc]

[Robert Sesek, 2014/06/05 14:22:50]

This is leaked, so you should put it in a scoped_nsobject<>.

+                                                initWithMenu: context_menu
+                                                   AndWindow: window];
+  // Set the menu as the contextual menu of contentView of test_window().
+  [[test_window() contentView] setMenu:context_menu];
+
+  // RightMouseDown in test_window() would close the bubble window and then
+  // dispaly the contextual menu.
+  NSEvent* event = cocoa_test_event_utils::RightMouseDownAtPointInWindow(
+                                NSMakePoint(10, 10),

[Robert Sesek, 2014/06/05 14:22:50]

nit: Only indent 6 spaces total, and you can join this line with the next.

+                                test_window());
+  // Verify bubble's window is closed when contextual menu is open.
+  CFRunLoopPerformBlock(CFRunLoopGetCurrent(), NSEventTrackingRunLoopMode, ^{
+    EXPECT_TRUE([menu_controller isMenuOpen]);

[Robert Sesek, 2014/06/05 14:22:50]

nit: Indent blocks 4 spaces.

+    EXPECT_FALSE([menu_controller isWindowVisible]);
+  });
+
+  EXPECT_FALSE([menu_controller isMenuOpen]);
+  EXPECT_FALSE([menu_controller didOpen]);
+
+  [NSApp sendEvent:event];
+
+  // When we got here, menu has already run its RunLoop.
+  // See ContextualMenuController::MenuWillOpen.

[Robert Sesek, 2014/06/05 14:22:50]

This is how you name a C++ method. This should be written
-[ContextMenuController menuWillOpen:].

+  EXPECT_FALSE([window isVisible]);
+
+  EXPECT_FALSE([menu_controller isMenuOpen]);
+  EXPECT_TRUE([menu_controller didOpen]);
+}
+