Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531033003:
MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac. (Closed)
DescriptionMacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac.
The old test always called -[NSWindow performKeyEquivalent:] but as of r434436
we need to ensure that both performKeyEquivalent: and keyDown: are called in
order for window accelerators to work.
BUG=665823
Committed: https://crrev.com/2394f02438a9b0c5c8aa20a099ed02d7a5867eba
Cr-Commit-Position: refs/heads/master@{#435918}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add parens around bitwise operation. #Patch Set 3 : Remove Views dependency from Cocoa code. #
Total comments: 4
Patch Set 4 : Fix tapted's remarks. #
Messages
Total messages: 22 (9 generated)
mblsha@yandex-team.ru changed reviewers: + tapted@chromium.org
mblsha@yandex-team.ru changed reviewers: + mark@chromium.org
Now I think I have enough reviewers.
Description was changed from ========== MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac. The old test always called -[NSWindow performKeyEquivalent:] but as of r434436 we need to ensure that both performKeyEquivalent: and keyDown: are called in order for window accelerators to work. ========== to ========== MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac. The old test always called -[NSWindow performKeyEquivalent:] but as of r434436 we need to ensure that both performKeyEquivalent: and keyDown: are called in order for window accelerators to work. BUG=665823 ==========
can you paste the failure you're getting into the crbug comment? And I think it would be better to add a new test somewhere rather than modifying the existing one. The way the EventGenerator converts events into NSEvents is a bit ad-hoc so it introduces more points of failure this way. (I'm not sure where the test should go yet - hopefully the info you put on the crbug will help with that -- i.e. to point out which part is currently the point of failure.) https://codereview.chromium.org/2531033003/diff/1/chrome/browser/global_keybo... File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right): https://codereview.chromium.org/2531033003/diff/1/chrome/browser/global_keybo... chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:18: #include "ui/views/test/event_generator_delegate_mac.h" I've been trying to avoid introducing ui/views dependencies in the existing Cocoa codebase -- see the comments in chrome/browser/ui/cocoa/DEPS https://codereview.chromium.org/2531033003/diff/1/ui/views/test/event_generat... File ui/views/test/event_generator_delegate_mac.mm (right): https://codereview.chromium.org/2531033003/diff/1/ui/views/test/event_generat... ui/views/test/event_generator_delegate_mac.mm:407: [ns_event modifierFlags] & (NSControlKeyMask | NSCommandKeyMask) && nit: extra parens around the bitwise operation
Added failure to the crbug. https://codereview.chromium.org/2531033003/diff/1/chrome/browser/global_keybo... File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right): https://codereview.chromium.org/2531033003/diff/1/chrome/browser/global_keybo... chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:18: #include "ui/views/test/event_generator_delegate_mac.h" On 2016/11/28 08:14:18, tapted wrote: > I've been trying to avoid introducing ui/views dependencies in the existing > Cocoa codebase -- see the comments in chrome/browser/ui/cocoa/DEPS I thought about doing a local function that does performKeyEquivalent: / sendEvent: locally at first but decided that a more general approach would by DRYer. Removed the dependency, but left the event_generator code intact, as it would still be useful if some other code needs to invoke Cmd+{/} inside a test, and it would provide a more realistic code coverage. https://codereview.chromium.org/2531033003/diff/1/ui/views/test/event_generat... File ui/views/test/event_generator_delegate_mac.mm (right): https://codereview.chromium.org/2531033003/diff/1/ui/views/test/event_generat... ui/views/test/event_generator_delegate_mac.mm:407: [ns_event modifierFlags] & (NSControlKeyMask | NSCommandKeyMask) && On 2016/11/28 08:14:18, tapted wrote: > nit: extra parens around the bitwise operation Done. I think performKeyEquivalent: emulation is a useful thing to have here.
lgtm with an extra comment in ActivateAccelerator() - thanks! https://codereview.chromium.org/2531033003/diff/1/ui/views/test/event_generat... File ui/views/test/event_generator_delegate_mac.mm (right): https://codereview.chromium.org/2531033003/diff/1/ui/views/test/event_generat... ui/views/test/event_generator_delegate_mac.mm:407: [ns_event modifierFlags] & (NSControlKeyMask | NSCommandKeyMask) && On 2016/11/30 12:35:33, themblsha wrote: > On 2016/11/28 08:14:18, tapted wrote: > > nit: extra parens around the bitwise operation > > Done. I think performKeyEquivalent: emulation is a useful thing to have here. I agree - I like it too :) https://codereview.chromium.org/2531033003/diff/40001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right): https://codereview.chromium.org/2531033003/diff/40001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:29: // -performKeyEquivalent: stage, we also need to invoke -sendEvent:. Thanks! this is much clearer. I would add a bit more like, // This is consistent with the way AppKit dispatches events when // -performKeyEquivalent: returns NO. See "The Path of Key Events" in the // Cocoa Event Architecture documentation. (I would not include the URL - https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Ev... - it's too long and Apple keep changing links on their developer site).
https://codereview.chromium.org/2531033003/diff/40001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right): https://codereview.chromium.org/2531033003/diff/40001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:15: #include "ui/base/accelerators/accelerator.h" (oh, and a nit: is this still needed?)
mblsha@yandex-team.ru changed reviewers: + thakis@chromium.org
+thakis Could you please take a look? https://codereview.chromium.org/2531033003/diff/40001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right): https://codereview.chromium.org/2531033003/diff/40001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:15: #include "ui/base/accelerators/accelerator.h" On 2016/11/30 23:53:03, tapted wrote: > (oh, and a nit: is this still needed?) Whoops. I wanted to use ui::Accelerator, but converting between different types of modifiers turned out to be too cumbersome. https://codereview.chromium.org/2531033003/diff/40001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:29: // -performKeyEquivalent: stage, we also need to invoke -sendEvent:. On 2016/11/30 23:52:03, tapted wrote: > Thanks! this is much clearer. I would add a bit more like, > > // This is consistent with the way AppKit dispatches events when > // -performKeyEquivalent: returns NO. See "The Path of Key Events" in the > // Cocoa Event Architecture documentation. > > > > (I would not include the URL - > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Ev... > > - it's too long and Apple keep changing links on their developer site). Done.
Is it possible to test this?
On 2016/12/01 19:19:26, Nico wrote: > Is it possible to test this? To test the updated event_generator_delegate_mac.mm code?
On 2016/12/02 09:57:55, themblsha wrote: > On 2016/12/01 19:19:26, Nico wrote: > > Is it possible to test this? > > To test the updated event_generator_delegate_mac.mm code? Oh, clicking through to the bug it looks like this is to fix an existing test. That suggests this code is already tested :-) lgtm then.
The CQ bit was checked by mblsha@yandex-team.ru
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/2531033003/#ps60001 (title: "Fix tapted's remarks.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480679057651310,
"parent_rev": "a15e2b565260f0ae47efbbd18c431407abd91c7e", "commit_rev":
"d907de4f50ebaf915e805b86d409a945f3a133c6"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac. The old test always called -[NSWindow performKeyEquivalent:] but as of r434436 we need to ensure that both performKeyEquivalent: and keyDown: are called in order for window accelerators to work. BUG=665823 ========== to ========== MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac. The old test always called -[NSWindow performKeyEquivalent:] but as of r434436 we need to ensure that both performKeyEquivalent: and keyDown: are called in order for window accelerators to work. BUG=665823 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac. The old test always called -[NSWindow performKeyEquivalent:] but as of r434436 we need to ensure that both performKeyEquivalent: and keyDown: are called in order for window accelerators to work. BUG=665823 ========== to ========== MacViews: Fix GlobalKeyboardShortcutsTest.SwitchTabsMac. The old test always called -[NSWindow performKeyEquivalent:] but as of r434436 we need to ensure that both performKeyEquivalent: and keyDown: are called in order for window accelerators to work. BUG=665823 Committed: https://crrev.com/2394f02438a9b0c5c8aa20a099ed02d7a5867eba Cr-Commit-Position: refs/heads/master@{#435918} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2394f02438a9b0c5c8aa20a099ed02d7a5867eba Cr-Commit-Position: refs/heads/master@{#435918} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
