Reland "Only dispatch menu events if they have a valid target. (https://codereview.chromium.org/284…

Reland "Only dispatch menu events if they have a valid target. (https://codereview.chromium.org/284903002/)"

Clean up MenuController at end of tests and on subsequent RunMenu calls.

BUG=370162
TEST=MenuControllerTest.EventTargeter
TEST=In touchview, open wrench menu and press a key (i.e. 'T'), nothing should
happen.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272322

[flackr, 2014-05-20 18:53:18]

Hey, patch set 1 is the original CL which failed with:
Direct leak of 664 byte(s) in 1 object(s) allocated from:
#0 0x4b7c7b in operator new(unsigned long)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
#1 0x5a4ffc in RunMenu ui/views/controls/menu/menu_controller_unittest.cc:158
#2 0x5a4ffc in views::MenuControllerTest_EventTargeter_Test::TestBody()
ui/views/controls/menu/menu_controller_unittest.cc:218

As I commented in the other review, https://codereview.chromium.org/287233005/,
it seems like it didn't matter that it wasn't destructed before as it was still
referenced by the active_instance_ in menu_controller.cc, but EventTargeter
constructs a second menu which then loses the reference to the original.

Patch set 2 makes MenuControllerTest delete the menu between tests or when
creating another menu. Please take a look, thanks.

[Nico, 2014-05-20 19:00:12]

lgtm

[sadrul, 2014-05-20 19:55:44]

https://codereview.chromium.org/286933007/diff/20001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/286933007/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:205: MenuController*
controller_;
Does it make sense to just have this as scoped_ptr<>?

[flackr, 2014-05-20 20:33:40]

https://codereview.chromium.org/286933007/diff/20001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/286933007/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:205: MenuController*
controller_;
On 2014/05/20 19:55:45, sadrul wrote:
> Does it make sense to just have this as scoped_ptr<>?

It will fail to destruct unless we reset the owner_ and showing_ attributes. I
could wrap that initialization / destruction in an anonymous class and use a
scoped_ptr if you think it's worth it.

To properly run the MenuController it would need a menu model and menu runner,
and then I think we could create the MenuController before telling the menu
runner to run and it would use that instance.

[sadrul, 2014-05-20 20:36:51]

lgtm. I am OK with leaving this as is.

[flackr, 2014-05-21 13:04:55]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:59:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/286933007/20001

[commit-bot: I haz the power, 2014-05-22 08:07:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/7880)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10922)

[commit-bot: I haz the power, 2014-05-22 08:10:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 08:10:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[flackr, 2014-05-22 14:04:44]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-05-22 14:06:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/286933007/40001

[commit-bot: I haz the power, 2014-05-22 21:30:50]

Change committed as 272322