Only dispatch menu events if they have a valid target.

Only dispatch menu events if they have a valid target.

BUG=370162
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=271283

[flackr, 2014-05-13 17:46:40]

Hey,

Please take a look! This is basically what we talked about, though I found that
when there was a focused input field that window was targeted rather than the
menu which is why the current check is just that there is a valid target (with a
TODO).

WDYT? Thanks!

[sadrul, 2014-05-14 03:23:10]

I think this looks great! Can you add a test in MenuControllerTest for this?

https://codereview.chromium.org/284903002/diff/1/ui/events/event_utils.h
File ui/events/event_utils.h (right):

https://codereview.chromium.org/284903002/diff/1/ui/events/event_utils.h#newc...
ui/events/event_utils.h:37: EVENTS_EXPORT Event* EventFromNative(const
base::NativeEvent& native_event);
Return a scoped_ptr<>

[flackr, 2014-05-15 21:12:56]

I finally managed to get a test working to verify this. I also realized that
even though the window targeter should prevent handling of the event it probably
shouldn't stop the menu from exiting if it's in the process of doing so.

Note, I had to add a MenuItemView to the menu so that the key handler would be
able to run successfully.

Please take a look, thanks.

https://codereview.chromium.org/284903002/diff/1/ui/events/event_utils.h
File ui/events/event_utils.h (right):

https://codereview.chromium.org/284903002/diff/1/ui/events/event_utils.h#newc...
ui/events/event_utils.h:37: EVENTS_EXPORT Event* EventFromNative(const
base::NativeEvent& native_event);
On 2014/05/14 03:23:10, sadrul wrote:
> Return a scoped_ptr<>

Done.

[sadrul, 2014-05-15 21:53:01]

https://codereview.chromium.org/284903002/diff/20001/ui/events/event_utils.cc
File ui/events/event_utils.cc (right):

https://codereview.chromium.org/284903002/diff/20001/ui/events/event_utils.cc...
ui/events/event_utils.cc:30: event.reset(new TranslatedKeyEvent(native_event,
false));
I think this should be a NOTREACHED() (the translated-events are supposed to be
generated by the IME system in chrome, and so should not be generated by
native-events).

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

https://codereview.chromium.org/284903002/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:204: #if defined(OS_LINUX)
and defined(USE_X11)
Does this test not work on chromeos?

https://codereview.chromium.org/284903002/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:215:
ui::POST_DISPATCH_QUIT_LOOP));
Hm. I removed this recently (https://codereview.chromium.org/274383002/). So
this becomes a little trickier. Perhaps you can have the expectation on
controller_->exit_type_ instead? i.e. in this case, after dispatching the escape
key, |exit_type_| should be something other than EXIT_NONE. But in the other
case below, it should still be EXIT_NONE after dispatching escape?

[flackr, 2014-05-15 22:13:01]

https://codereview.chromium.org/284903002/diff/20001/ui/events/event_utils.cc
File ui/events/event_utils.cc (right):

https://codereview.chromium.org/284903002/diff/20001/ui/events/event_utils.cc...
ui/events/event_utils.cc:30: event.reset(new TranslatedKeyEvent(native_event,
false));
On 2014/05/15 21:53:01, sadrul wrote:
> I think this should be a NOTREACHED() (the translated-events are supposed to
be
> generated by the IME system in chrome, and so should not be generated by
> native-events).

Done.

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

https://codereview.chromium.org/284903002/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:204: #if defined(OS_LINUX)
and defined(USE_X11)
On 2014/05/15 21:53:01, sadrul wrote:
> Does this test not work on chromeos?

OS_LINUX is true on chromeos, I was matching the platform of the dispatcher
(i.e. it's a *_linux.cc), see filename_rules.gypi:
https://code.google.com/p/chromium/codesearch#chromium/src/build/filename_rul...

https://codereview.chromium.org/284903002/diff/20001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller_unittest.cc:215:
ui::POST_DISPATCH_QUIT_LOOP));
On 2014/05/15 21:53:01, sadrul wrote:
> Hm. I removed this recently (https://codereview.chromium.org/274383002/). So
> this becomes a little trickier. Perhaps you can have the expectation on
> controller_->exit_type_ instead? i.e. in this case, after dispatching the
escape
> key, |exit_type_| should be something other than EXIT_NONE. But in the other
> case below, it should still be EXIT_NONE after dispatching escape?

Done.

[sadrul, 2014-05-15 22:14:13]

Cool. Thanks! lgtm

[flackr, 2014-05-16 21:15:56]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-05-16 21:17:18]

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

[commit-bot: I haz the power, 2014-05-17 06:45:15]

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:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-17 07:46:15]

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

[commit-bot: I haz the power, 2014-05-17 07:46:16]

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

[flackr, 2014-05-17 13:19:26]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-05-17 13:20:32]

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

[commit-bot: I haz the power, 2014-05-18 13:53:37]

Change committed as 271283

[Nico, 2014-05-18 18:59:51]

A revert of this CL has been created in
https://codereview.chromium.org/287233005/ by thakis@chromium.org.

The reason for reverting is: Made the asan bot red:

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
    #3 0xb2b838 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void>
testing/gtest/src/gtest.cc:2045
    #4 0xb2b838 in testing::Test::Run() testing/gtest/src/gtest.cc:2061
    #5 0xb2d2c8 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237
    #6 0xb2e006 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344
    #7 0xb3e7fa in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4065
    #8 0xb3de40 in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2045
    #9 0xb3de40 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697
    #10 0xafaee4 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231
    #11 0xafaee4 in base::TestSuite::Run() base/test/test_suite.cc:206
    #12 0xaf2656 in Run base/callback.h:401
    #13 0xaf2656 in base::(anonymous namespace)::LaunchUnitTestsInternal(int,
char**, base::Callback\u003Cint ()> const&, int)
base/test/launcher/unit_test_launcher.cc:494
    #14 0x83846a in main ui/views/run_all_unittests.cc:43
    #15 0x7ff88421176c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226



Probably easy to fix (and thus easy to reland), but from skimming the test it's
not obvious to me what's supposed to free controller_. I'm guessing the second
block in the new test just needs an explicit delete? (An "Owned by X" comment
close to controller_ would've helped me).