Fix BaseBubbleController close issue.

chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm

Index: chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm
diff --git a/chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm b/chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm
index 8e04a9ec123599553c7cf637f99c8ab424dc267b..78db7b1b941dcc889f915c4e3d3195206f6c7ed1 100644
--- a/chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm
@@ -18,6 +18,74 @@ 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 {
@@ -174,7 +242,7 @@ TEST_F(BaseBubbleControllerTest, ResignKeyCloses) {
 
 // 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;
@@ -194,7 +262,12 @@ TEST_F(BaseBubbleControllerTest, LionClickOutsideCloses) {
   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]);
 
@@ -202,7 +275,70 @@ TEST_F(BaseBubbleControllerTest, LionClickOutsideCloses) {
   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]);
 }
+