MacViews: Fix failing Menu Runner Cocoa Tests.

MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by
changing MenuCancelCallback() to expect MenuRunner::IsRunning() to be true,
immediately after the call to MenuRunner::Cancel(). This is correct since
MenuRunner::Cancel() only marks the nested message loop for termination. Since
we are in the last iteration of the nested message loop, MenuRunner::IsRunning()
should still return true.

An interactive test is also added to test this behavior for MenuRunnerImpl.

BUG=607403
Committed: https://crrev.com/dd1c18785711cd69951fac3dfd6964d08d296d66
Cr-Commit-Position: refs/heads/master@{#396664}

[karandeepb, 2016-05-05 10:50:37]

Description was changed from

==========
MenuRunnerCocoa tests
==========

to

==========
MacViews: Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by 
correcting the MenuRunnerImplCocoa::IsRunning() implementation to return whether

the menu is actually open or not. The variable |running_| is renamed to 
|in_run_menu_at_| to correctly reflect its purpose, since it is just used in
MenuRunnerImplCocoa::Release() to ensure we don't do a Release() while inside a 
RunMenuAt() call. It does not reflect whether the menu is open or not.

BUG=607403
==========

[karandeepb, 2016-05-05 11:00:33]

Description was changed from

==========
MacViews: Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by 
correcting the MenuRunnerImplCocoa::IsRunning() implementation to return whether

the menu is actually open or not. The variable |running_| is renamed to 
|in_run_menu_at_| to correctly reflect its purpose, since it is just used in
MenuRunnerImplCocoa::Release() to ensure we don't do a Release() while inside a 
RunMenuAt() call. It does not reflect whether the menu is open or not.

BUG=607403
==========

to

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002 since even after calling
MenuRunnerImplCocoa::Cancel(), MenuRunnerImplCocoa::IsRunning() will return true
while inside a RunMenuAt() call. This CL fixes these tests by correcting the
MenuRunnerImplCocoa::IsRunning() implementation to return whether the menu is 
actually open or not. The variable |running_| is renamed to |in_run_menu_at_| to
correctly reflect its purpose, since it is just used in 
MenuRunnerImplCocoa::Release() to ensure we don't do a Release() while inside a
RunMenuAt() call. It does not reflect whether the menu is open or not.

BUG=607403
==========

[karandeepb, 2016-05-05 11:01:11]

Description was changed from

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002 since even after calling
MenuRunnerImplCocoa::Cancel(), MenuRunnerImplCocoa::IsRunning() will return true
while inside a RunMenuAt() call. This CL fixes these tests by correcting the
MenuRunnerImplCocoa::IsRunning() implementation to return whether the menu is 
actually open or not. The variable |running_| is renamed to |in_run_menu_at_| to
correctly reflect its purpose, since it is just used in 
MenuRunnerImplCocoa::Release() to ensure we don't do a Release() while inside a
RunMenuAt() call. It does not reflect whether the menu is open or not.

BUG=607403
==========

to

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002 since even after calling 
MenuRunnerImplCocoa::Cancel(), MenuRunnerImplCocoa::IsRunning() will return true
while inside a RunMenuAt() call. This CL fixes these tests by correcting the 
MenuRunnerImplCocoa::IsRunning() implementation to return whether the menu is 
actually open or not. The variable |running_| is renamed to |in_run_menu_at_| to
correctly reflect its purpose, since it is just used in 
MenuRunnerImplCocoa::Release() to ensure we don't do a Release() while inside a
RunMenuAt() call. It does not reflect whether the menu is open or not.

BUG=607403
==========

[karandeepb, 2016-05-05 11:01:56]

Description was changed from

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002 since even after calling 
MenuRunnerImplCocoa::Cancel(), MenuRunnerImplCocoa::IsRunning() will return true
while inside a RunMenuAt() call. This CL fixes these tests by correcting the 
MenuRunnerImplCocoa::IsRunning() implementation to return whether the menu is 
actually open or not. The variable |running_| is renamed to |in_run_menu_at_| to
correctly reflect its purpose, since it is just used in 
MenuRunnerImplCocoa::Release() to ensure we don't do a Release() while inside a
RunMenuAt() call. It does not reflect whether the menu is open or not.

BUG=607403
==========

to

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002 since even after calling 
MenuRunnerImplCocoa::Cancel(), MenuRunnerImplCocoa::IsRunning() will return true
while inside a RunMenuAt() call. This CL fixes these tests by correcting the 
MenuRunnerImplCocoa::IsRunning() implementation to return whether the menu is 
actually open or not. The variable |running_| is renamed to |in_run_menu_at_| to
correctly reflect its purpose.

BUG=607403
==========

[karandeepb, 2016-05-05 11:03:19]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org

[karandeepb, 2016-05-05 11:03:20]

PTAL Trent. This is same as https://codereview.chromium.org/1876013002/#ps1 with
the only change being the variable in_menu_runloop_ is renamed and the condition
in MenuRunnerImplCocoa::Release() is changed, since I don't think the case
IsRunning() is true and in_run_menu_at_ is false is possible.

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

https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):

