MacViews: Fix failing Menu Runner Cocoa Tests.

ui/views/controls/menu/menu_runner_impl_cocoa.mm

Index: ui/views/controls/menu/menu_runner_impl_cocoa.mm
diff --git a/ui/views/controls/menu/menu_runner_impl_cocoa.mm b/ui/views/controls/menu/menu_runner_impl_cocoa.mm
index c6802381ab6c221eb9645b6e965f113b00fa0504..1605c3bf9a23a2cf37321124f022906cbe7c7c80 100644
--- a/ui/views/controls/menu/menu_runner_impl_cocoa.mm
+++ b/ui/views/controls/menu/menu_runner_impl_cocoa.mm
@@ -93,7 +93,7 @@ MenuRunnerImplInterface* MenuRunnerImplInterface::Create(
 }
 
 MenuRunnerImplCocoa::MenuRunnerImplCocoa(ui::MenuModel* menu)
-    : running_(false),
+    : in_run_menu_at_(false),
       delete_after_run_(false),
       closing_event_time_(base::TimeDelta()) {
   menu_controller_.reset(
@@ -101,11 +101,11 @@ MenuRunnerImplCocoa::MenuRunnerImplCocoa(ui::MenuModel* menu)
 }
 
 bool MenuRunnerImplCocoa::IsRunning() const {
-  return running_;
+  return [menu_controller_ isMenuOpen];

[tapted, 2016/05/06 00:37:56]

I think this is different to what menu_runner_impl.cc does, but the codepaths
are complicated :/. Do you think the tests need to be updated instead?

E.g. IsRunning() says "Returns true if we're in a nested message loop running
the menu.". Can you check whether the non-native menu controlled by
menu_runner_impl.cc returns false immediately after a cancel? We should try to
be consistent with that unless other stuff breaks outside of tests.

[karandeepb, 2016/05/25 04:19:33]

Not sure if my understanding is correct, but on Mac, calling Cancel() would call

[menu_ cancelTracking] which should end the nested run loop spun by Cocoa. So
this implementation is consistent with the comments on IsRunning().

As far as consistency with the implementation on other platforms is concerned,
calling
MenuController::Cancel() calls delegate_->OnMenuClosed(), which would make
IsRunning() return false immediately. Example see -
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/....
But this is only true for async and non
blocking runs(MenuRunner::FOR_DROP). For a sync menu, calling Cancel() 
would cause the run loop termination, hence causing IsRunning() to return false,
but only after the last run loop iteration completes. (like our current
implementation). Hence IsRunning() would not return false immediately after a
Cancel() for this case.

[tapted, 2016/05/25 05:07:19]

yup - but with this change Cocoa *would* return false for this case (right?)

The problem is when code *inside* the nested runloop calls Cancel() immediately
followed something else. Like you say - Cancel() doesn't immediately abort the
nested runloop - it just marks it to stop iterating. In
https://codereview.chromium.org/1876013002/ the problem was code that did
something like

HandleMenuActionFoo() {
  ..
  MenuRunner::Cancel();
  MenuRunner::Release();
}

MenuRunnerCocoaTest.DestroyAfterCanceling was added to cover this.

We need to compare code that does

HandleMenuActionFoo() {
  ..
  MenuRunner::Cancel();
  if (MenuRunner::IsRunning()) {
    // something.
  }
}

This should behave the same regardless of whether
MenuRunnerImplInterface::Create(..) returns a `new MenuRunnerImplCocoa` or a
`new MenuRunnerImplAdapter`.

I suspect that with this change, MenuRunner::IsRunning() will return false for
Cocoa, but true for the Adapter.

Perhaps we can add a new test to cover this -- menu_runner_cocoa_unittest.mm
doesn't currently try to perform tests using a MenuRunnerImpl

 }
 
 void MenuRunnerImplCocoa::Release() {
-  if (IsRunning()) {
+  if (in_run_menu_at_) {
     if (delete_after_run_)
       return;  // We already canceled.
 
@@ -121,11 +121,11 @@ MenuRunner::RunResult MenuRunnerImplCocoa::RunMenuAt(Widget* parent,
                                                      const gfx::Rect& bounds,
                                                      MenuAnchorPosition anchor,
                                                      int32_t run_types) {
+  in_run_menu_at_ = true;
   DCHECK(run_types & kNativeRunTypes);
   DCHECK(!IsRunning());
   DCHECK(parent);
   closing_event_time_ = base::TimeDelta();
-  running_ = true;
 
   if (run_types & MenuRunner::CONTEXT_MENU) {
     [NSMenu popUpContextMenu:[menu_controller_ menu]
@@ -146,7 +146,7 @@ MenuRunner::RunResult MenuRunnerImplCocoa::RunMenuAt(Widget* parent,
   }
 
   closing_event_time_ = ui::EventTimeForNow();
-  running_ = false;
+  in_run_menu_at_ = false;
 
   if (delete_after_run_) {
     delete this;