[macviews] hotkey handling has many broken edge cases. |
|||||||||||||||||||||||
Issue descriptionLong description of how hotkey handling currently works: The Window Server has the first opportunity to consume keyEquivalents. e.g. cmd+space (spotlight), cmd+tab (changing foreground application). These events never make it to the application. When Chrome receives the keyEquivalent, AppKit will call -[NSWindow performKeyEquivalent:] on the key window, which will forward to -[CommandDispatcher performKeyEquivalent:]. On first glance, the logic in CommandDispatcher would imply the following consumer order: (1) Extensions/Apps (2) The web contents [RenderWidgetHostViewCocoa] (3) keyEquivalents not present in the menu, e.g. cmd+1. [prePerformKeyEquivalent:] (4) NSWindow handling of the keyEquivalent via -[NSWindow performKeyEquivalent] (5) "Handle per-window shortcuts like Esc" [postPerformKeyEquivalent:] (6) Repeat (1) - (5) up the parent window hierarchy. If -[CommandDispatcher performKeyEquivalent:] returns NO, then AppKit will call (7) -[NSMenu performKeyEquivalent:] [most keyEquivalents, e.g. cmd+w, cmd+t, etc. are defined here] If (7) returns NO, then AppKit will call (8) -[NSApplication _handleSymbolicHotKey:] e.g. cmd + ` for intra-window activation (2) requires asynchronous communication with the renderer, so (2) short circuits the first pass of performKeyEquivalent: to send an IPC to the renderer. When the IPC comes back [and the renderer doesn't handle the keyEquivalent], -performKeyEquivalent: is called again. The second pass short circuits right before (2). As a result, any time a RenderWidgetHostViewCocoa is first responder for the window [which is almost always, the primary exception is when the omnibox is focused]: (3), (4), (5) and (6) are never called. (4) is a no-op in Chrome. (5) has always been a no-op, since it only affects the unmodified "esc" key, for which performKeyEquivalent: isn't even called. (6) is called when there's a child window [e.g. permissions bubble], and just forwards up to the parent. (3) actually has some real logic [the logic causing the test to fail], but since it's almost never called, it's not really relevant. The reason Chrome appears to work most of the time is because (2) does a lot more than it first appears: * It early exits if the hotkey is a symbolic hotkey, to allow (8) to fire. * It calls RenderWidgetHostDelegate::PreHandleKeyboardEvent, which calls BrowserFrameMac::PreHandleKeyboardEvent, BrowserView::FindCommandIdForAccelerator and BrowserCommandController::IsReservedCommandOrKey, which are given the option to pre-empt the renderer.
,
May 27 2018
,
May 29 2018
Aside to self: On 10.13.3, symbolic hotkey is determined by the HIToolbox method:
BOOL _IsSymbolicHotKeyEvent(NSEvent*e, uint32_t* out_1, uint8_t* out_2)
The behavior of AppKit in _NSKeyboardUIHandleSymbolicHotKey() is roughly:
BOOL ret = _IsSymbolicHotKeyEvent(NSEvent*e, uint32_t* out_1, uint8_t* out_2)
if (!ret || !*out_2) return;
switch (*out_1) {
<giant switch statement with many cases, examples include window cycling, help, dictionary, etc.>
}
,
May 30 2018
Here's how hotkey dispatch should work: (0) if the event is a symbolic hotkey, [CommandDispatcher performKeyEquivalent:] should return false to let AppKit handle it. (1) Give extensions/apps a chance to handle the hotkey. (2) CommandDispatcher should look for a matching menu item in the main NSMenu [making sure to call menuNeedsUpdate: on each submenu]. This will pick up user config changes to the hot key for each menu item. (2a) If a menu item matches, and the corresponding command is reserved (see BrowserCommandController::IsReservedCommandOrKey) CommandDispatcher should handle the command directly and return true. Aside: This makes the assumption that the concept of reserved commands applies equally to the WebContents, or other focusable views [e.g. omnibox]. (3) CommandDispatcher should look for a matching non-menu command [e.g. cmd + 1] for the hotkey. If this command is reserved, then CommandDispatcher should handle the command directly and return true. Aside: There are currently no reserved commands in this category. (4) The command should be sent to the active view [WebContents, omnibox, etc.] (4a) If the active view is a WebContents, this event must be sent asynchronously and then redispatched if the event was not handled. (5) Repeat (2) and (3) but handle the command regardless of whether it is reserved. (6) Bubble the hotkey to the parent.
,
May 31 2018
Thanks for documenting this Erik! We also have https://www.chromium.org/developers/design-documents/command-dispatch-mac but things may have changed a bit. Some notes. At step 0., There may be some added complexity to handle Issue 680809 ("system keyboard lock" - used for Chrome Remote Desktop basically). e.g. Cmd+` needs to go to WebContents "sometimes". Ideally Cmd+Tab is also sent, but there's no way to do that without being an Assistive application on macOS. There's also a step... "-1" say, which gives extensions an opportunity to drive hotkeys when Chrome does not have application focus. These are intercepted via an EventTap and suppressed early; before [NSApp sendEvent:] sees it. And there's an entire phase that precedes performKeyEquivalent: where AppKit does validateUserInterfaceItem. This can cause (2) or (5a) to ignore some commands since the menu item was disabled. There's also a step.. I guess between 2 and 3 which is the command handling (versus dispatching). Step 2 (NSMenu lookup) maps an NSEvent to a NSMenuItem's -action and -target. Usually target is `nil` which causes AppKit to follow the responder chain until it finds something that respondsToSelector(action). *Most* of Chrome's NSMenuItems use 'commandDispatch:` for the action, and this gets picked up either by NativeWidgetMacNSWindow or ChromeEventProcessingWindow . But commandDispatch: is super-overloaded - child windows have no option but to say "yes", even if they actually dispatch the command to their parent. And *some* NSMenuItems do not use commandDispatch:. E.g. IDS_CLOSE_WINDOW_MAC sends performClose: (most things in the Edit menu are also "real" actions). I think this can interfere with the command bubbling. It's possible we can restore some of our sanity by making all mainMenu items use commandDispatch: .. This could even involve phasing out MainMenu.xib entirely
,
May 31 2018
Thanks tapted. > We also have https://www.chromium.org/developers/design-documents/command-dispatch-mac I think I wrote that. ;) > At step 0., There may be some added complexity to handle Issue 680809 ("system keyboard lock" Thanks, I hadn't known about that. > There's also a step... "-1" say, which gives extensions an opportunity to drive hotkeys when Chrome does not have application focus. Do you have a link? The only global tap I could find was for mouse events: https://cs.chromium.org/chromium/src/ui/views/cocoa/cocoa_mouse_capture.mm?l=101 > And there's an entire phase that precedes performKeyEquivalent: where AppKit does validateUserInterfaceItem. I don't think we're on the same page here. validateUserInterfaceItem: is called prior to invoking an NSMenuItem. See https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/MenuList/Articles/EnablingMenuItems.html. > There's also a step.. I guess between 2 and 3 which is the command handling (versus dispatching). How are you defining the difference between "command handling" and "dispatching"? You're describing the default handling when an NSMenuItem is selected, which we'll need to support regardless since a user can always select an NSMenuItem using the mouse. I'm only concerned with hotkey handling. > And *some* NSMenuItems do not use commandDispatch: I'm not sure how this is relevant. You're describing what happens after a user clicks an NSMenuItem, or one is invoked via [NSMenu performKeyEquivalent:]. I'm interested in everything that happens before that. > It's possible we can restore some of our sanity by making all mainMenu items use commandDispatch: .. This could even involve phasing out MainMenu.xib entirely MainMenu.xib is red herring. Regardless of whether we have a .xib file, we need to configure the main menu to have the appropriate items.
,
Jun 1 2018
>> There's also a step... "-1" say, which gives extensions an opportunity to drive hotkeys when Chrome does not have application focus. >Do you have a link? The only global tap I could find was for mouse events: https://cs.chromium.org/chromium/src/ui/views/cocoa/cocoa_mouse_capture.mm?l=101 There's a Carbon event tap for media keys: http://cs.chromium.org/MediaKeysListenerImpl::StartWatchingMediaKeys() which is part of the `GlobalShortcutListener` for extensions, which is using .. a different carbon event handling API `InstallEventHandler and EventHandlerUPP` http://cs.chromium.org/GlobalShortcutListenerMac::StartWatchingHotKeys() >> And there's an entire phase that precedes performKeyEquivalent: where AppKit does validateUserInterfaceItem. > I don't think we're on the same page here. validateUserInterfaceItem: is called prior to invoking an NSMenuItem. See https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/MenuList/Articles/EnablingMenuItems.html. I think it's also part of searching for a menu item to invoke? E.g. the ([[NSApp mainMenu] performKeyEquivalent:event]) call in -[BrowserWindowUtils handleKeyboardEvent:inWindow:] will skip over items that validate with `NO`. (IIRC, a menuNeedsUpdate: happens early in NSMenu's -performKeyEquivalent: implementation). > How are you defining the difference between "command handling" and "dispatching"? dispatching is sending the event to the valid targets. Handling is what those target[s] choose to do with it (which may involve indicating to the dispatcher to stop looking for more targets). I guess I've got the Aura interfaces on my brain: ui::EventTarget, ui::EventDispatcher, ui::EventHandler (ui::EventProcessor.. ui::EventSource.. ui::EventSink.., | xargs sed -e 's/.*/&Delegate/g' ) Aura also has an explicit ui::EventTargeter which happens before dispatch (I guess -[NSView hitTest:] is the equivalent) > I'm interested in everything that happens before that. I think validation is part of what happens before. If validateUserInterfaceItem disables a menu item with a keyEquivalent, nothing will try to send the corresponding key event to that menu item.
,
Jun 1 2018
More notes to self about consumers of hotkeys: There are several mechanisms by which various features have found ways to get first access to NSEvents. By themselves, they mostly work correctly, but paired together, they rarely have the correct consumer ordering. The ordering is mostly dependent on where they chose to hook the events. * Global Cocoa/Carbon event taps (media keys, GlobalShortcutListener for extensions) * Normal extension bindings (via [BrowserWindowController handledByExtensionCommand:priority:] * System keyboard lock * KeyPressListener [Autofill] * CommandDispatcher [for NSMenu and non-NSMenu chrome hotkeys] * render_widget_host_view_cocoa.mm: EventIsReservedBySystem [for symbolic hotkeys] * RenderWidgetHostDelegate::PreHandleKeyboardEvent [similar to CommandDispatcher, but another implementation, slightly different].
,
Jun 1 2018
And 1 more: there's a developer API for extensions: developerPrivate.setShortcutHandlingSuspended, which hooks via ExtensionKeybindingRegistry::SetShortcutHandlingSuspended.
,
Jun 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f2f9568808ee117698e0a9008a0b52902d2363e commit 8f2f9568808ee117698e0a9008a0b52902d2363e Author: Erik Chen <erikchen@chromium.org> Date: Fri Jun 08 00:44:54 2018 Unification of macOS keyEquivalent (hotkey) handling. This CL makes the CommandDispatcher class responsible for all NSMenu-related keyEquivalent handling. The logic is now shared by both Views and Cocoa. This fixes many Views bugs, and some Cocoa bugs. Previously, there several different pieces of logic for handling NSMenu-related keyEquivalents. * If a RenderWidgetHostViewCocoa was the firstResponder, then all the logic in CommandDispatcher was skipped. * In both Cocoa and Views, there was logic in content/ via RenderWidget* classes that would short-circuit but also fail to handle the keyEquivalent. [e.g. if renderer process was killed]. * In Cocoa, content/ would call into BrowserWindowCocoa::PreHandleKeyboardEvent(). This had mostly the right behavior, except it missed out on edge-case logic in CommandDispatcher. * In Views, content/ would call into BrowserView::PreHandleKeyboardEvent. This relied on AcceleratorTable to lookup the command for a keyEquivalent. This entire class is unusable for keyEquivalents because it holds a static mapping, whereas on macOS, the mapping is user configurable and dynamic. * If the hotkey was not immediately consumed, it would be passed to the renderer process. If the renderer process chose not to consume the hotkey, the browser process would get it again. * In Cocoa, the event would be redispatched to CommandDispatcher. This had the right behavior. * In Views, the event would be sent to AcceleratorTable again, once again having the wrong behavior. * If any other class was firstResponder, then the logic in CommandDispatcher would trigger. This CL makes CommandDispatcher the only place to handle NSMenu-related keyEquivalents. This allows us to centralize the logic, including workarounds. New logic: * The CommandDispatcherDelegate is given an opportunity to process the keyEquivalent via prePerformKeyEquivalent:. If the keyEquivalent is reserved by the browser, it is immediately handled. * Next, the firstResponder is given a chance to process the keyEquivalent. If the firstResponder is a RenderWidgetHostViewCocoa, this goes through the same code paths as above, except BrowserWindowCocoa::PreHandleKeyboardEvent() is mostly a no-op. * Then, the CommandDispatcherDelegate gets another chance to process the keyEquivalent via postPerformKeyEquivalent:. If the firstResponder was a RenderWidgetHostViewCocoa which asynchronously chose to not process the event, then both Cocoa and Views will take the returning event and call redispatchEvent:, which will also call into -[CommandDispatcherDelegate postPerformKeyEquivalent:]. Change-Id: If724e125c6acf64ddd9f1d9cc296e761ffb2a886 Bug: 846893 , 836947 , 845465 , 47134 Reviewed-on: https://chromium-review.googlesource.com/1082818 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#565490} [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/accelerators_cocoa.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/accelerators_cocoa.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_interactive_uitest.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/browser_window_utils.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table_unittest_mac.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_ash.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_ash.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mac.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mac.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mash.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mash.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_view_unittest.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/desktop_browser_frame_aura.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/native_browser_frame.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/media_router/presentation_receiver_window_view.cc [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/test/BUILD.gn [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/content/browser/renderer_host/render_widget_host_view_cocoa.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/base/cocoa/command_dispatcher.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/base/cocoa/command_dispatcher.mm [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/events/keycodes/keyboard_code_conversion_mac.h [modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/events/keycodes/keyboard_code_conversion_mac.mm
,
Jun 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/315d4286bf50511d859f1c4f205966f4bccc5c22 commit 315d4286bf50511d859f1c4f205966f4bccc5c22 Author: Erik Chen <erikchen@chromium.org> Date: Fri Jun 08 20:50:42 2018 Change IDC_DEV_TOOLS_INSPECT to use virtual keycode on macOS. The mapping was added in https://codereview.chromium.org/3011002. There was no reason for it to use a character rather than a virtual keycode. After a recent refactor, this logic caused cmd + c to trigger IDC_DEV_TOOLS_INSPECT, since the shift key is ignored for character comparisons. Bug: 846893 , 850895 Change-Id: Ic74e418bd9fa55eda90dd3bd55b10a456abce581 Reviewed-on: https://chromium-review.googlesource.com/1093301 Commit-Queue: Erik Chen <erikchen@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#565735} [modify] https://crrev.com/315d4286bf50511d859f1c4f205966f4bccc5c22/chrome/browser/global_keyboard_shortcuts_mac.mm
,
Jun 11 2018
,
Jun 11 2018
,
Jun 11 2018
,
Jun 11 2018
,
Jun 13 2018
,
Jun 15 2018
,
Jun 15 2018
,
Jun 19 2018
,
Jun 19 2018
Triage: Marking as started
,
Jun 22 2018
,
Jun 22 2018
,
Jun 22 2018
,
Jul 2
,
Jul 10
,
Jul 12
,
Jul 12
,
Jul 12
,
Jul 12
,
Jul 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b commit 2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b Author: Erik Chen <erikchen@chromium.org> Date: Mon Jul 16 21:30:54 2018 Remove PlatformAccelerator. ui::Accelerator had a member platform_accelerator() which was only used on macOS, and provided redundant information. This CL removes the member. Change-Id: Ib3954a2c8ff7197606c946e207dd5d3bd75af66d Bug: 846893 , 702823 Reviewed-on: https://chromium-review.googlesource.com/1135644 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#575429} [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/global_keyboard_shortcuts_mac.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/accelerators_cocoa.h [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/accelerators_cocoa.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm [delete] https://crrev.com/0131dcca85a720678ded88d843088de9e0e263c8/chrome/browser/ui/cocoa/accelerators_cocoa_unittest.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/app_menu/app_menu_controller.h [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/confirm_quit_panel_controller.h [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/confirm_quit_panel_controller_unittest.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/share_menu_controller.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/toolbar/reload_button_cocoa.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/test/BUILD.gn [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/BUILD.gn [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/accelerator.cc [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/accelerator.h [delete] https://crrev.com/0131dcca85a720678ded88d843088de9e0e263c8/ui/base/accelerators/platform_accelerator.h [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/platform_accelerator_cocoa.h [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/platform_accelerator_cocoa.mm [modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/cocoa/menu_controller.mm
,
Jul 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b8b4f583548218b816cff7bb4dbfef27ed81a56 commit 7b8b4f583548218b816cff7bb4dbfef27ed81a56 Author: Xiaocheng Hu <xiaochengh@chromium.org> Date: Mon Jul 16 23:46:51 2018 Revert "Remove PlatformAccelerator." This reverts commit 2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b. Reason for revert: Suspected to cause failure in AcceleratorsCocoaBrowserTest.MainMenuAcceleratorsInMapping Original change's description: > Remove PlatformAccelerator. > > ui::Accelerator had a member platform_accelerator() which was only used on > macOS, and provided redundant information. This CL removes the member. > > Change-Id: Ib3954a2c8ff7197606c946e207dd5d3bd75af66d > Bug: 846893 , 702823 > Reviewed-on: https://chromium-review.googlesource.com/1135644 > Commit-Queue: Erik Chen <erikchen@chromium.org> > Reviewed-by: Nico Weber <thakis@chromium.org> > Cr-Commit-Position: refs/heads/master@{#575429} TBR=thakis@chromium.org,erikchen@chromium.org Change-Id: I3f142628a1ab406028a5f64db7d6acdf13c4d41b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 846893 , 702823 , 864272 Reviewed-on: https://chromium-review.googlesource.com/1139153 Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> Cr-Commit-Position: refs/heads/master@{#575484} [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/global_keyboard_shortcuts_mac.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa.h [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm [add] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa_unittest.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/app_menu/app_menu_controller.h [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/confirm_quit_panel_controller.h [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/confirm_quit_panel_controller_unittest.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/share_menu_controller.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/toolbar/reload_button_cocoa.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/test/BUILD.gn [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/BUILD.gn [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/accelerator.cc [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/accelerator.h [add] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/platform_accelerator.h [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/platform_accelerator_cocoa.h [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/platform_accelerator_cocoa.mm [modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/cocoa/menu_controller.mm
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e262e854ea05600f16a13f771bcf62bac25c6337 commit e262e854ea05600f16a13f771bcf62bac25c6337 Author: erikchen <erikchen@chromium.org> Date: Wed Jul 18 23:12:05 2018 [Reland #1] Remove PlatformAccelerator. The test AcceleratorsCocoaBrowserTest.MainMenuAcceleratorsInMapping had an incorrect early return, where it intended to use a continue. The first attempt to land this CL changed the early return to a continue. This broke the test. This CL fixes the test by: * Not performing any checks on menu items without tags, as they may just be macOS menu items added by the OS that we don't care about. * Adding missing accelerator combinations to AcceleratorsCocoa. This CL also removes some dead code in accelerators_cocoa.mm > ui::Accelerator had a member platform_accelerator() which was only used on > macOS, and provided redundant information. This CL removes the member. > > Bug: 846893 , 702823 > Reviewed-on: https://chromium-review.googlesource.com/1135644 > Commit-Queue: Erik Chen <erikchen@chromium.org> > Reviewed-by: Nico Weber <thakis@chromium.org> > Cr-Commit-Position: refs/heads/master@{#575429} Change-Id: I195bd174a96da58bff96e22495c5ea5b5bfe8549 Bug: 846893 , 702823 Reviewed-on: https://chromium-review.googlesource.com/1140211 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#576244} [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/global_keyboard_shortcuts_mac.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/accelerators_cocoa.h [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/accelerators_cocoa.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm [delete] https://crrev.com/e923a58c970eebeeba7d71f268346377e83efe1f/chrome/browser/ui/cocoa/accelerators_cocoa_unittest.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/app_menu/app_menu_controller.h [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/confirm_quit_panel_controller.h [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/confirm_quit_panel_controller_unittest.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/share_menu_controller.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/toolbar/reload_button_cocoa.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/test/BUILD.gn [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/BUILD.gn [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/accelerator.cc [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/accelerator.h [delete] https://crrev.com/e923a58c970eebeeba7d71f268346377e83efe1f/ui/base/accelerators/platform_accelerator.h [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/platform_accelerator_cocoa.h [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/platform_accelerator_cocoa.mm [modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/cocoa/menu_controller.mm
,
Jul 19
,
Jul 19
[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.
,
Jul 19
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by erikc...@chromium.org
, May 25 2018