https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_runner_impl_cocoa.mm:104: return [menu_controller_
isMenuOpen];
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]

PTAL Trent. Thanks.

https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):

https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_runner_impl_cocoa.mm:104: return [menu_controller_
isMenuOpen];
On 2016/05/06 00:37:56, tapted wrote:
> 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.

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]

https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):

https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
ui/views/controls/menu/menu_runner_impl_cocoa.mm:104: return [menu_controller_
isMenuOpen];
On 2016/05/25 04:19:33, karandeepb wrote:
> On 2016/05/06 00:37:56, tapted wrote:
> > 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.
> 
> 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.

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

[karandeepb, 2016-05-26 11:46:28]

On 2016/05/25 05:07:19, tapted wrote:
>
https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
> File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):
> 
>
https://codereview.chromium.org/1951223002/diff/1/ui/views/controls/menu/menu...
> ui/views/controls/menu/menu_runner_impl_cocoa.mm:104: return [menu_controller_
> isMenuOpen];
> On 2016/05/25 04:19:33, karandeepb wrote:
> > On 2016/05/06 00:37:56, tapted wrote:
> > > 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.
> > 
> > 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.
> 
> 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

PTAL Trent. 

Yes you are correct. I was confused since MenuRunnerImpl itself has different
behaviors for different menu types like asynchronous. But since the NSMenu is a
blocking one, it probably makes sense to replicate the behavior of a synchronous
menu. Have also added a new interactive test for MenuRunnerImpl which verifies
this behavior, and modified the tests for MenuRunnerImplCocoa. 

I also did some experimentation and discovered that Cocoa does not actually spin
up a nested message loop for an NSMenu. Instead it merely changes the run loop
mode to NSEventTrackingRunLoopMode. However currently our test callbacks run in
the default run loop mode only since we are using the menuWillOpen method on the
NSMenuDelegate. We might instead want to use [NSObject
performSelectorOnMainThread: @selector(..) withObject: nil waitUntilDone:NO
modes:@[NSEventTrackingRunLoopMode]] in our test callbacks, to test the behavior
when the menu has actually opened.

[tapted, 2016-05-27 02:58:55]

Thanks! lgtm, but make sure you update the CL description

https://codereview.chromium.org/1951223002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/menu_controller_interactive_uitest.cc (right):

https://codereview.chromium.org/1951223002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/menu_controller_interactive_uitest.cc:101:
~MenuRunnerCancelTest() override {}
nit: not needed?

[karandeepb, 2016-05-27 03:34:55]

Description was changed from

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002 since even after calling 
MenuRunnerImplCocoa::Cancel(), MenuRunnerImplCocoa::IsRunning() will return true
while inside a RunMenuAt() call. This CL fixes these tests by correcting the 
MenuRunnerImplCocoa::IsRunning() implementation to return whether the menu is 
actually open or not. The variable |running_| is renamed to |in_run_menu_at_| to
correctly reflect its purpose.

BUG=607403
==========

