Work around multiple bugs in the WrenchMenuControllerTest using a bit of hackery.

BUG=49206
TEST=Unit tests pass regularly.

Review URL: http://codereview.chromium.org/3017044

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@54312 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 43 insertions, 11 deletions

diff --git a/chrome/browser/cocoa/wrench_menu_controller.h b/chrome/browser/cocoa/wrench_menu_controller.h
index 080d75e..242848d 100644
--- a/chrome/browser/cocoa/wrench_menu_controller.h
+++ b/chrome/browser/cocoa/wrench_menu_controller.h
@@ -22,7 +22,7 @@ class WrenchMenuModel;
 //
 // This object is instantiated in Toolbar.xib and is configured by the
 // ToolbarController.
-@interface WrenchMenuController : MenuController <NSMenuDelegate> {
+@interface WrenchMenuController : MenuController<NSMenuDelegate> {
   IBOutlet NSView* editItem_;
   IBOutlet NSSegmentedControl* editControl_;
 
@@ -42,6 +42,21 @@ class WrenchMenuModel;
 // NSCarbonMenuWindow; this screws up the typical |-commandDispatch:| system.
 - (IBAction)dispatchWrenchMenuCommand:(id)sender;
 
+// Returns the weak reference to the WrenchMenuModel.
+- (WrenchMenuModel*)wrenchMenuModel;
+
+@end
+
+////////////////////////////////////////////////////////////////////////////////
+
+@interface WrenchMenuController (UnitTesting)
+// |-dispatchWrenchMenuCommand:| calls this after it has determined the tag of
+// the sender. The default implementation executes the command on the outermost
+// run loop using |-performSelector...withDelay:|. This is not desirable in
+// unit tests because it's hard to test around run loops in a deterministic
+// manner. To avoid those headaches, tests should provide an alternative
+// implementation.
+- (void)dispatchCommandInternal:(NSInteger)tag;
 @end
 
 #endif  // CHROME_BROWSER_COCOA_WRENCH_MENU_CONTROLLER_H_
diff --git a/chrome/browser/cocoa/wrench_menu_controller.mm b/chrome/browser/cocoa/wrench_menu_controller.mm
index 4f4c894..fca4d04 100644
--- a/chrome/browser/cocoa/wrench_menu_controller.mm
+++ b/chrome/browser/cocoa/wrench_menu_controller.mm
@@ -13,9 +13,8 @@
 #include "chrome/browser/wrench_menu_model.h"
 
 @interface WrenchMenuController (Private)
-- (WrenchMenuModel*)wrenchMenuModel;
 - (void)adjustPositioning;
-- (void)dispatchCommandInternal:(NSNumber*)tag;
+- (void)performCommandDispatch:(NSNumber*)tag;
 @end
 
 @implementation WrenchMenuController
@@ -104,16 +103,20 @@
   // TODO(rsesek): It'd be great if the zoom buttons didn't have to close the
   // menu. See http://crbug.com/48679 for more info.
   [menu_ cancelTracking];
+  [self dispatchCommandInternal:tag];
+}
+
+- (void)dispatchCommandInternal:(NSInteger)tag {
   // Executing certain commands from the nested run loop of the menu can lead
   // to wonky behavior (e.g. http://crbug.com/49716). To avoid this, schedule
   // the dispatch on the outermost run loop.
-  [self performSelector:@selector(dispatchCommandInternal:)
+  [self performSelector:@selector(performCommandDispatch:)
              withObject:[NSNumber numberWithInt:tag]
              afterDelay:0.0];
 }
 
 // Used to perform the actual dispatch on the outermost runloop.
-- (void)dispatchCommandInternal:(NSNumber*)tag {
+- (void)performCommandDispatch:(NSNumber*)tag {
   [self wrenchMenuModel]->ExecuteCommand([tag intValue]);
 }
 
diff --git a/chrome/browser/cocoa/wrench_menu_controller_unittest.mm b/chrome/browser/cocoa/wrench_menu_controller_unittest.mm
index c775013..fbbf740 100644
--- a/chrome/browser/cocoa/wrench_menu_controller_unittest.mm
+++ b/chrome/browser/cocoa/wrench_menu_controller_unittest.mm
@@ -14,11 +14,28 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
+// Override to avoid dealing with run loops in the testing environment.
+@implementation WrenchMenuController (UnitTesting)
+- (void)dispatchCommandInternal:(NSInteger)tag {
+  [self wrenchMenuModel]->ExecuteCommand(tag);
+}
+@end
+
+
 namespace {
 
 class MockWrenchMenuModel : public WrenchMenuModel {
  public:
   MockWrenchMenuModel() : WrenchMenuModel() {}
+  ~MockWrenchMenuModel() {
+    // This dirty, ugly hack gets around a bug in the test. In
+    // ~WrenchMenuModel(), there's a call to TabstripModel::RemoveObserver(this)
+    // which mysteriously leads to this crash: http://crbug.com/49206 .  It
+    // seems that the vector of observers is getting hosed somewhere between
+    // |-[ToolbarController dealloc]| and ~MockWrenchMenuModel(). This line
+    // short-circuits the parent destructor to avoid this crash.
+    tabstrip_model_ = NULL;
+  }
   MOCK_METHOD1(ExecuteCommand, void(int command_id));
 };
 
@@ -48,14 +65,12 @@ class WrenchMenuControllerTest : public CocoaTest {
   scoped_nsobject<ToolbarController> toolbar_controller_;
 };
 
-// Test crashes sometimes. http://crbug.com/49206
-TEST_F(WrenchMenuControllerTest, DISABLED_Initialized) {
+TEST_F(WrenchMenuControllerTest, Initialized) {
   EXPECT_TRUE([controller() menu]);
   EXPECT_GE([[controller() menu] numberOfItems], 5);
 }
 
-// Test crashes sometimes. http://crbug.com/49206
-TEST_F(WrenchMenuControllerTest, DISABLED_DispatchSimple) {
+TEST_F(WrenchMenuControllerTest, DispatchSimple) {
   scoped_nsobject<NSButton> button([[NSButton alloc] init]);
   [button setTag:IDC_ZOOM_PLUS];
 
@@ -66,8 +81,7 @@ TEST_F(WrenchMenuControllerTest, DISABLED_DispatchSimple) {
   [controller() dispatchWrenchMenuCommand:button.get()];
 }
 
-// Test crashes sometimes. http://crbug.com/49206
-TEST_F(WrenchMenuControllerTest, DISABLED_DispatchSegmentedControl) {
+TEST_F(WrenchMenuControllerTest, DispatchSegmentedControl) {
   // Set fake model to test dispatching.
   EXPECT_CALL(fake_model_, ExecuteCommand(IDC_CUT));
   [controller() setModel:&fake_model_];