Dismiss context menu when download bar is closed.

chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm

Index: chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
diff --git a/chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm b/chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
index cf7a98cc8bee7d407c4c62523202b709783aff70..509522564f49fb2988161c4107f375b820a4781b 100644
--- a/chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm
@@ -8,6 +8,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
 #include "chrome/browser/ui/cocoa/cocoa_profile_test.h"
+#import "chrome/browser/ui/cocoa/download/download_item_button.h"
 #import "chrome/browser/ui/cocoa/download/download_item_controller.h"
 #import "chrome/browser/ui/cocoa/download/download_shelf_controller.h"
 #include "content/public/test/mock_download_item.h"
@@ -44,6 +45,13 @@ using ::testing::ReturnRefOfCopy;
 - (void)awakeFromNib;
 @end
 
+@interface DownloadItemButton(DownloadItemButtonTest)
+
+- (BOOL)showingContextMenu;
+
+@end
+
+
 @implementation DownloadItemControllerWithInitCallback
 
 - (id)initWithDownload:(content::DownloadItem*)downloadItem
@@ -179,4 +187,31 @@ TEST_F(DownloadItemControllerTest, NormalDownloadBecomesDangerous) {
   [(id)shelf_ verify];
 }
 
+TEST_F(DownloadItemControllerTest, DismissesContextMenuWhenRemovedFromWindow) {
+  base::scoped_nsobject<DownloadItemController> item(CreateItemController());
+  DownloadItemButton* downloadItemButton = nil;
+  for (NSView *nextSubview in [[item view] subviews]) {
+    if ([nextSubview isKindOfClass:[DownloadItemButton class]]) {
+      downloadItemButton = static_cast<DownloadItemButton *>(nextSubview);
+      break;
+    }
+  }
+  // showContextMenu: calls [NSMenu popUpContextMenu:...], which blocks until
+  // the menu is dismissed. Use a block to cancel the menu while waiting for
+  // [NSMenu popUpContextMenu:...] to return.
+  [[NSOperationQueue mainQueue] addOperationWithBlock:^{
+    // Wait a bit to make sure [item showContextMenu:nil] is executing before
+    // proceeding.
+    [NSThread sleepForTimeInterval:0.25];

[asanka, 2015/05/13 16:58:55]

Let's avoid sleeping in tests.

Can we schedule a task (preferably via PostTask) to run [item remove] or
somesuch once the nested run loop starts? Granted it will run pretty early on in
the life of the popup context menu.

[shrike, 2015/05/13 23:25:51]

I'm using an NSOperationQueue to execute the code (via a block) while the menu
is popped up. The 0.25 delay gives the menu some time to pop up before the block
executes (actually, not a surefire strategy), but what I didn't realize is that
operations scheduled on the mainQueue are executed on the main thread, and on
passes through the run loop. So, I don't actually have to add any kind of delay:
the block gets added to the queue, but only runs after the menu pops up.

Re: using PostTask(), I tried that but the posted task seemed not to run until
the menu was dismissed? Does it make sense that
base::MessageLoop::current()->PostTask() would block while the modal loop runs?

[asanka, 2015/05/14 03:31:45]

Hm. Tasks posted via PostTask() for the UI message loop should run even in an
event tracking run loop.

[shrike, 2015/05/15 01:07:03]

Originally you didn't like the [NSTask sleep] call (agreed), but I don't
actually need to do that. Are you wanting me to reimplement using PostTask() or
is the current solution OK?

[asanka, 2015/05/15 14:56:11]

Is there a new patchset?

I'd prefer a PostTask and I thought you were looking into understanding why it
wasn't working. But I'm okay with using NSOperationQueue.

[shrike, 2015/05/15 15:55:51]

I think I was waiting on further direction before uploading.
Alright, I will dig into the PostTask issue. It might be pilot error, but I also
wonder if the unit_test environment differs from the regular browser.

[shrike, 2015/05/15 18:51:49]

It looks like the message loop isn't running in the unit test (now that I think
about it, this would have to be a browser test to expect there to be an active
message loop?):

   
../../chrome/browser/ui/cocoa/download/download_item_controller_unittest.mm:202:
Failure
    Value of: base::MessageLoop::current()->is_running()
      Actual: false
    Expected: true

+    EXPECT_TRUE([downloadItemButton showingContextMenu]);
+    [downloadItemButton
+        performSelectorOnMainThread:@selector(removeFromSuperview)
+                         withObject:nil
+                      waitUntilDone:NO];

[asanka, 2015/05/13 16:58:55]

The real trigger for the removal of the context menu is the removal of the
download. Can you test with [item remove] ?

[shrike, 2015/05/13 23:25:52]

I agree that makes sense. However, it appears that the shelf is not actually a
DownloadShelfController but is instead an OCMockObject instance, so calling
[item remove] or [(id)shelf remove:item] doesn't actually do anything?

+  }];
+  [item showContextMenu:nil];
+  EXPECT_FALSE([downloadItemButton showingContextMenu]);

[asanka, 2015/05/13 16:58:55]

Given that this line won't be reached until the context menu is dismissed, isn't
this a no-op?

[shrike, 2015/05/13 23:25:52]

Acknowledged.

+}
+
+
 }  // namespace