to

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by 
changing MenuCancelCallback() to expect MenuRunner::IsRunning() to be true, 
immediately after the call to MenuRunner::Cancel(). This is correct since 
MenuRunner::Cancel() only marks the nested message loop for termination. Since 
we are in the last iteration of the nested message loop, MenuRunner::IsRunning()
should still return true. 

An interactive test is also added to test this behavior for MenuRunnerImpl.

BUG=607403
==========

[karandeepb, 2016-05-27 03:36:04]

karandeepb@chromium.org changed reviewers:
+ sky@chromium.org

[karandeepb, 2016-05-27 03:36:04]

PTAL sky@ for owner review.

https://codereview.chromium.org/1951223002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/menu_controller_interactive_uitest.cc (right):

https://codereview.chromium.org/1951223002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/menu_controller_interactive_uitest.cc:97: class
MenuRunnerCancelTest : public MenuTestBase {
Let me know if a new file needs to be created for this.

https://codereview.chromium.org/1951223002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/menu_controller_interactive_uitest.cc (right):

https://codereview.chromium.org/1951223002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/menu_controller_interactive_uitest.cc:101:
~MenuRunnerCancelTest() override {}
On 2016/05/27 02:58:55, tapted wrote:
> nit: not needed?

Done.

[sky, 2016-05-27 15:41:42]

LGTM

[karandeepb, 2016-05-30 00:32:18]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-05-30 00:32:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1951223002/#ps60001
(title: "Address nit.")

[commit-bot: I haz the power, 2016-05-30 00:32:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951223002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1951223002/60001

[commit-bot: I haz the power, 2016-05-30 01:21:10]

Description was changed from

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by 
changing MenuCancelCallback() to expect MenuRunner::IsRunning() to be true, 
immediately after the call to MenuRunner::Cancel(). This is correct since 
MenuRunner::Cancel() only marks the nested message loop for termination. Since 
we are in the last iteration of the nested message loop, MenuRunner::IsRunning()
should still return true. 

An interactive test is also added to test this behavior for MenuRunnerImpl.

BUG=607403
==========

to

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by 
changing MenuCancelCallback() to expect MenuRunner::IsRunning() to be true, 
immediately after the call to MenuRunner::Cancel(). This is correct since 
MenuRunner::Cancel() only marks the nested message loop for termination. Since 
we are in the last iteration of the nested message loop, MenuRunner::IsRunning()
should still return true. 

An interactive test is also added to test this behavior for MenuRunnerImpl.

BUG=607403
==========

[commit-bot: I haz the power, 2016-05-30 01:21:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-30 01:22:25]

Description was changed from

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by 
changing MenuCancelCallback() to expect MenuRunner::IsRunning() to be true, 
immediately after the call to MenuRunner::Cancel(). This is correct since 
MenuRunner::Cancel() only marks the nested message loop for termination. Since 
we are in the last iteration of the nested message loop, MenuRunner::IsRunning()
should still return true. 

An interactive test is also added to test this behavior for MenuRunnerImpl.

BUG=607403
==========

to

==========
MacViews:Fix failing Menu Runner Cocoa Tests.

This CL fixes the following failing tests-
  -MenuRunnerCocoaTest.RunMenuAndCancel
  -MenuRunnerCocoaTest.RunMenuTwice

These tests regressed in crrev.com/1876013002. This CL fixes these tests by
changing MenuCancelCallback() to expect MenuRunner::IsRunning() to be true,
immediately after the call to MenuRunner::Cancel(). This is correct since
MenuRunner::Cancel() only marks the nested message loop for termination. Since
we are in the last iteration of the nested message loop, MenuRunner::IsRunning()
should still return true.

An interactive test is also added to test this behavior for MenuRunnerImpl.

BUG=607403

Committed: https://crrev.com/dd1c18785711cd69951fac3dfd6964d08d296d66
Cr-Commit-Position: refs/heads/master@{#396664}
==========

[commit-bot: I haz the power, 2016-05-30 01:22:26]

Patchset 4 (id:??) landed as
https://crrev.com/dd1c18785711cd69951fac3dfd6964d08d296d66
Cr-Commit-Position: refs/heads/master@{#396664}