Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
109413003:
Refactor GlobalShortcutListener. (Closed)
|
Created:
7 years ago by zhchbin Modified:
6 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Boris Smus Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRefactor GlobalShortcutListener.
1. Move method [Start/Stop]Listening from public to private.
2. The change of relationship between accelerator and observer: from
"one-to-many" to "one to one".
3. Better name: use suffix "Impl" for the platform-specified implementation
methods: |RegisterAcceleratorImpl| and |UnregisterAcceleratorImpl|.
BUG=329616, 302437
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242995
Patch Set 1 #Patch Set 2 : Stuff of other platforms. [Havn't compiled yet.] #Patch Set 3 : Fix linux keyboard code transfrom. #
Total comments: 20
Patch Set 4 : Address comments. #Patch Set 5 : Update as comments. #
Total comments: 3
Patch Set 6 : #Patch Set 7 : Clearer. #
Total comments: 2
Patch Set 8 : Clearer logic. #
Total comments: 57
Patch Set 9 : Address Mark's comments. #
Total comments: 27
Patch Set 10 : Another round #Patch Set 11 : sync. #Patch Set 12 : Sync. #Messages
Total messages: 37 (0 generated)
Finnur, please help me start try bot jobs. This is a refactor of GlobalShortcutListener, according to my understanding based on the discussion between you and Mark. The most biggest change is the relationship between accelerator/shortcut and observer, now it is one-to-one, no matter whether the accelerator is one of the multimedia keys or not. I think we'd better not to cover this logic since we have did it in the component of ExtensionKeybindingRegistry. And now GlobalShortcutListener can be one of basic infrastructure, which can be useful for "Issue 192654: Add ability to easily set a global shortcut to the app launcher" from my point of view. If you think this is ready for review, then I will add Mark (For the refactor) and Elliot(For the change of X11 part, there is a fix: the conversion between ui::KeyboardCode, X KeySym and X KeyCode) in. By the way, I'm thinking about adding a *unittest* for this class. Do you think the way that mock a observer, call |NotifyKeyPressed| directly instead of simulating a keyboard event can be acceptable? This could be easier. :) FYI to Boris.
On 2013/12/16 03:12:15, zhchbin wrote: > Finnur, please help me start try bot jobs. > > This is a refactor of GlobalShortcutListener, according to my understanding > based on the discussion between you and Mark. The most biggest change is the > relationship between accelerator/shortcut and observer, now it is one-to-one, no > matter whether the accelerator is one of the multimedia keys or not. I think > we'd better not to cover this logic since we have did it in the component of > ExtensionKeybindingRegistry. And now GlobalShortcutListener can be one of basic > infrastructure, which can be useful for "Issue 192654: Add ability to easily set > a global shortcut to the app launcher" from my point of view. > > If you think this is ready for review, then I will add Mark (For the refactor) > and Elliot(For the change of X11 part, there is a fix: the conversion between > ui::KeyboardCode, X KeySym and X KeyCode) in. > > By the way, I'm thinking about adding a *unittest* for this class. Do you think > the way that mock a observer, call |NotifyKeyPressed| directly instead of > simulating a keyboard event can be acceptable? This could be easier. :) > > FYI to Boris. I just flew back from the US to Europe and am a bit too jetlagged still to fully grok this change. I'll take a look at it tomorrow. Sorry for the delay.
Wasn't there a bug filed for the cleanup specifically? Maybe add that to the BUG= list? https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.cc:25: // Platform-register failed, mostly likely the shortcut has been registered by s/Platform-register failed/If the platform-specific registration fails/ https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.cc:27: if (RegisterAcceleratorImpl(accelerator) == false) if (!RegisterAcceleratorImpl(accelerator)) https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:37: virtual bool RegisterAccelerator( Why is this returning bool now? The call sites have not been updated to act on this data... https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:41: const ui::Accelerator& accelerator, Observer* observer); Do these functions need to be virtual still? (Maybe for testing?) https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:47: // is struck. Only called for keys that have observer registered. nit: Add 'an' in front of observer. https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_win.cc:74: DCHECK(hotkey_ids_.find(accelerator) == hotkey_ids_.end()); It is probably good to DCHECK that we don't try to register two observers, but I don't think this will ever hit because of the early return at the top of GlobalShortcutListener::RegisterAccelerator().
> Wasn't there a bug filed for the cleanup specifically? > Maybe add that to the > BUG= list? I didn't receive the email about this cleanup. Maybe Mark/Boris have did it but I didn't get it (I searched in the crbug.com also)? Or I need to add one to crbug.com? https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.cc:25: // Platform-register failed, mostly likely the shortcut has been registered by On 2013/12/17 09:58:31, Finnur wrote: > s/Platform-register failed/If the platform-specific registration fails/ Done. https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.cc:27: if (RegisterAcceleratorImpl(accelerator) == false) On 2013/12/17 09:58:31, Finnur wrote: > if (!RegisterAcceleratorImpl(accelerator)) Done. https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:37: virtual bool RegisterAccelerator( On 2013/12/17 09:58:31, Finnur wrote: > Why is this returning bool now? The call sites have not been updated to act on > this data... The caller should know whether the accelerator register successfully or now? But now the only call site: ExtensionCommandsGlobalRegistry::AddExtensionKeybinding do not care about this result yet, which should be improved I know. But I didn't make up my mind about what we can do there yet. https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:41: const ui::Accelerator& accelerator, Observer* observer); On 2013/12/17 09:58:31, Finnur wrote: > Do these functions need to be virtual still? (Maybe for testing?) Done. Oh, sorry, I forget to remove the "virtual". https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:47: // is struck. Only called for keys that have observer registered. On 2013/12/17 09:58:31, Finnur wrote: > nit: Add 'an' in front of observer. Done. https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_win.cc:74: DCHECK(hotkey_ids_.find(accelerator) == hotkey_ids_.end()); On 2013/12/17 09:58:31, Finnur wrote: > It is probably good to DCHECK that we don't try to register two observers, but I > don't think this will ever hit because of the early return at the top of > GlobalShortcutListener::RegisterAccelerator(). Hm... I have removed the |observer| from the function parameters. And DCHECK that we don't try to register two observers here is also never hitted, because the GlobalShortcutListener::RegisterAccelerator will return false if it happened. So, let's remove the DCHECK here.
https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:37: virtual bool RegisterAccelerator( s/or now/or not s/do not care/doesn't care
https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:37: virtual bool RegisterAccelerator( OK, so if RegisterAccelerator fails then we could skip adding the |target| to the |event_targets_| map. Hey.. then we won't get calls to unregister something we didn't register, and we can DCHECK instead of early returning in places like GlobalShortcutListenerWin::UnregisterAccelerator(). Can you try that? https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_win.cc:74: DCHECK(hotkey_ids_.find(accelerator) == hotkey_ids_.end()); Thanks. My point was also: Why are we early returning instead of DCHECKing in RegisterAccelerator? Is that because of media keys? If so, maybe document that fact?
https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:37: virtual bool RegisterAccelerator( On 2013/12/17 11:47:25, Finnur wrote: > OK, so if RegisterAccelerator fails then we could skip adding the |target| to > the |event_targets_| map. Hey.. then we won't get calls to unregister something > we didn't register, and we can DCHECK instead of early returning in places like > GlobalShortcutListenerWin::UnregisterAccelerator(). > > Can you try that? Done. Yeah, that's what we can do about |event_targets_|. However, the code I just added is tricky (See the extension_commands_global_registry.cc in the new patch.) Actually I'm thinking about how we can tell the users that their global hot key fail to register. "GlobalShortcutListener::UnregisterAccelerator" (Without |Win|)? https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_win.cc:74: DCHECK(hotkey_ids_.find(accelerator) == hotkey_ids_.end()); On 2013/12/17 11:47:25, Finnur wrote: > Thanks. My point was also: Why are we early returning instead of DCHECKing in > RegisterAccelerator? Is that because of media keys? If so, maybe document that > fact? I changed the relationship between Accelerator and Observer to one-to-one, which means that callers can register one accelerator successfully only when it is available (It haven't taken by callers or other native applications). So the logic in the |RegisterAccelerator| uses early return instead of DCHECKs. Comment updated. https://codereview.chromium.org/109413003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry.cc:92: iter->second.accelerator(), this)) You may ask why I didn't call RegisterAccelerator early, before the |push_back|. Think about multiply media keys.
https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener.h:37: virtual bool RegisterAccelerator( > However, the code I just > added is tricky Not anymore... (see my comment) > Actually I'm thinking about how we can tell the users that their global > hot key fail to register. I wouldn't worry about it. They get instant feedback on the chrome://extensions page when they try to set it and it doesn't work. We could improve it there, but I wouldn't worry about it in this context. > "GlobalShortcutListener::UnregisterAccelerator" (Without |Win|)? Not sure what you are telling me here... https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_win.cc:74: DCHECK(hotkey_ids_.find(accelerator) == hotkey_ids_.end()); Actually, when I think about it I now realize I probably made a mistake asking you to remove this. Sorry... https://codereview.chromium.org/109413003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry.cc:92: iter->second.accelerator(), this)) Wondering if this would be simpler (you'll need to remove lines 79-80) (and this is pseudo, haven't compiled it)... size_t target_count = event_targets_.find(iter->second.accelerator()) != event_targets_.end() ? event_targets_[iter->second.accelerator()].size() : 0; if (extensions::CommandService::IsMediaKey(iter->second.accelerator()) && target_count > 0U) continue; // Shortcut listener already listening for this media key. } DCHECK_EQ(0U, target_count); if (GlobalShortcutListener::GetInstance()->RegisterAccelerator( iter->second.accelerator(), this)) { event_targets_[iter->second.accelerator()].push_back( std::make_pair(extension->id(), iter->second.command_name())); }
https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/40001/chrome/browser/extension... chrome/browser/extensions/global_shortcut_listener_win.cc:74: DCHECK(hotkey_ids_.find(accelerator) == hotkey_ids_.end()); On 2013/12/17 14:31:39, Finnur wrote: > Actually, when I think about it I now realize I probably made a mistake asking > you to remove this. Sorry... Hm... Added back now. https://codereview.chromium.org/109413003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_commands_global_registry.cc:92: iter->second.accelerator(), this)) On 2013/12/17 14:31:40, Finnur wrote: > Wondering if this would be simpler (you'll need to remove lines 79-80) (and this > is pseudo, haven't compiled it)... > > size_t target_count = > event_targets_.find(iter->second.accelerator()) != event_targets_.end() ? > event_targets_[iter->second.accelerator()].size() : 0; > > if (extensions::CommandService::IsMediaKey(iter->second.accelerator()) && > target_count > 0U) > continue; // Shortcut listener already listening for this media key. Thanks very much! In this case we should also push a pair (extension id, command name) to the event_targets. Otherwise the test case "GlobalCommandsApiTest.GlobalDuplicatedMediaKey" will fail. Done with also adding a pair. > } > > DCHECK_EQ(0U, target_count); > if (GlobalShortcutListener::GetInstance()->RegisterAccelerator( > iter->second.accelerator(), this)) { > event_targets_[iter->second.accelerator()].push_back( > std::make_pair(extension->id(), iter->second.command_name())); > }
https://codereview.chromium.org/109413003/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:95: } Good catch. But I think a better alternative is (pseudo): if (!mediakey || target_count == 0) { DCHECK_EQ(0U, target_count); if (!GlobalShortcutListener::GetInstance()->RegisterAccelerator( accelerator, this)) continue; } event_targets_[accelerator].push_back( std::make_pair(extension->id(), iter->second.command_name()));
https://codereview.chromium.org/109413003/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:95: } On 2013/12/17 16:35:45, Finnur wrote: > Good catch. But I think a better alternative is (pseudo): > > if (!mediakey || target_count == 0) { > DCHECK_EQ(0U, target_count); > if (!GlobalShortcutListener::GetInstance()->RegisterAccelerator( > accelerator, this)) > continue; > } > > event_targets_[accelerator].push_back( > std::make_pair(extension->id(), iter->second.command_name())); Done. See the newest patch, actually the problem is simple, sorry for messing up.
LGTM
Mark, please review the refactor of GlobalShortcutListener. Elliot, please take a look at the change of X11 part, there is a fix: the conversion between ui::KeyboardCode, X KeySym and X KeyCode). Do you still remember last time review of implementation of GlobalShortcutListenerX11? :) https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.cc (left): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.cc:17: static base::LazyInstance<extensions::GlobalShortcutListenerWin> instance = Quote from the Mark: the lazy instance used to implement Instance(), which was determined to be overkill because this code is single-threaded. Finnur, WDYT? From what I have seen so far, GlobalShortcutListener is singleton, so it's OK to use it!! And if we implement it like (Code from mac implementation: https://codereview.chromium.org/60353008/patch/1010001/1020005 that suggested by Mark) : +// static +GlobalShortcutListener* GlobalShortcutListener::GetInstance() { + CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + static GlobalShortcutListenerMac *instance = + new GlobalShortcutListenerMac(); + return instance; +} We will have a memory leak: we didn't delete the |instance|.
Additionally, how about the thread CHECKs that I proposed? https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:91: DCHECK_EQ(1u, event_targets_[accelerator].size()); Is this the only check for multiple registration of media keys? It looks like it, unless there’s some other caller class that keeps track of it that I’m not aware of. Structuring this check as a DCHECK means that in release builds, two extensions will be able to request the same accelerator, and they will succeed. And in debug builds, two extensions doing the same thing will trigger a crash. Because extensions are entirely outside of our control, this isn’t the right approach. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:10: #include "base/observer_list.h" This #include is obsolete now. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:37: // been registered by callers or other native applications. We don’t support recognizing that an accelerator has been registered by another application on all platforms. The comment should specify that that this is a per-platform consideration. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:51: // Implemented by platform-specific implementations of this class. These four can be private. The base class calls them through their vtables, so it will have access to them. “protected” only gives the subclasses access to the base class’ version, but since they’re pure virtual, there is no base class version and “protected” doesn’t mean anything different than “private.” https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:60: typedef std::map< ui::Accelerator, Observer* > AcceleratorMap; No spaces inside the angle brackets. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo... under “Templates and Casts.” https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.h:26: // GlobalShortcutListener implementation. You should definitely discuss this change with Boris, because colliding with his work that’s been checked in and out and in and out without coordinating with him would be not so nice. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.cc (left): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.cc:17: static base::LazyInstance<extensions::GlobalShortcutListenerWin> instance = zhchbin wrote: > Quote from the Mark I am “the Mark” now! :) https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.cc:18: LAZY_INSTANCE_INITIALIZER; It’s true that lazy instances register to clean themselves up at exit, but we don’t actually care about doing unnecessary cleanup at exit. We’d rather exit rapidly than waste time tearing down things that don’t need to be torn down. You will find code that does this all over Chrome, either via statics pointers that are never deleted as I suggested in the Mac implementation review, or “leaky” lazy instances. Note that for the purposes of our memory analysis tools, these aren’t actually leaks, because the pointers are still stored in the relevant variable. A leak is only considered to occur when a pointer is lost without deallocating what it pointed to.
https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:91: DCHECK_EQ(1u, event_targets_[accelerator].size()); When you set a shortcut the current code removes the previous assignment, so this is really just an additional sanity check meant to catch when someone changes that code and makes a mistake. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.h:26: // GlobalShortcutListener implementation. I checked in Boris' CL (with the test disabled). This will tell us whether the test really is the cause of the failure or if it is something else. If it is the test's fault, then I think we should keep his CL in the tree and let him iterate on trunk getting the test right (so that he doesn't have to suffer through the refactorings, for one).
Mark, I will sync the code and address your comments tomorrow! Now it's midnight in China, sorry for the delay. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.h:26: // GlobalShortcutListener implementation. On 2013/12/19 15:24:13, Mark Mentovai wrote: > You should definitely discuss this change with Boris, because colliding with his > work that’s been checked in and out and in and out without coordinating with him > would be not so nice. I have cc'ed Boris in the first round of review. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.cc (left): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.cc:17: static base::LazyInstance<extensions::GlobalShortcutListenerWin> instance = On 2013/12/19 15:24:13, Mark Mentovai wrote: > zhchbin wrote: > > Quote from the Mark > > I am “the Mark” now! :) Oh, sorry for the typo. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.cc:18: LAZY_INSTANCE_INITIALIZER; Thanks for the detailed explanation, so different from what I have learned in school. On 2013/12/19 15:24:13, Mark Mentovai wrote: > It’s true that lazy instances register to clean themselves up at exit, but we > don’t actually care about doing unnecessary cleanup at exit. We’d rather exit > rapidly than waste time tearing down things that don’t need to be torn down. You > will find code that does this all over Chrome, either via statics pointers that > are never deleted as I suggested in the Mac implementation review, or “leaky” > lazy instances. Note that for the purposes of our memory analysis tools, these > aren’t actually leaks, because the pointers are still stored in the relevant > variable. A leak is only considered to occur when a pointer is lost without > deallocating what it pointed to.
https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:86: event_targets_[accelerator].push_back( Protected data members that are maintained by a class and its subclass are difficult to deal with conceptually. They’re also forbidden by the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... This member, event_targets_, is a protected member of the superclass, ExtensionKeybindingRegistry. It’s used in this function and in this class’ (the subclass’) destructor. It’s also used in the superclass’ RemoveExtensionKeybinding and NotifyEventTargets. There are other subclasses of ExtensionKeybindingRegistry, and they also operate on event_targets_ directly. It’s much harder to understand what’s going on and to maintain the code when a data member isn’t fully encapsulated. It also leads to duplication of code. For example, most or all of the subclasses have code just like this, or almost exactly like this: event_targets_[accelerator].push_back(std::make_pair(…, …)); if (!extensions::CommandService::IsMediaKey(accelerator)) DCHECK_EQ(1u, event_targets_[accelerator].size()); which is also written write here. This says to me that the code really belongs somewhere in the superclass. We shouldn’t be writing the same DCHECK all over the place. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:91: DCHECK_EQ(1u, event_targets_[accelerator].size()); Finnur wrote: > When you set a shortcut the current code removes the previous assignment, OK, I found chrome/browser/extensions/api/commands/command_service.cc extensions::CommandService::AddKeybindingPref which does this at its |bindings->HasKey(key)| check. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:100: GlobalShortcutListener::GetInstance()->UnregisterAccelerator( A similar question. What happens if: 1. extension A registers for a key 2. extension B registers for the same key, and bumps extension A off of it 3. extension A unregisters from the key I would hope that this wouldn’t actually remove extension B’s registration.
https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:86: event_targets_[accelerator].push_back( Glad you are reviewing this... :) I guess the superclass should make event_targets_ private and have an AddAcceleratorTarget, which encapsulates this logic? https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:100: GlobalShortcutListener::GetInstance()->UnregisterAccelerator( Correct. Step 2 can only happen if a user assigns extension B the same key that extension A was using, but at that point the sequence is actually 1, 3 and then 2.
As far as I’m concerned, expanding the fixes to include extension_commands_global_registry.h and its other subclasses can happen in a separate change. It doesn’t all need to be piled into this one. I’ll note it on the bug. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:86: event_targets_[accelerator].push_back( Finnur wrote: > Glad you are reviewing this... :) > > I guess the superclass should make event_targets_ private and have an > AddAcceleratorTarget, which encapsulates this logic? Yeah. There should also be a method that looks up a TargetList by ui::Accelerator (to replace .find(), needed for extension_keybinding_registry_gtk.cc HasPriorityHandler and extension_keybinding_registry_cocoa.mm ProcessKeyEvent) and a way to access all of the ui::Accelerators contained in event_targets_ (to replace .begin()/.end(), needed for this file and extension_keybinding_registry_views.cc in their destructors). These new methods should all be marked protected.
https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:8: #include "chrome/browser/chrome_notification_types.h" This isn’t used in here. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:10: #include "chrome/browser/extensions/extension_keybinding_registry.h" This isn’t necessary because the associated header (extension_commands_global_registry.h) has already #included this. This is part of our standard “include what you use” interpretation, but I can’t find a good reference that discusses it right now. The gist is that if you need something from extension_keybinding_registry.h in myfile.cc, it must be #included either in myfile.cc or myfile.h, but doesn’t need to be #included in both. (You would only #include it in myfile.h if it was needed there.) https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:11: #include "chrome/browser/extensions/extension_service.h" I don’t believe this one is used in this file. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:41: return &g_factory.Get(); This class is also single-threaded, right? (I’m assuming so because, although it lacks the thread CHECKs right now, there’s no protection against data being accessed on multiple threads simultaneously.) You can turn this lazy instance into a static pointer here too, as we were discussing elsewhere. And then you can drop lazy_instance.h. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:50: (nit) Extra blank line. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.cc:6: #include "chrome/browser/profiles/profile.h" I don’t believe this is needed here. In myfile.cc, we usually leave a blank line between #include "myfile.h" and the next group of #includes. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:12: #include "ui/gfx/native_widget_types.h" I don’t believe this is needed here. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_chromeos.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.h:8: #include "base/lazy_instance.h" This shouldn’t be needed here. It should be in the .cc instead (it’s currently missing from there) but may ultimately not be needed there either given other comments. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.h:24: friend struct base::DefaultLazyInstanceTraits<GlobalShortcutListenerChromeOS>; And this shouldn’t be needed either. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.h:32: const ui::Accelerator& accelerator) OVERRIDE; Good call on removing the unused “observer” parameter from the subclasses, which never used it. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.h:8: #include "base/lazy_instance.h" Same. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_ozone.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_ozone.h:8: #include "base/lazy_instance.h" Same. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.cc:18: LAZY_INSTANCE_INITIALIZER; zhchbin wrote: > Thanks for the detailed explanation, so different from what I have learned in > school. Generally we are pretty adamant about releasing resources when we’re done with them, but in the case where we’re not done with them until the process exits, and we know the system will clean up after us, it’s more expedient to just exit and not have to spend time tearing things down or deal with dependencies being deleted before their dependents. Only in cases where a resource leak would be truly global and persist after a process exits do we need to really worry about cleaning up after ourselves at exit. (This isn’t a general-purpose guideline: if you were developing a library instead of an application, you might want to have a way to shut down your library or allow it to be unloaded, and then you’d still want to do this cleanup because your code’s lifetime might be shorter than that of the process that hosts you.) For more information on how and why we arrived at the decisions we did, you can search chromium-dev for atexit. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.h:10: #include "base/lazy_instance.h" Same. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_x11.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.h:8: #include <set> C system headers before C++ system headers: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.h:11: #include "base/lazy_instance.h" Same.
Please take a look again. The code has synced with Finnur's CL: https://codereview.chromium.org/105713005. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:8: #include "chrome/browser/chrome_notification_types.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > This isn’t used in here. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:10: #include "chrome/browser/extensions/extension_keybinding_registry.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > This isn’t necessary because the associated header > (extension_commands_global_registry.h) has already #included this. This is part > of our standard “include what you use” interpretation, but I can’t find a good > reference that discusses it right now. The gist is that if you need something > from extension_keybinding_registry.h in myfile.cc, it must be #included either > in myfile.cc or myfile.h, but doesn’t need to be #included in both. (You would > only #include it in myfile.h if it was needed there.) Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:11: #include "chrome/browser/extensions/extension_service.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > I don’t believe this one is used in this file. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:41: return &g_factory.Get(); I have no idea currently, but it follows the instructions of ProfileKeyedAPIFactory. See: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... On 2013/12/19 21:03:17, Mark Mentovai wrote: > This class is also single-threaded, right? (I’m assuming so because, although it > lacks the thread CHECKs right now, there’s no protection against data being > accessed on multiple threads simultaneously.) You can turn this lazy instance > into a static pointer here too, as we were discussing elsewhere. And then you > can drop lazy_instance.h. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:50: On 2013/12/19 21:03:17, Mark Mentovai wrote: > (nit) Extra blank line. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.cc:6: #include "chrome/browser/profiles/profile.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > I don’t believe this is needed here. > > In myfile.cc, we usually leave a blank line between #include "myfile.h" and the > next group of #includes. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:10: #include "base/observer_list.h" On 2013/12/19 15:24:13, Mark Mentovai wrote: > This #include is obsolete now. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:12: #include "ui/gfx/native_widget_types.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > I don’t believe this is needed here. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:37: // been registered by callers or other native applications. On 2013/12/19 15:24:13, Mark Mentovai wrote: > We don’t support recognizing that an accelerator has been registered by another > application on all platforms. The comment should specify that that this is a > per-platform consideration. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:51: // Implemented by platform-specific implementations of this class. On 2013/12/19 15:24:13, Mark Mentovai wrote: > These four can be private. The base class calls them through their vtables, so > it will have access to them. “protected” only gives the subclasses access to the > base class’ version, but since they’re pure virtual, there is no base class > version and “protected” doesn’t mean anything different than “private.” Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:60: typedef std::map< ui::Accelerator, Observer* > AcceleratorMap; On 2013/12/19 15:24:13, Mark Mentovai wrote: > No spaces inside the angle brackets. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo... > under “Templates and Casts.” Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_chromeos.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.h:8: #include "base/lazy_instance.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > This shouldn’t be needed here. It should be in the .cc instead (it’s currently > missing from there) but may ultimately not be needed there either given other > comments. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.h:24: friend struct base::DefaultLazyInstanceTraits<GlobalShortcutListenerChromeOS>; On 2013/12/19 21:03:17, Mark Mentovai wrote: > And this shouldn’t be needed either. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_mac.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.h:8: #include "base/lazy_instance.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > Same. Done after sync with https://codereview.chromium.org/105713005/. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_ozone.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_ozone.h:8: #include "base/lazy_instance.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > Same. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_win.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_win.h:10: #include "base/lazy_instance.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > Same. Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_x11.h (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.h:8: #include <set> On 2013/12/19 21:03:17, Mark Mentovai wrote: > C system headers before C++ system headers: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... Done. https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.h:11: #include "base/lazy_instance.h" On 2013/12/19 21:03:17, Mark Mentovai wrote: > Same. Done.
This is already much easier to follow, more maintainable, and a stronger design. Great! https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:41: return &g_factory.Get(); zhchbin wrote: > I have no idea currently, but it follows the instructions of > ProfileKeyedAPIFactory. > > See: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... That says that you need to provide a GetFactoryInstance method. It doesn’t say that you have to use LazyInstance, it just uses LazyInstance as an example. This should work and be more direct as an implementation of GetFactoryInstance: CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); static ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry>* instance = new ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry>(); return instance; Note the added thread-safety CHECK. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.cc (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.cc:29: // The accelerator has been registered. This is a personal preference, but I find comments like this that say something about application state to be most effective when they’re written in the block that they apply to, instead of outside of it. So in this case: if (it != accelerator_map_.end()) { // The accelerator has been registered. return false; } since the accelerator hasn’t necessarily been registered unless it was found in the map, and only then do you want to “return false.” (You’d need to add the {braces} now because the conditioned block takes up more than one line.) https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.cc:51: DCHECK(it != accelerator_map_.end()); Good. Does DCHECK_NE work on this line, and DCHECK_EQ on line 53? https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:35: // true if register successfully, false if the specificied |accelerator| has Minor grammar nit: “Returns true if registered successfully, or false if…” https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:36: // been registered by callers or other native applications. Note that we don’t Instead of “by callers,” say “by another caller” https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:36: // been registered by callers or other native applications. Note that we don’t The apostrophe in “don’t” is a Unicode right single quote (U+2019) in UTF-8. We typically just use ASCII in source code. You don’t have to change this unless something breaks, but I’m mentioning it because something might break: the PRESUBMIT line length check, for example, might not understand UTF-8. (Now I see where you got this comment from. Most people don’t use the nice typographic quotes, but I know someone who does… :) https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:54: virtual void StartListening() = 0; To help people write subclass implementations, you should add a little more guidance on how these work. “Start/StopListening are called when transitioning between zero and nonzero registered accelerators. StartListening will be called after RegisterAcceleratorImpl and StopListening will be called after UnregisterAcceleratorImpl.” https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:56: virtual bool RegisterAcceleratorImpl(const ui::Accelerator& accelerator) = 0; And this one can use some guidance on what the return value is about: “An implementation should return false if registration did not complete successfully.” https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:16: static GlobalShortcutListenerChromeOS *instance = I personally don’t care as long as you stay consistent with the code around you, but the Chrome style guide wants the * to be on the type, not on the variable. http://www.chromium.org/developers/coding-style#Code_Formatting The same comment applies to the other subclass GetInstance implementations. Even the Mac one, which already uses this code. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:31: StopListening(); It’s kind of hard to definitively say how we want this to work because these objects are never actually destroyed, but if all of the implementation subclasses call StopListening in their destructors, then perhaps the StopListening call should be moved into the superclass. In fact, if you make that change, is_listening_ in all of the implementation subclasses looks totally extraneous. It can be moved into the superclass, eliminating even more code duplication. Right now, nothing uses is_listening_ for anything more than deciding whether to call StopListening from the destructor, and for some bookkeeping DCHECKs, which can also be moved to the superclass. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.mm:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. (This comment is in this file instead of global_shortcut_listener_mac.cc because that file has no content and I can’t add comments to it directly.) Thanks for finding that global_shortcut_listener_mac.cc was left behind as an empty file and deleting it. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.mm:136: LOG(ERROR) << "RegisterHotKey failed."; This message is not terribly useful. There’s nothing platform-specific like an error code here. Why does it have to happen in the subclass? I think you should remove the requirement that subclasses log a message when this happens. As we see here, there’s no useful information, like an error code, being conveyed. On the other hand, if you wanted to log a string corresponding to the accelerator whose registration failed, that would make sense to do in the superclass. Subclasses that can provide additional error information, like Windows which does provide an error code, can still do so at their option. (There is an error code you can use here: you can make RegisterHotKey return the OSStatus from RegisterEventHotKey, and then you can use OSSTATUS_LOG from base/mac_logging.h to produce a nice error message: |OSSTATUS_LOG(ERROR, status) << "RegisterHotKey";| https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_x11.cc (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.cc:27: static const unsigned int kModifiersMasks[] = { The “static” is pointless, since this is already in an unnamed namespace. You can remove it. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_x11.h (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.h:25: public base::MessagePumpDispatcher { I think that MessagePumpDispatcher and the Dispatch function are only needed #if !defined(TOOLKIT_GTK) in the same way that some of the other stuff is only used #if defined(TOOLKIT_GTK). As it stands now, Dispatch will never be called if TOOLKIT_GTK is true because StartListening won’t call AddDispatcherForRootWindow.
https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:41: return &g_factory.Get(); On 2013/12/20 17:54:47, Mark Mentovai wrote: > zhchbin wrote: > > I have no idea currently, but it follows the instructions of > > ProfileKeyedAPIFactory. > > > > See: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > That says that you need to provide a GetFactoryInstance method. It doesn’t say > that you have to use LazyInstance, it just uses LazyInstance as an example. This > should work and be more direct as an implementation of GetFactoryInstance: > > CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > static ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry>* instance = > new ProfileKeyedAPIFactory<ExtensionCommandsGlobalRegistry>(); > return instance; > > Note the added thread-safety CHECK. After searching with "GetFactoryInstance" in codesearch, I didn't find exceptional one which didn't follow this mechanism. Because not familiar with how "GetFactoryInstance" works, and have no enough time right now to make sure your proposal works without any problem, I choose to leave it behind. Sorry for it. However, if you think it must be included in this CL, I'm happy to change these lines of code. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.cc (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.cc:29: // The accelerator has been registered. On 2013/12/20 17:54:47, Mark Mentovai wrote: > This is a personal preference, but I find comments like this that say something > about application state to be most effective when they’re written in the block > that they apply to, instead of outside of it. So in this case: > > if (it != accelerator_map_.end()) { > // The accelerator has been registered. > return false; > } > > since the accelerator hasn’t necessarily been registered unless it was found in > the map, and only then do you want to “return false.” > > (You’d need to add the {braces} now because the conditioned block takes up more > than one line.) Done. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.cc:51: DCHECK(it != accelerator_map_.end()); On 2013/12/20 17:54:47, Mark Mentovai wrote: > Good. Does DCHECK_NE work on this line, and DCHECK_EQ on line 53? Actually, it will be a compile error if you use DCHECK_NE and DCHECK_EQ for Iterator. These equality/inequality checks require values must have operator<<(ostream, ...) defined. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener.h (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:35: // true if register successfully, false if the specificied |accelerator| has On 2013/12/20 17:54:47, Mark Mentovai wrote: > Minor grammar nit: “Returns true if registered successfully, or false if…” Done. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:36: // been registered by callers or other native applications. Note that we don’t On 2013/12/20 17:54:47, Mark Mentovai wrote: > Instead of “by callers,” say “by another caller” Done. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:36: // been registered by callers or other native applications. Note that we don’t On 2013/12/20 17:54:47, Mark Mentovai wrote: > The apostrophe in “don’t” is a Unicode right single quote (U+2019) in UTF-8. We > typically just use ASCII in source code. You don’t have to change this unless > something breaks, but I’m mentioning it because something might break: the > PRESUBMIT line length check, for example, might not understand UTF-8. > > (Now I see where you got this comment from. Most people don’t use the nice > typographic quotes, but I know someone who does… :) Done. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:54: virtual void StartListening() = 0; On 2013/12/20 17:54:47, Mark Mentovai wrote: > To help people write subclass implementations, you should add a little more > guidance on how these work. > > “Start/StopListening are called when transitioning between zero and nonzero > registered accelerators. StartListening will be called after > RegisterAcceleratorImpl and StopListening will be called after > UnregisterAcceleratorImpl.” Done. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener.h:56: virtual bool RegisterAcceleratorImpl(const ui::Accelerator& accelerator) = 0; On 2013/12/20 17:54:47, Mark Mentovai wrote: > And this one can use some guidance on what the return value is about: “An > implementation should return false if registration did not complete > successfully.” Done. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:16: static GlobalShortcutListenerChromeOS *instance = On 2013/12/20 17:54:47, Mark Mentovai wrote: > I personally don’t care as long as you stay consistent with the code around you, > but the Chrome style guide wants the * to be on the type, not on the variable. > > http://www.chromium.org/developers/coding-style#Code_Formatting > > The same comment applies to the other subclass GetInstance implementations. Even > the Mac one, which already uses this code. Done. I didn't pay attention to it when I follow the Mac one, since both kinds are accepted by the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Pointer_and_Re.... https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:31: StopListening(); On 2013/12/20 17:54:47, Mark Mentovai wrote: > It’s kind of hard to definitively say how we want this to work because these > objects are never actually destroyed, but if all of the implementation > subclasses call StopListening in their destructors, then perhaps the > StopListening call should be moved into the superclass. Calling a virtual on base class's destructor ends up ignoring the override in the derived class[0]. [0]: http://www.parashift.com/c%2B%2B-faq-lite/calling-virtuals-from-dtors.html > In fact, if you make that change, is_listening_ in all of the implementation > subclasses looks totally extraneous. It can be moved into the superclass, > eliminating even more code duplication. Right now, nothing uses is_listening_ > for anything more than deciding whether to call StopListening from the > destructor, and for some bookkeeping DCHECKs, which can also be moved to the > superclass. I believe that is_listening_ is still useful. It's using to prevent twice or more calling of StartListening. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_mac.mm (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.mm:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/12/20 17:54:47, Mark Mentovai wrote: > (This comment is in this file instead of global_shortcut_listener_mac.cc because > that file has no content and I can’t add comments to it directly.) > > Thanks for finding that global_shortcut_listener_mac.cc was left behind as an > empty file and deleting it. Git's contribution. :) https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_mac.mm:136: LOG(ERROR) << "RegisterHotKey failed."; On 2013/12/20 17:54:47, Mark Mentovai wrote: > This message is not terribly useful. There’s nothing platform-specific like an > error code here. Why does it have to happen in the subclass? > > I think you should remove the requirement that subclasses log a message when > this happens. As we see here, there’s no useful information, like an error code, > being conveyed. On the other hand, if you wanted to log a string corresponding > to the accelerator whose registration failed, that would make sense to do in the > superclass. Subclasses that can provide additional error information, like > Windows which does provide an error code, can still do so at their option. > > (There is an error code you can use here: you can make RegisterHotKey return the > OSStatus from RegisterEventHotKey, and then you can use OSSTATUS_LOG from > base/mac_logging.h to produce a nice error message: |OSSTATUS_LOG(ERROR, status) > << "RegisterHotKey";| Done with remove the requirement. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_x11.cc (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.cc:27: static const unsigned int kModifiersMasks[] = { On 2013/12/20 17:54:47, Mark Mentovai wrote: > The “static” is pointless, since this is already in an unnamed namespace. You > can remove it. Done. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_x11.h (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.h:25: public base::MessagePumpDispatcher { On 2013/12/20 17:54:47, Mark Mentovai wrote: > I think that MessagePumpDispatcher and the Dispatch function are only needed #if > !defined(TOOLKIT_GTK) in the same way that some of the other stuff is only used > #if defined(TOOLKIT_GTK). As it stands now, Dispatch will never be called if > TOOLKIT_GTK is true because StartListening won’t call > AddDispatcherForRootWindow. This class is implemented by me. I knew the problem and asked reviewers for style suggestion when committing it. With the ifdefs, code will become: class GlobalShortcutListenerX11 : #if !defined(TOOLKIT_GTK) public base::MessagePumpDispatcher, #endif public GlobalShortcutListener { Personally speaking, it seems a little ugly (Do you have any good suggestion?). Elliot told me that he wouldn't bother, so I left it behind[0]. But since you raise it again, let 's fix it this time. :) [0]: https://codereview.chromium.org/26775005/diff/1/chrome/browser/extensions/glo...
LGTM. Happy new year! https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.cc (right): https://codereview.chromium.org/109413003/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.cc:41: return &g_factory.Get(); zhchbin wrote: > After searching with "GetFactoryInstance" in codesearch, I didn't find > exceptional one which didn't follow this mechanism. Because not familiar with > how "GetFactoryInstance" works, and have no enough time right now to make sure > your proposal works without any problem, I choose to leave it behind. > > Sorry for it. However, if you think it must be included in this CL, I'm happy to > change these lines of code. The largest concern I have here is eliminating unnecessary exit-time work, as we discussed previously. But this change is otherwise in good shape, so let’s skip it for now and we can discuss it more in a followup if you’d prefer. https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/global_shortcut_listener_x11.h (right): https://codereview.chromium.org/109413003/diff/160001/chrome/browser/extensio... chrome/browser/extensions/global_shortcut_listener_x11.h:25: public base::MessagePumpDispatcher { zhchbin wrote: > Personally speaking, it seems a little ugly (Do you have any good suggestion?). > Elliot told me that he wouldn't bother, so I left it behind[0]. But since you > raise it again, let 's fix it this time. :) It may be ugly, but you had already worked #ifdefs into this code to handle the TOOLKIT_GTK case, so it seems kind of silly to not do the same for the !TOOLKIT_GTK case. I think it’s good now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/109413003/370001
On 2014/01/02 17:42:35, Mark Mentovai wrote: > LGTM. Happy new year! Thanks for the review! Happy new year! I will continue working on these related issues after my interview (I'm applying for Google Internship in China, hope I can get this precious opportunity.) and final examinations.
Retried try job too often on mac_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/109413003/370001
Retried try job too often on linux_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/109413003/370001
Retried try job too often on linux_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/109413003/370001
Retried try job too often on linux_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/109413003/370001
Retried try job too often on mac_rel for step(s) chromedriver2_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/109413003/370001
Message was sent while issue was closed.
Change committed as 242995 |
