Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
60353008:
Mac global keybindings (Closed)
|
Created:
7 years, 1 month ago by Boris Smus Modified:
7 years ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Mark Mentovai Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMac-specific implementation of the GlobalShortcutListener class that
listens for global shortcuts. Handles basic keyboard intercepting and
forwards its output to the base class for processing.
Adds two things:
1. Intercepts media keys. Uses an event tap for intercepting media keys
(PlayPause, NextTrack, PreviousTrack).
2. Binds keyboard shortcuts (hot keys). Carbon RegisterEventHotKey API for
binding to non-media key global hot keys (eg. Command-Shift-1).
Also added Mac tests.
BUG=302437
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240536
Patch Set 1 #
Total comments: 14
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 106
Patch Set 6 : #
Total comments: 26
Patch Set 7 : #
Total comments: 14
Patch Set 8 : #
Total comments: 36
Patch Set 9 : #
Total comments: 18
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 4
Patch Set 14 : #
Total comments: 36
Patch Set 15 : #
Total comments: 4
Patch Set 16 : #
Total comments: 41
Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #
Total comments: 1
Patch Set 20 : #
Total comments: 51
Patch Set 21 : #
Total comments: 16
Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #
Total comments: 24
Patch Set 25 : #
Total comments: 1
Patch Set 26 : #Patch Set 27 : Running try servers #Messages
Total messages: 71 (0 generated)
Added missing files.
In general, I like very much what I am seeing. I can't vouch for the correctness of the approach (specifically the OS calls to listen to the keyboard shortcuts), so you'd have to get a Mac specialist to take a look. But from a Commands API standpoint, this looks very promising. https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: NSAutoreleasePool *pool = [NSAutoreleasePool new]; nit: We prefer Foo* foo over Foo *foo. (multiple occurrences) https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:125: // Prevent having multiple mediaKeys threads nit: In general, we end all comments in period (namespace annotations and function static annotations are an exception). https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:126: [self stopWatchingMediaKeys]; Thinking out loud: Doesn't starting twice indicate a logic error in the implementation? Shouldn't we be asserting in that case? https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:145: - (void)stopWatchingMediaKeys { nit: linebreak https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:185: //int keyRepeat = (keyFlags & 0x1); Remove? https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:190: } nit: When the if clause is single line we omit the braces. https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:195: nit: lots of extra linebreaks.
New mac implementation uses RegisterEventHotKey for regular key bindings, and also has an event tap for latching on to media keys. This CL should also have a chrome for mac expert take a look. Finnur, any ideas? https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: NSAutoreleasePool *pool = [NSAutoreleasePool new]; On 2013/11/07 10:33:07, Finnur wrote: > nit: We prefer Foo* foo over Foo *foo. (multiple occurrences) Done. https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:125: // Prevent having multiple mediaKeys threads On 2013/11/07 10:33:07, Finnur wrote: > nit: In general, we end all comments in period (namespace annotations and > function static annotations are an exception). Done. https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:126: [self stopWatchingMediaKeys]; On 2013/11/07 10:33:07, Finnur wrote: > Thinking out loud: Doesn't starting twice indicate a logic error in the > implementation? Shouldn't we be asserting in that case? Yep, changed to an assertion. https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:145: - (void)stopWatchingMediaKeys { On 2013/11/07 10:33:07, Finnur wrote: > nit: linebreak Done. https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:185: //int keyRepeat = (keyFlags & 0x1); On 2013/11/07 10:33:07, Finnur wrote: > Remove? Done. https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:190: } On 2013/11/07 10:33:07, Finnur wrote: > nit: When the if clause is single line we omit the braces. Not my favorite, but I will comply :) https://codereview.chromium.org/60353008/diff/1/chrome/browser/extensions/glo... chrome/browser/extensions/global_shortcut_listener_mac.mm:195: On 2013/11/07 10:33:07, Finnur wrote: > nit: lots of extra linebreaks. Done.
Overall, with the caveat of my limited Mac experience, I think the approach is promising (from a Commands API perspective). I have a lot of small comments, nothing serious. You can probably clean it up (remove my LOG statements and guidance comments) and send it out for a Mac expert review. Speaking of which... > This CL should also have a chrome for mac expert > take a look. Finnur, any ideas? Nico (thakis@) has been reviewing some of the Mac Commands stuff. He's probably a good starting point and can forward to someone more appropriate if not the right candidate. Oh, you need a BUG= for this (perhaps crbug.com/302437?) https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:37: nit: Remove line break. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:58: int hotkey_id = 0; nit: Add line break and comment. Also, why not trailing underscore? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:61: // TODO: Consider using a bimap structure instead. style: All TODOs need an actor, e.g.: // TODO(smus): Lorum ipsus. (same goes for .mm file) https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:67: // A map of hot key refs so they can be unregistered later. nit: hot key? or hotkey? :) https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:69: IdHotkeyRefMap id_hotkey_refs_; That's a lot of maps, Mr. Magellan. It would be good if you could document why we need so many (and what each is used for, media keys only? all keys?). https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:71: nit: Remove line break. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:72: // A pointer to the global shortcut listener tap. This is kind of self-evident from the code. It would be more apt to describe what you use it for (media keys? other keys? all keys?) https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:73: withEvent:nsEvent]; As I recall, from my brief exposure to Mac, the colons need to align... Maybe I'm wrong. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:78: } Yup, you guessed it (single-line if, no braces) :) https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:121: return ui::VKEY_MEDIA_NEXT_TRACK; Is there no 'Stop' event? For example, see Chaobin's patch: https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension... https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:206: LOG(ERROR) << "GlobalShortcutListenerMac object created"; You can remove this and other LOG(ERROR) statements added. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:250: } nit: Single line. :) https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:253: // 4) Else, call base class RegisterAccelerator. You can remove these comments, they were only meant as convenience for the implementor. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:266: // 2) Call base class UnregisterAccelerator. Same here. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:267: bool isMediaKey = (accelerator.modifiers() == 0); Chaobin is adding a static IsMediaKey function to the CommandService, we should probably use that? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:271: } nit: Single line, your favorite. :) https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:346: // TODO: Move away from hardcoded CMD + Enter event. What does this mean? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:350: id_hotkey_refs_[hotkey_id] = hotKeyRef; Don't you need to advance hotkey_id now? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:364: nit: Extra line break.
Also, consider the tests that have been (or are being) added for this feature along with other platform-specific implementations and see if you can get them working on Mac. extension_commands_global_registry_apitest.cc (don't forget to look at Chaobin's in-flight CL, https://codereview.chromium.org/64273008/, as there are some more tests on the way there)
not lgtm. Please write a more substantial CL description and add a BUG=. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:10: #include <set> #include <map> since you use it. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:11: #include <IOKit/hidsystem/ev_keymap.h> IOKit/IOKit.h Does this need to be in the .h? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:12: #include <Carbon/Carbon.h> Same. Does this need to be in the .h? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:14: #include "ApplicationServices/ApplicationServices.h" This is a system header, so it should be in <>. Does this need to be in the .h though, or can it be in the .mm? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:25: // TODO(finnur/smus): Implement this class. This comment should be revised. It should also detail how to use this class. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:53: void RegisterHotKey(ui::Accelerator accelerator); Make these be const& parameters. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:14: #define SystemDefinedEventMediaKeys 8 Use type-safe C++ constants. Do not use #define for constants. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:18: typedef extensions::GlobalShortcutListenerMac GSL; This is not an appropriate typedef. Use |using extensions::GlobalShortcutListener| and refer to it by its actual typename. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:21: @public nit: one space before @public. But, I'm not sure you should have public ivars at all. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:27: GSL* gsl_; Similarly, this is not an appropriate variable name. |shortcutListener_|? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:40: static CGEventRef tapEventCallback( naming: TapEventCallback You mix and match anonymous namespace and static. The preference is generally for anonymous namespace. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: CGEventRef ret = event; Naming: |ret|? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:45: GlobalShortcutListenerTap* self = (GlobalShortcutListenerTap*) refcon; C-style casts are banned. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; Why do you need to convert this to a NSEvent? Can't you just use the CGEvent functions to get this exact same information? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; Naming: in C/C++, use under_scores for variables. Only in @interface ivars and @implementation methods do you use camelCase. Here and throughout this file. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:88: gsl_ = gsl; if ((self = [super init])) { shortcutListener_ = shortcutListener; } return self; https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:92: - (void)eventTapThread { Yikes – an entire thread just to listen for global keyboard shortcuts! Threads aren't free; is this really necessary? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:128: assert(eventTap_ == NULL); No assert(). Use Chromium's logging facilities for this. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:131: eventTap_ = CGEventTapCreate(kCGSessionEventTap, Per https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz...: This will only work if Chrome is running as root (it shouldn't be) or if Universal Access/Assistive Devices is turned on. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:183: using content::BrowserThread; This belongs after the #includes, not here. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:187: static base::LazyInstance<extensions::GlobalShortcutListenerMac> instance = Naming: g_instance https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:211: GlobalShortcutListenerMac::~GlobalShortcutListenerMac() { What about all the registered hotkeys? Won't those listeners be leaked? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:216: void GlobalShortcutListenerMac::StartListening() { The Start/StopListeneing pair of methods only affect media keys, not general hotkeys. Should they? If not, this deserves a comment. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:285: instance.Get().NotifyKeyPressed(accelerator); Why are you not using the |this| pointer? This method isn't static. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:303: OSStatus HotKeyHandler(EventHandlerCallRef nextHandler, EventRef theEvent, This doesn't belong here. This belongs with your other callback functions in the anonymous namespace below the @interface. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:303: OSStatus HotKeyHandler(EventHandlerCallRef nextHandler, EventRef theEvent, naming: You use HotKey here but Hotkey elsewhere. Please pick one and be consistent. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:329: // TODO: Understand what signature means. I think this is important to understand before committing.
https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:131: eventTap_ = CGEventTapCreate(kCGSessionEventTap, On 2013/11/18 15:25:03, rsesek wrote: > Per > https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz...: > > This will only work if Chrome is running as root (it shouldn't be) or if > Universal Access/Assistive Devices is turned on. Sorry, this isn't accurate. This code isn't tapping raw key events, but rather system events. FWICT, this should be fine.
PTAL. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:10: #include <set> On 2013/11/18 15:25:03, rsesek wrote: > #include <map> since you use it. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:11: #include <IOKit/hidsystem/ev_keymap.h> On 2013/11/18 15:25:03, rsesek wrote: > IOKit/IOKit.h > > Does this need to be in the .h? Moved to .mm, but unfortunately IOKit/IOKit.h doesn't exist. Nor does IOKit/IOKitLib.h include the NX_ constants I need. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:12: #include <Carbon/Carbon.h> On 2013/11/18 15:25:03, rsesek wrote: > Same. Does this need to be in the .h? Yes, this provides EventHotKeyRef https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:14: #include "ApplicationServices/ApplicationServices.h" On 2013/11/18 15:25:03, rsesek wrote: > This is a system header, so it should be in <>. Does this need to be in the .h > though, or can it be in the .mm? Moved to .mm and changed to <> syntax. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:25: // TODO(finnur/smus): Implement this class. On 2013/11/18 15:25:03, rsesek wrote: > This comment should be revised. It should also detail how to use this class. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:37: On 2013/11/18 11:55:57, Finnur wrote: > nit: Remove line break. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:53: void RegisterHotKey(ui::Accelerator accelerator); On 2013/11/18 15:25:03, rsesek wrote: > Make these be const& parameters. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:58: int hotkey_id = 0; On 2013/11/18 11:55:57, Finnur wrote: > nit: Add line break and comment. > Also, why not trailing underscore? Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:61: // TODO: Consider using a bimap structure instead. On 2013/11/18 11:55:57, Finnur wrote: > style: All TODOs need an actor, e.g.: > // TODO(smus): Lorum ipsus. > > (same goes for .mm file) Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:67: // A map of hot key refs so they can be unregistered later. On 2013/11/18 11:55:57, Finnur wrote: > nit: hot key? or hotkey? :) hotkey!!! https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:69: IdHotkeyRefMap id_hotkey_refs_; On 2013/11/18 11:55:57, Finnur wrote: > That's a lot of maps, Mr. Magellan. It would be good if you could document why > we need so many (and what each is used for, media keys only? all keys?). Ok, documented. I could have used one fewer map but that would have meant doing linear search. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:71: On 2013/11/18 11:55:57, Finnur wrote: > nit: Remove line break. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:72: // A pointer to the global shortcut listener tap. On 2013/11/18 11:55:57, Finnur wrote: > This is kind of self-evident from the code. It would be more apt to describe > what you use it for (media keys? other keys? all keys?) Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:14: #define SystemDefinedEventMediaKeys 8 On 2013/11/18 15:25:03, rsesek wrote: > Use type-safe C++ constants. Do not use #define for constants. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:18: typedef extensions::GlobalShortcutListenerMac GSL; On 2013/11/18 15:25:03, rsesek wrote: > This is not an appropriate typedef. Use |using > extensions::GlobalShortcutListener| and refer to it by its actual typename. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:21: @public On 2013/11/18 15:25:03, rsesek wrote: > nit: one space before @public. But, I'm not sure you should have public ivars at > all. Removed @public member. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:27: GSL* gsl_; On 2013/11/18 15:25:03, rsesek wrote: > Similarly, this is not an appropriate variable name. |shortcutListener_|? Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:40: static CGEventRef tapEventCallback( On 2013/11/18 15:25:03, rsesek wrote: > naming: TapEventCallback > > You mix and match anonymous namespace and static. The preference is generally > for anonymous namespace. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: CGEventRef ret = event; On 2013/11/18 15:25:03, rsesek wrote: > Naming: |ret|? Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:45: GlobalShortcutListenerTap* self = (GlobalShortcutListenerTap*) refcon; On 2013/11/18 15:25:03, rsesek wrote: > C-style casts are banned. Changed to static_cast. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; On 2013/11/18 15:25:03, rsesek wrote: > Why do you need to convert this to a NSEvent? Can't you just use the CGEvent > functions to get this exact same information? I looked for a way of doing this from the CGEvent, but could not find a good way of getting at the illusive data1 field that NSEvent exposes. Any ideas how to do this? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; On 2013/11/18 15:25:03, rsesek wrote: > Naming: in C/C++, use under_scores for variables. Only in @interface ivars and > @implementation methods do you use camelCase. Here and throughout this file. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:73: withEvent:nsEvent]; On 2013/11/18 11:55:57, Finnur wrote: > As I recall, from my brief exposure to Mac, the colons need to align... Maybe > I'm wrong. Couldn't confirm that from http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:78: } On 2013/11/18 11:55:57, Finnur wrote: > Yup, you guessed it (single-line if, no braces) :) Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:88: gsl_ = gsl; On 2013/11/18 15:25:03, rsesek wrote: > if ((self = [super init])) { > shortcutListener_ = shortcutListener; > } > return self; Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:92: - (void)eventTapThread { On 2013/11/18 15:25:03, rsesek wrote: > Yikes – an entire thread just to listen for global keyboard shortcuts! Threads > aren't free; is this really necessary? I think this is the best practice. Doing this on the main thread risks blocking it. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:121: return ui::VKEY_MEDIA_NEXT_TRACK; On 2013/11/18 11:55:57, Finnur wrote: > Is there no 'Stop' event? > > For example, see Chaobin's patch: > https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension... Mac keyboards don't have this button. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:128: assert(eventTap_ == NULL); On 2013/11/18 15:25:03, rsesek wrote: > No assert(). Use Chromium's logging facilities for this. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:131: eventTap_ = CGEventTapCreate(kCGSessionEventTap, On 2013/11/18 15:25:03, rsesek wrote: > Per > https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz...: > > This will only work if Chrome is running as root (it shouldn't be) or if > Universal Access/Assistive Devices is turned on. Self-addressed. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:131: eventTap_ = CGEventTapCreate(kCGSessionEventTap, On 2013/11/18 15:25:03, rsesek wrote: > Per > https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz...: > > This will only work if Chrome is running as root (it shouldn't be) or if > Universal Access/Assistive Devices is turned on. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:183: using content::BrowserThread; On 2013/11/18 15:25:03, rsesek wrote: > This belongs after the #includes, not here. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:187: static base::LazyInstance<extensions::GlobalShortcutListenerMac> instance = On 2013/11/18 15:25:03, rsesek wrote: > Naming: g_instance Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:206: LOG(ERROR) << "GlobalShortcutListenerMac object created"; On 2013/11/18 11:55:57, Finnur wrote: > You can remove this and other LOG(ERROR) statements added. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:211: GlobalShortcutListenerMac::~GlobalShortcutListenerMac() { On 2013/11/18 15:25:03, rsesek wrote: > What about all the registered hotkeys? Won't those listeners be leaked? Following other implementations (X11 and Win). I think the way it works is all registered accelerators should be unregistered first (via UnregisterAccelerator), and only then does this class get destroyed. cc: finnur@ https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:216: void GlobalShortcutListenerMac::StartListening() { On 2013/11/18 15:25:03, rsesek wrote: > The Start/StopListeneing pair of methods only affect media keys, not general > hotkeys. Should they? If not, this deserves a comment. Added clarifying comment. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:250: } On 2013/11/18 11:55:57, Finnur wrote: > nit: Single line. :) Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:253: // 4) Else, call base class RegisterAccelerator. On 2013/11/18 11:55:57, Finnur wrote: > You can remove these comments, they were only meant as convenience for the > implementor. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:266: // 2) Call base class UnregisterAccelerator. On 2013/11/18 11:55:57, Finnur wrote: > Same here. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:267: bool isMediaKey = (accelerator.modifiers() == 0); On 2013/11/18 11:55:57, Finnur wrote: > Chaobin is adding a static IsMediaKey function to the CommandService, we should > probably use that? Oh excellent, I added my own for now. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:271: } On 2013/11/18 11:55:57, Finnur wrote: > nit: Single line, your favorite. :) Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:285: instance.Get().NotifyKeyPressed(accelerator); On 2013/11/18 15:25:03, rsesek wrote: > Why are you not using the |this| pointer? This method isn't static. Fair point (I think). If so, same should apply to _win and _x11: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:303: OSStatus HotKeyHandler(EventHandlerCallRef nextHandler, EventRef theEvent, On 2013/11/18 15:25:03, rsesek wrote: > This doesn't belong here. This belongs with your other callback functions in the > anonymous namespace below the @interface. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:303: OSStatus HotKeyHandler(EventHandlerCallRef nextHandler, EventRef theEvent, On 2013/11/18 15:25:03, rsesek wrote: > naming: You use HotKey here but Hotkey elsewhere. Please pick one and be > consistent. Done. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:329: // TODO: Understand what signature means. On 2013/11/18 15:25:03, rsesek wrote: > I think this is important to understand before committing. Agreed. Added a comment explaining what it is. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:346: // TODO: Move away from hardcoded CMD + Enter event. On 2013/11/18 11:55:57, Finnur wrote: > What does this mean? Stale comment. Removed. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:350: id_hotkey_refs_[hotkey_id] = hotKeyRef; On 2013/11/18 11:55:57, Finnur wrote: > Don't you need to advance hotkey_id now? Added a comment explaining (it happens in the caller -- kinda ugly, not sure what to do about it). https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:364: On 2013/11/18 11:55:57, Finnur wrote: > nit: Extra line break. Done.
https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:211: GlobalShortcutListenerMac::~GlobalShortcutListenerMac() { Yes, we'll get calls to Unregister before the object is destroyed and the StopListening call DCHECKs if there's anything remaining on shutdown. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:216: void GlobalShortcutListenerMac::StartListening() { So, this Start/StopListening pair is meant as a convenience for platforms that can turn the faucet on (when getting the first accelerator) and off (when removing the last one). For example, Windows Chrome uses these events to register for an observer on SingletonHWND on Start and remove it on Stop. In other words, the Start/Stop pair avoids the need to check specifically whenever you add a shortcut whether it is the first shortcut to be added (so that the faucet needs to be turned on). This isn't quite as clean on Mac because you have a split system (one for MediaKeys and another for the rest). If you start watching Media Keys here then you end up in a situation where you might have extensions register only Non-MediaKey global shortcuts but you fire up the thread and start listening for MediaKeys anyway. I think you probably need to keep track of whether you've started the media key loop and start it on seeing the first MediaKey shortcut (and stop it when the last is removed). https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:285: instance.Get().NotifyKeyPressed(accelerator); Yeah, looks like. On 2013/11/18 23:13:41, smus wrote: > On 2013/11/18 15:25:03, rsesek wrote: > > Why are you not using the |this| pointer? This method isn't static. > > Fair point (I think). If so, same should apply to _win and _x11: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:26: // global keyboard shortcuts. nit: Add 'other' in front of 'global' or 'non-media keys' somewhere in the mix. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:56: // Determine if a key is a media key or not. Add a TODO(smus): Use the one in CommandService. ... or something. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:90: OSStatus HotKeyHandler(EventHandlerCallRef next_handler, EventRef event, nit: I think style-wise we prefer: void func( Foo* foo, Bar* bar) over void func(Foo* foo, Bar* bar) ... when it doesn't all fit. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:99: } nit: Single line if. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:113: } nit: Single line if. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:185: if (tapThreadRunLoop_) { Under what circumstances can these be nil when you call stop? If only under logic-error circumstances, shouldn't you make this a DCHECK instead to catch when you are calling this function twice? https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:279: // Register hotkey if they are keyboard shortcuts. nit: MediaKeys are keyboard shortcuts, are they not?
https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; On 2013/11/18 23:13:41, smus wrote: > On 2013/11/18 15:25:03, rsesek wrote: > > Why do you need to convert this to a NSEvent? Can't you just use the CGEvent > > functions to get this exact same information? > > I looked for a way of doing this from the CGEvent, but could not find a good way > of getting at the illusive data1 field that NSEvent exposes. Any ideas how to do > this? int64_t subtype = CGEventGetIntegerValueField(event, kCGMouseEventSubtype); https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz... https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:92: - (void)eventTapThread { On 2013/11/18 23:13:41, smus wrote: > On 2013/11/18 15:25:03, rsesek wrote: > > Yikes – an entire thread just to listen for global keyboard shortcuts! Threads > > aren't free; is this really necessary? > > I think this is the best practice. Doing this on the main thread risks blocking > it. What are you worried about blocking? You already have a cross-thread synchronization (waitUntilDone:YES below) to run something on the main thread, so you're still doing work there. I don't see how that's any different than installing the event tap on the main thread to begin with. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:68: typedef std::map< ui::Accelerator, int > HotKeyIdMap; nit: no space inside <> https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:7: #import <Cocoa/Cocoa.h> #include and using blocks should be alphabetized. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:18: #define EVENT_KEY @"event" These are still #defines. This should be: namespace { // This comment tells me what this constant is for. NSString* const kSomethingEventKey = @"event"; } // namespace https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:21: using extensions::GlobalShortcutListenerMac; Alphabetize. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:26: @private nit: one space indent, just like C++ https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:45: CGEventRef TapEventCallback( Actually, this should probably be EventTapCallback a TapEventCallback would be for tap gestures.
Another round. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; On 2013/11/19 18:34:58, rsesek wrote: > On 2013/11/18 23:13:41, smus wrote: > > On 2013/11/18 15:25:03, rsesek wrote: > > > Why do you need to convert this to a NSEvent? Can't you just use the CGEvent > > > functions to get this exact same information? > > > > I looked for a way of doing this from the CGEvent, but could not find a good > way > > of getting at the illusive data1 field that NSEvent exposes. Any ideas how to > do > > this? > > int64_t subtype = CGEventGetIntegerValueField(event, kCGMouseEventSubtype); > > https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz... Not sure I follow. Are you suggesting that CGEventGetIntegerValueField(event, kCGMouseEventSubtype); corresponds to [nsEvent data1]? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:92: - (void)eventTapThread { On 2013/11/19 18:34:58, rsesek wrote: > On 2013/11/18 23:13:41, smus wrote: > > On 2013/11/18 15:25:03, rsesek wrote: > > > Yikes – an entire thread just to listen for global keyboard shortcuts! > Threads > > > aren't free; is this really necessary? > > > > I think this is the best practice. Doing this on the main thread risks > blocking > > it. > > What are you worried about blocking? You already have a cross-thread > synchronization (waitUntilDone:YES below) to run something on the main thread, > so you're still doing work there. I don't see how that's any different than > installing the event tap on the main thread to begin with. Good point. Initially the thread was not synchronized (waitUntilDone:NO). I changed to the synchronized model after realizing that we need to know if a media key was handled by Chrome, in order to prevent the event from propagating to other apps, but only in this case. One possibility is to try to do waitUntilDone:YES, and then rebroadcast the event (not sure if this is possible). The other possibility is somehow doing this on the main thread (which seems to be what you are suggesting). Unfortunately I don't know how to do that. The event tap seems to need its own run loop, unless you know of some other way? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:216: void GlobalShortcutListenerMac::StartListening() { On 2013/11/19 13:10:05, Finnur wrote: > So, this Start/StopListening pair is meant as a convenience for platforms that > can turn the faucet on (when getting the first accelerator) and off (when > removing the last one). For example, Windows Chrome uses these events to > register for an observer on SingletonHWND on Start and remove it on Stop. In > other words, the Start/Stop pair avoids the need to check specifically whenever > you add a shortcut whether it is the first shortcut to be added (so that the > faucet needs to be turned on). > > This isn't quite as clean on Mac because you have a split system (one for > MediaKeys and another for the rest). If you start watching Media Keys here then > you end up in a situation where you might have extensions register only > Non-MediaKey global shortcuts but you fire up the thread and start listening for > MediaKeys anyway. > > I think you probably need to keep track of whether you've started the media key > loop and start it on seeing the first MediaKey shortcut (and stop it when the > last is removed). Ok, made this change. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:26: // global keyboard shortcuts. On 2013/11/19 13:10:05, Finnur wrote: > nit: Add 'other' in front of 'global' or 'non-media keys' somewhere in the mix. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:56: // Determine if a key is a media key or not. On 2013/11/19 13:10:05, Finnur wrote: > Add a TODO(smus): Use the one in CommandService. > ... or something. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:68: typedef std::map< ui::Accelerator, int > HotKeyIdMap; On 2013/11/19 18:34:58, rsesek wrote: > nit: no space inside <> Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:7: #import <Cocoa/Cocoa.h> On 2013/11/19 18:34:58, rsesek wrote: > #include and using blocks should be alphabetized. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:18: #define EVENT_KEY @"event" On 2013/11/19 18:34:58, rsesek wrote: > These are still #defines. This should be: > > namespace { > > // This comment tells me what this constant is for. > NSString* const kSomethingEventKey = @"event"; > > } // namespace Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:21: using extensions::GlobalShortcutListenerMac; On 2013/11/19 18:34:58, rsesek wrote: > Alphabetize. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:26: @private On 2013/11/19 18:34:58, rsesek wrote: > nit: one space indent, just like C++ Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:45: CGEventRef TapEventCallback( On 2013/11/19 18:34:58, rsesek wrote: > Actually, this should probably be EventTapCallback a TapEventCallback would be > for tap gestures. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:90: OSStatus HotKeyHandler(EventHandlerCallRef next_handler, EventRef event, On 2013/11/19 13:10:05, Finnur wrote: > nit: I think style-wise we prefer: > > void func( > Foo* foo, Bar* bar) > > over > > void func(Foo* foo, > Bar* bar) > > ... when it doesn't all fit. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:99: } On 2013/11/19 13:10:05, Finnur wrote: > nit: Single line if. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:113: } On 2013/11/19 13:10:05, Finnur wrote: > nit: Single line if. Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:185: if (tapThreadRunLoop_) { On 2013/11/19 13:10:05, Finnur wrote: > Under what circumstances can these be nil when you call stop? > If only under logic-error circumstances, shouldn't you make this a DCHECK > instead to catch when you are calling this function twice? Done. https://codereview.chromium.org/60353008/diff/290001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:279: // Register hotkey if they are keyboard shortcuts. On 2013/11/19 13:10:05, Finnur wrote: > nit: MediaKeys are keyboard shortcuts, are they not? Done.
I believe this addresses my concerns, so once rsesek is happy I'm happy. LGTM In the meantime, perhaps take a look at the test that Chaobin is adding, to see if it can be enabled on Mac? https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:335: } nit: Single-line if. Hard habit to break, eh? :)
https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; On 2013/11/20 04:28:51, smus wrote: > On 2013/11/19 18:34:58, rsesek wrote: > > On 2013/11/18 23:13:41, smus wrote: > > > On 2013/11/18 15:25:03, rsesek wrote: > > > > Why do you need to convert this to a NSEvent? Can't you just use the > CGEvent > > > > functions to get this exact same information? > > > > > > I looked for a way of doing this from the CGEvent, but could not find a good > > way > > > of getting at the illusive data1 field that NSEvent exposes. Any ideas how > to > > do > > > this? > > > > int64_t subtype = CGEventGetIntegerValueField(event, kCGMouseEventSubtype); > > > > > https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz... > > Not sure I follow. Are you suggesting that CGEventGetIntegerValueField(event, > kCGMouseEventSubtype); corresponds to [nsEvent data1]? What about kCGEventSourceUserData? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:92: - (void)eventTapThread { On 2013/11/20 04:28:51, smus wrote: > On 2013/11/19 18:34:58, rsesek wrote: > > On 2013/11/18 23:13:41, smus wrote: > > > On 2013/11/18 15:25:03, rsesek wrote: > > > > Yikes – an entire thread just to listen for global keyboard shortcuts! > > Threads > > > > aren't free; is this really necessary? > > > > > > I think this is the best practice. Doing this on the main thread risks > > blocking > > > it. > > > > What are you worried about blocking? You already have a cross-thread > > synchronization (waitUntilDone:YES below) to run something on the main thread, > > so you're still doing work there. I don't see how that's any different than > > installing the event tap on the main thread to begin with. > > Good point. Initially the thread was not synchronized (waitUntilDone:NO). I > changed to the synchronized model after realizing that we need to know if a > media key was handled by Chrome, in order to prevent the event from propagating > to other apps, but only in this case. > > One possibility is to try to do waitUntilDone:YES, and then rebroadcast the > event (not sure if this is possible). Rebroadcasting the event would post it out-of-order relative to the event stream. It's certainly doable (CGEventTapPostEvent, e.g.), but I'm not sure it's the right solution. > The other possibility is somehow doing this on the main thread (which seems to > be what you are suggesting). Unfortunately I don't know how to do that. The > event tap seems to need its own run loop, unless you know of some other way? Why do you think it needs its own run loop? The main/UI thread will return a CFRunLoopRef if you call CFRunLoopGetCurrent() in that thread. Once that happens, I think you can fold all of the ObjC class into the C++ class. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:8: #include <ApplicationServices/ApplicationServices.h> nit: alphabetize by header name, not by #include v #import https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:17: // The media keys subtype. nit: no indenting in namespaces, and blank lines between constatns. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:29: nit: no blank line https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:49: CGEventRef EventTapCallback( These should be in the anonymous namespace, too. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:105: GlobalShortcutListenerMac* shortcutListener = naming: shortcut_listener https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:209: int key_code = (([event data1] & 0xFFFF0000) >> 16); naming: in ObjC use camelCase, so keyCode
Ok, some big late-game changes: - Eliminated event tap thread. - Folded Obj-C class into C++ class - Simplified some handlers - Responded to nits. https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: NSEvent* nsEvent = [NSEvent eventWithCGEvent:event]; On 2013/11/20 16:23:57, rsesek wrote: > On 2013/11/20 04:28:51, smus wrote: > > On 2013/11/19 18:34:58, rsesek wrote: > > > On 2013/11/18 23:13:41, smus wrote: > > > > On 2013/11/18 15:25:03, rsesek wrote: > > > > > Why do you need to convert this to a NSEvent? Can't you just use the > > CGEvent > > > > > functions to get this exact same information? > > > > > > > > I looked for a way of doing this from the CGEvent, but could not find a > good > > > way > > > > of getting at the illusive data1 field that NSEvent exposes. Any ideas how > > to > > > do > > > > this? > > > > > > int64_t subtype = CGEventGetIntegerValueField(event, kCGMouseEventSubtype); > > > > > > > > > https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz... > > > > Not sure I follow. Are you suggesting that CGEventGetIntegerValueField(event, > > kCGMouseEventSubtype); corresponds to [nsEvent data1]? > > What about kCGEventSourceUserData? No, unfortunately that field is always zero. int data1Carbon = CGEventGetIntegerValueField(event, kCGEventSourceUserData); int data1Cocoa = [ns_event data1]; VLOG(0) << "Data1. carbon: " << data1Carbon << " cocoa: " << data1Cocoa; Yields [47820:1287:1120/083631:INFO:global_shortcut_listener_mac.mm(71)] Data1. carbon: 0 cocoa: 1313536 I asked around in #macdev on freenode as well, and was told to go the NSEvent route. Any other ideas? https://codereview.chromium.org/60353008/diff/210001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:92: - (void)eventTapThread { On 2013/11/20 16:23:57, rsesek wrote: > On 2013/11/20 04:28:51, smus wrote: > > On 2013/11/19 18:34:58, rsesek wrote: > > > On 2013/11/18 23:13:41, smus wrote: > > > > On 2013/11/18 15:25:03, rsesek wrote: > > > > > Yikes – an entire thread just to listen for global keyboard shortcuts! > > > Threads > > > > > aren't free; is this really necessary? > > > > > > > > I think this is the best practice. Doing this on the main thread risks > > > blocking > > > > it. > > > > > > What are you worried about blocking? You already have a cross-thread > > > synchronization (waitUntilDone:YES below) to run something on the main > thread, > > > so you're still doing work there. I don't see how that's any different than > > > installing the event tap on the main thread to begin with. > > > > Good point. Initially the thread was not synchronized (waitUntilDone:NO). I > > changed to the synchronized model after realizing that we need to know if a > > media key was handled by Chrome, in order to prevent the event from > propagating > > to other apps, but only in this case. > > > > One possibility is to try to do waitUntilDone:YES, and then rebroadcast the > > event (not sure if this is possible). > > Rebroadcasting the event would post it out-of-order relative to the event > stream. It's certainly doable (CGEventTapPostEvent, e.g.), but I'm not sure it's > the right solution. > > > The other possibility is somehow doing this on the main thread (which seems to > > be what you are suggesting). Unfortunately I don't know how to do that. The > > event tap seems to need its own run loop, unless you know of some other way? > > Why do you think it needs its own run loop? The main/UI thread will return a > CFRunLoopRef if you call CFRunLoopGetCurrent() in that thread. > > Once that happens, I think you can fold all of the ObjC class into the C++ > class. Okay, I've gone this route. I had a (big!) bug before where I was running CFRunLoopRun() on the main thread again, which understandably caused some undesired behavior. New approach seems to work pretty well, and definitely simplifies the code. Folded the Obj-C class. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:8: #include <ApplicationServices/ApplicationServices.h> On 2013/11/20 16:23:57, rsesek wrote: > nit: alphabetize by header name, not by #include v #import Done. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:17: // The media keys subtype. On 2013/11/20 16:23:57, rsesek wrote: > nit: no indenting in namespaces, and blank lines between constatns. Done. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:29: On 2013/11/20 16:23:57, rsesek wrote: > nit: no blank line Done. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:49: CGEventRef EventTapCallback( On 2013/11/20 16:23:57, rsesek wrote: > These should be in the anonymous namespace, too. Done. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:105: GlobalShortcutListenerMac* shortcutListener = On 2013/11/20 16:23:57, rsesek wrote: > naming: shortcut_listener Done. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:209: int key_code = (([event data1] & 0xFFFF0000) >> 16); On 2013/11/20 16:23:57, rsesek wrote: > naming: in ObjC use camelCase, so keyCode Done. https://codereview.chromium.org/60353008/diff/350001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:335: } On 2013/11/20 12:33:55, Finnur wrote: > nit: Single-line if. Hard habit to break, eh? :) Done.
Architecturally, this is looking much better. This round is mostly style points. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:10: #include <Carbon/Carbon.h> nit: blank line after https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:10: #include <Carbon/Carbon.h> Also nee #include <CoreFoundation/CoreFoundation.h> https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:37: void EnableTap(); This shouldn't be public. Why don't you friend the callback instead, or make the callback a static member in the private section? https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:22: const int SYSTEM_DEFINED_EVENT_MEDIA_KEYS = 8; naming: kSystemDefinedEventMediaKeys https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:31: NSAutoreleasePool* pool = [NSAutoreleasePool new]; You don't need to provide an autorelease pool here. And then I think you can remove out_event, too, and just return |event|. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:53: int data1 = [ns_event data1]; -data1 returns NSInteger, not int. They are different sizes on 64-bit. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:90: // Callback to the parent class. This comment isn't useful, remove. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:115: GlobalShortcutListenerMac::~GlobalShortcutListenerMac() { What about calling StopWatchingMediaKeys()? https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:144: //[tap_ startWatchingMediaKeys]; Don't check-in dead code. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:154: hotkey_id_ += 1; ++hotkey_id_; https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:180: bool GlobalShortcutListenerMac::IsMediaKey(const ui::Accelerator& accelerator) { Implementation order should match header order. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:195: bool GlobalShortcutListenerMac::OnKeyEvent(EventHotKeyID hotKeyID) { naming: hotkey_id Or should it really be hot_key_id? The capitalized version is HotKey, which should translate to underscores as hot_key_X, but throughout this file it's been written as hotkey_X. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:200: " modifiers: " << accelerator.modifiers(); nit: stream formatting rules state that the << should be on this line and align with the previous line's << http://www.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:205: bool GlobalShortcutListenerMac::OnMediaKeyEvent(int mediaKeyCode) { naming: media_key_code https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:206: ui::KeyboardCode keyCode = MediaKeyCodeToKeyboardCode(mediaKeyCode); naming: keyCode https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:236: event_hotkey_id.signature = 'chro'; You should use rimZ: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome.gyp&... https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:313: nit: double blank line
PTAL https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:10: #include <Carbon/Carbon.h> On 2013/11/21 16:29:56, rsesek wrote: > Also nee #include <CoreFoundation/CoreFoundation.h> Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:10: #include <Carbon/Carbon.h> On 2013/11/21 16:29:56, rsesek wrote: > nit: blank line after Don't follow. after the Carbon include? https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:22: const int SYSTEM_DEFINED_EVENT_MEDIA_KEYS = 8; On 2013/11/21 16:29:56, rsesek wrote: > naming: kSystemDefinedEventMediaKeys Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:31: NSAutoreleasePool* pool = [NSAutoreleasePool new]; On 2013/11/21 16:29:56, rsesek wrote: > You don't need to provide an autorelease pool here. And then I think you can > remove out_event, too, and just return |event|. Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:53: int data1 = [ns_event data1]; On 2013/11/21 16:29:56, rsesek wrote: > -data1 returns NSInteger, not int. They are different sizes on 64-bit. Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:90: // Callback to the parent class. On 2013/11/21 16:29:56, rsesek wrote: > This comment isn't useful, remove. Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:115: GlobalShortcutListenerMac::~GlobalShortcutListenerMac() { On 2013/11/21 16:29:56, rsesek wrote: > What about calling StopWatchingMediaKeys()? Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:144: //[tap_ startWatchingMediaKeys]; On 2013/11/21 16:29:56, rsesek wrote: > Don't check-in dead code. Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:154: hotkey_id_ += 1; On 2013/11/21 16:29:56, rsesek wrote: > ++hotkey_id_; Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:180: bool GlobalShortcutListenerMac::IsMediaKey(const ui::Accelerator& accelerator) { On 2013/11/21 16:29:56, rsesek wrote: > Implementation order should match header order. Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:195: bool GlobalShortcutListenerMac::OnKeyEvent(EventHotKeyID hotKeyID) { On 2013/11/21 16:29:56, rsesek wrote: > naming: hotkey_id > > Or should it really be hot_key_id? The capitalized version is HotKey, which > should translate to underscores as hot_key_X, but throughout this file it's been > written as hotkey_X. The capitalized version comes from the Carbon call RegisterEventHotKey, but writing out all of the underscores everywhere is, I think, too verbose. Fixed in this instance. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:200: " modifiers: " << accelerator.modifiers(); On 2013/11/21 16:29:56, rsesek wrote: > nit: stream formatting rules state that the << should be on this line and align > with the previous line's << > > http://www.chromium.org/developers/coding-style#Code_Formatting Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:205: bool GlobalShortcutListenerMac::OnMediaKeyEvent(int mediaKeyCode) { On 2013/11/21 16:29:56, rsesek wrote: > naming: media_key_code Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:206: ui::KeyboardCode keyCode = MediaKeyCodeToKeyboardCode(mediaKeyCode); On 2013/11/21 16:29:56, rsesek wrote: > naming: keyCode Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:236: event_hotkey_id.signature = 'chro'; On 2013/11/21 16:29:56, rsesek wrote: > You should use rimZ: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome.gyp&... Done. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:313: On 2013/11/21 16:29:56, rsesek wrote: > nit: double blank line Done.
https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:255: // TODO(smus): switch over to the cross-platform version of this method. Check in my CL just now: https://codereview.chromium.org/64273008/. Now you can sync the code and use that function. Cheers! :)
This should probably have some tests, too, right? https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:10: #include <Carbon/Carbon.h> On 2013/11/22 02:06:38, smus wrote: > On 2013/11/21 16:29:56, rsesek wrote: > > nit: blank line after > > Don't follow. after the Carbon include? Yes. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:195: bool GlobalShortcutListenerMac::OnKeyEvent(EventHotKeyID hotKeyID) { On 2013/11/22 02:06:38, smus wrote: > On 2013/11/21 16:29:56, rsesek wrote: > > naming: hotkey_id > > > > Or should it really be hot_key_id? The capitalized version is HotKey, which > > should translate to underscores as hot_key_X, but throughout this file it's > been > > written as hotkey_X. > > The capitalized version comes from the Carbon call RegisterEventHotKey, but > writing out all of the underscores everywhere is, I think, too verbose. Fixed > in this instance. If you want to write hotkey_X and not hot_key_X, then your methods should be HotkeyX not HotKeyX. That way at least your names are consistent, and the only inconsistency will be at the Carbon callsite. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:60: ui::KeyboardCode MediaKeyCodeToKeyboardCode(int keyCode); naming: key_code https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:63: // TODO(smus): Use the one in CommandService. What is "the one" referring to? https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:26: VLOG(0) << "HotKeyHandler fired with event: " << event; Are the VLOGs necessary? They seem to just add clutter. At the very least, they should be DVLOGs so that they're not shipped in a release binary. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: } // namespace nit: two spaces before // https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:84: ui::Accelerator accelerator = id_hotkeys_[hotkey_id.id]; Can this be a const& type? https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:278: nit: remove blank line (or insert // to keep it as part of the same comment) https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:281: CGEventRef GlobalShortcutListenerMac::EventTapCallback( // static above this
> This should probably have some tests, too, right? Yes. Look for Chaobin's recently landed Media Key support CL for platforms other than Mac. It includes test. You should probably enable those for Mac. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:63: // TODO(smus): Use the one in CommandService. Neo.
Ok, switched to CommandService::IsMediaKeys CL. Hoping to land this soon and do tests in a separate CL. https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/430001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:195: bool GlobalShortcutListenerMac::OnKeyEvent(EventHotKeyID hotKeyID) { On 2013/11/22 16:49:59, rsesek wrote: > On 2013/11/22 02:06:38, smus wrote: > > On 2013/11/21 16:29:56, rsesek wrote: > > > naming: hotkey_id > > > > > > Or should it really be hot_key_id? The capitalized version is HotKey, which > > > should translate to underscores as hot_key_X, but throughout this file it's > > been > > > written as hotkey_X. > > > > The capitalized version comes from the Carbon call RegisterEventHotKey, but > > writing out all of the underscores everywhere is, I think, too verbose. Fixed > > in this instance. > > If you want to write hotkey_X and not hot_key_X, then your methods should be > HotkeyX not HotKeyX. That way at least your names are consistent, and the only > inconsistency will be at the Carbon callsite. OK, moved to hot_key since HotKey is used generously in the Carbon API. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:60: ui::KeyboardCode MediaKeyCodeToKeyboardCode(int keyCode); On 2013/11/22 16:49:59, rsesek wrote: > naming: key_code Done. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:63: // TODO(smus): Use the one in CommandService. On 2013/11/22 17:52:53, Finnur wrote: > Neo. No longer relevant. Switched to CommandService::IsMediaKey https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:26: VLOG(0) << "HotKeyHandler fired with event: " << event; On 2013/11/22 16:49:59, rsesek wrote: > Are the VLOGs necessary? They seem to just add clutter. At the very least, they > should be DVLOGs so that they're not shipped in a release binary. Changed to DVLOGs. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: } // namespace On 2013/11/22 16:49:59, rsesek wrote: > nit: two spaces before // Done. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:84: ui::Accelerator accelerator = id_hotkeys_[hotkey_id.id]; On 2013/11/22 16:49:59, rsesek wrote: > Can this be a const& type? Done. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:255: // TODO(smus): switch over to the cross-platform version of this method. On 2013/11/22 08:08:16, zhchbin wrote: > Check in my CL just now: https://codereview.chromium.org/64273008/. Now you can > sync the code and use that function. > > Cheers! :) Thanks, switched to it. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:278: On 2013/11/22 16:49:59, rsesek wrote: > nit: remove blank line (or insert // to keep it as part of the same comment) Done. https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:281: CGEventRef GlobalShortcutListenerMac::EventTapCallback( On 2013/11/22 16:49:59, rsesek wrote: > // static above this Done.
The CL description also needs to be updated, since this no longer uses a dedicated thread. Please also try to wrap it ~80 cols (you can use `git cl description` to do so in your favorite $EDITOR). On 2013/11/22 20:48:08, smus wrote: > Ok, switched to CommandService::IsMediaKeys CL. Hoping to land this soon and do > tests in a separate CL. > I find that people (I've been guilty of this myself) seldom follow-up to add tests if they're not in the original CL. I'd strongly urge you to add some unit tests in this CL. https://codereview.chromium.org/60353008/diff/670001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/670001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:11: My mistake. I meant this blank line to be after CoreFoundation (separate system headers from C++ ones). https://codereview.chromium.org/60353008/diff/670001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:35: bool OnKeyEvent(EventHotKeyID hotkey_id); naming: hotkey
FYI I'll be OOO all next week. I'm CCing mark@, who would be another good person to have review this.
Mark/Robert, would be great to have a final look. https://codereview.chromium.org/60353008/diff/670001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/670001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:11: On 2013/11/22 21:36:35, rsesek wrote: > My mistake. I meant this blank line to be after CoreFoundation (separate system > headers from C++ ones). Done. https://codereview.chromium.org/60353008/diff/670001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:35: bool OnKeyEvent(EventHotKeyID hotkey_id); On 2013/11/22 21:36:35, rsesek wrote: > naming: hotkey Done.
https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:61: ui::KeyboardCode MediaKeyCodeToKeyboardCode(int key_code); This can be static. And private + static implies that it doesn’t really need to be part of the class at all, it can actually be an anonymous-namespaced function in the implementation file. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:64: bool AreMediaKeysRegistered(); The comment is accurate but the name of the function makes it sound like it checks for ALL media keys, not ANY media key. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:76: // A counter representing the current hotkey identifier. What does “current” mean? I looked at the implementation and see how you’re using it, but this comment isn’t helpful at all. I also have concerns about using a dumb counter. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:77: int hot_key_id_ = 0; Don’t initialize here, initialize in the constructor. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:23: const int kSystemDefinedEventMediaKeys = 8; Can you describe how you found this number? Documentation or something else? You should do this any time you need to hard-code constant that’s beyond our control. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:150: void GlobalShortcutListenerMac::RegisterHotKey( RegisterAccelerator, RegisterHotKey, StartWatchingMediaKeys, … It’s confusing to have all of these similar names while you’re reading code elsewhere in this file. You have to keep flipping back and forth to figure out which RegisterX function something is referring to. Can you define your terminology somewhere? Or choose more descriptive names that aren’t so much like synonyms? “Media keys” is an example of a descriptive name, but if the others aren’t descriptive, it’s not helpful: is a media key “hot” or not? Is it an “accelerator” or not? https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:160: InstallApplicationEventHandler(hot_key_function, 1, &event_type, this, NULL); You install this each time you pass through RegisterHotKey. That seems unnecessary, the application event handler itself is not specific to any specific hot key. You also never remove the application event handler (RemoveEventHandler). https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:163: event_hot_key_id.signature = 'rimZ'; You should get the bundle signature instead of hard-coding this here. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:164: event_hot_key_id.id = hot_key_id_; Perhaps the uses of hot_key_id_ here and on line 185 would be less scary if it were a parameter to this function instead of a direct access to the member variable. That way, you wouldn’t need the comment on line 184 that makes the whole operation seem very fragile and sensitive to the call sequence. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:168: modifiers += (accelerator.IsShiftDown() ? shiftKey : 0); These are bits, right? Use |= instead of +=. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:203: DCHECK(event_tap_ == NULL); It’s not necessarily going to be NULL because you never initialized it in the constructor. You should also initialize event_tap_source_. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:224: CFRunLoopAddSource(CFRunLoopGetCurrent(), event_tap_source_, You never remove this source (in StopWatchingMediaKeys?) https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:284: shortcut_listener->EnableTap(); EnableTap is a one-liner that is only used here. It’s OK to write it in-line here and get rid of the standalone above. CGEventTapEnable(shortcut_listener->event_tap_, TRUE); https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:288: // TODO(smus): do some error handling since eventWithCGEvent can fail. What kind of error handling? Why not do it? https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:289: NSEvent* ns_event = [NSEvent eventWithCGEvent:event]; Can you point (in a comment) to some documentation that describes what’s going on in the system-defined event and its data1 field? https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:293: [ns_event subtype] != kSystemDefinedEventMediaKeys) { For safety, [ns_event type] should be NSSystemDefined. I’m not sure if that’s the same as checking whether type == NX_SYSDEFINED, but since you’re operating on ns_event here, it seems prudent to check the type field on ns_event in preference to the CGEventType parameter. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:307: bool is_key_pressed = (((key_flags & 0xFF00) >> 8)) == 0xA; More magic constants that we don’t control. Documentation references are needed.
Added interactive tests for mac (thanks for the help, Finnur!), responded to Mark's feedback. PTAL. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:61: ui::KeyboardCode MediaKeyCodeToKeyboardCode(int key_code); On 2013/11/26 15:32:46, Mark Mentovai wrote: > This can be static. And private + static implies that it doesn’t really need to > be part of the class at all, it can actually be an anonymous-namespaced function > in the implementation file. Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:64: bool AreMediaKeysRegistered(); On 2013/11/26 15:32:46, Mark Mentovai wrote: > The comment is accurate but the name of the function makes it sound like it > checks for ALL media keys, not ANY media key. Renamed to IsAnyMediaKeyRegistered. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:76: // A counter representing the current hotkey identifier. On 2013/11/26 15:32:46, Mark Mentovai wrote: > What does “current” mean? > > I looked at the implementation and see how you’re using it, but this comment > isn’t helpful at all. > > I also have concerns about using a dumb counter. Updated comment. What's the issue with a dumb counter? https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:77: int hot_key_id_ = 0; On 2013/11/26 15:32:46, Mark Mentovai wrote: > Don’t initialize here, initialize in the constructor. Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:23: const int kSystemDefinedEventMediaKeys = 8; Unfortunately I can't find official docs. I don't believe it's a public API, but here is the earliest mention I could find: http://lists.apple.com/archives/cocoa-dev/2007/Aug/msg00499.html. Updated the comment. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:150: void GlobalShortcutListenerMac::RegisterHotKey( On 2013/11/26 15:32:46, Mark Mentovai wrote: > RegisterAccelerator, RegisterHotKey, StartWatchingMediaKeys, … > > It’s confusing to have all of these similar names while you’re reading code > elsewhere in this file. You have to keep flipping back and forth to figure out > which RegisterX function something is referring to. Can you define your > terminology somewhere? Or choose more descriptive names that aren’t so much like > synonyms? “Media keys” is an example of a descriptive name, but if the others > aren’t descriptive, it’s not helpful: is a media key “hot” or not? Is it an > “accelerator” or not? Accelerator is the Chrome terminology, so stuck with it. It encompasses both media keys and hot keys. HotKey is the Carbon terminology for keyboard shortcuts involving a modifier, so stuck with that too. I updated the class doc. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:160: InstallApplicationEventHandler(hot_key_function, 1, &event_type, this, NULL); From http://oreilly.com/catalog/learncarbon/chapter/ch06.html: "In particular, the handler will be disposed of automatically when you dispose of the target object it's associated with, so there's no need to call RemoveEventHandler explicitly unless for some reason you want to deinstall the handler while the underlying target object still exists." https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:163: event_hot_key_id.signature = 'rimZ'; On 2013/11/26 15:32:46, Mark Mentovai wrote: > You should get the bundle signature instead of hard-coding this here. Can you provide any pointer for doing this? I don't see any mention of the creator bundle signature in the codebase, so the way to do it would be via FSGetCatalogInfo? https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:164: event_hot_key_id.id = hot_key_id_; On 2013/11/26 15:32:46, Mark Mentovai wrote: > Perhaps the uses of hot_key_id_ here and on line 185 would be less scary if it > were a parameter to this function instead of a direct access to the member > variable. That way, you wouldn’t need the comment on line 184 that makes the > whole operation seem very fragile and sensitive to the call sequence. Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:168: modifiers += (accelerator.IsShiftDown() ? shiftKey : 0); On 2013/11/26 15:32:46, Mark Mentovai wrote: > These are bits, right? Use |= instead of +=. Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:203: DCHECK(event_tap_ == NULL); On 2013/11/26 15:32:46, Mark Mentovai wrote: > It’s not necessarily going to be NULL because you never initialized it in the > constructor. You should also initialize event_tap_source_. Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:224: CFRunLoopAddSource(CFRunLoopGetCurrent(), event_tap_source_, On 2013/11/26 15:32:46, Mark Mentovai wrote: > You never remove this source (in StopWatchingMediaKeys?) Oops, good point, thanks! https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:284: shortcut_listener->EnableTap(); On 2013/11/26 15:32:46, Mark Mentovai wrote: > EnableTap is a one-liner that is only used here. It’s OK to write it in-line > here and get rid of the standalone above. > > CGEventTapEnable(shortcut_listener->event_tap_, TRUE); Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:288: // TODO(smus): do some error handling since eventWithCGEvent can fail. On 2013/11/26 15:32:46, Mark Mentovai wrote: > What kind of error handling? Why not do it? Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:289: NSEvent* ns_event = [NSEvent eventWithCGEvent:event]; On 2013/11/26 15:32:46, Mark Mentovai wrote: > Can you point (in a comment) to some documentation that describes what’s going > on in the system-defined event and its data1 field? Added a reference to http://weblog.rogueamoeba.com/2007/09/29/ but these are basically undocumented APIs. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:293: [ns_event subtype] != kSystemDefinedEventMediaKeys) { On 2013/11/26 15:32:46, Mark Mentovai wrote: > For safety, [ns_event type] should be NSSystemDefined. I’m not sure if that’s > the same as checking whether type == NX_SYSDEFINED, but since you’re operating > on ns_event here, it seems prudent to check the type field on ns_event in > preference to the CGEventType parameter. Done. https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:307: bool is_key_pressed = (((key_flags & 0xFF00) >> 8)) == 0xA; On 2013/11/26 15:32:46, Mark Mentovai wrote: > More magic constants that we don’t control. Documentation references are needed. See above.
https://codereview.chromium.org/60353008/diff/770001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/60353008/diff/770001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:165: SendNativeKeyEventToXDisplay((ui::KeyboardCode)0x39, true, true, false); wait... what is this for? https://codereview.chromium.org/60353008/diff/770001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:171: SendNativeKeyEventMac((ui::KeyboardCode)25); I'm confused... The other platforms send Ctrl+Shift+1 Ctrl+Shift+A Ctrl+Shift+9 The first two are negative tests (if the keys do anything then the test fails) but the last is what causes the test to pass. So, on the face of it, the test should fail because the last shortcut is not sent...
https://codereview.chromium.org/60353008/diff/770001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/60353008/diff/770001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:165: SendNativeKeyEventToXDisplay((ui::KeyboardCode)0x39, true, true, false); On 2013/12/09 17:29:08, Finnur wrote: > wait... what is this for? Done. https://codereview.chromium.org/60353008/diff/770001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:171: SendNativeKeyEventMac((ui::KeyboardCode)25); On 2013/12/09 17:29:08, Finnur wrote: > I'm confused... The other platforms send > > Ctrl+Shift+1 > Ctrl+Shift+A > Ctrl+Shift+9 > > The first two are negative tests (if the keys do anything then the test fails) > but the last is what causes the test to pass. > > So, on the face of it, the test should fail because the last shortcut is not > sent... I uploaded some stale version of the CL. Here's an update without magic constants and weird Linux changes.
Thanks. That looks better. Can you flesh out the CL description and BUG= while we wait for Mark?
https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/490001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:26: VLOG(0) << "HotKeyHandler fired with event: " << event; On 2013/11/22 20:48:09, smus wrote: > On 2013/11/22 16:49:59, rsesek wrote: > > Are the VLOGs necessary? They seem to just add clutter. At the very least, > they > > should be DVLOGs so that they're not shipped in a release binary. > > Changed to DVLOGs. They still make the code considerably harder to read. Is there any reason to keep them? https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:79: CGEventRef cmd_d = CGEventCreateKeyboardEvent(src, kVK_Command, true); You should place these in a ScopedCFTypeRef so that you don't have to manually call CFRelease(). https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:57: nit: extra blank line https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:73: is_listening_ = false; Why not use an initializer list for this? https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:81: StopListening(); What's the point of calling this? It just sets a boolean. Leave a comment about when accelerators are actually unregistered. Is it possible to leak things from RegisterAccelerator? https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:105: const ui::Accelerator& accelerator = id_hot_keys_[hot_key_id.id]; What if it's not found?
https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:160: InstallApplicationEventHandler(hot_key_function, 1, &event_type, this, NULL); smus wrote: > From http://oreilly.com/catalog/learncarbon/chapter/ch06.html: > > "In particular, the handler will be disposed of automatically when you dispose > of the target object it's associated with, so there's no need to call > RemoveEventHandler explicitly unless for some reason you want to deinstall the > handler while the underlying target object still exists." The target object is the application event target (GetApplicationEventTarget()), not “this”. The application event target is never destroyed. Reiterating: You install this each time you pass through RegisterHotKey. That seems unnecessary, the application event handler itself is not specific to any specific hot key. You also never remove the application event handler (RemoveEventHandler). https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:79: CGEventRef cmd_d = CGEventCreateKeyboardEvent(src, kVK_Command, true); This sounds like the key event is for command-D, command-U, etc. Don’t abbreviate. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... You can rename src and loc too. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:80: CGEventRef cmd_u = CGEventCreateKeyboardEvent(src, kVK_Command, false); Why don’t you create these in the order that they will be posted? https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:86: CGEventSetFlags(key_d, kCGEventFlagMaskCommand | kCGEventFlagMaskShift); This should immediately follow the creation of key_d. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:87: CGEventSetFlags(key_u, kCGEventFlagMaskCommand | kCGEventFlagMaskShift); Ditto, key_u. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:89: CGEventTapLocation loc = kCGHIDEventTap; // kCGSessionEventTap also works http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: CFRelease(cmd_d); Use a base::ScopedCFTypeRef (base/mac/scoped_cftyperef.h) for all of these. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:73: int hot_key_id_; There are a bunch of different things in here that use int, and it’s not straightforward to tell if they’re all the same, or which ones are the same. Use typedefs to make the code more meaningful. Maybe there are already typedefs defined elsewhere that you can use. If not, introduce some. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:28: DVLOG(0) << "HotKeyHandler fired with event: " << event; Are these still useful? If development is wrapped up on this feature, I’d prefer not including any extraneous logging. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: int key_code) { No need to wrap this line. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:58: static base::LazyInstance<extensions::GlobalShortcutListenerMac> g_instance = You’re already in an unnamed namespace, “static” doesn’t add anything here. But see the next comment in GetInstance. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:68: return g_instance.Pointer(); g_instance is only referenced here, so you can just put it at function scope right here. If you do that, obviously, you use the “static” keyword. And since you already have the CHECK to make sure that this is on a single thread, you don’t need the thread-safety that LazyInstance provides on top of an ordinary static. You can just have a static GlobalShortcutListenerMac* right here. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:79: GlobalShortcutListenerMac::~GlobalShortcutListenerMac() { Is an object of this class ever created anywhere other than the static GetInstance method? https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:107: << " modifiers: " << accelerator.modifiers(); Line up the first << operator on this new line with the one above it. http://www.chromium.org/developers/coding-style This comment applies throughout. But I strongly urge you to just drop this logging. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:109: return true; Seems like there’s an interface here to signal that a key wasn’t found. You use it for media keys but not for non-media keys. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:190: event_hot_key_id.signature = 'rimZ'; Don’t hard-code the creator code. You can use CFBundleGetPackageInfo to get the real creator code. You should have a helper function in the unnamed namespace do this, and cache the result in a static.
PTAL https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/730001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:160: InstallApplicationEventHandler(hot_key_function, 1, &event_type, this, NULL); On 2013/12/09 19:31:54, Mark Mentovai wrote: > smus wrote: > > From http://oreilly.com/catalog/learncarbon/chapter/ch06.html: > > > > "In particular, the handler will be disposed of automatically when you dispose > > of the target object it's associated with, so there's no need to call > > RemoveEventHandler explicitly unless for some reason you want to deinstall the > > handler while the underlying target object still exists." > > The target object is the application event target (GetApplicationEventTarget()), > not “this”. The application event target is never destroyed. > > Reiterating: > > You install this each time you pass through RegisterHotKey. That seems > unnecessary, the application event handler itself is not specific to any > specific hot key. > > You also never remove the application event handler (RemoveEventHandler). Sorry, completely misunderstood the first time! Now installing handler in StartListening, removing in StopListening. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:79: CGEventRef cmd_d = CGEventCreateKeyboardEvent(src, kVK_Command, true); On 2013/12/09 19:31:54, Mark Mentovai wrote: > This sounds like the key event is for command-D, command-U, etc. Don’t > abbreviate. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... > > You can rename src and loc too. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:80: CGEventRef cmd_u = CGEventCreateKeyboardEvent(src, kVK_Command, false); On 2013/12/09 19:31:54, Mark Mentovai wrote: > Why don’t you create these in the order that they will be posted? Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:86: CGEventSetFlags(key_d, kCGEventFlagMaskCommand | kCGEventFlagMaskShift); On 2013/12/09 19:31:54, Mark Mentovai wrote: > This should immediately follow the creation of key_d. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:87: CGEventSetFlags(key_u, kCGEventFlagMaskCommand | kCGEventFlagMaskShift); On 2013/12/09 19:31:54, Mark Mentovai wrote: > Ditto, key_u. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:89: CGEventTapLocation loc = kCGHIDEventTap; // kCGSessionEventTap also works On 2013/12/09 19:31:54, Mark Mentovai wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: CFRelease(cmd_d); On 2013/12/09 19:31:54, Mark Mentovai wrote: > Use a base::ScopedCFTypeRef (base/mac/scoped_cftyperef.h) for all of these. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:73: int hot_key_id_; On 2013/12/09 19:31:54, Mark Mentovai wrote: > There are a bunch of different things in here that use int, and it’s not > straightforward to tell if they’re all the same, or which ones are the same. Use > typedefs to make the code more meaningful. Maybe there are already typedefs > defined elsewhere that you can use. If not, introduce some. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:28: DVLOG(0) << "HotKeyHandler fired with event: " << event; On 2013/12/09 19:31:54, Mark Mentovai wrote: > Are these still useful? If development is wrapped up on this feature, I’d prefer > not including any extraneous logging. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:43: int key_code) { On 2013/12/09 19:31:54, Mark Mentovai wrote: > No need to wrap this line. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:57: On 2013/12/09 19:17:42, rsesek wrote: > nit: extra blank line Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:58: static base::LazyInstance<extensions::GlobalShortcutListenerMac> g_instance = On 2013/12/09 19:31:54, Mark Mentovai wrote: > You’re already in an unnamed namespace, “static” doesn’t add anything here. But > see the next comment in GetInstance. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:68: return g_instance.Pointer(); On 2013/12/09 19:31:54, Mark Mentovai wrote: > g_instance is only referenced here, so you can just put it at function scope > right here. If you do that, obviously, you use the “static” keyword. And since > you already have the CHECK to make sure that this is on a single thread, you > don’t need the thread-safety that LazyInstance provides on top of an ordinary > static. You can just have a static GlobalShortcutListenerMac* right here. Mark, this code is mostly based on other (Windows, X11) implementations of the global shortcut listener. Do you think it's worth breaking consistency with those implementations? They have the same lifecycle. Finnur, any thoughts on this? https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:73: is_listening_ = false; On 2013/12/09 19:17:42, rsesek wrote: > Why not use an initializer list for this? Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:79: GlobalShortcutListenerMac::~GlobalShortcutListenerMac() { On 2013/12/09 19:31:54, Mark Mentovai wrote: > Is an object of this class ever created anywhere other than the static > GetInstance method? Don't believe so. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:81: StopListening(); On 2013/12/09 19:17:42, rsesek wrote: > What's the point of calling this? It just sets a boolean. Leave a comment about > when accelerators are actually unregistered. Is it possible to leak things from > RegisterAccelerator? There is some sanity checking too, which seems useful. Added a comment. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:105: const ui::Accelerator& accelerator = id_hot_keys_[hot_key_id.id]; On 2013/12/09 19:17:42, rsesek wrote: > What if it's not found? Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:107: << " modifiers: " << accelerator.modifiers(); On 2013/12/09 19:31:54, Mark Mentovai wrote: > Line up the first << operator on this new line with the one above it. > http://www.chromium.org/developers/coding-style > > This comment applies throughout. But I strongly urge you to just drop this > logging. Done. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:109: return true; On 2013/12/09 19:31:54, Mark Mentovai wrote: > Seems like there’s an interface here to signal that a key wasn’t found. You use > it for media keys but not for non-media keys. Media keys and hot keys are handled differently. Media keys use a global listener, while hot keys are registered specifically on a per-shortcut basis. This should actually return void. https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:190: event_hot_key_id.signature = 'rimZ'; On 2013/12/09 19:31:54, Mark Mentovai wrote: > Don’t hard-code the creator code. You can use CFBundleGetPackageInfo to get the > real creator code. You should have a helper function in the unnamed namespace do > this, and cache the result in a static. This is apparently implemented in base/mac/foundation_util.h
https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:68: return g_instance.Pointer(); As I recall, we've gotten a similar comment on one of the other platforms, so he is probably correct. I haven't had a time to look it up further as I'm dealing with a security issue right now (not related to this). Feel free to change it.
https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:68: return g_instance.Pointer(); On 2013/12/09 23:59:49, Finnur wrote: > As I recall, we've gotten a similar comment on one of the other platforms, so he > is probably correct. I haven't had a time to look it up further as I'm dealing > with a security issue right now (not related to this). Feel free to change it. Mark, I removed the lazy instantiation. Is this what you mean?
I will finish this review tomorrow. https://codereview.chromium.org/60353008/diff/850001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/850001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:65: if (g_instance == NULL) { Yes, this is what I meant. But also: - the “static” keyword on line 56 doesn’t contribute anything useful, since you’re already in an unnamed namespace there. - since this function is the only thing that uses g_instance, you might as well move it right into this function, in which case you should keep the “static”, obviously. I would advise changing the other-platform implementations if they also have the single-threaded property and a CHECK to enforce it.
On 2013/12/10 03:25:21, Mark Mentovai wrote: > I will finish this review tomorrow. > > https://codereview.chromium.org/60353008/diff/850001/chrome/browser/extension... > File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): > > https://codereview.chromium.org/60353008/diff/850001/chrome/browser/extension... > chrome/browser/extensions/global_shortcut_listener_mac.mm:65: if (g_instance == > NULL) { > Yes, this is what I meant. > > But also: > > - the “static” keyword on line 56 doesn’t contribute anything useful, since > you’re already in an unnamed namespace there. > - since this function is the only thing that uses g_instance, you might as well > move it right into this function, in which case you should keep the “static”, > obviously. > > I would advise changing the other-platform implementations if they also have the > single-threaded property and a CHECK to enforce it. I'd rather make this change in isolation, especially since I don't have easy access to other platforms to test them.
Yes, making changes to other platforms is fine. But why haven’t you moved the variable into the only function that uses it?
That should have read: making changes to other platforms in a separate change is fine.
https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:15: #include "base/lazy_instance.h" This is obsolete now. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:31: virtual ~GlobalShortcutListenerMac(); Destructor follows constructor. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:37: // Keyboard event callbacks. These two should be private. They’re not part of the external interface and it’s not sensible for code outside of this class to have access to them. This means that HotKeyHandler, currently in the implementation file in an unnamed namespace, needs to be promoted to be a private static function in this class as well, to be able to call the private OnKeyEvent. This follows the same pattern that you used for EventTapCallback calling OnMediaKeyEvent, which is correct (as long as OnMediaKeyEvent becomes private too). https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:38: void OnKeyEvent(EventHotKeyID hot_key_id); On the basis of the naming that’s now fairly uniform in this class (thanks!), this should be OnHotKeyEvent, to contrast with OnMediaKeyEvent. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:43: virtual void RegisterAccelerator( How is this ever called? It’s private, there are no “friends,” and there are no callers within this class. Because you’ve marked it OVERRIDE in a class that extends GlobalShortcutListener in which this method is public, I assume that you just meant to put this (and UnregisterAccelerator) into the public section. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:52: void RegisterHotKey(const ui::Accelerator& accelerator, int hot_key_id); int → HotKeyId? https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:70: typedef int HotKeyId; Move all of the typedefs up to the top of the private section. Their names are self-explanatory, they don’t need comments. You can leave the comments here on the declarations of the member variables. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:11: #include "base/mac/foundation_util.h" #import https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:25: const int kSystemDefinedEventMediaKeys = 8; kSystemDefinedEventMediaKeysSubtype to make it clear that this is used with -[NSEvent subtype] https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:31: int result = GetEventParameter(event, kEventParamDirectObject, OSStatus, not int. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: GlobalShortcutListenerMac* g_instance = NULL; Move into GetInstance as a static. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:64: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); This CHECK is good, but it’s only on this function and on the constructor. This kind of CHECK/DCHECK should also be on other “public” methods exposed by this class, to assert that consumers of this API are doing the right thing. I’d even put one in the destructor, although based on what you’ve told me, the destructor in this code’s current form will never be called. This kind of CHECK should also be on as the non-static “private” (or should-be-“private” per my previous comment) methods of this class that are the targets of system callbacks, On[Hot]KeyEvent and OnMediaKeyEvent, to assert that interactions with the system libraries are working exactly as you expect. Since this class isn’t thread-safe, I think these assertions would be a smart idea. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:72: : is_listening_(false), Now that you’re using event_handler_ properly, its value (as a boolean) has exactly the same meaning as is_listening_. You don’t need two member variables to track the same thing. Save some memory, reduce complexity. Get rid of is_listening_, add event_handler_(NULL) to the initializer list here, and fix up the uses of is_listening_ to use event_handler_ in the destructor, StartListening, and StopListening. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:96: // Install an event handler for hot keys. Personally, I would do this only when it’s needed, on demand from RegisterHotKey (and do the inverse operation presently in StopListening in UnregisterHotKey). That’s the way that you made Start/StopWatchingMediaKeys work, and in general, I like it better. The existing API already has this Start/StopListening feature, and mixing the desired Start/StopListening semantics, which I assume to be “don’t respond to a global shortcut unless StartListening has been called” with doing on-demand event handler installation would get a little complicated. RegisterHotKey would need to call a new InstallEventHandlerIfListeningAndKeysAreRegistered function, and StartListening would have to be {listening_ = true; InstallEventHandlerIfListeningAndKeysRegistered(); }. Maybe that kind of complexity isn’t justified. The reason I’m mentioning it is that you actually have different semantics for media keys than you have for hot keys. Media keys don’t care at all whether StartListening or StopListening has been called. If any media key is registered, it’s alive and active. That doesn’t match the behavior for hot keys. I think that the behaviors of the two classes of keys should definitely match. And if honoring Start/StopListening is correct, then that’s what they should do. On the other hand, if Start/StopListening are merely intended to be advisory, to do system-specific setup and teardown, then the media keys’ behavior is not incorrect. In that case, I still think that the behaviors of the hot keys and media keys should match, but it’d be at your option to decide which semantics you’d prefer to implement. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:117: const ui::Accelerator& accelerator = id_hot_keys_[hot_key_id.id]; At https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... , Robert said “What if it's not found?” and you responded “Done.” You did answer a similar question of mine, but nothing here was actually done. I read the implied second half of Robert’s question as “If it’s a possibility, you should handle that case, but if it should never happen, do you want to assert so with a DCHECK?” https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:150: if (!CommandService::IsMediaKey(accelerator)) Rather than calling IsMediaKey multiple times, how about if (CommandService::IsMediaKey(accelerator)) { if (!IsAnyMediaKeyRegistered()) { StartWatchingMediaKeys(); } } else { RegisterHotKey(accelerator, hot_key_id_); } https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:168: int id = hot_key_ids_[accelerator]; What if wasn’t registered to begin with? https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:168: int id = hot_key_ids_[accelerator]; There’s a typedef for this now, HotKeyId. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:181: const ui::Accelerator& accelerator, int hot_key_id) { int → HotKeyId? https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:182: EventHotKeyRef hot_key_ref; Don’t declare until use, down where you call RegisterEventHotKey. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:196: unichar character; You don’t use either of these. You can eliminate them and pass NULL as the last two parameters to ui::MacKeyCodeForWindowsKeyCode. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:211: int id = hot_key_ids_[accelerator]; What if it wasn’t registered to begin with? I asked the same question in UnregisterAccelerator on line 168, but UnregisterAccelerator calls this function before it manipulates hot_key_ids_ or the other maps, so this will actually be the first place in the current code that would become a problem if a caller passed in an accelerator that wasn’t registered to the public API UnregisterAccelerator. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:211: int id = hot_key_ids_[accelerator]; int → HotKeyId? https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:222: DCHECK(event_tap_ == NULL); DCHECK(event_tap_source_ == NULL) too? https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:248: CFRunLoopRemoveSource(CFRunLoopGetCurrent(), event_tap_source_, DCHECK(event_tap_) and DCHECK(event_tap_source_) ? https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:306: int key_code = ((data1 & 0xFFFF0000) >> 16); Outer (parentheses) unnecessary here and on line 313.
https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:15: #include "base/lazy_instance.h" On 2013/12/10 16:30:32, Mark Mentovai wrote: > This is obsolete now. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:31: virtual ~GlobalShortcutListenerMac(); On 2013/12/10 16:30:32, Mark Mentovai wrote: > Destructor follows constructor. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:37: // Keyboard event callbacks. On 2013/12/10 16:30:32, Mark Mentovai wrote: > These two should be private. They’re not part of the external interface and it’s > not sensible for code outside of this class to have access to them. > > This means that HotKeyHandler, currently in the implementation file in an > unnamed namespace, needs to be promoted to be a private static function in this > class as well, to be able to call the private OnKeyEvent. > > This follows the same pattern that you used for EventTapCallback calling > OnMediaKeyEvent, which is correct (as long as OnMediaKeyEvent becomes private > too). Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:38: void OnKeyEvent(EventHotKeyID hot_key_id); On 2013/12/10 16:30:32, Mark Mentovai wrote: > On the basis of the naming that’s now fairly uniform in this class (thanks!), > this should be OnHotKeyEvent, to contrast with OnMediaKeyEvent. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:52: void RegisterHotKey(const ui::Accelerator& accelerator, int hot_key_id); On 2013/12/10 16:30:32, Mark Mentovai wrote: > int → HotKeyId? Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:70: typedef int HotKeyId; On 2013/12/10 16:30:32, Mark Mentovai wrote: > Move all of the typedefs up to the top of the private section. Their names are > self-explanatory, they don’t need comments. You can leave the comments here on > the declarations of the member variables. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:11: #include "base/mac/foundation_util.h" On 2013/12/10 16:30:32, Mark Mentovai wrote: > #import Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:25: const int kSystemDefinedEventMediaKeys = 8; On 2013/12/10 16:30:32, Mark Mentovai wrote: > kSystemDefinedEventMediaKeysSubtype > > to make it clear that this is used with -[NSEvent subtype] Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:31: int result = GetEventParameter(event, kEventParamDirectObject, On 2013/12/10 16:30:32, Mark Mentovai wrote: > OSStatus, not int. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:56: GlobalShortcutListenerMac* g_instance = NULL; On 2013/12/10 16:30:32, Mark Mentovai wrote: > Move into GetInstance as a static. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:64: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2013/12/10 16:30:32, Mark Mentovai wrote: > This CHECK is good, but it’s only on this function and on the constructor. This > kind of CHECK/DCHECK should also be on other “public” methods exposed by this > class, to assert that consumers of this API are doing the right thing. I’d even > put one in the destructor, although based on what you’ve told me, the destructor > in this code’s current form will never be called. > > This kind of CHECK should also be on as the non-static “private” (or > should-be-“private” per my previous comment) methods of this class that are the > targets of system callbacks, On[Hot]KeyEvent and OnMediaKeyEvent, to assert that > interactions with the system libraries are working exactly as you expect. Since > this class isn’t thread-safe, I think these assertions would be a smart idea. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:72: : is_listening_(false), On 2013/12/10 16:30:32, Mark Mentovai wrote: > Now that you’re using event_handler_ properly, its value (as a boolean) has > exactly the same meaning as is_listening_. You don’t need two member variables > to track the same thing. Save some memory, reduce complexity. Get rid of > is_listening_, add event_handler_(NULL) to the initializer list here, and fix up > the uses of is_listening_ to use event_handler_ in the destructor, > StartListening, and StopListening. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:96: // Install an event handler for hot keys. On 2013/12/10 16:30:32, Mark Mentovai wrote: > Personally, I would do this only when it’s needed, on demand from RegisterHotKey > (and do the inverse operation presently in StopListening in UnregisterHotKey). > That’s the way that you made Start/StopWatchingMediaKeys work, and in general, I > like it better. > > The existing API already has this Start/StopListening feature, and mixing the > desired Start/StopListening semantics, which I assume to be “don’t respond to a > global shortcut unless StartListening has been called” with doing on-demand > event handler installation would get a little complicated. RegisterHotKey would > need to call a new InstallEventHandlerIfListeningAndKeysAreRegistered function, > and StartListening would have to be {listening_ = true; > InstallEventHandlerIfListeningAndKeysRegistered(); }. Maybe that kind of > complexity isn’t justified. > > The reason I’m mentioning it is that you actually have different semantics for > media keys than you have for hot keys. Media keys don’t care at all whether > StartListening or StopListening has been called. If any media key is registered, > it’s alive and active. That doesn’t match the behavior for hot keys. > > I think that the behaviors of the two classes of keys should definitely match. > And if honoring Start/StopListening is correct, then that’s what they should do. > On the other hand, if Start/StopListening are merely intended to be advisory, to > do system-specific setup and teardown, then the media keys’ behavior is not > incorrect. In that case, I still think that the behaviors of the hot keys and > media keys should match, but it’d be at your option to decide which semantics > you’d prefer to implement. RegisterAccelerator can be called at any point before or after StartListening. I've now moved on to this lazy event handler installation model you suggested, but would rather hold on to the is_listening_ private member just for sanity checking. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:117: const ui::Accelerator& accelerator = id_hot_keys_[hot_key_id.id]; On 2013/12/10 16:30:32, Mark Mentovai wrote: > At > https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension... > , Robert said “What if it's not found?” and you responded “Done.” You did answer > a similar question of mine, but nothing here was actually done. > > I read the implied second half of Robert’s question as “If it’s a possibility, > you should handle that case, but if it should never happen, do you want to > assert so with a DCHECK?” Oops, my mistake. Added a DCHECK. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:150: if (!CommandService::IsMediaKey(accelerator)) On 2013/12/10 16:30:32, Mark Mentovai wrote: > Rather than calling IsMediaKey multiple times, how about > > if (CommandService::IsMediaKey(accelerator)) { > if (!IsAnyMediaKeyRegistered()) { > StartWatchingMediaKeys(); > } > } else { > RegisterHotKey(accelerator, hot_key_id_); > } Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:168: int id = hot_key_ids_[accelerator]; On 2013/12/10 16:30:32, Mark Mentovai wrote: > There’s a typedef for this now, HotKeyId. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:168: int id = hot_key_ids_[accelerator]; On 2013/12/10 16:30:32, Mark Mentovai wrote: > What if wasn’t registered to begin with? Added a DCHECK https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:181: const ui::Accelerator& accelerator, int hot_key_id) { On 2013/12/10 16:30:32, Mark Mentovai wrote: > int → HotKeyId? Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:182: EventHotKeyRef hot_key_ref; On 2013/12/10 16:30:32, Mark Mentovai wrote: > Don’t declare until use, down where you call RegisterEventHotKey. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:196: unichar character; On 2013/12/10 16:30:32, Mark Mentovai wrote: > You don’t use either of these. You can eliminate them and pass NULL as the last > two parameters to ui::MacKeyCodeForWindowsKeyCode. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:211: int id = hot_key_ids_[accelerator]; On 2013/12/10 16:30:32, Mark Mentovai wrote: > What if it wasn’t registered to begin with? > > I asked the same question in UnregisterAccelerator on line 168, but > UnregisterAccelerator calls this function before it manipulates hot_key_ids_ or > the other maps, so this will actually be the first place in the current code > that would become a problem if a caller passed in an accelerator that wasn’t > registered to the public API UnregisterAccelerator. Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:211: int id = hot_key_ids_[accelerator]; On 2013/12/10 16:30:32, Mark Mentovai wrote: > int → HotKeyId? Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:222: DCHECK(event_tap_ == NULL); On 2013/12/10 16:30:32, Mark Mentovai wrote: > DCHECK(event_tap_source_ == NULL) too? Done. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:248: CFRunLoopRemoveSource(CFRunLoopGetCurrent(), event_tap_source_, On 2013/12/10 16:30:32, Mark Mentovai wrote: > DCHECK(event_tap_) and DCHECK(event_tap_source_) ? These are checked below, but I restructured to be clearer. https://codereview.chromium.org/60353008/diff/870001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:306: int key_code = ((data1 & 0xFFFF0000) >> 16); On 2013/12/10 16:30:32, Mark Mentovai wrote: > Outer (parentheses) unnecessary here and on line 313. Done.
Before I look at the latest patch set: Can you (or maybe Finnur) help explain the API as I found it in the base class? I see that StartListening and StopListening are pure virtual and implemented by each platform-specific derived class. They are called directly by the base RegisterAccelerator and UnregisterAccelerator when the first accelerator is added and the last accelerator is removed. This architecture actually suggests that the public interface is supposed to just be Register/UnregisterAccelerator, and that callers from outside this class are actually not expected to call Start/StopListening directly. Is this true? If so, Start/StopListening should both be moved to the private section. If they’re private, some of the paranoia around the implementation’s behaviors when Start/StopListening is mixed with Register/UnregisterAccelerator will evaporate, because it won’t be possible for Start/StopListening to be called other than by the implementation (via Register/UnregisterAccelerator).
Your understanding is correct, Mark. Making them global was probably just an oversight. They should only be called by the base class.
Thanks, Finnur. Based on that, I’d like to see a follow-up change to fix the API. As it stands now, someone’s going to find public methods called Stop/StartListening, and call them without thinking twice about it, and not realize what sorts of assumptions they’re breaking and what sorts of bugs they’re causing. Maybe it’ll work totally fine on one platform, so they won’t notice, all the while breaking the other platforms. No good. So… 1. Let’s move Start/StopListening into the private: section in the base class and all implementation subclasses. That addresses this API problem. 2. Let’s also move Register/UnregisterAccelerator into the public: section in the implementation subclasses. Right now, it’s public: in the base, but private: in the subclasses, even though the private platform-specific implementations are actually the public entry point in each case, accessed through the public base, which is virtual. I was confused for a while when I saw that these functions, which are obviously supposed to be the public API, were marked private in the implementation, and I wondered just how anyone would actually ever manage to register an accelerator, until I read the base class. It seems kind of silly that if you hold a GlobalShortcutListener* which is really implemented as GlobalShortcutListener*, you can call RegisterAccelerator, but if you hold a GlobalShortcutListenerMac* directly, you can’t. This can all be done in a single separate change (which can occur in parallel with this change), and I’d be happy to review it. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:48: static GlobalShortcutListenerMac *g_instance = This is no longer global, you don’t need the g_ prefix. Just “instance” is fine. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:68: StopListening(); If event_handler_ is true, then hot_key_ids_ will be non-empty, and this will trigger a DCHECK in StopListening. How do you want this to behave? This code here contradicts the assertions. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:78: DCHECK(!hot_key_ids_.empty()); // Don't start if no hot key registered. It’s not really “don’t start,” it’s “this should never happen and is fatal in a debug build.” https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:124: // MediaKeys can have multiple targets, all keyed off of the same I hadn’t noticed this property before, but now that I’m reading this comment more closely… Seems like the desired behavior here is to allow RegisterAccelerator to be called multiple times with the same accelerator and different observers. True? If so, what’s supposed to happen if you RegisterAccelerator twice for the same accelerator with two different observers (A and B), and then UnregisterAccelerator the accelerator with only one observer (B)? It would seem to me that the desired behavior under this regime would be to leave the accelerator hooked up and attached to observer A. But in reality, the implementation in UnregisterAccelerator seems like it’ll stop listening for this accelerator altogether, and although A is still hooked up in the base class, system events for that keystroke will never fire if a hot key, or events will be dropped in this class if a media key. If the desired behavior is in fact to only allow one observer to be registered for an accelerator, then I would think that you’d need to provide some mechanism (return value?) for this method to signal whether the accelerator was successfully registered or not. Otherwise, the caller has no way to know whether it needs to call UnregisterAccelerator. With the existing structure, something could wind up unregistering someone else’s accelerator or triggering a DCHECK failure or a crash if that happened. How’s it supposed to work? https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:220: // If no more hot keys are registered, remove the event handler. The comment says “if no more hot keys are registered,” but the code doesn’t seem to do that. The first UnregisterHotKey will cause any other registered hot keys to stop responding until a subsequent RegisterHotKey. It doesn’t seem like you tested this. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:283: bool GlobalShortcutListenerMac::IsAnyHotKeyRegistered() { Who calls this? It seems like you could use this to decide whether to Start/StopWatchingHotKeys if such methods existed. Did you intend to call this from UnregisterHotKey? The Start/StopWatchingHotKeys function isn’t a bad idea, as a nice parallel to Start/StopWatchingMediaKeys.
> How’s it supposed to work? You are correct. MediaKeys are, due to their nature, the exception to the one-target-per-shortcut rule and if two targets (A and B) register for a certain MediaKey and one unregisters (lets say B) then target A should still receive notifications when that MediaKey is struck.
Thanks. This CL should be fixed up to nail down those behaviors too. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:37: typedef int HotKeyId; Wait…to stay consistent in terminology, this is an AcceleratorId, right? Since you use it for both hot and media keys.
Added (Start|Stop)WatchingHotKeys for symmetry, also renamed HotKeyId and related structures. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.h:37: typedef int HotKeyId; On 2013/12/11 15:01:51, Mark Mentovai wrote: > Wait…to stay consistent in terminology, this is an AcceleratorId, right? Since > you use it for both hot and media keys. Renamed it to KeyId for brevity. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:48: static GlobalShortcutListenerMac *g_instance = On 2013/12/11 02:55:02, Mark Mentovai wrote: > This is no longer global, you don’t need the g_ prefix. Just “instance” is fine. Done. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:68: StopListening(); On 2013/12/11 02:55:02, Mark Mentovai wrote: > If event_handler_ is true, then hot_key_ids_ will be non-empty, and this will > trigger a DCHECK in StopListening. How do you want this to behave? This code > here contradicts the assertions. The comment above: // By this point, UnregisterAccelerator should have been called for all // keyboard shortcuts. Suggests that by this point event_handler_ should never be true. But I think it's worthwhile calling StopListening just in case, for parity with other platforms' implementations. I've brought it back to use is_listening_ which we should keep for similar reasons. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:78: DCHECK(!hot_key_ids_.empty()); // Don't start if no hot key registered. On 2013/12/11 02:55:02, Mark Mentovai wrote: > It’s not really “don’t start,” it’s “this should never happen and is fatal in a > debug build.” I think “this should never happen and is fatal in a debug build" is implied by the DCHECK. Removed comment. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:124: // MediaKeys can have multiple targets, all keyed off of the same On 2013/12/11 02:55:02, Mark Mentovai wrote: > I hadn’t noticed this property before, but now that I’m reading this comment > more closely… > > Seems like the desired behavior here is to allow RegisterAccelerator to be > called multiple times with the same accelerator and different observers. True? > > If so, what’s supposed to happen if you RegisterAccelerator twice for the same > accelerator with two different observers (A and B), and then > UnregisterAccelerator the accelerator with only one observer (B)? > > It would seem to me that the desired behavior under this regime would be to > leave the accelerator hooked up and attached to observer A. But in reality, the > implementation in UnregisterAccelerator seems like it’ll stop listening for this > accelerator altogether, and although A is still hooked up in the base class, > system events for that keystroke will never fire if a hot key, or events will be > dropped in this class if a media key. > > If the desired behavior is in fact to only allow one observer to be registered > for an accelerator, then I would think that you’d need to provide some mechanism > (return value?) for this method to signal whether the accelerator was > successfully registered or not. Otherwise, the caller has no way to know whether > it needs to call UnregisterAccelerator. With the existing structure, something > could wind up unregistering someone else’s accelerator or triggering a DCHECK > failure or a crash if that happened. > > How’s it supposed to work? Looks like I copied this code directly from the other implementations (Win, X11). So I don't understand how this is supposed to work either. Why are media keys the exception? Can't hot keys also be requested by multiple targets? How does this work in other platforms? https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:220: // If no more hot keys are registered, remove the event handler. On 2013/12/11 02:55:02, Mark Mentovai wrote: > The comment says “if no more hot keys are registered,” but the code doesn’t seem > to do that. The first UnregisterHotKey will cause any other registered hot keys > to stop responding until a subsequent RegisterHotKey. It doesn’t seem like you > tested this. Done. https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:283: bool GlobalShortcutListenerMac::IsAnyHotKeyRegistered() { On 2013/12/11 02:55:02, Mark Mentovai wrote: > Who calls this? It seems like you could use this to decide whether to > Start/StopWatchingHotKeys if such methods existed. Did you intend to call this > from UnregisterHotKey? The Start/StopWatchingHotKeys function isn’t a bad idea, > as a nice parallel to Start/StopWatchingMediaKeys. Done.
I’m going to wait to re-review this until the multiple key registration thing is ironed out. I think this is getting close now.
https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:124: // MediaKeys can have multiple targets, all keyed off of the same This should work the same way on all platforms: MediaKeys have multiple targets, other keys do not. Here's why: Lets assume you have two extensions, A and B, that have two wildly differing feature sets. A does a FindInPage replacement, the other replaces all images on the page with cat images. It doesn't make sense to bind the same shortcut (say Ctrl+F) to both of those features because you'd want them mutually exclusive -- It would be weird for all the images to turn to cats *and* the Find box to open at the same time. For MediaKeys that's a different story because they have special predefined system functions that are of interest to many extensions. You might have more than one recipient who wants to listen for say VolumeUp (Example: a music app and a video player, to name two). Exclusivity doesn't make sense in that scenario because it would mean you'd have to manually specify who is allowed to receive the event, and switch as you change apps.
https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/890001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:124: // MediaKeys can have multiple targets, all keyed off of the same And to clarify... If a second extension comes in and says "hey, I also want to listen to MediaStop" it is fine to return early here because we've already setup the listening framework for MediaStop. There's no need to have two of those. The base class keeps track of who wants to get notified about MediaStop and it is the responsibility of the base class to notify them (See GlobalShortcutListener::NotifyKeyPressed, which is called from OnHotKeyEvent and OnMediaKeyEvent above).
Right, the problem isn’t in RegisterAccelerator, it’s in UnregisterAccelerator. The Windows and X11 implementations (the only other subclasses that aren’t just stubs) seem to share this problem. None of the implementation subclasses actually track that multiple observers registered for a single accelerator, so the first time one of them tries to unregister from the accelerator, it stops being watched entirely. I also don’t see anything in the base class or the implementation subclasses that distinguishes between media keys and non-media accelerators. Nothing seems to enforce the behavior of allowing multiple observers for a media key but not for a non-media accelerator. All subclass RegisterAccelerator implementations have the comment that says that it’s explicitly OK to allow an already-registered shortcut because of media keys, but there doesn’t seem to be anything that checks that non-media keys don’t get multiple observers registered. ExtensionCommandsGlobalRegistry, currently the only caller, does have a DCHECK for this to make sure that its clients behave correctly, but since GlobalShortcutListener exposes its interface publicly to the entire application, it’s way too easy for someone else to come along as a client of that interface, successfully register an observer for an already-watched non-media accelerator, and never have any warning that they’re doing anything wrong. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:153: // Ensure this accelerator is registered. The Windows implementation has an entirely different approach: // We may get asked to unregister something that we couldn't register (for // example if the shortcut was already taken by another app), so we // need to handle that gracefully. HotkeyIdMap::iterator it = hotkey_ids_.find(accelerator); if (it == hotkey_ids_.end()) return; The X11 implementation is the same, but without the comment.
> Right, the problem isn’t in RegisterAccelerator, it’s in UnregisterAccelerator. > The Windows and X11 implementations (the only other subclasses that aren’t just > stubs) seem to share this problem. None of the implementation subclasses > actually track that multiple observers registered for a single accelerator, so > the first time one of them tries to unregister from the accelerator, it stops > being watched entirely. At first I thought no that can't be, but now I'm not so sure. Should be easy to test though, Boris? > I also don’t see anything in the base class or the implementation subclasses > that distinguishes between media keys and non-media accelerators. Nothing seems > to enforce the behavior of allowing multiple observers for a media key but not > for a non-media accelerator. Isn't this what you are looking for? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > All subclass RegisterAccelerator implementations > have the comment that says that it’s explicitly OK to allow an > already-registered shortcut because of media keys, but there doesn’t seem to be > anything that checks that non-media keys don’t get multiple observers > registered. ExtensionCommandsGlobalRegistry, currently the only caller, does > have a DCHECK for this to make sure that its clients behave correctly, but since > GlobalShortcutListener exposes its interface publicly to the entire application, > it’s way too easy for someone else to come along as a client of that interface, > successfully register an observer for an already-watched non-media accelerator, > and never have any warning that they’re doing anything wrong. If some of these concerns can be alleviated by moving functions into protected/private then I'm all for it. The test coverage we have already on other platforms is enough so that if it compiles and the tests pass on the bots then we are good to go.
On Wed, Dec 11, 2013 at 7:31 PM, <finnur@chromium.org> wrote: > The Windows and X11 implementations (the only other subclasses that aren’t >> > just > >> stubs) seem to share this problem. None of the implementation subclasses >> actually track that multiple observers registered for a single >> accelerator, so >> the first time one of them tries to unregister from the accelerator, it >> stops >> being watched entirely. >> > > At first I thought no that can't be, but now I'm not so sure. Should be > easy to > test though, Boris? It's true that every implementation of UnregisterAccelerator will do its platform-specific de-registration regardless of the number of observers bound to the accelerator. However, UnregisterAccelerator is only called if the target list is empty: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... So this is actually a safe thing to do, works on OS X (tested it), and should work across platforms. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
So then the answer is “the implementations are all broken but it doesn’t currently matter because there’s only one client and only one observer.” That’s fine until someone wants to add another client. There should be a followup to clean this all up for each platform. I’d be happy to review that, too. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes. As I recall, the GlobalShortcut* classes shouldn't need to worry about whether there is one target or many targets for the shortcut that was struck. Their job is to make it easy to listen for a particular shortcut and have that event delivered to the observers.
Mark/Finnur, are there pending issues on this particular CL? Thanks. On Thu, Dec 12, 2013 at 10:55 AM, <finnur@chromium.org> wrote: > Yes. As I recall, the GlobalShortcut* classes shouldn't need to worry about > whether there is one target or many targets for the shortcut that was > struck. > Their job is to make it easy to listen for a particular shortcut and have > that > event delivered to the observers. > > https://codereview.chromium.org/60353008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
This is currently in a state where it’s no less capable or different than the other existing platform implementations, except for the one behavior difference noted below. However, during the course of this review, lots of areas for improvement in the base class, the caller, and this and other implementations were identified. What’s the plan for addressing that feedback? Are there any bugs filed, and has anyone signed up to do the work? https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:71: // This should never happen. (nit) lowercase T, since you’re continuing the sentence. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:72: if (IsAnyMediaKeyRegistered()) If you’re going to go to the trouble of stopping listening for media keys in this never-should-happen case, you should do the same for hot keys. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:154: DCHECK(accelerator_ids_.find(accelerator) != accelerator_ids_.end()); If the goal of this CL is to achieve parity with existing platform implementations, even where their behaviors are weird, you should fix this. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:161: KeyId id = accelerator_ids_[accelerator]; Avoid naming a variable id in an Objective-C file (this is) because that’s an Objective-C type name. Call this one key_id. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:212: KeyId id = accelerator_ids_[accelerator]; Same thing here. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:374: (nit) Extra blank line.
https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:120: void GlobalShortcutListenerMac::RegisterAccelerator( Can you add CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); to this and UnregisterAccelerator? Since they’re public entry points to here, you want to make sure that nobody calls them from the wrong thread, since this class is not thread-safe. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:178: (nit) Extra blank line.
LGTM w/ nits from my side, though please address my question and Mark's comments as well. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:81: CGEventSourceCreate(kCGEventSourceStateHIDSystemState); nit: indent 2 more spaces https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:112: #if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_MACOSX) nit: 80 columns https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:174: if (!IsAnyHotKeyRegistered() && event_handler_) { Why the |&& event_handler_| check here?
Thanks for another round of feedback. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:81: CGEventSourceCreate(kCGEventSourceStateHIDSystemState); On 2013/12/12 20:33:10, rsesek wrote: > nit: indent 2 more spaces Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:112: #if defined(OS_WIN) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_MACOSX) On 2013/12/12 20:33:10, rsesek wrote: > nit: 80 columns Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:71: // This should never happen. On 2013/12/12 19:51:20, Mark Mentovai wrote: > (nit) lowercase T, since you’re continuing the sentence. Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:72: if (IsAnyMediaKeyRegistered()) On 2013/12/12 19:51:20, Mark Mentovai wrote: > If you’re going to go to the trouble of stopping listening for media keys in > this never-should-happen case, you should do the same for hot keys. Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:120: void GlobalShortcutListenerMac::RegisterAccelerator( On 2013/12/12 20:04:50, Mark Mentovai wrote: > Can you add > > CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > > to this and UnregisterAccelerator? Since they’re public entry points to here, > you want to make sure that nobody calls them from the wrong thread, since this > class is not thread-safe. Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:153: // Ensure this accelerator is registered. On 2013/12/11 22:59:57, Mark Mentovai wrote: > The Windows implementation has an entirely different approach: > > // We may get asked to unregister something that we couldn't register (for > // example if the shortcut was already taken by another app), so we > // need to handle that gracefully. > HotkeyIdMap::iterator it = hotkey_ids_.find(accelerator); > if (it == hotkey_ids_.end()) > return; > > The X11 implementation is the same, but without the comment. Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:154: DCHECK(accelerator_ids_.find(accelerator) != accelerator_ids_.end()); On 2013/12/12 19:51:20, Mark Mentovai wrote: > If the goal of this CL is to achieve parity with existing platform > implementations, even where their behaviors are weird, you should fix this. Ok, switched to use the same approach as on other platforms. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:161: KeyId id = accelerator_ids_[accelerator]; On 2013/12/12 19:51:20, Mark Mentovai wrote: > Avoid naming a variable id in an Objective-C file (this is) because that’s an > Objective-C type name. Call this one key_id. Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:174: if (!IsAnyHotKeyRegistered() && event_handler_) { On 2013/12/12 20:33:10, rsesek wrote: > Why the |&& event_handler_| check here? Fair point, this should never happen (which is captured by the DCHECK in StopWatchingHotKeys) https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:178: On 2013/12/12 20:04:50, Mark Mentovai wrote: > (nit) Extra blank line. Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:212: KeyId id = accelerator_ids_[accelerator]; On 2013/12/12 19:51:20, Mark Mentovai wrote: > Same thing here. Done. https://codereview.chromium.org/60353008/diff/950001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:374: On 2013/12/12 19:51:20, Mark Mentovai wrote: > (nit) Extra blank line. Done.
I think this is all baked now. Can you or Finnur speak to the next-step concerns I raised in https://codereview.chromium.org/60353008/#msg55 ?
I don't have a problem with addressing concerns raised with the architecture and I think we should probably articulate those concerns in the bug database. I haven't had time to really sink my teeth into this CL as I've been a little head over heels with other things this week and will be travelling from the US to Europe on Sunday. That also means that I don't fully grok the concerns that have been raised, so I'm probably not a good candidate to articulate them. Boris, can you work with Mark to articulate these concerns and log actionable items in the bug database, that one of me/you/Chaobin can tackle maybe next week?
LGTM. Let’s enumerate the areas for ongoing improvements. Thanks for hanging in there.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/smus@chromium.org/60353008/970001
On 2013/12/12 22:21:00, Finnur wrote: > I don't have a problem with addressing concerns raised with the architecture and > I think we should probably articulate those concerns in the bug database. I > haven't had time to really sink my teeth into this CL as I've been a little head > over heels with other things this week and will be travelling from the US to > Europe on Sunday. That also means that I don't fully grok the concerns that have > been raised, so I'm probably not a good candidate to articulate them. > > Boris, can you work with Mark to articulate these concerns and log actionable > items in the bug database, that one of me/you/Chaobin can tackle maybe next > week? I am available for following up once these concerns is articulated in the issue list/bug database. Thanks for the opportunity.
Message was sent while issue was closed.
Change committed as 240536
Message was sent while issue was closed.
Tests EventUtilsTest.TestEventFlagsFromNSEvent and EventUtilsTest.TestWindowOpenDispositionFromNSEvent have become flaky in the range 240532-240543 I am suspecting this change. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
Message was sent while issue was closed.
I did not find anything I believe is a more probable cause for the new failure on mac 10.6 and 10.8 I find it very strange that the key and mouse event values would be flaky. Perhaps a race condition or uninitialized data? Anyways, let's roll out this CL and we'll see... If the failures don't go away I will roll it back in.
Message was sent while issue was closed.
Junov, I fail to see how this changelist could possibly cause those tests to fail. The whole feature is behind a flag (--enable-global-commands), the object Boris is adding isn't even constructed unless the flag is given and the only change to common code is trivial.
Yeah, he explained that to me. The failures went away after the revert though. As soon as the blink roll goes through, I will revert the revert, and we'll see what happens. On Fri, Dec 13, 2013 at 3:04 PM, <finnur@chromium.org> wrote: > Junov, I fail to see how this changelist could possibly cause those tests > to > fail. The whole feature is behind a flag (--enable-global-commands), the > object > Boris is adding isn't even constructed unless the flag is given and the > only > change to common code is trivial. > > https://codereview.chromium.org/60353008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
https://codereview.chromium.org/60353008/diff/970001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/60353008/diff/970001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_mac.mm:49: new GlobalShortcutListenerMac(); Hm... Who will delete this object?
Message was sent while issue was closed.
I filed http://crbug.com/329616 for follow-up refinements. We should continue the discussion of the required follow-ups on that bug and on the changes that wind up hanging off of it.
Message was sent while issue was closed.
Just to document what happened with this Patch: the revert was reverted, the test failures re-appeared, so the patch was reverted again. So we have pretty strong evidence that this CL interferes with ui_unittests. Ironically, ui_unittest does run any of the code touched in this patch. In fact it doesn't even link against the code in this patch. My theory is that code changes in extension_commands_global_registry_apitest.cc are probably making some state changes that are not being restored, and subsequent unit tests are behaving as if a modifier key (Alt, control, command) were being held down while the test is executing. |
